Re: [PATCH net v3 2/2] 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 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 (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] phy_interrupt() now decides whether a postponed interrupt on a
  MAC-managed-PM PHY is a wakeup by checking only…
- [Medium] The patch adds a second, unsynchronized consumer of irq_rerun
  and writer of irq_suspended in phy_free_interrupt() (phy.c:1518-1523).

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch.

--

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

This commit makes mdio_bus_phy_suspend() set irq_suspended for PHYs whose
PM is managed by the MAC. It also makes mdio_bus_phy_resume() replay
postponed interrupts for those PHYs. For these PHYs, phy_interrupt() no
longer reports non-WoL wakeup events, and phy_free_interrupt() now clears
irq_suspended and re-enables a line left disabled by a postponed interrupt
before freeing it.

> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 9af303d60bd6b..c3863a72a5dab 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -1421,12 +1421,16 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
>  
>  		if (netdev) {
>  			struct device *parent = netdev->dev.parent;
> +			/* Without WoL, a MAC that manages PHY PM takes the
> +			 * link down itself: that interrupt is not a wakeup.
> +			 */
> +			bool may_wake = !phydev->mac_managed_pm;
>  
>  			if (netdev->ethtool->wol_enabled)
>  				pm_system_wakeup();
> -			else if (device_may_wakeup(&netdev->dev))
> +			else if (may_wake && device_may_wakeup(&netdev->dev))
>  				pm_wakeup_dev_event(&netdev->dev, 0, true);
> -			else if (parent && device_may_wakeup(parent))
> +			else if (may_wake && parent && device_may_wakeup(parent))
>  				pm_wakeup_dev_event(parent, 0, true);
>  		}

[Severity: Medium]
For mac_managed_pm PHYs, does this leave netdev->ethtool->wol_enabled as
the only thing that can turn a postponed interrupt into a wakeup?

Elsewhere, phylib also counts the PHY's own wakeup state as WoL.
phy_suspend() does:

	phydev->wol_enabled = phy_may_wakeup(phydev) ||
			      (netdev && netdev->ethtool->wol_enabled);

and phy_may_wakeup() checks the PHY device itself:

	if (phy_can_wakeup(phydev))
		return device_may_wakeup(&phydev->mdio.dev);

	return phy_drv_wol_enabled(phydev);

The @wol_enabled kernel-doc in include/linux/phy.h also says "the PHY or
the attached MAC". mdio_bus_phy_may_suspend() notes that not all network
drivers set the netdev wol_enabled flag.

Before this patch, interrupts on these PHYs went straight to the PHY
driver's handler. Some of those handlers report their own wake event. For
example, rtl8211f_handle_interrupt() calls pm_wakeup_event() on PME, and
the broadcom and air_an8801 drivers do something similar.

Suppose phy_may_wakeup() is true but netdev->ethtool->wol_enabled is
false, and a WoL interrupt arrives between mdio_bus_phy_suspend() and the
noirq phase. The interrupt is postponed, the line is disabled, and no
wakeup is reported. Could that wake event be lost?

The ethtool set_wol paths always set the netdev flag, so this would need
WoL configured some other way, for example by firmware or by a driver at
probe time. Would also checking phy_may_wakeup(phydev) here match the rest
of phylib?

The new comment "Without WoL, a MAC that manages PHY PM takes the link
down itself" also only covers the netdev flag.

[ ... ]

> @@ -1507,6 +1511,17 @@ EXPORT_SYMBOL(phy_request_interrupt);
>  void phy_free_interrupt(struct phy_device *phydev)
>  {
>  	phy_disable_interrupts(phydev);
> +
> +	/* The PHY is reachable: stop postponing, and while our handler is still
> +	 * registered re-enable a line that a postponed interrupt left disabled.
> +	 */
> +	phydev->irq_suspended = false;
> +	synchronize_irq(phydev->irq);
> +	if (phydev->irq_rerun) {
> +		phydev->irq_rerun = false;
> +		enable_irq(phydev->irq);
> +	}
> +
>  	free_irq(phydev->irq, phydev);
>  }

