Thread (15 messages) flat view 15 messages, 7 authors, 2023-08-10

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help