* [RFC PATCH net-next] net: phy: Don't suspend/resume device not in use
@ 2024-03-07 15:45 Jan Petrous (OSS)
2024-03-07 16:22 ` Andrew Lunn
0 siblings, 1 reply; 8+ messages in thread
From: Jan Petrous (OSS) @ 2024-03-07 15:45 UTC (permalink / raw
To: netdev@vger.kernel.org, Andrew Lunn, Heiner Kallweit,
Russell King, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni
In the case when an MDIO bus contains PHY device not attached to
the any netdev or is attached to the external netdev, controlled
by another driver and the driver is disabled, the bus, when PM suspend
occurs, is trying to suspend/resume also the unattached phydev.
/* Synopsys DWMAC built-in driver (stmmac) */
gmac0: ethernet@4033c000 {
compatible = "snps,dwc-qos-ethernet", "nxp,s32cc-dwmac";
phy-handle = <&gmac0_mdio_c_phy4>;
phy-mode = "rgmii-id";
gmac0_mdio: mdio@0 {
compatible = "snps,dwmac-mdio";
/* AQR107 attached to pfe_netif1 */
gmac0_mdio_c_phy1: ethernet-phy@1 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <1>;
};
/* KSZ9031RNX attached to gmac0 */
gmac0_mdio_c_phy4: ethernet-phy@4 {
reg = <4>;
};
};
};
/* PFE controller, loadable driver pfeng.ko */
pfe: pfe@46000000 {
compatible = "nxp,s32g-pfe";
/* Network interface 'pfe1' */
pfe_netif1: ethernet@11 {
compatible = "nxp,s32g-pfe-netif";
phy-mode = "sgmii";
phy-handle = <&gmac0_mdio_c_phy1>;
};
};
Because such device didn't go through attach process, internal
parameters like phy_dev->interface is set to the default value, which
is not correct for some drivers. Ie. Aquantia PHY AQR107 doesn't
support PHY_INTERFACE_MODE_GMII and trying to use phy_init_hw()
in mdio_bus_phy_resume() ends up with the following error caused
by initial check of supported interfaces in aqr107_config_init():
[ 63.927708] Aquantia AQR113C stmmac-0:08: PM: failed to resume: error -19']
Signed-off-by: Jan Petrous <jan.petrous@oss.nxp.com>
---
drivers/net/phy/phy_device.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3611ea64875e..30c03ac6b84c 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -311,6 +311,10 @@ static __maybe_unused int mdio_bus_phy_suspend(struct device *dev)
{
struct phy_device *phydev = to_phy_device(dev);
+ /* Don't suspend device if not in use state */
+ if (phydev->state <= PHY_READY)
+ return 0;
+
if (phydev->mac_managed_pm)
return 0;
@@ -344,6 +348,10 @@ static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
struct phy_device *phydev = to_phy_device(dev);
int ret;
+ /* Don't resume device which wasn't previously in use state */
+ if (phydev->state <= PHY_READY)
+ return 0;
+
if (phydev->mac_managed_pm)
return 0;
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC PATCH net-next] net: phy: Don't suspend/resume device not in use
2024-03-07 15:45 [RFC PATCH net-next] net: phy: Don't suspend/resume device not in use Jan Petrous (OSS)
@ 2024-03-07 16:22 ` Andrew Lunn
2024-03-07 16:44 ` Jan Petrous (OSS)
0 siblings, 1 reply; 8+ messages in thread
From: Andrew Lunn @ 2024-03-07 16:22 UTC (permalink / raw
To: Jan Petrous (OSS)
Cc: netdev@vger.kernel.org, Heiner Kallweit, Russell King,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
> Because such device didn't go through attach process, internal
> parameters like phy_dev->interface is set to the default value, which
> is not correct for some drivers. Ie. Aquantia PHY AQR107 doesn't
> support PHY_INTERFACE_MODE_GMII and trying to use phy_init_hw()
> in mdio_bus_phy_resume() ends up with the following error caused
> by initial check of supported interfaces in aqr107_config_init():
>
> [ 63.927708] Aquantia AQR113C stmmac-0:08: PM: failed to resume: error -19']
>
> Signed-off-by: Jan Petrous <jan.petrous@oss.nxp.com>
> ---
> drivers/net/phy/phy_device.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 3611ea64875e..30c03ac6b84c 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -311,6 +311,10 @@ static __maybe_unused int mdio_bus_phy_suspend(struct device *dev)
> {
> struct phy_device *phydev = to_phy_device(dev);
>
> + /* Don't suspend device if not in use state */
> + if (phydev->state <= PHY_READY)
> + return 0;
> +
> if (phydev->mac_managed_pm)
> return 0;
If nothing is using it, suspending it does however make sense. It is
consuming power, which could be saved by suspending it. It makes the
code asymmetrical, but i would throw this hunk away.
>
> @@ -344,6 +348,10 @@ static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
> struct phy_device *phydev = to_phy_device(dev);
> int ret;
>
> + /* Don't resume device which wasn't previously in use state */
> + if (phydev->state <= PHY_READY)
> + return 0;
> +
This is the real fix to your problem. phy_attach_direct() also does a
phy_resume(), so if the device does come along and claim the PHY after
the resume, it looks like this should work, without the matching
suspend.
Andrew
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [RFC PATCH net-next] net: phy: Don't suspend/resume device not in use
2024-03-07 16:22 ` Andrew Lunn
@ 2024-03-07 16:44 ` Jan Petrous (OSS)
2024-03-15 7:55 ` [PATCH net-next v2] net: phy: don't resume " Jan Petrous (OSS)
0 siblings, 1 reply; 8+ messages in thread
From: Jan Petrous (OSS) @ 2024-03-07 16:44 UTC (permalink / raw
To: Andrew Lunn, Jan Petrous (OSS)
Cc: netdev@vger.kernel.org, Heiner Kallweit, Russell King,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
>
> > Because such device didn't go through attach process, internal
> > parameters like phy_dev->interface is set to the default value, which
> > is not correct for some drivers. Ie. Aquantia PHY AQR107 doesn't
> > support PHY_INTERFACE_MODE_GMII and trying to use phy_init_hw()
> > in mdio_bus_phy_resume() ends up with the following error caused
> > by initial check of supported interfaces in aqr107_config_init():
> >
> > [ 63.927708] Aquantia AQR113C stmmac-0:08: PM: failed to resume: error -
> 19']
> >
> > Signed-off-by: Jan Petrous <jan.petrous@oss.nxp.com>
> > ---
> > drivers/net/phy/phy_device.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index 3611ea64875e..30c03ac6b84c 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -311,6 +311,10 @@ static __maybe_unused int
> mdio_bus_phy_suspend(struct device *dev)
> > {
> > struct phy_device *phydev = to_phy_device(dev);
> >
> > + /* Don't suspend device if not in use state */
> > + if (phydev->state <= PHY_READY)
> > + return 0;
> > +
> > if (phydev->mac_managed_pm)
> > return 0;
>
> If nothing is using it, suspending it does however make sense. It is
> consuming power, which could be saved by suspending it. It makes the
> code asymmetrical, but i would throw this hunk away.
Yes, I also thought about still having possibility to suspend it, even not used.
I'm ok with removal the hunk.
>
> >
> > @@ -344,6 +348,10 @@ static __maybe_unused int
> mdio_bus_phy_resume(struct device *dev)
> > struct phy_device *phydev = to_phy_device(dev);
> > int ret;
> >
> > + /* Don't resume device which wasn't previously in use state */
> > + if (phydev->state <= PHY_READY)
> > + return 0;
> > +
>
> This is the real fix to your problem. phy_attach_direct() also does a
> phy_resume(), so if the device does come along and claim the PHY after
> the resume, it looks like this should work, without the matching
> suspend.
Well, I like symmetry, this was the reason I checked both PM directions.
But you are right, it works for me with resume hunk only.
Thanks.
/Jan
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH net-next v2] net: phy: don't resume device not in use
2024-03-07 16:44 ` Jan Petrous (OSS)
@ 2024-03-15 7:55 ` Jan Petrous (OSS)
2024-03-19 11:00 ` Paolo Abeni
2024-03-19 12:02 ` Andrew Lunn
0 siblings, 2 replies; 8+ messages in thread
From: Jan Petrous (OSS) @ 2024-03-15 7:55 UTC (permalink / raw
To: Andrew Lunn, netdev@vger.kernel.org
Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
In the case when an MDIO bus contains PHY device not attached to
any netdev or is attached to the external netdev, controlled
by another driver and the driver is disabled, the bus, when PM resume
occurs, is trying to resume also the unattached phydev.
/* Synopsys DWMAC built-in driver (stmmac) */
gmac0: ethernet@4033c000 {
compatible = "snps,dwc-qos-ethernet", "nxp,s32cc-dwmac";
phy-handle = <&gmac0_mdio_c_phy4>;
phy-mode = "rgmii-id";
gmac0_mdio: mdio@0 {
compatible = "snps,dwmac-mdio";
/* AQR107 */
gmac0_mdio_c_phy1: ethernet-phy@1 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <1>;
};
/* KSZ9031RNX */
gmac0_mdio_c_phy4: ethernet-phy@4 {
reg = <4>;
};
};
};
/* PFE controller, loadable driver pfeng.ko */
pfe: pfe@46000000 {
compatible = "nxp,s32g-pfe";
/* Network interface 'pfe1' */
pfe_netif1: ethernet@11 {
compatible = "nxp,s32g-pfe-netif";
phy-mode = "sgmii";
phy-handle = <&gmac0_mdio_c_phy1>;
};
};
Because such device didn't go through attach process, internal
parameters like phy_dev->interface are set to default values, which
can be incorrect for some drivers. Ie. Aquantia PHY AQR107 doesn't
support PHY_INTERFACE_MODE_GMII and trying to use phy_init()
in mdio_bus_phy_resume ends up with the following error caused
by initial check of supported interfaces in aqr107_config_init():
[ 63.927708] Aquantia AQR113C stmmac-0:08: PM: failed to resume: error -19']
The fix is intentionally assymetric to support PM suspend of the device.
Signed-off-by: Jan Petrous <jan.petrous@oss.nxp.com>
---
drivers/net/phy/phy_device.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 8297ef681bf5..507eb0570e0e 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -355,6 +355,10 @@ static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
struct phy_device *phydev = to_phy_device(dev);
int ret;
+ /* Don't resume device which wasn't previously in use state */
+ if (phydev->state <= PHY_READY)
+ return 0;
+
if (phydev->mac_managed_pm)
return 0;
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v2] net: phy: don't resume device not in use
2024-03-15 7:55 ` [PATCH net-next v2] net: phy: don't resume " Jan Petrous (OSS)
@ 2024-03-19 11:00 ` Paolo Abeni
2024-03-19 12:05 ` Andrew Lunn
2024-03-19 12:02 ` Andrew Lunn
1 sibling, 1 reply; 8+ messages in thread
From: Paolo Abeni @ 2024-03-19 11:00 UTC (permalink / raw
To: Jan Petrous (OSS), Andrew Lunn, netdev@vger.kernel.org
Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
Jakub Kicinski
On Fri, 2024-03-15 at 07:55 +0000, Jan Petrous (OSS) wrote:
> In the case when an MDIO bus contains PHY device not attached to
> any netdev or is attached to the external netdev, controlled
> by another driver and the driver is disabled, the bus, when PM resume
> occurs, is trying to resume also the unattached phydev.
>
> /* Synopsys DWMAC built-in driver (stmmac) */
> gmac0: ethernet@4033c000 {
> compatible = "snps,dwc-qos-ethernet", "nxp,s32cc-dwmac";
>
> phy-handle = <&gmac0_mdio_c_phy4>;
> phy-mode = "rgmii-id";
>
> gmac0_mdio: mdio@0 {
> compatible = "snps,dwmac-mdio";
>
> /* AQR107 */
> gmac0_mdio_c_phy1: ethernet-phy@1 {
> compatible = "ethernet-phy-ieee802.3-c45";
> reg = <1>;
> };
>
> /* KSZ9031RNX */
> gmac0_mdio_c_phy4: ethernet-phy@4 {
> reg = <4>;
> };
> };
> };
>
> /* PFE controller, loadable driver pfeng.ko */
> pfe: pfe@46000000 {
> compatible = "nxp,s32g-pfe";
>
> /* Network interface 'pfe1' */
> pfe_netif1: ethernet@11 {
> compatible = "nxp,s32g-pfe-netif";
>
> phy-mode = "sgmii";
> phy-handle = <&gmac0_mdio_c_phy1>;
> };
> };
>
> Because such device didn't go through attach process, internal
> parameters like phy_dev->interface are set to default values, which
> can be incorrect for some drivers. Ie. Aquantia PHY AQR107 doesn't
> support PHY_INTERFACE_MODE_GMII and trying to use phy_init()
> in mdio_bus_phy_resume ends up with the following error caused
> by initial check of supported interfaces in aqr107_config_init():
>
> [ 63.927708] Aquantia AQR113C stmmac-0:08: PM: failed to resume: error -19']
>
> The fix is intentionally assymetric to support PM suspend of the device.
>
> Signed-off-by: Jan Petrous <jan.petrous@oss.nxp.com>
Please note that the 'net-next' tree is closed for the merge window.
You will have to repost in when the tree will re-open in a week or so.
However this change could be suitable for the 'net' tree, if Andrew
agrees. If, please re-sent targeting such tree and including a
reasonable 'Fixes' tag.
Thanks,
Paolo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v2] net: phy: don't resume device not in use
2024-03-15 7:55 ` [PATCH net-next v2] net: phy: don't resume " Jan Petrous (OSS)
2024-03-19 11:00 ` Paolo Abeni
@ 2024-03-19 12:02 ` Andrew Lunn
1 sibling, 0 replies; 8+ messages in thread
From: Andrew Lunn @ 2024-03-19 12:02 UTC (permalink / raw
To: Jan Petrous (OSS)
Cc: netdev@vger.kernel.org, Heiner Kallweit, Russell King,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
On Fri, Mar 15, 2024 at 07:55:48AM +0000, Jan Petrous (OSS) wrote:
> In the case when an MDIO bus contains PHY device not attached to
> any netdev or is attached to the external netdev, controlled
> by another driver and the driver is disabled, the bus, when PM resume
> occurs, is trying to resume also the unattached phydev.
>
> /* Synopsys DWMAC built-in driver (stmmac) */
> gmac0: ethernet@4033c000 {
> compatible = "snps,dwc-qos-ethernet", "nxp,s32cc-dwmac";
>
> phy-handle = <&gmac0_mdio_c_phy4>;
> phy-mode = "rgmii-id";
>
> gmac0_mdio: mdio@0 {
> compatible = "snps,dwmac-mdio";
>
> /* AQR107 */
> gmac0_mdio_c_phy1: ethernet-phy@1 {
> compatible = "ethernet-phy-ieee802.3-c45";
> reg = <1>;
> };
>
> /* KSZ9031RNX */
> gmac0_mdio_c_phy4: ethernet-phy@4 {
> reg = <4>;
> };
> };
> };
>
> /* PFE controller, loadable driver pfeng.ko */
> pfe: pfe@46000000 {
> compatible = "nxp,s32g-pfe";
>
> /* Network interface 'pfe1' */
> pfe_netif1: ethernet@11 {
> compatible = "nxp,s32g-pfe-netif";
>
> phy-mode = "sgmii";
> phy-handle = <&gmac0_mdio_c_phy1>;
> };
> };
>
> Because such device didn't go through attach process, internal
> parameters like phy_dev->interface are set to default values, which
> can be incorrect for some drivers. Ie. Aquantia PHY AQR107 doesn't
> support PHY_INTERFACE_MODE_GMII and trying to use phy_init()
> in mdio_bus_phy_resume ends up with the following error caused
> by initial check of supported interfaces in aqr107_config_init():
>
> [ 63.927708] Aquantia AQR113C stmmac-0:08: PM: failed to resume: error -19']
>
> The fix is intentionally assymetric to support PM suspend of the device.
>
> Signed-off-by: Jan Petrous <jan.petrous@oss.nxp.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v2] net: phy: don't resume device not in use
2024-03-19 11:00 ` Paolo Abeni
@ 2024-03-19 12:05 ` Andrew Lunn
2024-03-19 12:31 ` Florian Fainelli
0 siblings, 1 reply; 8+ messages in thread
From: Andrew Lunn @ 2024-03-19 12:05 UTC (permalink / raw
To: Paolo Abeni
Cc: Jan Petrous (OSS), netdev@vger.kernel.org, Heiner Kallweit,
Russell King, David S. Miller, Eric Dumazet, Jakub Kicinski
> Please note that the 'net-next' tree is closed for the merge window.
> You will have to repost in when the tree will re-open in a week or so.
>
> However this change could be suitable for the 'net' tree, if Andrew
> agrees. If, please re-sent targeting such tree and including a
> reasonable 'Fixes' tag.
This is the sort of change that i think it should only be in net-next.
Suspend/resume is complex and not tested too well. There is a chance
of regression with this change. So we should introduce it slowly.
Andrew
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v2] net: phy: don't resume device not in use
2024-03-19 12:05 ` Andrew Lunn
@ 2024-03-19 12:31 ` Florian Fainelli
0 siblings, 0 replies; 8+ messages in thread
From: Florian Fainelli @ 2024-03-19 12:31 UTC (permalink / raw
To: Andrew Lunn, Paolo Abeni
Cc: Jan Petrous (OSS), netdev@vger.kernel.org, Heiner Kallweit,
Russell King, David S. Miller, Eric Dumazet, Jakub Kicinski
On 3/19/2024 5:05 AM, Andrew Lunn wrote:
>> Please note that the 'net-next' tree is closed for the merge window.
>> You will have to repost in when the tree will re-open in a week or so.
>>
>> However this change could be suitable for the 'net' tree, if Andrew
>> agrees. If, please re-sent targeting such tree and including a
>> reasonable 'Fixes' tag.
>
> This is the sort of change that i think it should only be in net-next.
> Suspend/resume is complex and not tested too well. There is a chance
> of regression with this change. So we should introduce it slowly.
Totally agree.
--
Florian
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-03-19 12:31 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-07 15:45 [RFC PATCH net-next] net: phy: Don't suspend/resume device not in use Jan Petrous (OSS)
2024-03-07 16:22 ` Andrew Lunn
2024-03-07 16:44 ` Jan Petrous (OSS)
2024-03-15 7:55 ` [PATCH net-next v2] net: phy: don't resume " Jan Petrous (OSS)
2024-03-19 11:00 ` Paolo Abeni
2024-03-19 12:05 ` Andrew Lunn
2024-03-19 12:31 ` Florian Fainelli
2024-03-19 12:02 ` Andrew Lunn
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.