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

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

[Igor Velkov]

When the MAC driver manages PHY PM, phylib never sets irq_suspended, and
a PHY interrupt during the sleep transition runs the PHY driver's
handler while the MDIO bus may be powered down. On Helios64 (dwmac-rk)
this hangs the board on suspend as well as on resume.

Patch 1 moves the two flags out of a bitfield they share with the link
state written under phydev->lock; patch 2 sets irq_suspended for these
PHYs too.

mac_managed_pm (fba863b81604) means the MAC driver suspends and resumes
the PHY; it does not cover the PHY interrupt. Neither phylink nor any
MAC driver touches irq_suspended, and resume_device_irqs() runs at the
end of the noirq phase, before the normal resume callbacks of the MAC,
the MDIO bus and the PHY, so callback order does not help the
interrupt. v1 asked whether phylink_suspend()/phylink_resume() would be
the better place; v2 keeps the fix in phylib, which also covers MAC
drivers that use phylib without phylink.

Changes in v2:
- new patch 1: bools instead of bitfields (Sashiko); sizeof(struct
  phy_device) is unchanged on arm64
- phy_free_interrupt(): re-enable a shared line after free_irq()
  instead of dropping irq_rerun before it (Sashiko)
- reworded the comment in mdio_bus_phy_suspend() (Andrew)
- described and tested the suspend-side window, noted the wakeup
  event; the v1 tests with Wake-on-LAN (Helios64 20/20, ODROID-HC4
  10/10) were on v1 code
- W=1 allmodconfig/allyesconfig: no new warnings

Not tested: patch 1 alone, and a vanilla net kernel on hardware; the
tests ran on an Armbian 7.3 kernel whose Helios64 DT describes the PHY
interrupt.

Link to v1: https://lore.kernel.org/r/20261002043548.1302145-1-iav@iav.lv

Igor Velkov (2):
  net: phy: move irq_suspended and irq_rerun out of the bitfield
  net: phy: postpone PHY interrupts during sleep with MAC-managed PM

 drivers/net/phy/phy.c        |  9 ++++++++
 drivers/net/phy/phy_device.c | 44 ++++++++++++++++++++++--------------
 include/linux/phy.h          |  5 ++--
 3 files changed, 39 insertions(+), 19 deletions(-)


base-commit: 7076f684d5132e6a6145e1da2fa74771ecd052e5
-- 
2.43.0

[PATCH net v2 1/2] net: phy: move irq_suspended and irq_rerun out of the bitfield

[Igor Velkov]

irq_suspended and irq_rerun share a word with link, autoneg_complete
and interrupts. phy_interrupt() sets irq_rerun from the IRQ thread
without phydev->lock, while the state machine writes link and
autoneg_complete under it and phy_request_interrupt()/
phy_free_interrupt() write interrupts, so one read-modify-write can
undo another. A lost irq_rerun leaves the PHY interrupt disabled after
resume, as phy_interrupt() has already disabled the line.

The next patch sets irq_suspended for PHYs whose MAC driver manages
PHY PM; with stmmac that happens before the MAC stops the PHY, which
widens the window.

Make both flags bools.

