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

[PATCH net v4 0/5] 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-4 fix the postponing that phylib applies to other PHYs, so
that patch 5 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 lets only one run of the handler disable the line
when an edge reruns the IRQ thread. Patch 3 keeps that disable balanced
when phy_free_interrupt() runs during the transition. Patch 4 undoes
the postponing when a PHY PM callback fails. Patch 5 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 5 extends that to
MAC-managed PHYs.

With Wake-on-LAN, any postponed interrupt reports a system wakeup, so a
PHY that does not mask its link interrupt in suspend can abort suspend
when the MAC stops it. RTL8211F masks it when it is a wakeup source;
other PHYs were not tested. Tested only on Helios64 (dwmac-rk,
RTL8211F).

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

Not changed: a nested threaded PHY interrupt is not replayed
(irq_wake_thread() has no thread to wake), as before this series. Not
in this series: mdio_bus_phy_may_suspend() does not ask
phy_may_wakeup() about an attached PHY.

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 (5):
      net: phy: move irq_suspended and irq_rerun out of the bitfield
      net: phy: disable the line of a postponed PHY interrupt only once
      net: phy: balance a postponed PHY interrupt when the interrupt is freed
      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        |  46 ++++++++++++----
 drivers/net/phy/phy_device.c | 125 ++++++++++++++++++++++++++++++++++---------
 include/linux/phy.h          |  36 ++++++++++---
 3 files changed, 164 insertions(+), 43 deletions(-)
---
base-commit: fcc00b15cb21a6de76826e5012b2c758505f5e65
change-id: 20261029-b4-phylib-mac-pm-irq-v3-f656d4e1c64f

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

[PATCH net v4 1/5] 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")
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..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 v4 2/5] net: phy: disable the line of a postponed PHY interrupt only once

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

Make irq_rerun atomic, as the next patch adds a second setter, and let
only the run that sets it disable the line. Mark the irq_suspended
accesses with READ_ONCE() and WRITE_ONCE(): the handler reads it while
the PM callbacks write it.

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

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 9af303d60bd6..b47f0d3d99e5 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,9 @@ 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 (!atomic_xchg(&phydev->irq_rerun, 1))
+			disable_irq_nosync(irq);
 		return IRQ_HANDLED;
 	}
 
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 79dba43d703a..d5d0efc10418 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 = true;
+		WRITE_ONCE(phydev->irq_suspended, true);
 		synchronize_irq(phydev->irq);
 	}
 
