[v1] net: phy: fix may not suspend when phy has WoL

[PATCH] net: phy: fix may not suspend when phy has WoL

Posted by WangYuli 1 week, 5 days ago

From: Wentao Guan <guanwentao@uniontech.com>

When system suspends and mdio_bus_phy goes to suspend, if the phy
enabled wol, phy_suspend will returned -EBUSY, and break system
suspend.

Commit 93f41e67dc8f ("net: phy: fix WoL handling when suspending
the PHY") fixes the case when netdev->wol_enabled=1, but some case,
netdev->wol_enabled=0 and phydev set wol_enabled enabled, so check
phydev->wol_enabled.

This case happens when using some out of tree ethernet drivers or
phy drivers.

Log:
YT8531S Gigabit Ethernet phytmac_mii_bus-PHYT0046:00:07: PM: dpm_run_callback(): mdio_bus_phy_suspend+0x0/0x10c returns -16
YT8531S Gigabit Ethernet phytmac_mii_bus-PHYT0046:00:07: PM: failed to suspend: error -16
PM: Some devices failed to suspend, or early wake event detected
YT8531S Gigabit Ethernet phytmac_mii_bus-PHYT0046:00:07: PM: dpm_run_callback(): mdio_bus_phy_suspend+0x0/0x10c returns -16
YT8531S Gigabit Ethernet phytmac_mii_bus-PHYT0046:00:07: PM: failed to suspend: error -16
PM: Some devices failed to suspend, or early wake event detected
YT8531S Gigabit Ethernet phytmac_mii_bus-PHYT0046:00:07: PM: dpm_run_callback(): mdio_bus_phy_suspend+0x0/0x10c returns -16
YT8531S Gigabit Ethernet phytmac_mii_bus-PHYT0046:00:07: PM: failed to freeze: error -16

Link: https://lore.kernel.org/all/20240827092446.7948-1-guanwentao@uniontech.com/
Fixes: 481b5d938b4a ("net: phy: provide phy_resume/phy_suspend helpers")
Signed-off-by: Wentao Guan <guanwentao@uniontech.com>
Signed-off-by: WangYuli <wangyuli@uniontech.com>
---
 drivers/net/phy/phy_device.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index bc24c9f2786b..12af590bfd99 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -315,6 +315,19 @@ static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
 	if (netdev->ethtool->wol_enabled)
 		return false;
 
+	/* Ethernet and phy device wol state may not same, netdev->wol_enabled
+	 * disabled, and phydev set wol_enabled enabled, so netdev->wol_enabled
+	 * is not enough.
+	 * Check phydev->wol_enabled.
+	 */
+	struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
+
+	phy_ethtool_get_wol(phydev, &wol);
+	if (wol.wolopts) {
+		phydev_warn(phydev, "Phy and mac wol are not compatible\n");
+		return false;
+	}
+
 	/* As long as not all affected network drivers support the
 	 * wol_enabled flag, let's check for hints that WoL is enabled.
 	 * Don't suspend PHY if the attached netdev parent may wake up.
-- 
2.45.2

Re: [PATCH] net: phy: fix may not suspend when phy has WoL

Posted by Russell King (Oracle) 1 week, 5 days ago

On Mon, Nov 11, 2024 at 04:06:27PM +0800, WangYuli wrote:
> From: Wentao Guan <guanwentao@uniontech.com>
> 
> When system suspends and mdio_bus_phy goes to suspend, if the phy
> enabled wol, phy_suspend will returned -EBUSY, and break system
> suspend.
> 
> Commit 93f41e67dc8f ("net: phy: fix WoL handling when suspending
> the PHY") fixes the case when netdev->wol_enabled=1, but some case,
> netdev->wol_enabled=0 and phydev set wol_enabled enabled, so check
> phydev->wol_enabled.

I think a better question would be... why do we propagate the -EBUSY
error code from phy_suspend() in mdio_bus_phy_suspend() ? It returns
-EBUSY "If the device has WOL enabled, we cannot suspend the PHY" so
it seems ignoring this error code would avoid adding yet more
complexity, trying to match the conditions in mdio_bus_phy_may_suspend()
with those in phy_suspend().

In any case, there's a helper for reading the WoL state.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: phy: fix may not suspend when phy has WoL

Posted by Wentao Guan 1 week, 5 days ago

Well question, but I do not know any knowledge about phydrv->suspend may or not return EBUSY?
Around the WoL state handling is becoming complex for checking the same logic in phy_suspend
and mdio_bus_phy_may_suspend.
I still do not know why that we need to use -EBUSY in 11 years ago 
commit("481b5d9" net: phy: provide phy_resume/phy_suspend helpers)
If it is not need, maybe a simple solution is change EBUSY to 0 and add a warning print?

And discussing the patch, it is same as commit 4f534b7f0 in v6.12-rc1, and it context should before
"if (!drv || !phydrv->suspend)" check[It goes wrong when I moving the patch from our dist branch to upstream branch]
, so I send a NAK for the patch.

BRs
Wentao Guan

Re:[PATCH] net: phy: fix may not suspend when phy has WoL

Posted by Wentao Guan 1 week, 5 days ago

The following commit is applied in v6.12-rc1 [1],and discussed in link [2].

Link:
[1]:https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v6.12-rc1&id=4f534b7f0c8d2a9ec557f9c7d77f96d29518c666
[2]:https://lore.kernel.org/all/20240628060318.458925-1-youwan@nfschina.com/

Re:[PATCH] net: phy: fix may not suspend when phy has WoL

Posted by Wentao Guan 1 week, 5 days ago

NAK

Re: [PATCH] net: phy: fix may not suspend when phy has WoL

Posted by Andrew Lunn 1 week, 5 days ago

On Mon, Nov 11, 2024 at 04:24:53PM +0800, Wentao Guan wrote:
> NAK

A NACK should include an explanation why. I see you do have followup
emails, i assume you explain why there. In future, please include the
explanation with the NACK.

	Andrew

Re: [PATCH] net: phy: fix may not suspend when phy has WoL

Posted by Wentao Guan 1 week, 5 days ago

Thanks, I will pay attention to it in the future.

BRs
Wentao Guan