Messages in this thread | | | Date | Wed, 9 Aug 2023 16:58:23 +0300 | From | Vladimir Oltean <> | Subject | Re: [PATCH] net: phy: Don't disable irqs on shutdown if WoL is enabled |
| |
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: > 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 <u.kleine-koenig@pengutronix.de> > --- > 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
| |