[Severity: Medium]
Can this race with mdio_bus_phy_suspend() or mdio_bus_phy_resume_irq()?

irq_suspended and irq_rerun are plain bools with no shared lock.
synchronize_irq() only orders each writer against the IRQ thread.

phy_free_interrupt() can be reached through phy_disconnect() from
kernel-internal close/reopen paths. The PM core does not serialize those
paths with the PHY's PM callbacks.

One example is stmmac_reset_subtask(), which runs dev_close() and
dev_open() under RTNL from priv->wq. That workqueue comes from
create_singlethread_workqueue(), so it is not freezable. The PHY PM
callbacks don't take RTNL.

On resume, both sides could see irq_rerun set:

CPU1 mdio_bus_phy_resume_irq()      CPU2 phy_free_interrupt()
if (phydev->irq_rerun) {            if (phydev->irq_rerun) {
    phydev->irq_rerun = false;          phydev->irq_rerun = false;
    enable_irq(phydev->irq);            enable_irq(phydev->irq);
    irq_wake_thread(...);               ...
                                        free_irq(phydev->irq, phydev);

Would the second enable_irq() trigger an "Unbalanced enable for IRQ"
warning? Could irq_wake_thread() also run for a line that is being freed?

On suspend, the order could be:

CPU2 phy_free_interrupt()
    phydev->irq_suspended = false;
    synchronize_irq(phydev->irq);
    /* irq_rerun is false here */
                                    CPU1 mdio_bus_phy_suspend()
                                        phydev->irq_suspended = true;
    /* shared-line interrupt */
    phy_interrupt() (IRQ thread)
        phydev->irq_rerun = true;
        disable_irq_nosync(irq);
    free_irq(phydev->irq, phydev);

Would that leave an IRQF_SHARED line disabled for its other users? The
stale irq_rerun would also make a later mdio_bus_phy_resume_irq() call
enable_irq() on a newly requested line.

Before this patch, nothing on the PM side wrote these fields for
mac_managed_pm PHYs, so this interleaving could not happen for them.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 79dba43d703a2..7d14861850704 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -336,18 +336,20 @@ static __maybe_unused int mdio_bus_phy_suspend(struct device *dev)

[ ... ]

>  	if (phy_interrupt_is_valid(phydev)) {
>  		phydev->irq_suspended = true;
>  		synchronize_irq(phydev->irq);
>  	}
>  
> +	if (phydev->mac_managed_pm)
> +		return 0;
> +

[ ... ]

> @@ -364,13 +366,33 @@ static __maybe_unused int mdio_bus_phy_suspend(struct device *dev)
>  	return phy_suspend(phydev);
>  }

[Severity: Medium]
This isn't a bug introduced by this patch, but what happens to
irq_suspended when phy_suspend() fails here?

For PHYs without mac_managed_pm, mdio_bus_phy_suspend() sets
irq_suspended and stops the state machine. It then returns
phy_suspend(phydev) without undoing either one on error.

phy_suspend() can return -EBUSY:

	if (phydev->wol_enabled && !(phydrv->flags & PHY_ALWAYS_CALL_SUSPEND))
		return -EBUSY;

mdio_bus_phy_may_suspend() doesn't check phy_may_wakeup() when a netdev
is attached, so that case reaches this call. MDIO errors from
drv->suspend() are passed back as well.

When the callback fails, device_suspend() in drivers/base/power/main.c
never sets dev->power.is_suspended. device_resume() then skips the
device:

	if (!dev->power.is_suspended)
		goto Complete;

So mdio_bus_phy_resume() never runs for this PHY. Does irq_suspended stay
true after that? If so, the first PHY interrupt afterwards would disable
the line for good, and the state machine would stay stopped.

[ ... ]

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