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

[PATCH net v5 0/4] 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.

mac_managed_pm hands phy_suspend(), phy_resume() and the state machine
to the MAC driver, since commit fba863b81604 ("net: phy: make PHY PM
ops a no-op if MAC driver manages PHY PM"); the PHY interrupt stays
with phylib, and so does postponing it.

Patches 1-3 fix the postponing that phylib applies to other PHYs, so
that patch 4 can extend it to MAC-managed ones. Patch 1 moves the two
flags out of a bitfield they share with the link state written under
phydev->lock. Patch 2 keeps the line of a postponed interrupt balanced
when an edge reruns the IRQ thread or phy_free_interrupt() runs during
the transition. Patch 3 undoes the postponing when phy_suspend() fails
and ends it when the PHY fails to resume. Patch 4 sets irq_suspended
for MAC-managed PHYs too and decides the wakeup event of a postponed
interrupt before the device that owns the MDIO bus suspends.

Not covered: MDIO accesses from closing or opening the interface during
the transition, and a MAC resume that fails to power the bus up. On a
shared line, a postponed PHY interrupt disables the line for its other
users until the PHY resumes; patch 4 extends that to MAC-managed PHYs. A
nested threaded PHY interrupt is not replayed (irq_wake_thread() has no
thread to wake), and mdio_bus_phy_may_suspend() does not ask
phy_may_wakeup() about an attached PHY.

With MAC-managed PM, a postponed interrupt reports a system wakeup only
when Wake-on-LAN is enabled in the PHY. A PHY that leaves its link
interrupt unmasked while armed for Wake-on-LAN can then abort suspend
when the MAC changes the link. Going by their suspend code, BCM54210E
leaves it unmasked, while RTL8211F and AN8801R mask it. Tested on
Helios64 (dwmac-rk, RTL8211F); Haoning Cheng tested v4 on i.MX6 (fec).

Changes in v5:
- v4 patch 2 alone could leave irq_rerun set across
  phy_free_interrupt() and a new request, after which a postponed
  level interrupt fired without end; it is merged with v4 patch 3 into
  patch 2. phy_free_interrupt() re-enables the line after free_irq(),
  not before, and only while other users of a shared line remain, so
  the handler cannot keep running on a source it cannot quiet;
  irq_rerun stays a bool (Sashiko on v4)
- patch 3: when the PHY fails to resume, the line a postponed
  interrupt disabled stays off, as before this series, instead of
  rerunning a handler that may not reach the PHY (Sashiko on v4)
- patch 4: with mac_managed_pm, only Wake-on-LAN in the PHY makes a
  postponed interrupt a wakeup; the ethtool flag may describe
  Wake-on-LAN in the MAC, as with fec (Sashiko on v4); for a PHY that
  phy_suspend() already suspended, the PHY part of its Wake-on-LAN
  state is kept in the new wol_phy
- patch 4: Fixes: now points at 1758bde2e4aa, which added the
  postponing after the early return for MAC-managed PHY PM
- comments say that postponing lasts until the transition ends
- Haoning Cheng's Tested-by on v4 kept on patch 1 only: patches 2-4
  changed
- rebased on net
- tests rerun on v5
- Link to v4: https://lore.kernel.org/r/20261031-b4-phylib-mac-pm-irq-v3-v4-0-5ba23b3ff9a4@iav.lv

Changes in v4:
- new patches 2-4, one pre-existing defect each
- patch 2: an edge that reruns the IRQ thread no longer disables the
  line twice
- patch 3: phy_free_interrupt() during the transition keeps the line
  balanced (Sashiko on v3)
- patch 4: a failing PHY PM callback no longer leaves the interrupt
  postponed (Sashiko on v2 and v3)
- patch 5: the wakeup event is decided in mdio_bus_phy_suspend(), where
  phy_may_wakeup() can still read the PHY (Sashiko on v3); Fixes: now
  points at the stmmac commit that took its PHYs out of postponing
- v3 asked phylib or phylink_suspend(): phylib, as macb sets
  mac_managed_pm but suspends with phylink_stop()
- tests rerun on v4
- Link to v3: https://lore.kernel.org/r/20261029-b4-phylib-mac-pm-irq-v3-v3-0-5907045f3210@iav.lv

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
- Link to v2: https://lore.kernel.org/r/20261026045343.837889-1-iav@iav.lv

Signed-off-by: Igor Velkov <iav@iav.lv>
---
Igor Velkov (4):
      net: phy: move irq_suspended and irq_rerun out of the bitfield
      net: phy: keep the line of a postponed PHY interrupt balanced
      net: phy: stop postponing PHY interrupts when a PHY PM callback fails
      net: phy: postpone PHY interrupts during sleep with MAC-managed PM

 drivers/net/phy/phy.c        |  45 +++++++++++----
 drivers/net/phy/phy_device.c | 134 ++++++++++++++++++++++++++++++++++---------
 include/linux/phy.h          |  35 +++++++++--
 3 files changed, 169 insertions(+), 45 deletions(-)
---
base-commit: 8d5ce4751fad9499ea408163af7f410f18a85782
change-id: 20261029-b4-phylib-mac-pm-irq-v3-f656d4e1c64f

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

[PATCH net v5 1/4] 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 last patch of this series 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, MAC drivers included, is a wide audit, 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")
Tested-by: Haoning Cheng <Haoning.CHENG@cn.bosch.com>
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 06c5a8c8cc2a..f00086d72771 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..bbe28ff97e51 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 without phydev->lock, so not bitfields */
+	bool irq_suspended;
+	bool irq_rerun;
 
 	unsigned default_timestamp:1;
 

-- 
2.43.0

[PATCH net v5 2/4] net: phy: keep the line of a postponed PHY interrupt balanced

[Igor Velkov via B4 Relay]

From: Igor Velkov <iav@iav.lv>

phy_interrupt() postpones interrupts during a system sleep transition:
it sets irq_rerun and disables the line, and mdio_bus_phy_resume()
re-enables the line once and reruns the handler. Two paths leave the
line unbalanced:

- On an edge-triggered line the IRQ core does not mask a ONESHOT
  interrupt, so an edge that arrives before disable_irq_nosync() runs
  the thread again. That second run disables the line again, and the
  line stays disabled after resume.
- phy_free_interrupt() can run between the postponed interrupt and
  mdio_bus_phy_resume() and leaves irq_rerun set. Other users of a
  shared line stay without interrupts until mdio_bus_phy_resume().
  Without other users, a new request starts the line at depth 0, and
  the enable_irq() in mdio_bus_phy_resume() is unbalanced.

MAC drivers that close the interface in their suspend callback, such as
cpsw, and ravb with Wake-on-LAN off, reach phy_free_interrupt() through
phy_disconnect(), and so does dev_close() in a kernel-internal reset of
the MAC such as stmmac_reset_subtask(), whose workqueue does not freeze.
These drivers set mac_managed_pm, which a later patch brings under
postponing.

Let only the run that sets irq_rerun disable the line. Let
phy_free_interrupt() consume irq_rerun after free_irq(), which waits for
the handler, and give the depth back with enable_irq() if a postponed
interrupt took it. Other users of a shared line get the line back. A
line the PHY was alone on is left alone: free_irq() has shut it down,
and the next request resets its depth. Re-enabling after free_irq(), not
before, leaves no PHY handler to run again on a source that
phy_disable_interrupts() did not clear.

Add irq_pm_lock, so that phy_free_interrupt() and the rerun after
resume do not both re-enable the line, and move the postponing and the
rerun into helpers. phy_interrupt() never takes the lock, so
synchronize_irq() and free_irq() can wait for the handler under it.
irq_rerun stays a plain bool: one thread runs the handler, and the
other two read it only under the lock, after free_irq() or
synchronize_irq(). Mark the irq_suspended accesses with READ_ONCE() and
WRITE_ONCE(): the handler reads it while the PM callbacks write it.

This does not keep close and open off the MDIO bus: on close,
phy_disconnect() calls phy_stop(), phy_disable_interrupts() and, in
phy_detach(), phy_suspend(); on open, phy_attach_direct() calls
phy_init_hw() and phy_request_interrupt() calls
phy_enable_interrupts(). A MAC driver that powers its MDIO bus down
during sleep must not close or open the interface until the bus is
back.

Fixes: 1758bde2e4aa ("net: phy: Don't trigger state machine while in suspend")
Signed-off-by: Igor Velkov <iav@iav.lv>
---
Haoning Cheng's Tested-by on v4 is not carried: this patch merges v4
patches 2 and 3 and reworks phy_free_interrupt().
---
 drivers/net/phy/phy.c        | 20 +++++++++++++---
 drivers/net/phy/phy_device.c | 56 +++++++++++++++++++++++++++-----------------
 include/linux/phy.h          |  7 ++++--
 3 files changed, 57 insertions(+), 26 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 9af303d60bd6..ea22ebb14a83 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1416,7 +1416,7 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 	/* Wakeup interrupts may occur during a system sleep transition.
 	 * Postpone handling until the PHY has resumed.
 	 */
-	if (IS_ENABLED(CONFIG_PM_SLEEP) && phydev->irq_suspended) {
+	if (IS_ENABLED(CONFIG_PM_SLEEP) && READ_ONCE(phydev->irq_suspended)) {
 		struct net_device *netdev = phydev->attached_dev;
 
 		if (netdev) {
@@ -1430,8 +1430,11 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 				pm_wakeup_dev_event(parent, 0, true);
 		}
 
-		phydev->irq_rerun = true;
-		disable_irq_nosync(irq);
+		/* Disable the line only once: an edge can rerun the thread. */
+		if (!phydev->irq_rerun) {
+			phydev->irq_rerun = true;
+			disable_irq_nosync(irq);
+		}
 		return IRQ_HANDLED;
 	}
 
@@ -1507,7 +1510,18 @@ EXPORT_SYMBOL(phy_request_interrupt);
 void phy_free_interrupt(struct phy_device *phydev)
 {
 	phy_disable_interrupts(phydev);
+
+	mutex_lock(&phydev->irq_pm_lock);
 	free_irq(phydev->irq, phydev);
+	/* Give the depth a postponed interrupt took back to the other users of
+	 * a shared line; requesting a line we were alone on resets its depth.
+	 */
+	if (phydev->irq_rerun) {
+		phydev->irq_rerun = false;
+		if (irq_has_action(phydev->irq))
+			enable_irq(phydev->irq);
+	}
+	mutex_unlock(&phydev->irq_pm_lock);
 }
 EXPORT_SYMBOL(phy_free_interrupt);
 
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index f00086d72771..d3815ee21541 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -332,6 +332,38 @@ static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
 	return !phydev->suspended;
 }
 
+/* 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.
+ */
+static __maybe_unused void mdio_bus_phy_suspend_irq(struct phy_device *phydev)
+{
+	if (!phy_interrupt_is_valid(phydev))
+		return;
+
+	WRITE_ONCE(phydev->irq_suspended, true);
+	synchronize_irq(phydev->irq);
+}
+
+/* Rerun interrupts which were postponed by phy_interrupt()
+ * because they occurred during the system sleep transition.
+ */
+static __maybe_unused void mdio_bus_phy_resume_irq(struct phy_device *phydev)
+{
+	WRITE_ONCE(phydev->irq_suspended, false);
+	if (!phy_interrupt_is_valid(phydev))
+		return;
+
+	mutex_lock(&phydev->irq_pm_lock);
+	synchronize_irq(phydev->irq);
+	if (phydev->irq_rerun) {
+		phydev->irq_rerun = false;
+		enable_irq(phydev->irq);
+		irq_wake_thread(phydev->irq, phydev);
+	}
+	mutex_unlock(&phydev->irq_pm_lock);
+}
+
 static __maybe_unused int mdio_bus_phy_suspend(struct device *dev)
 {
 	struct phy_device *phydev = to_phy_device(dev);
@@ -339,14 +371,7 @@ static __maybe_unused int mdio_bus_phy_suspend(struct 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.
-	 */
-	if (phy_interrupt_is_valid(phydev)) {
-		phydev->irq_suspended = true;
-		synchronize_irq(phydev->irq);
-	}
+	mdio_bus_phy_suspend_irq(phydev);
 
 	/* We must stop the state machine manually, otherwise it stops out of
 	 * control, possibly with the phydev->lock held. Upon resume, netdev
@@ -393,19 +418,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);
@@ -711,6 +724,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 	dev->max_n_ports = 1;
 
 	mutex_init(&dev->lock);
+	mutex_init(&dev->irq_pm_lock);
 	INIT_DELAYED_WORK(&dev->state_queue, phy_state_machine);
 
 	/* Request the appropriate module unconditionally; don't
diff --git a/include/linux/phy.h b/include/linux/phy.h
index bbe28ff97e51..a2a4d318e334 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -651,8 +651,9 @@ struct phy_oatc14_sqi_capability {
  * @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_rerun: Flag indicating interrupts occurred while PHY was suspended,
- *             requiring a rerun of the interrupt handler after resume
+ * @irq_rerun: Set by the postponed interrupt that disabled the line
+ * @irq_pm_lock: Serializes phy_free_interrupt() against the rerun of a
+ *               postponed interrupt; never taken by phy_interrupt()
  * @default_timestamp: Flag indicating whether we are using the phy
  *		       timestamp as the default one
  * @interface: enum phy_interface_t value
@@ -729,6 +730,8 @@ struct phy_device {
 	/* Written without phydev->lock, so not bitfields */
 	bool irq_suspended;
 	bool irq_rerun;
+	/* Serializes phy_free_interrupt() against mdio_bus_phy_resume_irq() */
+	struct mutex irq_pm_lock;
 
 	unsigned default_timestamp:1;
 

-- 
2.43.0

[PATCH net v5 3/4] net: phy: stop postponing PHY interrupts when a PHY PM callback fails

[Igor Velkov via B4 Relay]

From: Igor Velkov <iav@iav.lv>

When phy_suspend() fails in mdio_bus_phy_suspend(), irq_suspended
stays set and the state machine stays stopped: the PM core does not
resume a device whose suspend callback failed, and the next PHY
interrupt disables the line until the next resume. When phy_init_hw()
or phy_resume() fail, mdio_bus_phy_resume() returns before the rerun,
with irq_suspended set and a postponed interrupt left disabled.

Undo the postponing and restart the state machine when phy_suspend()
fails. When phy_init_hw() or phy_resume() fail, stop postponing too, but
leave a line that a postponed interrupt disabled off, as before this
patch: the PHY may be out of reach, and its handler could not quiet a
level-triggered source. The line comes back on the next successful
resume; phy_free_interrupt() gives it back to the other users of a
shared line, and a new request restarts it.

Fixes: 1758bde2e4aa ("net: phy: Don't trigger state machine while in suspend")
Signed-off-by: Igor Velkov <iav@iav.lv>
---
Haoning Cheng's Tested-by on v4 is not carried: a failed resume now
leaves the line disabled.
---
 drivers/net/phy/phy.c        |  2 +-
 drivers/net/phy/phy_device.c | 29 ++++++++++++++++++++++++-----
 2 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index ea22ebb14a83..62b7f428f9da 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1414,7 +1414,7 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 	irqreturn_t ret;
 
 	/* Wakeup interrupts may occur during a system sleep transition.
-	 * Postpone handling until the PHY has resumed.
+	 * Postpone handling until the transition ends.
 	 */
 	if (IS_ENABLED(CONFIG_PM_SLEEP) && READ_ONCE(phydev->irq_suspended)) {
 		struct net_device *netdev = phydev->attached_dev;
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index d3815ee21541..110c9dfcbd91 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -333,8 +333,8 @@ static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
 }
 
 /* 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 PHY is inaccessible. Set flag to postpone handling until the
+ * transition ends. Wait for concurrent interrupt handler to complete.
  */
 static __maybe_unused void mdio_bus_phy_suspend_irq(struct phy_device *phydev)
 {
@@ -367,6 +367,7 @@ static __maybe_unused void mdio_bus_phy_resume_irq(struct phy_device *phydev)
 static __maybe_unused int mdio_bus_phy_suspend(struct device *dev)
 {
 	struct phy_device *phydev = to_phy_device(dev);
+	int ret;
 
 	if (phydev->mac_managed_pm)
 		return 0;
@@ -384,9 +385,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;
 }
 
 static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
@@ -412,11 +424,11 @@ static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
 
 	ret = phy_init_hw(phydev);
 	if (ret < 0)
-		return ret;
+		goto err;
 
 	ret = phy_resume(phydev);
 	if (ret < 0)
-		return ret;
+		goto err;
 no_resume:
 	mdio_bus_phy_resume_irq(phydev);
 
@@ -424,6 +436,13 @@ static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
 		phy_start_machine(phydev);
 
 	return 0;
+
+err:
+	/* Stop postponing, but leave a line that a postponed interrupt
+	 * disabled off: its handler may not reach a PHY that did not resume.
+	 */
+	WRITE_ONCE(phydev->irq_suspended, false);
+	return ret;
 }
 
 static SIMPLE_DEV_PM_OPS(mdio_bus_phy_pm_ops, mdio_bus_phy_suspend,

-- 
2.43.0

[PATCH net v5 4/4] 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(). The commit in
the Fixes tag added the postponing after the early return for
MAC-managed PHY PM, and stmmac has set mac_managed_pm since commit
f151c147b3af ("net: stmmac: Enable mac_managed_pm phylink config").

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 resume callbacks of
the bus and its parents (with stmmac, the MAC) and replays the postponed
interrupt then. The PM core resumes children of a device whose resume
callback failed, and phylib cannot tell: if the MAC fails to power the
bus up, the replay still reaches the bus. phy_suspend(), phy_resume()
and the state machine stay with the MAC driver, and no MAC driver needs
a change. Doing this in phylink_suspend() instead would miss macb, which
sets mac_managed_pm but suspends with phylink_stop().

A postponed interrupt cannot ask the PHY whether Wake-on-LAN is enabled:
phy_may_wakeup() may read the PHY over MDIO. Decide that in
mdio_bus_phy_suspend() instead, which runs before the suspend callback
of the device that owns the MDIO bus, and store the wakeup event to
report. For a PHY whose MAC manages PHY PM, this MDIO access in the
suspend path is new; phy_suspend() makes the same one for other PHYs. A
PHY on the MDIO bus of another MAC can already be suspended there:
phy_attach_direct() links its MAC to it, so the MAC suspends first. For
a suspended PHY without driver-model wakeup, do not read the PHY: use
wol_phy, where phy_suspend() now keeps the answer of phy_may_wakeup().
wol_enabled also counts Wake-on-LAN in the MAC.
mdio_bus_phy_suspend_irq() sets irq_suspended with a store-release after
writing irq_wake, and phy_interrupt() reads it with a load-acquire, so a
postponed interrupt sees the event decided for it.

A PHY reports a system wakeup when phy_may_wakeup() says Wake-on-LAN is
enabled in it, as phy_suspend() already counts it; this is new for PHYs
without MAC-managed PM too. Otherwise a PHY without MAC-managed PM
reports what phy_interrupt() reports today: Wake-on-LAN set through
ethtool, then the netdev and its parent. A PHY with MAC-managed PM
reports nothing: the MAC stops the PHY itself, its ethtool flag may
describe Wake-on-LAN in the MAC, as with fec, and the MAC device may
wake the system on its own, so a link interrupt the MAC causes must not
abort suspend.

Tested on Helios64 with 7.3.4, s2idle with the link up:
- no Wake-on-LAN, GMAC clocks off in suspend: 10 of 10 cycles suspended
  and resumed; on 7.3 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, so phylink_suspend() also drops the link
  speed: a magic packet woke the board 4 of 4 times, no suspend
  aborted.

Fixes: 1758bde2e4aa ("net: phy: Don't trigger state machine while in suspend")
Signed-off-by: Igor Velkov <iav@iav.lv>
---
Haoning Cheng's Tested-by on v4 is not carried: the wakeup policy for
MAC-managed PHYs changed.
---
 drivers/net/phy/phy.c        | 25 ++++++++++++-------
 drivers/net/phy/phy_device.c | 57 ++++++++++++++++++++++++++++++++++++++++----
 include/linux/phy.h          | 23 ++++++++++++++++--
 3 files changed, 89 insertions(+), 16 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 62b7f428f9da..1a7648c381f7 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1416,18 +1416,25 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 	/* Wakeup interrupts may occur during a system sleep transition.
 	 * Postpone handling until the transition ends.
 	 */
-	if (IS_ENABLED(CONFIG_PM_SLEEP) && READ_ONCE(phydev->irq_suspended)) {
+	if (IS_ENABLED(CONFIG_PM_SLEEP) &&
+	    /* Pairs with smp_store_release() in mdio_bus_phy_suspend_irq() */
+	    smp_load_acquire(&phydev->irq_suspended)) {
 		struct net_device *netdev = phydev->attached_dev;
 
-		if (netdev) {
-			struct device *parent = netdev->dev.parent;
-
-			if (netdev->ethtool->wol_enabled)
-				pm_system_wakeup();
-			else if (device_may_wakeup(&netdev->dev))
+		switch (phydev->irq_wake) {
+		case PHY_IRQ_WAKE_SYSTEM:
+			pm_system_wakeup();
+			break;
+		case PHY_IRQ_WAKE_NETDEV:
+			if (netdev)
 				pm_wakeup_dev_event(&netdev->dev, 0, true);
-			else if (parent && device_may_wakeup(parent))
-				pm_wakeup_dev_event(parent, 0, true);
+			break;
+		case PHY_IRQ_WAKE_PARENT:
+			if (netdev && netdev->dev.parent)
+				pm_wakeup_dev_event(netdev->dev.parent, 0, true);
+			break;
+		case PHY_IRQ_WAKE_NONE:
+			break;
 		}
 
 		/* Disable the line only once: an edge can rerun the thread. */
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 110c9dfcbd91..2dc13dd1afb6 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -332,6 +332,44 @@ static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
 	return !phydev->suspended;
 }
 
+static __maybe_unused enum phy_irq_wake
+phy_irq_wake_source(struct phy_device *phydev)
+{
+	struct net_device *netdev = phydev->attached_dev;
+	bool phy_wol;
+
+	if (!netdev)
+		return PHY_IRQ_WAKE_NONE;
+
+	/* phy_suspend() cached the answer of a suspended PHY, which may
+	 * not be reachable over MDIO.
+	 */
+	if (phydev->suspended && !phy_can_wakeup(phydev))
+		phy_wol = phydev->wol_phy;
+	else
+		phy_wol = phy_may_wakeup(phydev);
+
+	if (phy_wol)
+		return PHY_IRQ_WAKE_SYSTEM;
+
+	/* With MAC-managed PM, only Wake-on-LAN in the PHY makes its
+	 * interrupt a wakeup: the MAC stops the PHY itself.
+	 */
+	if (phydev->mac_managed_pm)
+		return PHY_IRQ_WAKE_NONE;
+
+	if (netdev->ethtool->wol_enabled)
+		return PHY_IRQ_WAKE_SYSTEM;
+
+	if (device_may_wakeup(&netdev->dev))
+		return PHY_IRQ_WAKE_NETDEV;
+
+	if (netdev->dev.parent && device_may_wakeup(netdev->dev.parent))
+		return PHY_IRQ_WAKE_PARENT;
+
+	return PHY_IRQ_WAKE_NONE;
+}
+
 /* Wakeup interrupts may occur during the system sleep transition when
  * the PHY is inaccessible. Set flag to postpone handling until the
  * transition ends. Wait for concurrent interrupt handler to complete.
@@ -341,7 +379,10 @@ static __maybe_unused void mdio_bus_phy_suspend_irq(struct phy_device *phydev)
 	if (!phy_interrupt_is_valid(phydev))
 		return;
 
-	WRITE_ONCE(phydev->irq_suspended, true);
+	/* A postponed handler cannot read the PHY: decide the event now */
+	phydev->irq_wake = phy_irq_wake_source(phydev);
+	/* Pairs with smp_load_acquire() in phy_interrupt() */
+	smp_store_release(&phydev->irq_suspended, true);
 	synchronize_irq(phydev->irq);
 }
 
@@ -369,11 +410,14 @@ static __maybe_unused int mdio_bus_phy_suspend(struct device *dev)
 	struct phy_device *phydev = to_phy_device(dev);
 	int ret;
 
+	/* An interrupt can fire while the MAC has the MDIO bus powered
+	 * down, whether or not the MAC manages PHY PM.
+	 */
+	mdio_bus_phy_suspend_irq(phydev);
+
 	if (phydev->mac_managed_pm)
 		return 0;
 
-	mdio_bus_phy_suspend_irq(phydev);
-
 	/* 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
@@ -406,8 +450,10 @@ 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;
@@ -2127,7 +2173,8 @@ int phy_suspend(struct phy_device *phydev)
 	if (phydev->suspended || !phydrv)
 		return 0;
 
-	phydev->wol_enabled = phy_may_wakeup(phydev) ||
+	phydev->wol_phy = phy_may_wakeup(phydev);
+	phydev->wol_enabled = phydev->wol_phy ||
 			      (netdev && netdev->ethtool->wol_enabled);
 	/* If the device has WOL enabled, we cannot suspend the PHY */
 	if (phydev->wol_enabled && !(phydrv->flags & PHY_ALWAYS_CALL_SUSPEND))
diff --git a/include/linux/phy.h b/include/linux/phy.h
index a2a4d318e334..b1fdc4504b68 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -573,6 +573,20 @@ struct phy_oatc14_sqi_capability {
 	u8 sqiplus_bits;
 };
 
+/**
+ * enum phy_irq_wake - wakeup event a postponed PHY interrupt reports
+ * @PHY_IRQ_WAKE_NONE: none
+ * @PHY_IRQ_WAKE_SYSTEM: pm_system_wakeup(), Wake-on-LAN is enabled
+ * @PHY_IRQ_WAKE_NETDEV: pm_wakeup_dev_event() for the attached netdev
+ * @PHY_IRQ_WAKE_PARENT: pm_wakeup_dev_event() for the netdev's parent
+ */
+enum phy_irq_wake {
+	PHY_IRQ_WAKE_NONE,
+	PHY_IRQ_WAKE_SYSTEM,
+	PHY_IRQ_WAKE_NETDEV,
+	PHY_IRQ_WAKE_PARENT,
+};
+
 /**
  * struct phy_device - An instance of a PHY
  *
@@ -600,6 +614,7 @@ struct phy_oatc14_sqi_capability {
  * @mac_managed_pm: Set true if MAC driver takes of suspending/resuming PHY
  * @wol_enabled: Set to true if the PHY or the attached MAC have Wake-on-LAN
  * 		 enabled.
+ * @wol_phy: Set to true if phy_suspend() found that the PHY may wake the system
  * @is_genphy_driven: PHY is driven by one of the generic PHY drivers
  * @state: State of the PHY for management purposes
  * @dev_flags: Device-specific flags used by the PHY driver.
@@ -649,9 +664,11 @@ 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 it ends
  * @irq_rerun: Set by the postponed interrupt that disabled the line
+ * @irq_wake: Wakeup event a postponed interrupt reports, decided by
+ *            mdio_bus_phy_suspend()
  * @irq_pm_lock: Serializes phy_free_interrupt() against the rerun of a
  *               postponed interrupt; never taken by phy_interrupt()
  * @default_timestamp: Flag indicating whether we are using the phy
@@ -716,6 +733,7 @@ struct phy_device {
 	unsigned is_on_sfp_module:1;
 	unsigned mac_managed_pm:1;
 	unsigned wol_enabled:1;
+	unsigned wol_phy:1;
 	unsigned is_genphy_driven:1;
 
 	unsigned autoneg:1;
@@ -730,6 +748,7 @@ struct phy_device {
 	/* Written without phydev->lock, so not bitfields */
 	bool irq_suspended;
 	bool irq_rerun;
+	enum phy_irq_wake irq_wake;
 	/* Serializes phy_free_interrupt() against mdio_bus_phy_resume_irq() */
 	struct mutex irq_pm_lock;
 

-- 
2.43.0

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by LLM and not proven at runtime). If it was triggered
   please include the symptoms, like the stack trace or kernel messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[Igor Velkov]

Hi Haoning,

Thanks for testing v4. v5 changes patches 2-4 (freeing the interrupt,
a failed resume, the wakeup policy for MAC-managed PHYs), so I kept your
Tested-by on patch 1 only. Could you rerun your fec tests on v5?

Igor

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

[Igor Velkov]

> - Whether the issue was actually triggered, or is only theoretical

Patch 4 fixes a hang seen on Helios64 (dwmac-rk, RTL8211F): in s2idle
without WoL the board hung or reset 10 of 10 times without the series.
The PHY IRQ thread stalls its CPU on an MDIO read while the GMAC clocks
are off, so that CPU never answers the NMI:

  watchdog: CPU0: Watchdog detected hard LOCKUP on cpu 1
  After 10 seconds, these CPUS still haven't responded to the NMI: 1

Patches 1-3 fix defects found by code inspection and Sashiko review;
they were not triggered.

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

[Jakub Kicinski]

On Thu, 12 Nov 2026 01:55:16 +0200 Igor Velkov wrote:
> Patch 4 fixes a hang seen on Helios64 (dwmac-rk, RTL8211F): in s2idle
> without WoL the board hung or reset 10 of 10 times without the series.
> The PHY IRQ thread stalls its CPU on an MDIO read while the GMAC clocks
> are off, so that CPU never answers the NMI:
> 
>   watchdog: CPU0: Watchdog detected hard LOCKUP on cpu 1
>   After 10 seconds, these CPUS still haven't responded to the NMI: 1
> 
> Patches 1-3 fix defects found by code inspection and Sashiko review;
> they were not triggered.

please remember to add this to the cover letter

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch adds the comment "/* Written without phydev->lock, so
  not bitfields */" above irq_suspended and irq_rerun in struct…

--

net: phy: move irq_suspended and irq_rerun out of the bitfield

This commit turns irq_suspended and irq_rerun in struct phy_device from
1-bit bitfields into bools. phy_interrupt() sets irq_rerun without
phydev->lock, and that write could lose an update to link and
autoneg_complete, which share the same word. After this change the two
flags are separate memory locations.

> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 21afc2082b174..bbe28ff97e51c 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 without phydev->lock, so not bitfields */

[Severity: Low]
This isn't a bug, but the new comment states a rule that the
neighbouring fields don't follow. Several of them are written without
phydev->lock. They are still 1-bit bitfields in the same run as link
and autoneg_complete, and the state machine writes those two under the
lock.

interrupts, the field just above, is written in phy_config_interrupt():

drivers/net/phy/phy.c:phy_config_interrupt() {
    ...
	phydev->interrupts = interrupts ? 1 : 0;
    ...
}

This is reached from phy_request_interrupt()/phy_free_interrupt(). Some
PHY drivers also set it in their suspend/resume paths (dp83867, micrel,
marvell-88q2xxx).

phy_suspend() writes wol_enabled and suspended:

drivers/net/phy/phy_device.c:phy_suspend() {
    ...
	phydev->wol_enabled = phy_may_wakeup(phydev) ||
    ...
		phydev->suspended = true;
    ...
}

mdio_bus_phy_suspend() and mdio_bus_phy_resume() write
suspended_by_mdio_bus:

	phydev->suspended_by_mdio_bus = 1;

The commit message itself names these unlocked writers of the same word.

Later in the series, "net: phy: postpone PHY interrupts during sleep
with MAC-managed PM" adds another bitfield to this word, wol_phy:1.
phy_suspend() writes it without phydev->lock, and phy_irq_wake_source()
reads it.

Could the comment say that these two fields are split out because
phy_interrupt() writes them from the IRQ thread? As written, a reader
could assume the remaining bitfields are all lock-protected.

If any of those unlocked writers can run at the same time as each
other, or as the locked link/autoneg_complete writers, the lost-update
problem this commit describes would still apply to them. I haven't
shown that concurrency here.

> +	bool irq_suspended;
> +	bool irq_rerun;
>  
>  	unsigned default_timestamp:1;
>

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

Re: [PATCH net v5 2/4] net: phy: keep the line of a postponed PHY interrupt balanced

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · 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] phy_free_interrupt() (drivers/net/phy/phy.c) can re-enable a
  shared line that the PHY may still be asserting.
- [Medium] phy_free_interrupt() gives back the depth that the postponed
  disable_irq_nosync() took.
- [Low] In phy_free_interrupt() (drivers/net/phy/phy.c), the check
  irq_has_action(phydev->irq) and the following enable_irq(phydev->irq)…

--

net: phy: keep the line of a postponed PHY interrupt balanced

This patch makes only the phy_interrupt() run that sets irq_rerun disable
the line. phy_free_interrupt() now consumes irq_rerun after free_irq() and
calls enable_irq() when other actions remain on the line. It also adds
irq_pm_lock to serialize phy_free_interrupt() against the rerun in
mdio_bus_phy_resume_irq().

> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 9af303d60bd6b..ea22ebb14a832 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c

[ ... ]

> @@ -1507,7 +1510,18 @@ EXPORT_SYMBOL(phy_request_interrupt);
>  void phy_free_interrupt(struct phy_device *phydev)
>  {
>  	phy_disable_interrupts(phydev);
> +
> +	mutex_lock(&phydev->irq_pm_lock);
>  	free_irq(phydev->irq, phydev);
> +	/* Give the depth a postponed interrupt took back to the other users of
> +	 * a shared line; requesting a line we were alone on resets its depth.
> +	 */
> +	if (phydev->irq_rerun) {
> +		phydev->irq_rerun = false;
> +		if (irq_has_action(phydev->irq))
> +			enable_irq(phydev->irq);

[Severity: Medium]
Can this unmask a shared line while the PHY is still asserting it?

phy_free_interrupt() ignores the return value of phy_disable_interrupts().
Also, a postponed interrupt never acknowledged the PHY status: the
postponed path in phy_interrupt() returns IRQ_HANDLED without calling the
driver's handle_interrupt().

If the MDIO write in phy_disable_interrupts() fails, the PHY interrupt
mask and latched status stay set. On a level-triggered shared line,
enable_irq() then unmasks the line with no PHY handler installed.

If none of the remaining handlers claims the interrupt, would
note_interrupt() eventually disable the line with IRQS_SPURIOUS_DISABLED
for every remaining user?

That failure looks reachable with cpsw_new once the later patch "net: phy:
postpone PHY interrupts during sleep with MAC-managed PM" is applied. The
davinci_mdio controller is a child created by devm_of_platform_populate(),
so it suspends first, and davinci_mdio_suspend() calls
pm_runtime_force_suspend(). Then:

cpsw_suspend()
  cpsw_ndo_stop()
    phy_disconnect()
      phy_free_interrupt()
        phy_disable_interrupts()
          davinci_mdio_write()
            pm_runtime_resume_and_get()  <- fails, runtime PM disabled

Before this patch, the line stayed disabled until mdio_bus_phy_resume(),
which runs after the MAC reopens and installs the PHY handler again. With
this patch, the line is unmasked from the close until
suspend_device_irqs(). It is unmasked again from resume_device_irqs()
until the MAC reopens. Nothing clears the source in either window.

The commit message says:

    Re-enabling after free_irq(), not
    before, leaves no PHY handler to run again on a source that
    phy_disable_interrupts() did not clear.

That covers the PHY handler, but what about the other sharers on the
line? Should the enable_irq() be skipped when phy_disable_interrupts()
fails?

[Severity: Medium]
Is this enable_irq() still balanced if phy_free_interrupt() runs inside
the noirq window on a wake-armed line?

The commit message names stmmac_reset_subtask(), whose workqueue does not
freeze, as a path into phy_free_interrupt() during the sleep transition.
stmmac_suspend() does not set STMMAC_DOWN, so the subtask can run.

bcm_phy_set_wol() calls enable_irq_wake(phydev->irq). Several irqchips set
IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND: gpio-mxc, gpio-vf610, pinctrl-msm,
qcom-pdc, pinctrl-amd and pinctrl-starfive-jhb100.

Consider this sequence, with another action still on the line:

phy_interrupt()
  disable_irq_nosync()           depth 0 -> 1

suspend_device_irq()
  __enable_irq()                 depth 1 -> 0
  irqd_set(irqd, IRQD_IRQ_ENABLED_ON_SUSPEND)

phy_free_interrupt()
  irq_has_action() is true
  enable_irq()
    __enable_irq() case 0        WARN "Unbalanced enable for IRQ"

resume_irq()
  __disable_irq()                depth 0 -> 1

The line is not IRQS_SUSPENDED, so nothing enables it again on resume.
irq_rerun has already been consumed, so mdio_bus_phy_resume_irq() does
nothing either. A shared re-request from dev_open() does not touch the
depth.

Would the line then stay disabled after resume for every user, including
the reopened PHY? Before this patch, irq_rerun stayed set until
mdio_bus_phy_resume(), which ran after resume_irq() had restored depth 1.

[Severity: Low]
Can the irq_has_action() check race with another driver requesting the
same line?

By this point free_irq() has already dropped desc->request_mutex.
irq_has_action() only returns a snapshot. irq_pm_lock is per-phydev, so
other drivers don't take it.

Suppose the PHY was the last user and a postponed interrupt left depth
at 1:

free_irq()
  irq_shutdown()                 depth 1 -> 2

                                 other driver: request_irq()
                                   __setup_irq()
                                     irq_startup()   depth = 0

irq_has_action() returns true
enable_irq()
  __enable_irq() case 0          WARN "Unbalanced enable for IRQ"

If the new requester used IRQ_NOAUTOEN, irq_startup() resets the depth to
1 instead. This enable_irq() would then start a line that driver asked to
keep disabled.

> +	}
> +	mutex_unlock(&phydev->irq_pm_lock);
>  }
>  EXPORT_SYMBOL(phy_free_interrupt);

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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · 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] In mdio_bus_phy_resume() (drivers/net/phy/phy_device.c), the
  new err: path runs WRITE_ONCE(phydev->irq_suspended, false) after…
- [Medium] The commit message says a line that the new err path leaves
  disabled 'comes back on the next successful resume'.
- [Low] The patch moves 'phydev->suspended_by_mdio_bus = 1' in
  mdio_bus_phy_suspend() (drivers/net/phy/phy_device.c) from before…

--

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 postponing and restarts the state machine. It also
moves the suspended_by_mdio_bus assignment so it happens only after
phy_suspend() succeeds. In mdio_bus_phy_resume(), a phy_init_hw() or
phy_resume() failure now clears irq_suspended, but a line that a postponed
interrupt disabled stays off.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index d3815ee21541a..110c9dfcbd916 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -384,9 +385,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;

[Severity: Low]

This isn't a bug, but moving the suspended_by_mdio_bus assignment after
phy_suspend() also changes resume behaviour. The commit message doesn't
mention this.

Before, the flag stayed set when phy_suspend() failed. The resume callback
is skipped in that case, so nothing cleared it.

On a later cycle where mdio_bus_phy_may_suspend() returns false (WoL now
enabled, or the PHY already suspended by phy_detach()), the stale flag was
still set. mdio_bus_phy_resume() would then run phy_init_hw() and
phy_resume() on a PHY the MDIO bus did not suspend.

Could the commit message describe this change as well, so reviewers and
stable backporters can see it?

>  
> -	return phy_suspend(phydev);
> +	return 0;
>  }

[ ... ]

> @@ -424,6 +436,13 @@ static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
>  		phy_start_machine(phydev);
>  
>  	return 0;
> +
> +err:
> +	/* Stop postponing, but leave a line that a postponed interrupt
> +	 * disabled off: its handler may not reach a PHY that did not resume.
> +	 */
> +	WRITE_ONCE(phydev->irq_suspended, false);

[Severity: Medium]

What happens here if no interrupt was postponed during the transition?
In that case irq_rerun is false and the line is still enabled at the
irqchip.

Before this patch, irq_suspended stayed set after a failed resume. The
first assertion would then take the postpone branch in phy_interrupt(),
disable the line once and return IRQ_HANDLED without any MDIO access.

Now that irq_suspended is cleared here, the next assertion skips that
branch and goes straight to:

drivers/net/phy/phy.c:phy_interrupt() {
    ...
	mutex_lock(&phydev->lock);
	ret = phydev->drv->handle_interrupt(phydev);
	mutex_unlock(&phydev->lock);
    ...
}

That handler runs on a PHY whose phy_init_hw() or phy_resume() failed.
The comment above keeps a disabled line off because "its handler may not
reach a PHY that did not resume". Doesn't the same reasoning apply to a
line that is still enabled?

phy_init_hw() calls config_intr() before disable_autonomous_eee() and
before phy_resume(), and both of those can still fail. The PHY can
therefore have its interrupt sources armed even though resume returned an
error.

If the PHY is unreachable, a handler such as rtl8211f_handle_interrupt()
fails the status read, calls phy_error() (which does WARN_ON(1)) and
returns IRQ_NONE. The source is never cleared, so the
IRQF_ONESHOT | IRQF_SHARED threaded line can re-assert after each unmask.

Could this cause a stream of WARN splats until the spurious IRQ detector
disables the line for all of its users? On a shared line where another
device acks its own interrupt, the detector may never trip, and the WARN
would repeat on every interrupt.

If the PHY is reachable, the handler calls phy_trigger_machine() or
phy_process_error(). Either one restarts the state machine that this
error path deliberately left stopped.

Would phylib then be driving a PHY whose config_init, config_intr,
disable_autonomous_eee or drv->resume never finished?

[Severity: Medium]

The commit message says:

  The line comes back on the next successful resume; ...

Does that next resume actually re-initialise the PHY?
mdio_bus_phy_resume() clears suspended_by_mdio_bus before phy_init_hw()
and phy_resume() run:

drivers/net/phy/phy_device.c:mdio_bus_phy_resume() {
    ...
	if (!phydev->suspended_by_mdio_bus)
		goto no_resume;

	phydev->suspended_by_mdio_bus = 0;
    ...
}

phydev->suspended is cleared in only two places: in phy_init_hw() after a
successful soft_reset, and in __phy_resume() on success. So it stays true
in these cases:

  - soft_reset fails
  - the driver has no soft_reset, and config_init, config_intr,
    disable_autonomous_eee or drv->resume fails

On the next sleep cycle, mdio_bus_phy_may_suspend() ends with:

	return !phydev->suspended;

so mdio_bus_phy_suspend() returns 0 without calling phy_suspend() or
setting suspended_by_mdio_bus. The next mdio_bus_phy_resume() then jumps
to no_resume. There, mdio_bus_phy_resume_irq() does this because
irq_rerun is still set:

	if (phydev->irq_rerun) {
		phydev->irq_rerun = false;
		enable_irq(phydev->irq);
		irq_wake_thread(phydev->irq, phydev);
	}

and phy_start_machine() runs after it.

Doesn't this rerun handle_interrupt() on the same PHY that never resumed?
That is the case this error path is meant to prevent.

Clearing suspended_by_mdio_bus early is older than this patch. However,
the safety argument in the commit message relies on this recovery path.

> +	return ret;
>  }

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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 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] The new wakeup policy treats every postponed PHY interrupt as
  a system wakeup whenever phy_may_wakeup() is true, including link-…