@@ -394,14 +394,13 @@ static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
 		return ret;
 no_resume:
 	if (phy_interrupt_is_valid(phydev)) {
-		phydev->irq_suspended = false;
+		WRITE_ONCE(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;
+		if (atomic_xchg(&phydev->irq_rerun, 0)) {
 			enable_irq(phydev->irq);
 			irq_wake_thread(phydev->irq, phydev);
 		}
diff --git a/include/linux/phy.h b/include/linux/phy.h
index bbe28ff97e51..48b4e852f230 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -651,8 +651,7 @@ 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
  * @default_timestamp: Flag indicating whether we are using the phy
  *		       timestamp as the default one
  * @interface: enum phy_interface_t value
@@ -728,7 +727,7 @@ struct phy_device {
 	unsigned interrupts:1;
 	/* Written without phydev->lock, so not bitfields */
 	bool irq_suspended;
-	bool irq_rerun;
+	atomic_t irq_rerun;
 
 	unsigned default_timestamp:1;
 

-- 
2.43.0

[PATCH net v4 3/5] net: phy: balance a postponed PHY interrupt when the interrupt is freed

[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 and reruns the handler. phy_free_interrupt() can
run in between 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 ravb and cpsw, reach it 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 phy_free_interrupt() set irq_rerun too, so that a postponed
interrupt that follows leaves the line enabled. If a handler set it
first, phy_free_interrupt() waits for that handler and re-enables the
line before free_irq(), while the line is still its own, whether or
not other users remain. On a level-triggered line whose source
phy_disable_interrupts() did not clear, the interrupt then fires until
free_irq(), in the same call, removes the handler. 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
that take it. phy_interrupt() never takes the lock, so synchronize_irq()
and free_irq() can wait for the handler under 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>
---
 drivers/net/phy/phy.c        | 16 ++++++++++++-
 drivers/net/phy/phy_device.c | 54 ++++++++++++++++++++++++++++----------------
 include/linux/phy.h          |  8 ++++++-
 3 files changed, 56 insertions(+), 22 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index b47f0d3d99e5..49098b481388 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1430,7 +1430,9 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 				pm_wakeup_dev_event(parent, 0, true);
 		}
 
-		/* Disable the line only once: an edge can rerun the thread. */
+		/* Disable the line only once: an edge can rerun the thread, and
+		 * phy_free_interrupt() presets irq_rerun to keep it enabled.
+		 */
 		if (!atomic_xchg(&phydev->irq_rerun, 1))
 			disable_irq_nosync(irq);
 		return IRQ_HANDLED;
@@ -1508,7 +1510,19 @@ EXPORT_SYMBOL(phy_request_interrupt);
 void phy_free_interrupt(struct phy_device *phydev)
 {
 	phy_disable_interrupts(phydev);
+
+	/* Re-enable a line left disabled by a postponed interrupt while it
+	 * is still ours, and keep later postponed interrupts from disabling it.
+	 */
+	mutex_lock(&phydev->irq_pm_lock);
+	if (atomic_xchg(&phydev->irq_rerun, 1)) {
+		/* Wait for the handler that set irq_rerun to disable the line */
+		synchronize_irq(phydev->irq);
+		enable_irq(phydev->irq);
+	}
 	free_irq(phydev->irq, phydev);
+	atomic_set(&phydev->irq_rerun, 0);
+	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 d5d0efc10418..ef5e1cd3a831 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -332,6 +332,37 @@ 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 (atomic_xchg(&phydev->irq_rerun, 0)) {
+		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 +370,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)) {
-		WRITE_ONCE(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,18 +417,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)) {
-		WRITE_ONCE(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 (atomic_xchg(&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);
@@ -710,6 +723,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 48b4e852f230..04916fc29ed7 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -651,7 +651,11 @@ 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: Set by the postponed interrupt that disabled the line
+ * @irq_rerun: Set by the postponed interrupt that disabled the line, and
+ *             by phy_free_interrupt() to keep the line enabled
+ * @irq_pm_lock: Serializes phy_free_interrupt() against the rerun of a
+ *               postponed interrupt, so that only one of them re-enables
+ *               the line; 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
@@ -728,6 +732,8 @@ struct phy_device {
 	/* Written without phydev->lock, so not bitfields */
 	bool irq_suspended;
 	atomic_t 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 v4 4/5] 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, and rerun the postponed interrupt when phy_init_hw() or
phy_resume() fail, which gives the line back to the other users of a
shared line. A handler that then cannot read the PHY reports it through
phy_error(), as most PHY drivers do on any MDIO error.

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

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index ef5e1cd3a831..390069ec13ca 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -366,6 +366,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;
@@ -383,9 +384,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)
@@ -411,11 +423,11 @@ static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
 
 	ret = phy_init_hw(phydev);
 	if (ret < 0)
-		return ret;
+		goto err_resume_irq;
 
 	ret = phy_resume(phydev);
 	if (ret < 0)
-		return ret;
+		goto err_resume_irq;
 no_resume:
 	mdio_bus_phy_resume_irq(phydev);
 
@@ -423,6 +435,10 @@ static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
 		phy_start_machine(phydev);
 
 	return 0;
+
+err_resume_irq:
+	mdio_bus_phy_resume_irq(phydev);
+	return ret;
 }
 
 static SIMPLE_DEV_PM_OPS(mdio_bus_phy_pm_ops, mdio_bus_phy_suspend,

-- 
2.43.0

[PATCH net v4 5/5] 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(). stmmac
has set mac_managed_pm since the commit in the Fixes tag, which took
its PHYs out of the postponing added by commit 1758bde2e4aa ("net: phy:
Don't trigger state machine while in suspend").

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: macb, for one, calls phylink_stop() and
pm_runtime_force_suspend() from its own suspend callback, not
phylink_suspend().

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.
For a suspended PHY without driver-model wakeup, use the wol_enabled
that phy_suspend() stored, and do not read the PHY.

Any PHY reports a system wakeup when Wake-on-LAN is set through ethtool
or reported by phy_may_wakeup(), the two sources phy_suspend() counts.
Without it, a MAC that manages PHY PM stops the PHY itself, and the
link interrupt that follows is not a wakeup: it must not abort suspend
just because the MAC device may wake the system.

Tested on Helios64 with 7.3.1, 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: a magic packet woke the board 4 of 4 times,
  no suspend aborted.

Fixes: f151c147b3af ("net: stmmac: Enable mac_managed_pm phylink config")
Signed-off-by: Igor Velkov <iav@iav.lv>
---
 drivers/net/phy/phy.c        | 27 +++++++++++++++--------
 drivers/net/phy/phy_device.c | 52 ++++++++++++++++++++++++++++++++++++++++----
 include/linux/phy.h          | 22 +++++++++++++++++--
 3 files changed, 86 insertions(+), 15 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 49098b481388..54c099f96b4b 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1411,23 +1411,32 @@ int phy_disable_interrupts(struct phy_device *phydev)
 static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 {
 	struct phy_device *phydev = phy_dat;
+	bool postponed;
 	irqreturn_t ret;
 
+	/* Pairs with smp_store_release() in mdio_bus_phy_suspend_irq() */
+	postponed = smp_load_acquire(&phydev->irq_suspended);
+
 	/* Wakeup interrupts may occur during a system sleep transition.
 	 * Postpone handling until the PHY has resumed.
 	 */
-	if (IS_ENABLED(CONFIG_PM_SLEEP) && READ_ONCE(phydev->irq_suspended)) {
+	if (IS_ENABLED(CONFIG_PM_SLEEP) && postponed) {
 		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, and
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 390069ec13ca..ebcbae86f902 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -332,16 +332,55 @@ 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_enabled;
+	else
+		phy_wol = phy_may_wakeup(phydev);
+
+	if (netdev->ethtool->wol_enabled || phy_wol)
+		return PHY_IRQ_WAKE_SYSTEM;
+
+	/* A MAC that manages PHY PM stops the PHY itself: the link
+	 * interrupt that follows is not a wakeup.
+	 */
+	if (phydev->mac_managed_pm)
+		return PHY_IRQ_WAKE_NONE;
+
+	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 PHY
  * has resumed. Wait for concurrent interrupt handler to complete.
+ * Decide which wakeup event a postponed interrupt reports now, while
+ * the PHY can still be read.
  */
 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);
+	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);
 }
 
@@ -368,11 +407,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
@@ -405,8 +447,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;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 04916fc29ed7..c2e09963c003 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
  *
@@ -649,10 +663,13 @@ 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: Set by the postponed interrupt that disabled the line, and
  *             by phy_free_interrupt() to keep the line enabled
+ * @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, so that only one of them re-enables
  *               the line; never taken by phy_interrupt()
@@ -732,6 +749,7 @@ struct phy_device {
 	/* Written without phydev->lock, so not bitfields */
 	bool irq_suspended;
 	atomic_t 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 v4 0/5] 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 code inspection). If it was triggered please include
   the symptoms, like the stack trace or error 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 v4 0/5] net: phy: postpone PHY interrupts during sleep with MAC-managed PM

[Igor Velkov]

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

Patch 5 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-4 fix defects found by code inspection and Sashiko review;
they were not triggered.

Re: [PATCH net v4 2/5] net: phy: disable the line of a postponed PHY interrupt only once

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 (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] This commit introduces the problem, and a later commit in the
  series fixes it.

Pre-existing issues:
- [Medium] Not fixed by this commit, and the series fixes it later.

--

net: phy: disable the line of a postponed PHY interrupt only once

This commit changes phydev->irq_rerun to an atomic_t. Only the postponed
phy_interrupt() run that sets it now calls disable_irq_nosync(), which
avoids a double disable when an edge reruns the thread during a sleep
transition. The irq_suspended accesses also get READ_ONCE() and
WRITE_ONCE() annotations.

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

[ ... ]

> @@ -1430,8 +1430,9 @@ 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 (!atomic_xchg(&phydev->irq_rerun, 1))
> +			disable_irq_nosync(irq);
>  		return IRQ_HANDLED;
>  	}

[Severity: Medium]
What happens here if irq_rerun is still 1 from an earlier cycle and the
line has since been re-requested?

At this commit, only the successful no_resume path of
mdio_bus_phy_resume() clears irq_rerun. phy_free_interrupt() does not
reset it:

drivers/net/phy/phy.c:phy_free_interrupt() {
	phy_disable_interrupts(phydev);
	free_irq(phydev->irq, phydev);
}

When the PHY is the only user of the line, free_irq() shuts it down. The
next request_threaded_irq() in phy_request_interrupt() goes through
irq_startup(), which sets desc->depth = 0. So the line is enabled while
irq_rerun is still 1.

There seem to be two ways to reach that state:

  a) mdio_bus_phy_resume() returns early on a phy_init_hw() or
     phy_resume() failure. That leaves irq_suspended == true and
     irq_rerun == 1. Later the interface is cycled through
     phy_disconnect()/phy_connect().

  b) A MAC driver without mac_managed_pm frees and re-requests the PHY
     IRQ in its own PM callbacks. The parent MAC resumes before the PHY,
     so the re-request runs while irq_suspended is still true.

In both cases, the next PHY interrupt takes the postponed branch and sees
irq_rerun == 1. It skips disable_irq_nosync() and returns IRQ_HANDLED
without calling drv->handle_interrupt().

The PHY interrupt status is never cleared. On a level-triggered
IRQF_ONESHOT line, the IRQ core unmasks after the thread returns and the
interrupt fires again right away.

Could this become an IRQ storm, with no end in case a) and lasting until
PHY resume in case b)?

