Re: [PATCH net v4 5/5] net: phy: postpone PHY interrupts during sleep with MAC-managed PM
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-11-01 08:36:39
Also in:
linux-arm-kernel, linux-rockchip, lkml
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider. Critical: 0 · High: 1 · Medium: 0 · Low: 0 If you believe none of the issues are legitimate, please reply to each issue of Medium severity or higher and explain why the AI is wrong. If any of them are legitimate and you plan to respin, please let patchwork know by including "pw-bot: cr" as a separate line at the end of your reply (one such reply per series is enough). - [High] net: phy: false wakeup in phy_interrupt() during mac_managed_pm suspend --
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c@@ -332,16 +332,55 @@ static bool mdio_bus_phy_may_suspend(struct phy_device *phydev) return !phydev->suspended; } +static __maybe_unused enum phy_irq_wake +phy_irq_wake_source(struct phy_device *phydev) +{ + struct net_device *netdev = phydev->attached_dev; + bool phy_wol; + + if (!netdev) + return PHY_IRQ_WAKE_NONE; + + /* phy_suspend() cached the answer of a suspended PHY, which may + * not be reachable over MDIO. + */ + if (phydev->suspended && !phy_can_wakeup(phydev)) + phy_wol = phydev->wol_enabled; + else + phy_wol = phy_may_wakeup(phydev); + + if (netdev->ethtool->wol_enabled || phy_wol) + return PHY_IRQ_WAKE_SYSTEM; + + /* A MAC that manages PHY PM stops the PHY itself: the link + * interrupt that follows is not a wakeup. + */ + if (phydev->mac_managed_pm) + return PHY_IRQ_WAKE_NONE; + + if (device_may_wakeup(&netdev->dev)) + return PHY_IRQ_WAKE_NETDEV; + + if (netdev->dev.parent && device_may_wakeup(netdev->dev.parent)) + return PHY_IRQ_WAKE_PARENT; + + return PHY_IRQ_WAKE_NONE; +} + /* Wakeup interrupts may occur during the system sleep transition when * the PHY is inaccessible. Set flag to postpone handling until the PHY * has resumed. Wait for concurrent interrupt handler to complete. + * Decide which wakeup event a postponed interrupt reports now, while + * the PHY can still be read. */ static __maybe_unused void mdio_bus_phy_suspend_irq(struct phy_device *phydev) { if (!phy_interrupt_is_valid(phydev)) return; - WRITE_ONCE(phydev->irq_suspended, true); + phydev->irq_wake = phy_irq_wake_source(phydev); + /* Pairs with smp_load_acquire() in phy_interrupt() */ + smp_store_release(&phydev->irq_suspended, true); synchronize_irq(phydev->irq); }
[Severity: High] phy_irq_wake_source() runs from mdio_bus_phy_suspend(), before the MAC's suspend callback. For a mac_managed_pm PHY, that callback still changes the link afterwards. The mac_managed_pm check below only covers the case without Wake-on-LAN. With Wake-on-LAN enabled we return PHY_IRQ_WAKE_SYSTEM, so any link interrupt the MAC causes itself makes phy_interrupt() call pm_system_wakeup() and abort the suspend. I can see two ways to get there. WoL in the PHY on stmmac: stmmac_suspend() calls phylink_suspend(), which goes through phylink_stop() -> phy_stop() -> phy_suspend(). With wol_enabled set and no PHY_ALWAYS_CALL_SUSPEND, phy_suspend() returns -EBUSY without calling the driver's suspend, so the link interrupt stays unmasked. phylink_suspend() then calls phylink_speed_down(), since stmmac sets wol_phy_speed_ctrl, and phy_speed_down() restarts autonegotiation to drop to 10Mbps. The link goes down while irq_suspended is set and irq_wake is PHY_IRQ_WAKE_SYSTEM. WoL in the MAC only, fec for example: netdev->ethtool->wol_enabled is set while phy_wol is false, and fec_suspend() calls phy_stop() and gates its clocks. phy_suspend() again leaves the link interrupt of the PHY enabled. rtl8211f_suspend(), for one, neither calls genphy_suspend() nor masks the link status interrupt when wol_enabled is set without driver-model wakeup. So the link-down interrupt is reported as a system wakeup, although the PHY interrupt is not a wakeup source here at all; the MAC is. Before this patch these PHYs did not have irq_suspended set during the MAC's suspend callback, so neither case could abort suspend. This looks like a regression for anyone using WoL behind a MAC that manages PHY PM. Did the Helios64 Wake-on-LAN test go through the speed downshift in phylink_suspend()? I'm not sure what the right fix is, so two questions. Should the mac_managed_pm case count only phy_wol, and not the ethtool flag that may describe MAC WoL? And for PHY WoL, can the wakeup be armed only after the MAC has finished suspending, for example from a later suspend phase, so that link changes the MAC makes itself are not reported? -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261031-b4-phylib-mac-pm-irq-v3-v4-0-5ba23b3ff9a4%40iav.lv