Fixes: 1758bde2e4aa ("net: phy: Don't trigger state machine while in suspend")
Signed-off-by: Igor Velkov <iav@iav.lv>
---
 include/linux/phy.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 21afc2082b17..ec2c4d9a15d3 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -726,8 +726,9 @@ struct phy_device {
 
 	/* Interrupts are enabled */
 	unsigned interrupts:1;
-	unsigned irq_suspended:1;
-	unsigned irq_rerun:1;
+	/* irq_rerun is set by the IRQ thread: not bitfields */
+	bool irq_suspended;
+	bool irq_rerun;
 
 	unsigned default_timestamp:1;
 
-- 
2.43.0

Re: [PATCH net v2 1/2] net: phy: move irq_suspended and irq_rerun out of the bitfield

[Andrew Lunn]

On Mon, Oct 26, 2026 at 06:53:42AM +0200, Igor Velkov wrote:
> irq_suspended and irq_rerun share a word with link, autoneg_complete
> and interrupts. phy_interrupt() sets irq_rerun from the IRQ thread
> without phydev->lock

As you say, it is an IRQ thread. Because interrupt handlers might need
to access the PHY, which is a blocking operation,
request_threaded_irq() is used in phy_request_interrupt(). So you can
just take the mutex and make the RMW thread atomic.

     Andrew

Re: [PATCH net v2 1/2] net: phy: move irq_suspended and irq_rerun out of the bitfield

[Igor Velkov]

On Mon, Oct 26, 2026, Andrew Lunn wrote:
> As you say, it is an IRQ thread. Because interrupt handlers might need
> to access the PHY, which is a blocking operation,
> request_threaded_irq() is used in phy_request_interrupt(). So you can
> just take the mutex and make the RMW thread atomic.

The lock in phy_interrupt() only orders against writers that hold it.
phy_suspend() writes wol_enabled and suspended unlocked, e.g. from
phy_stop() in the MAC's suspend while the link-down interrupt may set
irq_rerun, and it cannot take the lock: phy_may_wakeup() can call
phy_ethtool_get_wol(), which takes it. mdio_bus_phy_suspend() and
phy_config_interrupt() write that word unlocked too. As bools,
irq_suspended and irq_rerun share it with none of them.
Would you still prefer the lock?

[PATCH net v2 2/2] 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 and
phy_interrupt() runs the PHY driver's handler at any point of the sleep
transition. If the MAC driver powers the MDIO bus down, the MDIO access
in that handler can stall the CPU. This happens on both sides:

- suspend: the MAC's suspend callback stops the PHY, the link goes
  down, and the handler runs while the MAC powers down;
- resume: resume_device_irqs() re-enables a pending interrupt before
  the MAC resumes.

On Helios64 (dwmac-rk, RTL8211F with its interrupt line described in
DT) both hang the board: the handler reads INSR while the GMAC clocks
are off, after rk_gmac_suspend() or before rk_gmac_resume().

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. As for other PHYs, an interrupt in that window now signals a
wakeup event if Wake-on-LAN is enabled or the device may wake the
system.

MAC drivers that close the interface in suspend free the interrupt
before the PHY resumes, so phy_free_interrupt() re-enables a line left
disabled by a postponed interrupt if other handlers still share it.

Tested on Helios64 with 7.3, s2idle without Wake-on-LAN, GMAC clocks
off in suspend: 10 of 10 cycles got through suspend, 4 of them then
hit a separate PCIe error on resume. The same kernel without these
changes hung or reset after the link went down in 10 of 10.

Fixes: 1758bde2e4aa ("net: phy: Don't trigger state machine while in suspend")
Signed-off-by: Igor Velkov <iav@iav.lv>
---
 drivers/net/phy/phy.c        |  9 ++++++++
 drivers/net/phy/phy_device.c | 44 ++++++++++++++++++++++--------------
 2 files changed, 36 insertions(+), 17 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index fce9bc7be330..00aa48e807a5 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1508,6 +1508,15 @@ void phy_free_interrupt(struct phy_device *phydev)
 {
 	phy_disable_interrupts(phydev);
 	free_irq(phydev->irq, phydev);
+
+	/* Undo the disable_irq_nosync() of a postponed interrupt if the line
+	 * is still shared; otherwise the next irq_startup() resets the depth.
+	 */
+	if (phydev->irq_rerun) {
+		phydev->irq_rerun = 0;
+		if (irq_has_action(phydev->irq))
+			enable_irq(phydev->irq);
+	}
 }
 EXPORT_SYMBOL(phy_free_interrupt);
 
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 5d4508bcfbe9..e2807e08e436 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -336,18 +336,20 @@ 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.
+	 * This holds when the MAC manages PHY PM too: an interrupt can still
+	 * fire while the MAC has the PHY or the MDIO bus powered down.
 	 */
 	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
@@ -364,13 +366,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;
@@ -393,19 +415,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 v2 2/2] 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: 0 · Medium: 2 (1) · Low: 1 (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] For PHYs whose PM is managed by the MAC driver
  (mac_managed_pm), mdio_bus_phy_suspend() now sets irq_suspended before…