In case b), mdio_bus_phy_resume() then runs:

		if (atomic_xchg(&phydev->irq_rerun, 0)) {
			enable_irq(phydev->irq);

on a line at depth 0. Would that trigger the "Unbalanced enable for IRQ"
warning? Before this patch, every postponed run disabled the line, so
the later enable_irq() stayed balanced.

Two later patches in this series appear to resolve this: "net: phy:
balance a postponed PHY interrupt when the interrupt is freed" and "net:
phy: stop postponing PHY interrupts when a PHY PM callback fails". The
first resets irq_rerun in phy_free_interrupt(). The second routes the
resume error paths through mdio_bus_phy_resume_irq().

Since this commit has a Fixes: tag, would a stable backport of it
without those follow-ups bring in this regression?

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 79dba43d703a2..d5d0efc104187 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 = true;
> +		WRITE_ONCE(phydev->irq_suspended, true);
>  		synchronize_irq(phydev->irq);
>  	}

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. What
clears irq_suspended if phy_suspend() fails?

mdio_bus_phy_suspend() sets the flag here and then ends with:

	phydev->suspended_by_mdio_bus = 1;

	return phy_suspend(phydev);

If phy_suspend() returns an error, the PM core skips this device's
resume callback in device_resume(), so irq_suspended stays true.

