Re: [PATCH net v4 4/5] net: phy: stop postponing PHY interrupts when a PHY PM callback fails

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1

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] Interrupt storm and WARN flood when a PHY PM callback fails
  because MDIO access is broken, which can then disable the shared IRQ…
- [Low] Comments that say interrupt handling is postponed 'until the PHY
  has resumed' are now wrong on the failure paths.

--

net: phy: stop postponing PHY interrupts when a PHY PM callback fails

This patch changes mdio_bus_phy_suspend() so that a failed phy_suspend()
undoes the interrupt postponement and restarts the state machine. It also
changes mdio_bus_phy_resume() so that a failed phy_init_hw() or
phy_resume() reruns the postponed PHY interrupt. Before this, the function
returned with the line still disabled.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index ef5e1cd3a8319..390069ec13cac 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -383,9 +384,20 @@ static __maybe_unused int mdio_bus_phy_suspend(struct device *dev)
>  	if (!mdio_bus_phy_may_suspend(phydev))
>  		return 0;
>  
> +	ret = phy_suspend(phydev);
> +	if (ret) {
> +		/* The PM core skips the resume callback of a device whose
> +		 * suspend callback failed.
> +		 */
> +		mdio_bus_phy_resume_irq(phydev);
> +		if (phy_uses_state_machine(phydev))
> +			phy_start_machine(phydev);
> +		return ret;
> +	}
> +
>  	phydev->suspended_by_mdio_bus = 1;
>  
> -	return phy_suspend(phydev);
> +	return 0;
>  }

[ ... ]

> @@ -411,11 +423,11 @@ static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
>  
>  	ret = phy_init_hw(phydev);
>  	if (ret < 0)
> -		return ret;
> +		goto err_resume_irq;
>  
>  	ret = phy_resume(phydev);
>  	if (ret < 0)
> -		return ret;
> +		goto err_resume_irq;
>  no_resume:
>  	mdio_bus_phy_resume_irq(phydev);
>  
> @@ -423,6 +435,10 @@ static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
>  		phy_start_machine(phydev);
>  
>  	return 0;
> +
> +err_resume_irq:
> +	mdio_bus_phy_resume_irq(phydev);
> +	return ret;

[Severity: Medium]
Can this cause an interrupt storm when phy_init_hw() or phy_resume() fails
because the PHY can't be reached over MDIO?

Suppose a postponed interrupt set irq_rerun during the sleep transition.
mdio_bus_phy_resume_irq() then re-enables the line and wakes the thread. It
does not check whether the PHY can be accessed:

drivers/net/phy/phy_device.c:mdio_bus_phy_resume_irq() {
    ...
	if (atomic_xchg(&phydev->irq_rerun, 0)) {
		enable_irq(phydev->irq);
		irq_wake_thread(phydev->irq, phydev);
	}
    ...
}

irq_suspended is now clear, so phy_interrupt() calls
drv->handle_interrupt(). If the MDIO read fails, a typical handler can't
acknowledge the source:

drivers/net/phy/micrel.c:kszphy_handle_interrupt() {
	irq_status = phy_read(phydev, MII_KSZPHY_INTCS);
	if (irq_status < 0) {
		phy_error(phydev);
		return IRQ_NONE;
	}
    ...
}

dp83848_handle_interrupt() does the same thing. phy_error() is an
unconditional WARN_ON(1) followed by phy_process_error(). That only sets
PHY_ERROR and triggers the state machine. It does not mask the interrupt in
the PHY and does not free the IRQ.

The line is requested with IRQF_ONESHOT | IRQF_SHARED. If the PHY is still
powered and keeps its level INT_N asserted, the line fires again as soon as
the thread finishes.

Would each pass then cost a failed MDIO transaction and a full WARN
backtrace? That would repeat until the spurious IRQ detector disables the
line for every sharer. With panic_on_warn set, the first rerun would panic.

The rollback in mdio_bus_phy_suspend() above takes the same path when
drv->suspend() fails on an MDIO error.

Before this patch, the line just stayed disabled in this case. The commit
message says:

    ... which gives the line back to the other users of a
    shared line. A handler that then cannot read the PHY reports it through
    phy_error(), as most PHY drivers do on any MDIO error.

Can other users of a shared line actually get it back if the PHY's own
interrupt source can't be acknowledged? Whether INT_N stays asserted while
MDIO fails depends on the hardware. Nothing on this path guards against it,
though.

[Severity: Low]
This isn't a bug, but several comments still say the postponed interrupt is
handled only once the PHY has resumed. On the new failure paths, the
postponement ends even though the PHY never resumed:

drivers/net/phy/phy.c:phy_interrupt():
	/* Wakeup interrupts may occur during a system sleep transition.
	 * Postpone handling until the PHY has resumed.
	 */

drivers/net/phy/phy_device.c, above mdio_bus_phy_suspend_irq():
/* 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.
 */

include/linux/phy.h, struct phy_device kernel-doc:
 * @irq_suspended: Flag indicating PHY is suspended and therefore interrupt
 *                 handling shall be postponed until PHY has resumed

A later patch in the series, "net: phy: postpone PHY interrupts during
sleep with MAC-managed PM", changes the phy.h kernel-doc to "postponed
until mdio_bus_phy_resume()". The comments in phy.c and phy_device.c are
still unchanged at the end of the series.

Should those two comments be updated as well?

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