Re: [PATCH net v4 2/5] net: phy: disable the line of a postponed PHY interrupt only once

[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 (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] This commit introduces the problem, and a later commit in the
  series fixes it.

Pre-existing issues:
- [Medium] Not fixed by this commit, and the series fixes it later.

--

net: phy: disable the line of a postponed PHY interrupt only once

This commit changes phydev->irq_rerun to an atomic_t. Only the postponed
phy_interrupt() run that sets it now calls disable_irq_nosync(), which
avoids a double disable when an edge reruns the thread during a sleep
transition. The irq_suspended accesses also get READ_ONCE() and
WRITE_ONCE() annotations.

> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 9af303d60bd6b..b47f0d3d99e55 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c

[ ... ]

> @@ -1430,8 +1430,9 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
>  				pm_wakeup_dev_event(parent, 0, true);
>  		}
>  
> -		phydev->irq_rerun = true;
> -		disable_irq_nosync(irq);
> +		/* Disable the line only once: an edge can rerun the thread. */
> +		if (!atomic_xchg(&phydev->irq_rerun, 1))
> +			disable_irq_nosync(irq);
>  		return IRQ_HANDLED;
>  	}

[Severity: Medium]
What happens here if irq_rerun is still 1 from an earlier cycle and the
line has since been re-requested?

At this commit, only the successful no_resume path of
mdio_bus_phy_resume() clears irq_rerun. phy_free_interrupt() does not
reset it:

drivers/net/phy/phy.c:phy_free_interrupt() {
	phy_disable_interrupts(phydev);
	free_irq(phydev->irq, phydev);
}

When the PHY is the only user of the line, free_irq() shuts it down. The
next request_threaded_irq() in phy_request_interrupt() goes through
irq_startup(), which sets desc->depth = 0. So the line is enabled while
irq_rerun is still 1.

There seem to be two ways to reach that state:

  a) mdio_bus_phy_resume() returns early on a phy_init_hw() or
     phy_resume() failure. That leaves irq_suspended == true and
     irq_rerun == 1. Later the interface is cycled through
     phy_disconnect()/phy_connect().

  b) A MAC driver without mac_managed_pm frees and re-requests the PHY
     IRQ in its own PM callbacks. The parent MAC resumes before the PHY,
     so the re-request runs while irq_suspended is still true.

In both cases, the next PHY interrupt takes the postponed branch and sees
irq_rerun == 1. It skips disable_irq_nosync() and returns IRQ_HANDLED
without calling drv->handle_interrupt().

The PHY interrupt status is never cleared. On a level-triggered
IRQF_ONESHOT line, the IRQ core unmasks after the thread returns and the
interrupt fires again right away.

Could this become an IRQ storm, with no end in case a) and lasting until
PHY resume in case b)?

In case b), mdio_bus_phy_resume() then runs:

		if (atomic_xchg(&phydev->irq_rerun, 0)) {
			enable_irq(phydev->irq);

on a line at depth 0. Would that trigger the "Unbalanced enable for IRQ"
warning? Before this patch, every postponed run disabled the line, so
the later enable_irq() stayed balanced.

Two later patches in this series appear to resolve this: "net: phy:
balance a postponed PHY interrupt when the interrupt is freed" and "net:
phy: stop postponing PHY interrupts when a PHY PM callback fails". The
first resets irq_rerun in phy_free_interrupt(). The second routes the
resume error paths through mdio_bus_phy_resume_irq().

Since this commit has a Fixes: tag, would a stable backport of it
without those follow-ups bring in this regression?

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 79dba43d703a2..d5d0efc104187 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -344,7 +344,7 @@ static __maybe_unused int mdio_bus_phy_suspend(struct device *dev)
>  	 * has resumed. Wait for concurrent interrupt handler to complete.
>  	 */
>  	if (phy_interrupt_is_valid(phydev)) {
> -		phydev->irq_suspended = true;
> +		WRITE_ONCE(phydev->irq_suspended, true);
>  		synchronize_irq(phydev->irq);
>  	}

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. What
clears irq_suspended if phy_suspend() fails?

mdio_bus_phy_suspend() sets the flag here and then ends with:

	phydev->suspended_by_mdio_bus = 1;

	return phy_suspend(phydev);

If phy_suspend() returns an error, the PM core skips this device's
resume callback in device_resume(), so irq_suspended stays true.

Every later phy_interrupt() run then takes the postponed branch. The
first one disables the line, and nothing re-enables it until another
full suspend/resume cycle.

mdio_bus_phy_resume() seems to end up the same way on its error paths:

	ret = phy_init_hw(phydev);
	if (ret < 0)
		return ret;

	ret = phy_resume(phydev);
	if (ret < 0)
		return ret;
no_resume:

Both returns skip the block that clears irq_suspended, consumes
irq_rerun and calls enable_irq().

Would the PHY link interrupt then stay disabled, so link changes are
lost?

The later patch in this series, "net: phy: stop postponing PHY
interrupts when a PHY PM callback fails", appears to fix both paths by
calling mdio_bus_phy_resume_irq() on these failures.

[ ... ]

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