Every later phy_interrupt() run then takes the postponed branch. The
first one disables the line, and nothing re-enables it until another
full suspend/resume cycle.

mdio_bus_phy_resume() seems to end up the same way on its error paths:

	ret = phy_init_hw(phydev);
	if (ret < 0)
		return ret;

	ret = phy_resume(phydev);
	if (ret < 0)
		return ret;
no_resume:

Both returns skip the block that clears irq_suspended, consumes
irq_rerun and calls enable_irq().

Would the PHY link interrupt then stay disabled, so link changes are
lost?

The later patch in this series, "net: phy: stop postponing PHY
interrupts when a PHY PM callback fails", appears to fix both paths by
calling mdio_bus_phy_resume_irq() on these failures.

[ ... ]

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

Re: [PATCH net v4 3/5] net: phy: balance a postponed PHY interrupt when the interrupt is freed

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] net: phy: SCHED_FIFO IRQ thread livelock in phy_free_interrupt()
- [Medium] phy_free_interrupt() (drivers/net/phy/phy.c) now calls
  enable_irq() before free_irq() whenever a postponed interrupt left the…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  mdio_bus_phy_suspend() (drivers/net/phy/phy_device.c) sets…

--

net: phy: balance a postponed PHY interrupt when the interrupt is freed

This patch makes phy_free_interrupt() preset irq_rerun. If a postponed
interrupt has already left the line disabled, phy_free_interrupt() now
re-enables it before free_irq(). A new irq_pm_lock serializes this against
the rerun after resume, and the suspend/resume IRQ handling moves into the
helpers mdio_bus_phy_suspend_irq() and mdio_bus_phy_resume_irq().

> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index b47f0d3d99e55..49098b4813884 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c

[ ... ]

> @@ -1508,7 +1510,19 @@ EXPORT_SYMBOL(phy_request_interrupt);
>  void phy_free_interrupt(struct phy_device *phydev)
>  {
>  	phy_disable_interrupts(phydev);
> +
> +	/* Re-enable a line left disabled by a postponed interrupt while it
> +	 * is still ours, and keep later postponed interrupts from disabling it.
> +	 */
> +	mutex_lock(&phydev->irq_pm_lock);
> +	if (atomic_xchg(&phydev->irq_rerun, 1)) {
> +		/* Wait for the handler that set irq_rerun to disable the line */
> +		synchronize_irq(phydev->irq);
> +		enable_irq(phydev->irq);
> +	}
>  	free_irq(phydev->irq, phydev);

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

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

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

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

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

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

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

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

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

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

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

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

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

In that case the sequence would be:

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

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

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

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

The commit message says:

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

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

> +	atomic_set(&phydev->irq_rerun, 0);
> +	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 d5d0efc104187..ef5e1cd3a8319 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -339,14 +370,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)) {
> -		WRITE_ONCE(phydev->irq_suspended, true);
> -		synchronize_irq(phydev->irq);
> -	}
> +	mdio_bus_phy_suspend_irq(phydev);

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

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

	phydev->suspended_by_mdio_bus = 1;

	return phy_suspend(phydev);
}

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

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

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

