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

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

[Igor Velkov]

mdio_bus_phy_suspend() and mdio_bus_phy_resume() return early when the
MAC driver manages PHY PM, so the PHY never gets irq_suspended. A PHY
interrupt that wakes the system fires as soon as resume_device_irqs()
re-enables the line, before the MAC driver resumes, and phy_interrupt()
runs the PHY driver's handler at once. If the MDIO bus was powered down
in suspend, the MDIO access in that handler stalls the CPU.

On Helios64 (dwmac-rk, RTL8211F with an interrupt line, Wake-on-LAN in
the PHY) the wake interrupt reads INSR while the GMAC clocks are off,
and the board hangs within a few suspend cycles.

Set irq_suspended for these PHYs too. The PHY device is a child of its
MDIO bus, so mdio_bus_phy_resume() runs after the bus is back (with
stmmac, after the MAC) and replays the postponed interrupt then.
phy_suspend(), phy_resume() and the state machine stay with the MAC
driver.

Drop a pending rerun when the interrupt is freed: MAC drivers that close
the interface in suspend free and request it again before the PHY
resumes.

Tested on Helios64 with 7.3-rc5, the GMAC powered down in suspend and
Wake-on-LAN in the PHY: 20 of 20 magic-packet wakes. Without it the
same setup hung within 1 to 7 cycles in 14 of 16 runs, some on
instrumented builds; one run passed 25 cycles. ODROID-HC4
(dwmac-meson8b, RTL8211F with an interrupt line): 10 of 10.

Fixes: 1758bde2e4aa ("net: phy: Don't trigger state machine while in suspend")
Assisted-by: LLM
Signed-off-by: Igor Velkov <iav@iav.lv>
---
This replaces the dwmac-rk change that kept the GMAC powered instead:
https://lore.kernel.org/r/20260930-dwmac-rk-phy-wol-v1-1-9fdc50bd9ae4@iav.lv