--

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

This commit sets irq_suspended for PHYs whose MAC manages PHY PM. With it
set, phy_interrupt() postpones the PHY driver's handler for the whole
sleep transition instead of accessing an MDIO bus that may be powered
down. The wakeup event a postponed interrupt reports is now decided in
mdio_bus_phy_suspend() and stored in the new irq_wake field.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 110c9dfcbd916..2dc13dd1afb66 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -332,6 +332,44 @@ static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
>  	return !phydev->suspended;
>  }
>  
> +static __maybe_unused enum phy_irq_wake
> +phy_irq_wake_source(struct phy_device *phydev)
> +{
> +	struct net_device *netdev = phydev->attached_dev;
> +	bool phy_wol;
> +
> +	if (!netdev)
> +		return PHY_IRQ_WAKE_NONE;
> +
> +	/* phy_suspend() cached the answer of a suspended PHY, which may
> +	 * not be reachable over MDIO.
> +	 */
> +	if (phydev->suspended && !phy_can_wakeup(phydev))
> +		phy_wol = phydev->wol_phy;
> +	else
> +		phy_wol = phy_may_wakeup(phydev);
> +
> +	if (phy_wol)
> +		return PHY_IRQ_WAKE_SYSTEM;
> +
> +	/* With MAC-managed PM, only Wake-on-LAN in the PHY makes its
> +	 * interrupt a wakeup: the MAC stops the PHY itself.
> +	 */
> +	if (phydev->mac_managed_pm)
> +		return PHY_IRQ_WAKE_NONE;

[Severity: Medium]
Can a link-change interrupt caused by the MAC's own suspend path abort
system suspend here?

Take a MAC-managed PHY where phy_may_wakeup() is true. The phy_may_wakeup()
check comes before the mac_managed_pm check, so irq_wake is set to
PHY_IRQ_WAKE_SYSTEM. This happens in mdio_bus_phy_suspend(), which runs
before the MAC's suspend callback.

With stmmac, which sets config->wol_phy_speed_ctrl = true, the MAC
suspend then does:

stmmac_suspend()
  phylink_suspend()
    phylink_stop()
    phylink_speed_down(pl, false)    /* gated on phy_may_wakeup() */
      phy_speed_down(phydev, false)
        phy_config_aneg()            /* restarts autoneg, link drops */

phy_stop() does not mask the PHY interrupt. On PHYs whose INT pin carries
both link and WoL events, the link-fail interrupt then fires while
irq_suspended is set, and phy_interrupt() does:

		case PHY_IRQ_WAKE_SYSTEM:
			pm_system_wakeup();
			break;

This runs without reading the interrupt status, so a link change cannot be
told apart from a PME/WoL event. The phy_speed_down() kernel-doc already
warns about this:

  WARNING: Setting sync to false may cause the system being unable to suspend
  in case the PHY generates an interrupt when finishing the autonegotiation.

Resume speeds the link back up and the next suspend speeds it down again.
Wouldn't every retry abort in the same way?

Two configurations where this looks reachable:

AR8031/AR8035 (at803x) with WoL enabled. AT803X_INTR_ENABLE_WOL shares
AT803X_INTR_ENABLE and the INT pin with the link fail/success bits. These
PHYs lack PHY_ALWAYS_CALL_SUSPEND, so phy_suspend() returns -EBUSY and the
link interrupts are never masked.

BCM54xx with a DT interrupt and no WoL configured. bcm54xx_phy_probe()
calls device_init_wakeup(&phydev->mdio.dev, true) whenever
bcm54xx_phy_can_wakeup() is true, which is whenever
phy_interrupt_is_valid(). So phy_may_wakeup() is true by default, and the
speed-down and PHY_IRQ_WAKE_SYSTEM apply on every suspend.

Before this patch, MAC-managed PHYs never reported a wakeup from
phy_interrupt(). The driver handler read the status, and handlers such as
rtl8211f_handle_interrupt() report a wakeup only on the PME bit.

This also seems to contradict the commit message:

  ... and the MAC device may
  wake the system on its own, so a link interrupt the MAC causes must not
  abort suspend.

The Helios64 result ("no suspend aborted") seems to depend on
rtl8211f_suspend() writing RTL8211F_INER_PME to RTL821x_INER before the
speed-down. That masks the link interrupt, and other PHY drivers do not do
the same.

The postponed path also never reads or clears the status. Doesn't that
leave a level-triggered INT asserted, with the line disabled by
disable_irq_nosync() until resume?

[ ... ]

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