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

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

- [High] On a shared IRQ line, the new `phydev->irq_rerun = 0` in
  phy_free_interrupt() (drivers/net/phy/phy.c:1511) loses track of the…
- [Medium] phy_free_interrupt() clears phydev->irq_rerun (phy.c:1511)
  before free_irq() (phy.c:1512) has synchronized with the threaded…
- [Medium] irq_suspended and irq_rerun are 1-bit fields
  (include/linux/phy.h:705-706).

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but for non-MAC-
  managed PHYs, mdio_bus_phy_suspend() sets irq_suspended and stops the…

--

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

This patch makes mdio_bus_phy_suspend() set irq_suspended for PHYs whose
PM is managed by the MAC driver. mdio_bus_phy_resume() then replays any
postponed interrupt. It also clears irq_rerun in phy_free_interrupt(), so
MAC drivers that close the interface during suspend don't replay an
interrupt for a freed handler.

> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index fce9bc7be3304..cd189ecee55ea 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -1507,6 +1507,8 @@ EXPORT_SYMBOL(phy_request_interrupt);
>  void phy_free_interrupt(struct phy_device *phydev)
>  {
>  	phy_disable_interrupts(phydev);
> +	/* A freed interrupt has nothing left to rerun on resume. */
> +	phydev->irq_rerun = 0;
>  	free_irq(phydev->irq, phydev);
>  }

[Severity: High]
Can this leave the IRQ line disabled for good when the line is shared?

When phy_interrupt() postpones an interrupt, it also raises the disable
depth:

drivers/net/phy/phy.c:phy_interrupt() {
    ...
		phydev->irq_rerun = 1;
		disable_irq_nosync(irq);
		return IRQ_HANDLED;
    ...
}

Clearing irq_rerun here drops the only record of that disable, and no
enable_irq() balances it. This relies on free_irq() and the later
re-request to reset desc->depth.

That reset only happens on an exclusive line. phy_request_interrupt()
always passes IRQF_SHARED. __free_irq() only shuts the line down when the
last action is removed:

kernel/irq/manage.c:__free_irq() {
    ...
	if (!desc->action) {
		irq_settings_clr_disable_unlazy(desc);
		/* Only shutdown. Deactivate after synchronize_hardirq() */
		irq_shutdown(desc);
	}
    ...
}

On re-request, __setup_irq() only calls irq_startup(), which sets the
depth back to 0, in its !shared branch.

With this patch, mac_managed_pm PHYs now get irq_suspended set. MAC
drivers such as ravb and sh_eth close the interface in suspend, so the
sequence could be:

mdio_bus_phy_suspend()
    phydev->irq_suspended = 1;
phy_interrupt()             /* fires before the MAC suspend callback */
    phydev->irq_rerun = 1;
    disable_irq_nosync(irq);     /* depth + 1 */
ravb_suspend()->ravb_close()->phy_disconnect()->phy_free_interrupt()
    phydev->irq_rerun = 0;
    free_irq()              /* another action remains, no irq_shutdown() */
ravb_resume()->...->phy_request_interrupt()
    __setup_irq()           /* shared path, depth unchanged */
mdio_bus_phy_resume()->mdio_bus_phy_resume_irq()
    irq_rerun == 0, so enable_irq() is skipped

If another PHY or device stays registered on the line across suspend,
does the line then stay masked for every user after resume?

Would it be better to balance the disable here instead? For example:
synchronize, then call enable_irq() if irq_rerun is set, all before
free_irq().

[Severity: Medium]
Is there a window where irq_rerun gets set again after this clear?

irq_rerun is cleared before free_irq() has synchronized with the
threaded handler. A phy_interrupt() thread may already be running, or
may have been woken by an interrupt that arrived before
phy_disable_interrupts(). That thread can still take the postponed
branch afterwards:

CPU1                                  CPU2 (IRQ thread)
phy_free_interrupt()
  phy_disable_interrupts()
  phydev->irq_rerun = 0;
                                      phy_interrupt()
                                        phydev->irq_rerun = 1;
                                        disable_irq_nosync(irq);
  free_irq()
    __synchronize_irq()

phy_free_interrupt() then returns with irq_rerun still set.

On a non-shared line, the MAC resume requests the IRQ again and
irq_startup() resets desc->depth to 0. mdio_bus_phy_resume_irq() then
sees the stale irq_rerun and calls enable_irq() at depth 0. That hits
the warning in __enable_irq():

		WARN(1, KERN_WARNING "Unbalanced enable for IRQ %d\n",

An unrequested irq_wake_thread() follows.

If the interface is not reopened, won't enable_irq() and
irq_wake_thread() run on a line that has no phylib action?

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 94b2e85e00a37..9488189648417 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -375,13 +376,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 if
phy_suspend() fails here for a PHY that is not MAC managed?

By this point, mdio_bus_phy_suspend() has already set irq_suspended and
stopped the state machine. Neither step is undone on error.

device_suspend() only sets dev->power.is_suspended on success.
device_resume() skips any device without it:

drivers/base/power/main.c:device_resume() {
    ...
	if (!dev->power.is_suspended)
		goto Complete;
    ...
}

So after an aborted system suspend, mdio_bus_phy_resume() never runs
for this PHY. Do its interrupts then stay postponed and disabled, with
the state machine stopped?

>  
> +static __maybe_unused void mdio_bus_phy_resume_irq(struct phy_device *phydev)
> +{
> +	if (!phy_interrupt_is_valid(phydev))
> +		return;
> +
> +	phydev->irq_suspended = 0;
> +	synchronize_irq(phydev->irq);

[Severity: Medium]
Can these unlocked bitfield writes lose updates now that the postpone
window also covers mac_managed_pm PHYs?

irq_suspended and irq_rerun are 1-bit fields in struct phy_device. They
share a storage unit with suspended, suspended_by_mdio_bus,
mac_managed_pm, wol_enabled, link, autoneg_complete, interrupts and
other bits.

phy_interrupt() writes phydev->irq_rerun = 1 without holding
phydev->lock. This patch adds another unlocked write,
phydev->irq_rerun = 0, in phy_free_interrupt().

Here, irq_suspended is cleared before synchronize_irq(). An IRQ thread
may still be doing its own read-modify-write of the same word at that
point.

For mac_managed_pm PHYs, irq_suspended now stays set across the MAC
driver's suspend and resume callbacks. Those callbacks run PHY code
that writes suspended, wol_enabled, link and autoneg_complete under
phydev->lock:

  phylink_suspend()/phy_stop()
  phylink_resume()/phy_resume()
  the state machine

The postponed branch of phy_interrupt() does not take phydev->lock.

If the irq_rerun = 1 update is lost, could the line stay disabled with
no replay?

If the irq_suspended = 0 update here is lost, would later interrupts be
postponed forever?

A lost suspended or link bit would also leave the PHY state flags wrong.

> +
> +	/* Rerun interrupts which were postponed by phy_interrupt()
> +	 * because they occurred during the system sleep transition.
> +	 */

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002043548.1302145-1-iav%40iav.lv