Re: [PATCH] net: phy: Don't disable irqs on shutdown if WoL is enabled
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: 2023-08-09 13:58:40
Also in:
lkml
Hi Uwe, (I hope the threading won't be broken) On Fri, Aug 04, 2023 at 09:17:57AM +0200, Uwe Kleine-König wrote:
quoted hunk ↗ jump to hunk
Most PHYs signal WoL using an interrupt. So disabling interrupts breaks WoL at least on PHYs covered by the marvell driver. So skip disabling irqs on shutdown if WoL is enabled. While at it also explain the motivation that irqs are disabled at all. Fixes: e2f016cf7751 ("net: phy: add a shutdown procedure") Signed-off-by: Uwe Kleine-König <redacted> --- Hello, while I'm not sure that disabling interrupts is a good idea in general, this change at least should fix the WoL case. Note that this change is only compile tested as next doesn't boot on my test machine (because of https://git.kernel.org/linus/b3574f579ece24439c90e9a179742c61205fbcfa) and 6.1 (which is the other kernel I have running) doesn't know about .wol_enabled. I don't want to delay this fix until I bisected this new issue. Assuming this patch is eligible for backporting to stable, maybe point out that it depends on v6.5-rc1~163^2~286^2~2 ("net: phy: Allow drivers to always call into ->suspend()"). Didn't try to backport that. Best regards Uwe drivers/net/phy/phy_device.c | 9 +++++++++ 1 file changed, 9 insertions(+)diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 61921d4dbb13..6d1526bdd1d7 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c@@ -3340,6 +3340,15 @@ static void phy_shutdown(struct device *dev) if (phydev->state == PHY_READY || !phydev->attached_dev) return; + /* Most phys signal WoL via the irq line. So for these irqs shouldn't be + * disabled. + */ + if (phydev->wol_enabled) + return; + + /* On shutdown disable irqs to prevent an irq storm on systems where the + * irq line is shared by several devices. + */ phy_disable_interrupts(phydev); }-- 2.40.1
I think the idea is not bad and something along these lines might be the way to go, but I don't think it works (as currently implemented, and tested by me, prints below). Upon a quick search, phydev->wol_enabled is only set from phy_suspend(), and phy_suspend() isn't invoked from the ethnl_set_wol() call stack. Confirmed this way: $ ethtool -s end0 wol g # &enet0 in the device tree $ reboot -f Rebooting. [ 288.682444] Qualcomm Atheros AR8031/AR8033 mdio@2d24000:02: phy_shutdown: wol_enabled 0 [ 288.690935] Qualcomm Atheros AR8031/AR8033 mdio@2d24000:01: phy_shutdown: wol_enabled 0 [ 288.736145] reboot: Restarting system This is on the arch/arm/boot/dts/nxp/ls/ls1021a-tsn.dts, the same board as the one which Ioana worked on (with the shared AR8031 PHY interrupts). Sure, it needs to be mentioned that WoL + shared PHY interrupts is not by any means an impossible combination, but it still won't work reliably. I guess that's ok temporarily, since WoL requires user opt-in, so in the default configuration, the LS1021A-TSN is not broken by this change (in its functional variant). pw-bot: cr