Build-tested on net: allmodconfig and allyesconfig with W=1, no new
warnings.

 drivers/net/phy/phy.c        |  2 ++
 drivers/net/phy/phy_device.c | 43 ++++++++++++++++++++++--------------
 2 files changed, 28 insertions(+), 17 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index fce9bc7be330..cd189ecee55e 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);
 }
 EXPORT_SYMBOL(phy_free_interrupt);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 94b2e85e00a3..948818964841 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -347,18 +347,19 @@ static __maybe_unused int mdio_bus_phy_suspend(struct device *dev)
 {
 	struct phy_device *phydev = to_phy_device(dev);
 
-	if (phydev->mac_managed_pm)
-		return 0;
-
 	/* 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.
+	 * The MDIO bus may be powered down even when the MAC manages PHY PM.
 	 */
 	if (phy_interrupt_is_valid(phydev)) {
 		phydev->irq_suspended = 1;
 		synchronize_irq(phydev->irq);
 	}
 
+	if (phydev->mac_managed_pm)
+		return 0;
+
 	/* We must stop the state machine manually, otherwise it stops out of
 	 * control, possibly with the phydev->lock held. Upon resume, netdev
 	 * may call phy routines that try to grab the same lock, and that may
@@ -375,13 +376,33 @@ static __maybe_unused int mdio_bus_phy_suspend(struct device *dev)
 	return phy_suspend(phydev);
 }
 
+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);
+
+	/* Rerun interrupts which were postponed by phy_interrupt()
+	 * because they occurred during the system sleep transition.
+	 */
+	if (phydev->irq_rerun) {
+		phydev->irq_rerun = 0;
+		enable_irq(phydev->irq);
+		irq_wake_thread(phydev->irq, phydev);
+	}
+}
+
 static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
 {
 	struct phy_device *phydev = to_phy_device(dev);
 	int ret;
 
-	if (phydev->mac_managed_pm)
+	if (phydev->mac_managed_pm) {
+		mdio_bus_phy_resume_irq(phydev);
 		return 0;
+	}
 
 	if (!phydev->suspended_by_mdio_bus)
 		goto no_resume;
@@ -404,19 +425,7 @@ static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
 	if (ret < 0)
 		return ret;
 no_resume:
-	if (phy_interrupt_is_valid(phydev)) {
-		phydev->irq_suspended = 0;
-		synchronize_irq(phydev->irq);
-
-		/* Rerun interrupts which were postponed by phy_interrupt()
-		 * because they occurred during the system sleep transition.
-		 */
-		if (phydev->irq_rerun) {
-			phydev->irq_rerun = 0;
-			enable_irq(phydev->irq);
-			irq_wake_thread(phydev->irq, phydev);
-		}
-	}
+	mdio_bus_phy_resume_irq(phydev);
 
 	if (phy_uses_state_machine(phydev))
 		phy_start_machine(phydev);
-- 
2.43.0

Re: [PATCH net] net: phy: postpone PHY interrupts during sleep with MAC-managed PM

[Igor Velkov]

+Cc Rafael, who acked the commit this fixes.

--
Igor Velkov

Re: [PATCH net] net: phy: postpone PHY interrupts during sleep with MAC-managed PM

[Igor Velkov]

Adding Rafael, who acked 1758bde2e4aa from the Fixes: tag; I missed him in Cc.

Re: [PATCH net] net: phy: postpone PHY interrupts during sleep with MAC-managed PM

[Andrew Lunn]

On Fri, Oct 02, 2026 at 07:35:48AM +0300, Igor Velkov wrote:
> mdio_bus_phy_suspend() and mdio_bus_phy_resume() return early when the
> MAC driver manages PHY PM

One obvious question. What exactly does it mean when the MAC driver
manages PHY PM. Maybe calling irq_suspend is part of that management?

> interrupt that wakes the system fires as soon as resume_device_irqs()
> re-enables the line, before the MAC driver resumes, and phy_interrupt()
> runs the PHY driver's handler at once. If the MDIO bus was powered down
> in suspend, the MDIO access in that handler stalls the CPU.

Ignoring MAC managed for a minute....

As far as i understand, suspend goes from leaf to root. Resume goes
from root to leaf. With WoL, we leave the PHY unsuspended, and the IRQ
controller as well. But the MDIO bus driver should get suspended and
then the MAC driver. On resume, the MAC driver is resumed, then the
MDIO bus driver, and then the PHY. At this point, during resume we can
handle the interrupt. It can then do MDIO transfers, since everything
towards the root should be up and running.

What exactly is going wrong with the ordering in your case?

     Andrew

Re: [PATCH net] net: phy: postpone PHY interrupts during sleep with MAC-managed PM

[Andrew Lunn]

>  	/* 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.
> +	 * The MDIO bus may be powered down even when the MAC manages PHY PM.

Just think about that statement. MAC manages _PHY_ PM. It is not MAC
managed _PHY and MDIO_ bus PM.

To me, this is key, because it means your whole approach is
wrong. Lets start with a clear definition what MAC manages PHY PM
is. Then we can decide on a solution.

    Andrew

---
pw-bot: cr

Re: [PATCH net] net: phy: postpone PHY interrupts during sleep with MAC-managed PM

[Igor Velkov]

On Sun, Oct 04, 2026 at 04:28:50PM +0200, Andrew Lunn wrote:
> One obvious question. What exactly does it mean when the MAC driver
> manages PHY PM. Maybe calling irq_suspend is part of that management?

The commit that added the flag, fba863b81604 ("net: phy: make PHY PM
ops a no-op if MAC driver manages PHY PM"), says the MAC drivers "take
care of suspending/resuming the PHY", so that "the MAC PM callbacks can
handle any dependency between MAC and PHY PM". The bug it fixed was
phy_init_hw() from mdio_bus_phy_resume() running after the MAC's
phy_start().

irq_suspended came later, in 1758bde2e4aa ("net: phy: Don't trigger
state machine while in suspend"), for a different window, and was
placed after the existing mac_managed_pm return; its commit message
does not mention mac_managed_pm. So nothing sets irq_suspended for
these PHYs: phylib returns early and no MAC driver touches the flag.

> What exactly is going wrong with the ordering in your case?

The callback order is fine; the interrupt does not wait for it.
resume_device_irqs() runs at the end of dpm_resume_noirq(), before the
early and normal phases. The wake interrupt is pending by then, so
phy_interrupt() runs at once, while stmmac_resume(), which powers the
GMAC up through rk_gmac_resume(), runs only in the normal phase.
1758bde2e4aa describes the same window: "between dpm_resume_noirq()
and mdio_bus_phy_resume()".

As for the MDIO bus: with stmmac it is the MAC's own registers;
mdio_bus_class has no PM callbacks, so the bus goes down with the MAC
in rk_gmac_suspend().

If the MAC should cover this window instead, irq_suspended could be
set and cleared in phylink_suspend()/phylink_resume(), but only
stmmac, lan78xx and asix call them; macb, axienet, am65-cpsw, ngbe and
the phylib-only drivers (fec, ravb, bcmgenet, cpsw, ...) would stay
uncovered. Which would you prefer?

Either way, v2 will reword the comment you quoted: the point is not the
MDIO bus but that the interrupt can fire before the MAC resumes the PHY.

--
Igor Velkov

Re: [PATCH net] net: phy: postpone PHY interrupts during sleep with MAC-managed PM

[netdev-bot+sashiko]

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