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

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

[Igor Velkov via B4 Relay]

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.

Open question on 2/2: a postponed interrupt leaves the PHY line
disabled until resume. With Wake-on-LAN, v3 still signals a wakeup
from it, so suspend would abort when the link changes after
phy_stop(): phylink_suspend() calls phylink_speed_down() after
phylink_stop() when the PHY is the wake source (stmmac sets
wol_phy_speed_ctrl), and a PHY that keeps its link interrupt enabled
raises one there. RTL8211F masks it in suspend when its wakeup is
enabled, so Helios64 woke on a magic packet 3 of 3 times. Never
signalling a
wakeup would let the system sleep with a wake-armed PHY line left
disabled. Is the v3 behaviour acceptable, or should the MAC set
irq_suspended once it has stopped the PHY (phylink_suspend() plus a
phylib helper for MACs without phylink)?

Changes in v3:
- with mac_managed_pm, a postponed interrupt signals a wakeup event only
  with Wake-on-LAN; with MAC power/wakeup enabled and no WoL, v2 aborted
  s2idle on Helios64 3 of 3 times, v3 0 of 3 (reported by Sashiko)
- phy_free_interrupt(): stop postponing and re-enable the line before
  free_irq(), not after it (reported by Sashiko)
- mdio_bus_phy_suspend() comment narrowed to the MDIO bus (reported by
  Sashiko)
- patch 1: Andrew suggested taking phydev->lock in phy_interrupt();
  the commit message says why that does not close the race
- tests rerun on v3

Not changed: the replay does not reach a nested threaded PHY interrupt
(irq_wake_thread() has no thread to wake). That code predates this
series; v3 sends MAC-managed PHYs through it as well.

Link to v2: https://lore.kernel.org/r/20261026045343.837889-1-iav@iav.lv

Signed-off-by: Igor Velkov <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        | 21 +++++++++++++++++---
 drivers/net/phy/phy_device.c | 46 +++++++++++++++++++++++++++-----------------
 include/linux/phy.h          | 10 ++++++----
 3 files changed, 52 insertions(+), 25 deletions(-)
---
base-commit: fcc00b15cb21a6de76826e5012b2c758505f5e65
change-id: 20261029-b4-phylib-mac-pm-irq-v3-f656d4e1c64f

Best regards,
-- 
Igor Velkov <iav@iav.lv>

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

[Igor Velkov via B4 Relay]

From: Igor Velkov <iav@iav.lv>

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.

Taking phydev->lock in phy_interrupt() alone does not close it:
phy_suspend() stores suspended in the same word and cannot hold the
lock across phy_may_wakeup(), which may take it, and phy_init_hw() and
mdio_bus_phy_suspend() write the word unlocked too. Locking every
writer means auditing about 200 stores to that word in 63 files, 19 of
them MAC drivers, and one missed now or added later reopens the race.

Make both flags bools.

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        | 2 +-
 drivers/net/phy/phy_device.c | 6 +++---
 include/linux/phy.h          | 5 +++--
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index fce9bc7be330..9af303d60bd6 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1430,7 +1430,7 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 				pm_wakeup_dev_event(parent, 0, true);
 		}
 
-		phydev->irq_rerun = 1;
+		phydev->irq_rerun = true;
 		disable_irq_nosync(irq);
 		return IRQ_HANDLED;
 	}
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 5d4508bcfbe9..79dba43d703a 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 = 1;
+		phydev->irq_suspended = true;
 		synchronize_irq(phydev->irq);
 	}
 
@@ -394,14 +394,14 @@ static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
 		return ret;
 no_resume:
 	if (phy_interrupt_is_valid(phydev)) {
-		phydev->irq_suspended = 0;
+		phydev->irq_suspended = false;
 		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;
+			phydev->irq_rerun = false;
 			enable_irq(phydev->irq);
 			irq_wake_thread(phydev->irq, phydev);
 		}
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 21afc2082b17..0e0066186ba2 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;
+	/* Written from the IRQ thread and PM callbacks: not bitfields */
+	bool irq_suspended;
+	bool irq_rerun;
 
 	unsigned default_timestamp:1;
 

-- 
2.43.0

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

[Igor Velkov via B4 Relay]

From: Igor Velkov <iav@iav.lv>

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. An interrupt in that window signals a wakeup event only if
Wake-on-LAN is enabled: the MAC powers the PHY down itself, and the
link interrupt that follows must not abort suspend just because the MAC
device may wake the system.

MAC drivers that close the interface in suspend free the interrupt
before the PHY resumes. The PHY is still reachable there, so
phy_free_interrupt() stops postponing and re-enables a line left
disabled by a postponed interrupt before it frees its handler.

Tested on Helios64 with 7.3, s2idle with the link up:
- no Wake-on-LAN, GMAC clocks off in suspend: 10 of 10 cycles suspended
  and resumed; without this series the same test hung or reset the
  board after the link went down 10 of 10 times;