- [Medium] When a PHY's interrupt is a nested threaded IRQ (PHY INT
  routed through an I2C/SPI GPIO expander or MFD irqchip that uses…
- [Low] The new depth fixup in phy_free_interrupt() is an unsynchronized
  check-then-act.

Pre-existing issues:
- [Medium] For a non-MAC-managed PHY, mdio_bus_phy_suspend() sets
  irq_suspended and stops the state machine, then returns the result of…
- [Low] Topology: a PHY on another MAC's MDIO bus, linked to its consumer
  MAC via device_link_add() in phy_attach_direct().

--

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. It replays the postponed interrupt from
mdio_bus_phy_resume() through a new mdio_bus_phy_resume_irq() helper. It
also makes phy_free_interrupt() re-enable a line left disabled by a
postponed interrupt if other handlers still share it.

> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index fce9bc7be3304..00aa48e807a5e 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -1508,6 +1508,15 @@ void phy_free_interrupt(struct phy_device *phydev)
>  {
>  	phy_disable_interrupts(phydev);
>  	free_irq(phydev->irq, phydev);
> +
> +	/* Undo the disable_irq_nosync() of a postponed interrupt if the line
> +	 * is still shared; otherwise the next irq_startup() resets the depth.
> +	 */
> +	if (phydev->irq_rerun) {
> +		phydev->irq_rerun = 0;
> +		if (irq_has_action(phydev->irq))
> +			enable_irq(phydev->irq);
> +	}

[Severity: Low]
Is this check-then-act safe if another driver calls request_irq() on the
same shared line at the same time?

By the time irq_has_action() runs, free_irq() has already dropped
desc->request_mutex and desc->lock. irq_has_action() only takes
rcu_read_lock().

If the PHY's action was the last one on the line, this interleaving
looks possible:

CPU1                                 CPU2
phy_free_interrupt()
  free_irq()
    __free_irq()
      irq_shutdown()  /* depth++ */
                                     request_threaded_irq(IRQF_SHARED)
                                       irq_startup()
                                         desc->depth = 0;
  irq_has_action()    /* true */
  enable_irq()
    __enable_irq()    /* depth == 0 */
      WARN(1, KERN_WARNING "Unbalanced enable for IRQ %d\n", ...)

__enable_irq() does not change the depth when it is 0, so this seems
limited to a warning. The window is also narrow: irq_rerun must still be
set when the interface is closed during suspend, before
mdio_bus_phy_resume() runs.

The opposite order is harmless, because the depth++ in irq_shutdown()
absorbs the enable.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 5d4508bcfbe99..e2807e08e4365 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -336,18 +336,20 @@ 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.
> +	 * This holds when the MAC manages PHY PM too: an interrupt can still
> +	 * fire while the MAC has the PHY or the MDIO bus powered down.
>  	 */

[Severity: Low]
This isn't a regression introduced by this patch, but does this comment
claim more than the change covers?

When a PHY sits on another MAC's MDIO bus, phy_attach_direct() links it
to its consumer MAC:

	if (dev && phydev->mdio.bus->parent && dev->dev.parent != phydev->mdio.bus->parent)
		phydev->devlink = device_link_add(dev->dev.parent, &phydev->mdio.dev,
						  DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS);

In that topology the consumer MAC suspends before the PHY and resumes
after it. The handler can then still run, or be replayed from
mdio_bus_phy_resume_irq(), while the consumer MAC has cut the PHY's
regulator or reference clock (FEC, for example).

The MDIO bus owner is still an ancestor of the PHY, so the bus stall case
is covered.

Reads of an unpowered PHY over a live bus were also possible before this
patch. The pending interrupt was replayed from resume_device_irqs() in
dpm_resume_noirq(), which is even earlier than now.

Could the comment be narrowed to the MDIO bus case?

>  	if (phy_interrupt_is_valid(phydev)) {
>  		phydev->irq_suspended = 1;
>  		synchronize_irq(phydev->irq);
>  	}
>  
> +	if (phydev->mac_managed_pm)
> +		return 0;
> +

[Severity: Medium]
Can this abort system suspend for MAC-managed PHYs when wakeup is enabled
on the MAC device but no ethtool WoL is configured?

The PHY device suspends before its MDIO bus and the MAC, so irq_suspended
is now set before the MAC's suspend callback runs. With stmmac and
priv->wolopts == 0, the MAC then powers the PHY down itself:

stmmac_suspend()
  phylink_suspend(priv->phylink, false)
    phylink_stop()
      phy_stop()
        phy_suspend()

phy_suspend() keeps the PHY powered only when phy_may_wakeup() is true or
netdev->ethtool->wol_enabled is set. Here the PHY powers down and raises
the link down interrupt that the commit message describes.

That interrupt now goes through the postpone branch of phy_interrupt():

	else if (device_may_wakeup(&netdev->dev))
		pm_wakeup_dev_event(&netdev->dev, 0, true);
	else if (parent && device_may_wakeup(parent))
		pm_wakeup_dev_event(parent, 0, true);

When pmt is set, stmmac_dvr_probe() calls
device_set_wakeup_capable(priv->device, 1). If power/wakeup is then
enabled on that device through sysfs, this hard wakeup event makes
pm_wakeup_pending() abort the suspend with -EBUSY. That would happen on
every attempt made while the link is up.

Core-managed PHYs avoid this, because mdio_bus_phy_may_suspend() keeps
the PHY powered in that case:

	if (netdev->dev.parent && device_may_wakeup(netdev->dev.parent))
		return false;

MAC-managed PHYs never go through that check. Before this patch, the
interrupt was handled in place and suspend went ahead.

>  	/* 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

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. For
PHYs that are not MAC-managed, mdio_bus_phy_suspend() sets irq_suspended
and stops the state machine. It then returns the result of phy_suspend()
without undoing either step if that call fails:

	phydev->suspended_by_mdio_bus = 1;

	return phy_suspend(phydev);

The PM core skips resume for a device whose suspend failed. Does that
leave irq_suspended set? If so, phy_interrupt() would postpone every
later interrupt and leave the line disabled.

For mac_managed_pm PHYs, the ones newly affected here, this function
always returns 0 after setting the flag. So this patch adds no new
failure path.

[ ... ]

> @@ -364,13 +366,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);
> +	}
> +}

[Severity: Medium]
Does this replay work when the PHY interrupt is a nested threaded IRQ?
An example is PHY INT routed through an I2C or SPI GPIO expander that
uses handle_nested_irq().

__setup_irq() does not create a thread for nested IRQs:

	if (new->thread_fn && !nested) {
		ret = setup_irq_thread(new, irq, false);

So action->thread is NULL, and irq_wake_thread() does nothing.

The postpone branch in phy_interrupt() also returned without calling
handle_interrupt(). The PHY's interrupt status was therefore never read
or acked:

	phydev->irq_rerun = 1;
	disable_irq_nosync(irq);
	return IRQ_HANDLED;

enable_irq() only resends if IRQS_PENDING was set. Take a parent that
accepts only edge triggers and filters masked sources, in the style of
pca953x. The PHY INT would stay asserted with no new edge, and link
interrupts could stay dead after resume.

The replay code itself predates this patch. However, mac_managed_pm PHYs
with a valid IRQ now go through it. The link goes down while the MAC
suspends, as the commit message describes, so a postponed interrupt is
likely on every suspend cycle.

[ ... ]

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