| Message ID | ACDD37BE39A4EE18+20241111080627.1076283-1-wangyuli@uniontech.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: fix may not suspend when phy has WoL | expand |
NAK
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/
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.
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
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
Thanks, I will pay attention to it in the future. BRs Wentao Guan
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.