[net-next,v1] net: phy: phy_device: fix PHY WOL enabled, PM failed to suspend

[net-next,v1] net: phy: phy_device: fix PHY WOL enabled, PM failed to suspend

[Youwan Wang]

If the PHY of the mido bus is enabled with Wake-on-LAN (WOL),
we cannot suspend the PHY. Although the WOL status has been
checked in phy_suspend(), returning -EBUSY(-16) would cause
the Power Management (PM) to fail to suspend. Since
phy_suspend() is an exported symbol (EXPORT_SYMBOL),
timely error reporting is needed. Therefore, an additional
check is performed here. If the PHY of the mido bus is enabled
with WOL, we skip calling phy_suspend() to avoid PM failure.

Thank you all for your analysis.
I am using the Linux kernel version 6.6, the current system is
utilizing ACPI firmware. However, in terms of configuration,
the system only includes MAC layer configuration while lacking
PHY configuration. Furthermore, it has been observed that the
phydev->attached_dev is NULL, phydev is "stmmac-0:01", it not
attached, but it will affect suspend and resume. The actually
attached "stmmac-0:00" will not dpm_run_callback():
mdio_bus_phy_suspend().

The current modification is solely aimed at ensuring that PHY
have WOL enabled but are not attached are calling phy_suspend()

log:
[    5.932502] YT8521 Gigabit Ethernet stmmac-0:00: attached PHY driver
(mii_bus:phy_addr=stmmac-0:00, irq=POLL)
[    5.932512] YT8521 Gigabit Ethernet stmmac-0:01: attached PHY driver
(mii_bus:phy_addr=stmmac-0:01, irq=POLL)
[   24.566289] YT8521 Gigabit Ethernet stmmac-0:00: yt8521_read_status,
link down, media: UTP

log:
[  322.631362] OOM killer disabled.
[  322.631364] Freezing remaining freezable tasks
[  322.632536] Freezing remaining freezable tasks completed (elapsed 0.001 seconds)
[  322.632540] printk: Suspending console(s) (use no_console_suspend to debug)
[  322.633052] YT8521 Gigabit Ethernet stmmac-0:01:
PM: dpm_run_callback(): mdio_bus_phy_suspend+0x0/0x110 [libphy] returns -16
[  322.633071] YT8521 Gigabit Ethernet stmmac-0:01:
PM: failed to suspend: error -16
[  322.669699] PM: Some devices failed to suspend, or early wake event detected
[  322.669949] OOM killer enabled.
[  322.669951] Restarting tasks ... done.
[  322.671008] random: crng reseeded on system resumption
[  322.671014] PM: suspend exit

log:
[  260.814763] YT8521 Gigabit Ethernet stmmac-0:01:
PM: dpm_run_callback():mdio_bus_phy_resume+0x0/0x160 [libphy] returns -95
[  260.814782] YT8521 Gigabit Ethernet stmmac-0:01:
PM: failed to resume: error -95