- MAC power/wakeup enabled, no Wake-on-LAN: 3 of 3 slept;
- Wake-on-LAN in the PHY: a magic packet woke the board 3 of 3 times.

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        | 19 +++++++++++++++++--
 drivers/net/phy/phy_device.c | 44 +++++++++++++++++++++++++++-----------------
 include/linux/phy.h          |  5 +++--
 3 files changed, 47 insertions(+), 21 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 9af303d60bd6..c3863a72a5da 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1421,12 +1421,16 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 
 		if (netdev) {
 			struct device *parent = netdev->dev.parent;
+			/* Without WoL, a MAC that manages PHY PM takes the
+			 * link down itself: that interrupt is not a wakeup.
+			 */
+			bool may_wake = !phydev->mac_managed_pm;
 
 			if (netdev->ethtool->wol_enabled)
 				pm_system_wakeup();
-			else if (device_may_wakeup(&netdev->dev))
+			else if (may_wake && device_may_wakeup(&netdev->dev))
 				pm_wakeup_dev_event(&netdev->dev, 0, true);
-			else if (parent && device_may_wakeup(parent))
+			else if (may_wake && parent && device_may_wakeup(parent))
 				pm_wakeup_dev_event(parent, 0, true);
 		}
 
@@ -1507,6 +1511,17 @@ EXPORT_SYMBOL(phy_request_interrupt);
 void phy_free_interrupt(struct phy_device *phydev)
 {
 	phy_disable_interrupts(phydev);
+
+	/* The PHY is reachable: stop postponing, and while our handler is still
+	 * registered re-enable a line that a postponed interrupt left disabled.
+	 */
+	phydev->irq_suspended = false;
+	synchronize_irq(phydev->irq);
+	if (phydev->irq_rerun) {
+		phydev->irq_rerun = false;
+		enable_irq(phydev->irq);
+	}
+
 	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 79dba43d703a..7d1486185070 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 MDIO bus powered down.
 	 */
 	if (phy_interrupt_is_valid(phydev)) {
 		phydev->irq_suspended = true;
 		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 = false;
+	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 = false;
+		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 = false;
-		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 = false;
-			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);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 0e0066186ba2..bb62a80d6a30 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -649,8 +649,9 @@ struct phy_oatc14_sqi_capability {
  * @mdix_ctrl: User setting of crossover
  * @pma_extable: Cached value of PMA/PMD Extended Abilities Register
  * @interrupts: Flag interrupts have been enabled
- * @irq_suspended: Flag indicating PHY is suspended and therefore interrupt
- *                 handling shall be postponed until PHY has resumed
+ * @irq_suspended: Flag indicating a system sleep transition in progress, so
+ *                 interrupt handling shall be postponed until
+ *                 mdio_bus_phy_resume()
  * @irq_rerun: Flag indicating interrupts occurred while PHY was suspended,
  *             requiring a rerun of the interrupt handler after resume
  * @default_timestamp: Flag indicating whether we are using the phy

-- 
2.43.0

Re: [PATCH net v3 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 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · 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).

- [Medium] phy_interrupt() now decides whether a postponed interrupt on a
  MAC-managed-PM PHY is a wakeup by checking only…
- [Medium] The patch adds a second, unsynchronized consumer of irq_rerun
  and writer of irq_suspended in phy_free_interrupt() (phy.c:1518-1523).

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch.

--

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

This commit makes mdio_bus_phy_suspend() set irq_suspended for PHYs whose
PM is managed by the MAC. It also makes mdio_bus_phy_resume() replay
postponed interrupts for those PHYs. For these PHYs, phy_interrupt() no
longer reports non-WoL wakeup events, and phy_free_interrupt() now clears
irq_suspended and re-enables a line left disabled by a postponed interrupt
before freeing it.

> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 9af303d60bd6b..c3863a72a5dab 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -1421,12 +1421,16 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
>  
>  		if (netdev) {
>  			struct device *parent = netdev->dev.parent;
> +			/* Without WoL, a MAC that manages PHY PM takes the
> +			 * link down itself: that interrupt is not a wakeup.
> +			 */
> +			bool may_wake = !phydev->mac_managed_pm;
>  
>  			if (netdev->ethtool->wol_enabled)
>  				pm_system_wakeup();
> -			else if (device_may_wakeup(&netdev->dev))
> +			else if (may_wake && device_may_wakeup(&netdev->dev))
>  				pm_wakeup_dev_event(&netdev->dev, 0, true);
> -			else if (parent && device_may_wakeup(parent))
> +			else if (may_wake && parent && device_may_wakeup(parent))
>  				pm_wakeup_dev_event(parent, 0, true);
>  		}

[Severity: Medium]
For mac_managed_pm PHYs, does this leave netdev->ethtool->wol_enabled as
the only thing that can turn a postponed interrupt into a wakeup?

Elsewhere, phylib also counts the PHY's own wakeup state as WoL.
phy_suspend() does:

	phydev->wol_enabled = phy_may_wakeup(phydev) ||
			      (netdev && netdev->ethtool->wol_enabled);

and phy_may_wakeup() checks the PHY device itself:

	if (phy_can_wakeup(phydev))
		return device_may_wakeup(&phydev->mdio.dev);

	return phy_drv_wol_enabled(phydev);

The @wol_enabled kernel-doc in include/linux/phy.h also says "the PHY or
the attached MAC". mdio_bus_phy_may_suspend() notes that not all network
drivers set the netdev wol_enabled flag.

Before this patch, interrupts on these PHYs went straight to the PHY
driver's handler. Some of those handlers report their own wake event. For
example, rtl8211f_handle_interrupt() calls pm_wakeup_event() on PME, and
the broadcom and air_an8801 drivers do something similar.

Suppose phy_may_wakeup() is true but netdev->ethtool->wol_enabled is
false, and a WoL interrupt arrives between mdio_bus_phy_suspend() and the
noirq phase. The interrupt is postponed, the line is disabled, and no
wakeup is reported. Could that wake event be lost?

The ethtool set_wol paths always set the netdev flag, so this would need
WoL configured some other way, for example by firmware or by a driver at
probe time. Would also checking phy_may_wakeup(phydev) here match the rest
of phylib?

The new comment "Without WoL, a MAC that manages PHY PM takes the link
down itself" also only covers the netdev flag.

[ ... ]

> @@ -1507,6 +1511,17 @@ EXPORT_SYMBOL(phy_request_interrupt);
>  void phy_free_interrupt(struct phy_device *phydev)
>  {
>  	phy_disable_interrupts(phydev);
> +
> +	/* The PHY is reachable: stop postponing, and while our handler is still
> +	 * registered re-enable a line that a postponed interrupt left disabled.
> +	 */
> +	phydev->irq_suspended = false;
> +	synchronize_irq(phydev->irq);
> +	if (phydev->irq_rerun) {
> +		phydev->irq_rerun = false;
> +		enable_irq(phydev->irq);
> +	}
> +
>  	free_irq(phydev->irq, phydev);
>  }

[Severity: Medium]
Can this race with mdio_bus_phy_suspend() or mdio_bus_phy_resume_irq()?

irq_suspended and irq_rerun are plain bools with no shared lock.
synchronize_irq() only orders each writer against the IRQ thread.

phy_free_interrupt() can be reached through phy_disconnect() from
kernel-internal close/reopen paths. The PM core does not serialize those
paths with the PHY's PM callbacks.

One example is stmmac_reset_subtask(), which runs dev_close() and
dev_open() under RTNL from priv->wq. That workqueue comes from
create_singlethread_workqueue(), so it is not freezable. The PHY PM
callbacks don't take RTNL.

On resume, both sides could see irq_rerun set:

CPU1 mdio_bus_phy_resume_irq()      CPU2 phy_free_interrupt()
if (phydev->irq_rerun) {            if (phydev->irq_rerun) {
    phydev->irq_rerun = false;          phydev->irq_rerun = false;
    enable_irq(phydev->irq);            enable_irq(phydev->irq);
    irq_wake_thread(...);               ...
                                        free_irq(phydev->irq, phydev);

Would the second enable_irq() trigger an "Unbalanced enable for IRQ"
warning? Could irq_wake_thread() also run for a line that is being freed?

On suspend, the order could be:

CPU2 phy_free_interrupt()
    phydev->irq_suspended = false;
    synchronize_irq(phydev->irq);
    /* irq_rerun is false here */
                                    CPU1 mdio_bus_phy_suspend()
                                        phydev->irq_suspended = true;
    /* shared-line interrupt */
    phy_interrupt() (IRQ thread)
        phydev->irq_rerun = true;
        disable_irq_nosync(irq);
    free_irq(phydev->irq, phydev);

Would that leave an IRQF_SHARED line disabled for its other users? The
stale irq_rerun would also make a later mdio_bus_phy_resume_irq() call
enable_irq() on a newly requested line.

Before this patch, nothing on the PM side wrote these fields for
mac_managed_pm PHYs, so this interleaving could not happen for them.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 79dba43d703a2..7d14861850704 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)

[ ... ]

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

[ ... ]

> @@ -364,13 +366,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 to
irq_suspended when phy_suspend() fails here?

For PHYs without mac_managed_pm, mdio_bus_phy_suspend() sets
irq_suspended and stops the state machine. It then returns
phy_suspend(phydev) without undoing either one on error.

phy_suspend() can return -EBUSY:

	if (phydev->wol_enabled && !(phydrv->flags & PHY_ALWAYS_CALL_SUSPEND))
		return -EBUSY;

mdio_bus_phy_may_suspend() doesn't check phy_may_wakeup() when a netdev
is attached, so that case reaches this call. MDIO errors from
drv->suspend() are passed back as well.

When the callback fails, device_suspend() in drivers/base/power/main.c
never sets dev->power.is_suspended. device_resume() then skips the
device:

	if (!dev->power.is_suspended)
		goto Complete;

So mdio_bus_phy_resume() never runs for this PHY. Does irq_suspended stay
true after that? If so, the first PHY interrupt afterwards would disable
the line for good, and the state machine would stay stopped.

[ ... ]

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