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

drivers/net/phy/phy.c static irqreturn_t phy_interrupt(int irq, void *phy_dat) · line 1437

[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?

drivers/net/phy/phy_device.c static __maybe_unused int mdio_bus_phy_suspend(struct device *dev) · line 349

[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

drivers/net/phy/phy.c EXPORT_SYMBOL(phy_request_interrupt); · line 1523

[Severity: High]
Can the re-enable here livelock the PHY's own irq thread before free_irq() is reached?

phy_request_interrupt() requests the line with a NULL primary handler and IRQF_ONESHOT | IRQF_SHARED, so phy_interrupt() runs in an irq thread that irq_thread() switches to SCHED_FIFO. The callers named in the commit message run as SCHED_NORMAL tasks: dev_close() from the ravb and cpsw suspend callbacks, and stmmac_reset_subtask().

Suppose irq_suspended is still true here, and the level source was not cleared by phy_disable_interrupts(). The commit message allows for that. It can happen if the MDIO write fails, since the return value is ignored here. It can also happen if config_intr() masks without acking the latched status, or if another sharer asserts the line. Then:

phy_free_interrupt()
atomic_xchg(&phydev->irq_rerun, 1) <- returns 1, line disabled
synchronize_irq(phydev->irq)
enable_irq(phydev->irq) <- line unmasked, fires
phy_interrupt() <- irq thread, SCHED_FIFO
irq_suspended is true: postponed path atomic_xchg() returns 1, no disable_irq_nosync() pm_system_wakeup() / pm_wakeup_dev_event() return IRQ_HANDLED
irq_finalize_oneshot() <- line unmasked again
line fires, IRQTF_RUNTHREAD set before the thread can sleep irq_wait_for_interrupt() returns at once, loop repeats
free_irq(phydev->irq, phydev) <- starved

phy_interrupt() keeps returning IRQ_HANDLED, so threads_handled keeps advancing and note_interrupt() never treats the storm as spurious.

The irq thread and the caller can share a CPU, for example on a uniprocessor system or with overlapping affinity. In that case the SCHED_FIFO thread preempts phy_free_interrupt(), and free_irq() only runs in whatever time RT throttling or the fair server hands back, if any. That hangs or badly stalls the close in the MAC suspend callback or in the reset worker. Each pass through the postponed path also raises a wakeup event, which aborts the sleep transition in progress.

The commit message says the interrupt then fires until free_irq(), in the same call, removes the handler. That assumes the caller keeps running. With irq_rerun preset to 1, the handler has no way left to take the line down again by itself.

How is the source expected to be quiet by the time enable_irq() runs? Should phy_free_interrupt() skip the re-enable when phy_disable_interrupts() fails? Or should it make sure the latched status is acked first? Or is there a different way to hand the depth back without unmasking the line under a handler that cannot quiet it?

[Severity: Medium]
Can this enable_irq() leave a still-asserted level interrupt unmasked for the other users of a shared line?

phy_request_interrupt() always requests the line with IRQF_ONESHOT | IRQF_SHARED. The postponed branch of phy_interrupt() returns IRQ_HANDLED without calling drv->handle_interrupt(), so nothing reads or acks the latched PHY status.

phy_free_interrupt() ignores the return value of phy_disable_interrupts(). That call is the only step here that deasserts the PHY's INT output. For example, marvell_config_intr() returns right after a failed IMASK write, before it acks the event register:

drivers/net/phy/marvell.c:marvell_config_intr() {
    ...
	} else {
		err = phy_write(phydev, MII_M1011_IMASK,
				MII_M1011_IMASK_CLEAR);
		if (err)
			return err;

		err = marvell_ack_interrupt(phydev);
	}
    ...
}

In that case the sequence would be:

phy_free_interrupt()

  phy_disable_interrupts()       <- fails, INT stays asserted
  enable_irq(phydev->irq)        <- line unmasked
  free_irq(phydev->irq, phydev)  <- removes only the PHY's action

If other sharers remain, free_irq() leaves the line enabled, and their handlers return IRQ_NONE. Could the spurious IRQ detector then disable the line for every sharer ("irq N: nobody cared")?

__setup_irq() only clears IRQS_SPURIOUS_DISABLED when another shared handler is requested. Resume would not end that outage.

Before this patch, a disabled shared line stayed masked until mdio_bus_phy_resume(). On the suspended_by_mdio_bus path, phy_init_hw() quiets the source over MDIO before that enable_irq() runs.

The commit message says:

On a level-triggered line whose source phy_disable_interrupts() did not clear, the interrupt then fires until free_irq(), in the same call, removes the handler.

Does that hold only when the PHY is the only user of the line? The free_irq() kerneldoc also says that for shared lines "the caller must ensure the interrupt is disabled on the card it drives".

drivers/net/phy/phy_device.c static __maybe_unused int mdio_bus_phy_suspend(struct device *dev) · line 373

[Severity: Medium]
This isn't a bug introduced by this patch, but what happens to irq_suspended if phy_suspend() fails at the end of mdio_bus_phy_suspend()?

drivers/net/phy/phy_device.c:mdio_bus_phy_suspend() {
    ...
	if (!mdio_bus_phy_may_suspend(phydev))
		return 0;

	phydev->suspended_by_mdio_bus = 1;

	return phy_suspend(phydev);
}

phy_suspend() can fail with -EBUSY when phydev->wol_enabled is set and the driver lacks PHY_ALWAYS_CALL_SUSPEND. It can also return an error from the driver's ->suspend(). In either case the PM core skips the resume callback for this device, so nothing calls mdio_bus_phy_resume_irq() or phy_start_machine().

Would every later PHY interrupt then take the postponed path in phy_interrupt(), with the line disabled once and irq_rerun left at 1? And would link handling stay stopped until a later suspend/resume cycle succeeds?

This behaviour dates back to 1758bde2e4aa. A later patch in this series, "net: phy: stop postponing PHY interrupts when a PHY PM callback fails", adds the unwind. On a phy_suspend() failure it calls mdio_bus_phy_resume_irq() and phy_start_machine(), and it sets suspended_by_mdio_bus only on success.

drivers/net/phy/phy_device.c static __maybe_unused int mdio_bus_phy_resume(struct device *dev) · line 441

[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

drivers/net/phy/phy_device.c static bool mdio_bus_phy_may_suspend(struct phy_device *phydev) · line 386

[Severity: High]
phy_irq_wake_source() runs from mdio_bus_phy_suspend(), before the MAC's suspend callback. For a mac_managed_pm PHY, that callback still changes the link afterwards. The mac_managed_pm check below only covers the case without Wake-on-LAN. With Wake-on-LAN enabled we return PHY_IRQ_WAKE_SYSTEM, so any link interrupt the MAC causes itself makes phy_interrupt() call pm_system_wakeup() and abort the suspend.

I can see two ways to get there.

WoL in the PHY on stmmac: stmmac_suspend() calls phylink_suspend(), which goes through phylink_stop() -> phy_stop() -> phy_suspend(). With wol_enabled set and no PHY_ALWAYS_CALL_SUSPEND, phy_suspend() returns -EBUSY without calling the driver's suspend, so the link interrupt stays unmasked. phylink_suspend() then calls phylink_speed_down(), since stmmac sets wol_phy_speed_ctrl, and phy_speed_down() restarts autonegotiation to drop to 10Mbps. The link goes down while irq_suspended is set and irq_wake is PHY_IRQ_WAKE_SYSTEM.

WoL in the MAC only, fec for example: netdev->ethtool->wol_enabled is set while phy_wol is false, and fec_suspend() calls phy_stop() and gates its clocks. phy_suspend() again leaves the link interrupt of the PHY enabled. rtl8211f_suspend(), for one, neither calls genphy_suspend() nor masks the link status interrupt when wol_enabled is set without driver-model wakeup. So the link-down interrupt is reported as a system wakeup, although the PHY interrupt is not a wakeup source here at all; the MAC is.

Before this patch these PHYs did not have irq_suspended set during the MAC's suspend callback, so neither case could abort suspend. This looks like a regression for anyone using WoL behind a MAC that manages PHY PM. Did the Helios64 Wake-on-LAN test go through the speed downshift in phylink_suspend()?

I'm not sure what the right fix is, so two questions. Should the mac_managed_pm case count only phy_wol, and not the ethtool flag that may describe MAC WoL? And for PHY WoL, can the wakeup be armed only after the MAC has finished suspending, for example from a later suspend phase, so that link changes the MAC makes itself are not reported?

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