Signed-off-by: Youwan Wang <youwan@nfschina.com>
---
 drivers/net/phy/phy_device.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 2ce74593d6e4..0564decf701f 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -270,6 +270,7 @@ static DEFINE_MUTEX(phy_fixup_lock);
 
 static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
 {
+	struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
 	struct device_driver *drv = phydev->mdio.dev.driver;
 	struct phy_driver *phydrv = to_phy_driver(drv);
 	struct net_device *netdev = phydev->attached_dev;
@@ -277,6 +278,15 @@ static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
 	if (!drv || !phydrv->suspend)
 		return false;
 
+	/* If the PHY on the mido bus is not attached but has WOL enabled
+	 * we cannot suspend the PHY.
+	 */
+	phy_ethtool_get_wol(phydev, &wol);
+	phydev->wol_enabled = !!(wol.wolopts);
+	if (!netdev && phydev->wol_enabled &&
+	    !(phydrv->flags & PHY_ALWAYS_CALL_SUSPEND))
+		return false;
+
 	/* PHY not attached? May suspend if the PHY has not already been
 	 * suspended as part of a prior call to phy_disconnect() ->
 	 * phy_detach() -> phy_suspend() because the parent netdev might be the
-- 
2.25.1

Re: [net-next,v1] net: phy: phy_device: fix PHY WOL enabled, PM failed to suspend

[Paolo Abeni]

Hi,

On Thu, 2024-07-04 at 20:32 +0800, Youwan Wang wrote:
> If the PHY of the mido bus is enabled with Wake-on-LAN (WOL),
> we cannot suspend the PHY. Although the WOL status has been
> checked in phy_suspend(), returning -EBUSY(-16) would cause
> the Power Management (PM) to fail to suspend. Since
> phy_suspend() is an exported symbol (EXPORT_SYMBOL),
> timely error reporting is needed. Therefore, an additional
> check is performed here. If the PHY of the mido bus is enabled
> with WOL, we skip calling phy_suspend() to avoid PM failure.
> 
> Thank you all for your analysis.

Please, use an incremental version number (in this case: 'v2') when
sending a new revision of this patch, it will make easier to track the
previous discussion. Even when the changes affect only the
changelog/commit message.

> I am using the Linux kernel version 6.6, the current system is
> utilizing ACPI firmware. However, in terms of configuration,
> the system only includes MAC layer configuration while lacking
> PHY configuration. Furthermore, it has been observed that the
> phydev->attached_dev is NULL, phydev is "stmmac-0:01", it not
> attached, but it will affect suspend and resume. The actually
> attached "stmmac-0:00" will not dpm_run_callback():
> mdio_bus_phy_suspend().

It looks like the underlying issue is still under investigation.
As noted by Andrew, the NULL attached_dev hints at some other root
cause issue, possibly elsewhere. Please reply to Andrew's questions:

https://lore.kernel.org/netdev/b61cae2b-6b94-465e-b4e4-6c220c6c66d9@lunn.ch/

before posting a new revision. At very least the replies should be
reflected in some additional info in the commit message.

Thanks,

Paolo

Re: [net-next,v1] net: phy: phy_device: fix PHY WOL enabled, PM failed to suspend

[Andrew Lunn]

On Mon, Jul 01, 2024 at 02:21:44PM +0800, Youwan Wang wrote:

Please always start a new thread with a new version of a
patchset. Tools like patchwork require this, and without the patchset
in patchwork, it is not going to be applied.

> If the PHY of the mido bus is enabled with Wake-on-LAN (WOL),
> we cannot suspend the PHY. Although the WOL status has been
> checked in phy_suspend(), returning -EBUSY(-16) would cause
> the Power Management (PM) to fail to suspend. Since
> phy_suspend() is an exported symbol (EXPORT_SYMBOL),
> timely error reporting is needed. Therefore, an additional
> check is performed here. If the PHY of the mido bus is enabled
> with WOL, we skip calling phy_suspend() to avoid PM failure.
> 
> Thank you all for your analysis.
> I am using the Linux kernel version 6.6, the current system is
> utilizing ACPI firmware. However, in terms of configuration,
> the system only includes MAC layer configuration while lacking
> PHY configuration. Furthermore, it has been observed that the
> phydev->attached_dev is NULL
> 
> Is it possible to add a judgment about netdev is NULL?
> if (!netdev && phydev->wol_enabled &&
>     !(phydrv->flags & PHY_ALWAYS_CALL_SUSPEND))

Comments like this should be placed below the --- so they don't make
it into the commit message.

Why is phydev->attached_dev NULL? Was a MAC never attached to the PHY?
Has the MAC disconnected the PHY as part of the suspend? It would be
odd that a device being used for WoL would disconnect the PHY.

> 
> log:
> [  322.631362] OOM killer disabled.
> [  322.631364] Freezing remaining freezable tasks
> [  322.632536] Freezing remaining freezable tasks completed (elapsed 0.001 seconds)
> [  322.632540] printk: Suspending console(s) (use no_console_suspend to debug)
> [  322.633052] YT8521 Gigabit Ethernet stmmac-0:01:
> PM: dpm_run_callback(): mdio_bus_phy_suspend+0x0/0x110 [libphy] returns -16
> [  322.633071] YT8521 Gigabit Ethernet stmmac-0:01:
> PM: failed to suspend: error -16
> [  322.669699] PM: Some devices failed to suspend, or early wake event detected
> [  322.669949] OOM killer enabled.
> [  322.669951] Restarting tasks ... done.
> [  322.671008] random: crng reseeded on system resumption
> [  322.671014] PM: suspend exit
> 
> If the YT8521 driver adds phydrv->flags, ask the YT8521 driver to process
> WOL at suspend and resume time, the phydev->suspended_by_mdio_bus=1
> flag would cause the resume failure.
> 
> log:
> [  260.814763] YT8521 Gigabit Ethernet stmmac-0:01:
> PM: dpm_run_callback():mdio_bus_phy_resume+0x0/0x160 [libphy] returns -95
> [  260.814782] YT8521 Gigabit Ethernet stmmac-0:01:
> PM: failed to resume: error -95

-95 is EOPNOTSUPP. Where is that coming from?

	Andrew