>  
>  	/* We must stop the state machine manually, otherwise it stops out of
>  	 * control, possibly with the phydev->lock held. Upon resume, netdev

[ ... ]

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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · 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] Interrupt storm and WARN flood when a PHY PM callback fails
  because MDIO access is broken, which can then disable the shared IRQ…
- [Low] Comments that say interrupt handling is postponed 'until the PHY
  has resumed' are now wrong on the failure paths.

--

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 postponement and restarts the state machine. It also
changes mdio_bus_phy_resume() so that a failed phy_init_hw() or
phy_resume() reruns the postponed PHY interrupt. Before this, the function
returned with the line still disabled.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index ef5e1cd3a8319..390069ec13cac 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

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

[ ... ]

> @@ -411,11 +423,11 @@ static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
>  
>  	ret = phy_init_hw(phydev);
>  	if (ret < 0)
> -		return ret;
> +		goto err_resume_irq;
>  
>  	ret = phy_resume(phydev);
>  	if (ret < 0)
> -		return ret;
> +		goto err_resume_irq;
>  no_resume:
>  	mdio_bus_phy_resume_irq(phydev);
>  
> @@ -423,6 +435,10 @@ static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
>  		phy_start_machine(phydev);
>  
>  	return 0;
> +
> +err_resume_irq:
> +	mdio_bus_phy_resume_irq(phydev);
> +	return ret;

[Severity: Medium]
Can this cause an interrupt storm when phy_init_hw() or phy_resume() fails
because the PHY can't be reached over MDIO?

Suppose a postponed interrupt set irq_rerun during the sleep transition.
mdio_bus_phy_resume_irq() then re-enables the line and wakes the thread. It
does not check whether the PHY can be accessed:

drivers/net/phy/phy_device.c:mdio_bus_phy_resume_irq() {
    ...
	if (atomic_xchg(&phydev->irq_rerun, 0)) {
		enable_irq(phydev->irq);
		irq_wake_thread(phydev->irq, phydev);
	}
    ...
}

irq_suspended is now clear, so phy_interrupt() calls
drv->handle_interrupt(). If the MDIO read fails, a typical handler can't
acknowledge the source:

drivers/net/phy/micrel.c:kszphy_handle_interrupt() {
	irq_status = phy_read(phydev, MII_KSZPHY_INTCS);
	if (irq_status < 0) {
		phy_error(phydev);
		return IRQ_NONE;
	}
    ...
}

dp83848_handle_interrupt() does the same thing. phy_error() is an
unconditional WARN_ON(1) followed by phy_process_error(). That only sets
PHY_ERROR and triggers the state machine. It does not mask the interrupt in
the PHY and does not free the IRQ.

The line is requested with IRQF_ONESHOT | IRQF_SHARED. If the PHY is still
powered and keeps its level INT_N asserted, the line fires again as soon as
the thread finishes.

Would each pass then cost a failed MDIO transaction and a full WARN
backtrace? That would repeat until the spurious IRQ detector disables the
line for every sharer. With panic_on_warn set, the first rerun would panic.

The rollback in mdio_bus_phy_suspend() above takes the same path when
drv->suspend() fails on an MDIO error.

Before this patch, the line just stayed disabled in this case. The commit
message says:

    ... which gives the line back to the other users of a
    shared line. A handler that then cannot read the PHY reports it through
    phy_error(), as most PHY drivers do on any MDIO error.

Can other users of a shared line actually get it back if the PHY's own
interrupt source can't be acknowledged? Whether INT_N stays asserted while
MDIO fails depends on the hardware. Nothing on this path guards against it,
though.

[Severity: Low]
This isn't a bug, but several comments still say the postponed interrupt is
handled only once the PHY has resumed. On the new failure paths, the
postponement ends even though the PHY never resumed:

drivers/net/phy/phy.c:phy_interrupt():
	/* Wakeup interrupts may occur during a system sleep transition.
	 * Postpone handling until the PHY has resumed.
	 */

drivers/net/phy/phy_device.c, above mdio_bus_phy_suspend_irq():
/* Wakeup interrupts may occur during the system sleep transition when
 * the PHY is inaccessible. Set flag to postpone handling until the PHY
 * has resumed. Wait for concurrent interrupt handler to complete.
 */

include/linux/phy.h, struct phy_device kernel-doc:
 * @irq_suspended: Flag indicating PHY is suspended and therefore interrupt
 *                 handling shall be postponed until PHY has resumed

A later patch in the series, "net: phy: postpone PHY interrupts during
sleep with MAC-managed PM", changes the phy.h kernel-doc to "postponed
until mdio_bus_phy_resume()". The comments in phy.c and phy_device.c are
still unchanged at the end of the series.

Should those two comments be updated as well?

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

Re: [PATCH net v4 5/5] 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: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] net: phy: false wakeup in phy_interrupt() during mac_managed_pm
  suspend

--

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -332,16 +332,55 @@ 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_enabled;
> +	else
> +		phy_wol = phy_may_wakeup(phydev);
> +
> +	if (netdev->ethtool->wol_enabled || phy_wol)
> +		return PHY_IRQ_WAKE_SYSTEM;
> +
> +	/* A MAC that manages PHY PM stops the PHY itself: the link
> +	 * interrupt that follows is not a wakeup.
> +	 */
> +	if (phydev->mac_managed_pm)
> +		return PHY_IRQ_WAKE_NONE;
> +
> +	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 PHY
>   * has resumed. Wait for concurrent interrupt handler to complete.
> + * Decide which wakeup event a postponed interrupt reports now, while
> + * the PHY can still be read.
>   */
>  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);
> +	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);
>  }
>  

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

I can see two ways to get there.

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

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

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

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

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