Re: [PATCH net v5 4/4] net: phy: postpone PHY interrupts during sleep with MAC-managed PM

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] The new wakeup policy treats every postponed PHY interrupt as
  a system wakeup whenever phy_may_wakeup() is true, including link-…

--

net: phy: postpone PHY interrupts during sleep with MAC-managed PM

This commit sets irq_suspended for PHYs whose MAC manages PHY PM. With it
set, phy_interrupt() postpones the PHY driver's handler for the whole
sleep transition instead of accessing an MDIO bus that may be powered
down. The wakeup event a postponed interrupt reports is now decided in
mdio_bus_phy_suspend() and stored in the new irq_wake field.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 110c9dfcbd916..2dc13dd1afb66 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -332,6 +332,44 @@ 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_phy;
> +	else
> +		phy_wol = phy_may_wakeup(phydev);
> +
> +	if (phy_wol)
> +		return PHY_IRQ_WAKE_SYSTEM;
> +
> +	/* With MAC-managed PM, only Wake-on-LAN in the PHY makes its
> +	 * interrupt a wakeup: the MAC stops the PHY itself.
> +	 */
> +	if (phydev->mac_managed_pm)
> +		return PHY_IRQ_WAKE_NONE;

[Severity: Medium]
Can a link-change interrupt caused by the MAC's own suspend path abort
system suspend here?

Take a MAC-managed PHY where phy_may_wakeup() is true. The phy_may_wakeup()
check comes before the mac_managed_pm check, so irq_wake is set to
PHY_IRQ_WAKE_SYSTEM. This happens in mdio_bus_phy_suspend(), which runs
before the MAC's suspend callback.

With stmmac, which sets config->wol_phy_speed_ctrl = true, the MAC
suspend then does:

stmmac_suspend()
  phylink_suspend()
    phylink_stop()
    phylink_speed_down(pl, false)    /* gated on phy_may_wakeup() */
      phy_speed_down(phydev, false)
        phy_config_aneg()            /* restarts autoneg, link drops */

phy_stop() does not mask the PHY interrupt. On PHYs whose INT pin carries
both link and WoL events, the link-fail interrupt then fires while
irq_suspended is set, and phy_interrupt() does:

		case PHY_IRQ_WAKE_SYSTEM:
			pm_system_wakeup();
			break;

This runs without reading the interrupt status, so a link change cannot be
told apart from a PME/WoL event. The phy_speed_down() kernel-doc already
warns about this:

  WARNING: Setting sync to false may cause the system being unable to suspend
  in case the PHY generates an interrupt when finishing the autonegotiation.

Resume speeds the link back up and the next suspend speeds it down again.
Wouldn't every retry abort in the same way?

Two configurations where this looks reachable:

AR8031/AR8035 (at803x) with WoL enabled. AT803X_INTR_ENABLE_WOL shares
AT803X_INTR_ENABLE and the INT pin with the link fail/success bits. These
PHYs lack PHY_ALWAYS_CALL_SUSPEND, so phy_suspend() returns -EBUSY and the
link interrupts are never masked.

BCM54xx with a DT interrupt and no WoL configured. bcm54xx_phy_probe()
calls device_init_wakeup(&phydev->mdio.dev, true) whenever
bcm54xx_phy_can_wakeup() is true, which is whenever
phy_interrupt_is_valid(). So phy_may_wakeup() is true by default, and the
speed-down and PHY_IRQ_WAKE_SYSTEM apply on every suspend.

Before this patch, MAC-managed PHYs never reported a wakeup from
phy_interrupt(). The driver handler read the status, and handlers such as
rtl8211f_handle_interrupt() report a wakeup only on the PME bit.

This also seems to contradict the commit message:

  ... and the MAC device may
  wake the system on its own, so a link interrupt the MAC causes must not
  abort suspend.

The Helios64 result ("no suspend aborted") seems to depend on
rtl8211f_suspend() writing RTL8211F_INER_PME to RTL821x_INER before the
speed-down. That masks the link interrupt, and other PHY drivers do not do
the same.

The postponed path also never reads or clears the status. Doesn't that
leave a level-triggered INT asserted, with the line disabled by
disable_irq_nosync() until resume?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-b4-phylib-mac-pm-irq-v3-v5-0-bbfdb5afb7d3%40iav.lv