[PATCH net v3 0/2] net: fix a stale phylink PHY pointer and a lost PHY interrupt

[PATCH net v3 0/2] net: fix a stale phylink PHY pointer and a lost PHY interrupt

[Aleksei Sviridkin]

Two independent fixes, both found while chasing a PHY whose driver is a
module on a rootfs that is not mounted yet when a DSA switch probes.
Neither one depends on that setup, and neither depends on the other.

Patch 1: phylink_bringup_phy() records the PHY in pl->phydev before its
last fallible step, so a failure there leaves a pointer to a PHY the
caller has already detached. A later phylink_disconnect_phy() detaches
it a second time and drops references the first detach already
released.

Patch 2: a PHY that goes through a generic-driver bind cycle comes out
of it in polling mode for good. The specific driver that binds
afterwards never sees the interrupt the firmware node declared.

Tested on an MT7981B board: an Airoha EN8811H on an MT7531 switch port,
its interrupt declared in the device tree, its driver a module on the
rootfs. lan4 attaches with irq=15 rather than irq=POLL, the line is
claimed as mt-eint 0 in /proc/interrupts, and its counter goes 1 -> 3
-> 5 across two forced aneg restarts, matching the link dropping and
coming back each time, and holding steady in between. wan, whose
internal PHY has no interrupt in the device tree, still attaches with
irq=POLL: that is the observation which says the bus table cannot hand
back an interrupt the device never had. Patch 2's other exit, the one
in phy_attach_direct(), needs a generic probe to fail and is
compile-tested only.

---
Changes in v3:
 - retargeted at net (Andrew Lunn, Paolo Abeni)
 - patch 2: added a Fixes: tag naming the commit that introduced
   phylib, where both halves of the cycle arrived together; the commit
   message drops the phrase Andrew flagged and says what
   mdiobus_alloc() does to bus->irq[] instead, and answers why the
   restore is conditional; the analysis below the scissors is replaced
   by a link to v2 (Andrew Lunn, Paolo Abeni)
 - patch 2: reword the new helper's comment to the mdiobus_alloc()
   form (Andrew Lunn)
 - patch 1: carries Andrew's Reviewed-by, otherwise untouched
 - that comment is the only diff change since v2; no code flow
   changed, so the test results above still describe this code
 - v2: https://lore.kernel.org/netdev/20260824024029.41310-1-f@lex.la/
 - v1: https://lore.kernel.org/netdev/20260822155259.87146-1-f@lex.la/

Aleksei Sviridkin (2):
  net: phylink: unwind the PHY binding when bringup fails late
  net: phy: restore the interrupt after a generic-driver bind cycle

 drivers/net/phy/phy_device.c | 14 ++++++++++++++
 drivers/net/phy/phylink.c    | 29 ++++++++++++++++++++---------
 2 files changed, 34 insertions(+), 9 deletions(-)

-- 
2.55.0

[PATCH net v3 1/2] net: phylink: unwind the PHY binding when bringup fails late

[Aleksei Sviridkin]

phylink_bringup_phy() records the PHY in pl->phydev before its last
fallible step: on a MAC whose phylink ops implement LPI,
phy_eee_rx_clock_stop() can fail with a real MDIO error. The callers
unwind with phy_detach(), which knows nothing about pl->phydev, so a
pointer to a PHY that is no longer attached outlives the failed
connect.

What that costs depends on how the caller got here.
phylink_connect_phy() and the SFP path go through
phylink_attach_phy(), which refuses to attach while pl->phydev is set
and turns a transient MDIO error into a permanent -EBUSY.
phylink_fwnode_phy_connect() has no such check, so a later connect
overwrites the stale pointer and hides the problem. A disconnect does
not: phylink_disconnect_phy() hands that pointer to phy_disconnect(),
and the second phy_detach() on the same PHY drops references the first
one already released.

Clear the binding on the failure path. This is the same operation
phylink_disconnect_phy() performs, so both now share a helper. The
PHY-side fields are left to phy_detach(), which every caller already
runs on this path.

Fixes: 03abf2a7c654 ("net: phylink: add EEE management")
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/phylink.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 5b8e956902fb..a55e4a64028f 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2083,6 +2083,18 @@ static int phylink_validate_phy(struct phylink *pl, struct phy_device *phy,
 	return phylink_validate(pl, supported, state);
 }
 
+/* Disassociate @phy from @pl. Caller must hold pl->phydev_mutex. */
+static void phylink_clear_phydev(struct phylink *pl, struct phy_device *phy)
+{
+	mutex_lock(&phy->lock);
+	mutex_lock(&pl->state_mutex);
+	pl->phydev = NULL;
+	pl->phy_enable_tx_lpi = false;
+	pl->mac_tx_clk_stop = false;
+	mutex_unlock(&pl->state_mutex);
+	mutex_unlock(&phy->lock);
+}
+
 static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 			       phy_interface_t interface)
 {
@@ -2197,6 +2209,12 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 	if (ret == 0 && phy_interrupt_is_valid(phy))
 		phy_request_interrupt(phy);
 
+	if (ret) {
+		mutex_lock(&pl->phydev_mutex);
+		phylink_clear_phydev(pl, phy);
+		mutex_unlock(&pl->phydev_mutex);
+	}
+
 	return ret;
 }
 
@@ -2347,15 +2365,8 @@ void phylink_disconnect_phy(struct phylink *pl)
 
 	mutex_lock(&pl->phydev_mutex);
 	phy = pl->phydev;
-	if (phy) {
-		mutex_lock(&phy->lock);
-		mutex_lock(&pl->state_mutex);
-		pl->phydev = NULL;
-		pl->phy_enable_tx_lpi = false;
-		pl->mac_tx_clk_stop = false;
-		mutex_unlock(&pl->state_mutex);
-		mutex_unlock(&phy->lock);
-	}
+	if (phy)
+		phylink_clear_phydev(pl, phy);
 	mutex_unlock(&pl->phydev_mutex);
 
 	if (phy) {
-- 
2.55.0

[PATCH net v3 2/2] net: phy: restore the interrupt after a generic-driver bind cycle

[Aleksei Sviridkin]

A PHY with no specific driver available at attach time gets the generic
one, and phy_probe() sets phydev->irq to PHY_POLL because that driver
has no interrupt callbacks. Neither end of that bind cycle puts the
value back: phy_detach() releases the generic driver so a real one can
bind later, and a generic probe that fails never reaches phy_detach()
at all.

The specific driver that binds afterwards therefore starts with
irq == PHY_POLL, and the PHY is polled for the rest of the uptime with
no warning on that path. A DSA switch that connects its user ports
before the rootfs holding the PHY driver module is mounted hits this on
every boot.

mdiobus_alloc() fills bus->irq[] with PHY_POLL for every address, and
the bind cycle never writes to that table, so the entry still holds
whatever the bus registered there. Restore phydev->irq from it on both
exits, and only where the cycle left PHY_POLL. That guard preserves an
interrupt mode a MAC installed on the attached PHY after connect, and
it keeps a restored interrupt number out of the
phy_connect_direct()/phy_disconnect() asymmetry, where such a MAC would
have the interrupt requested and never freed.

Fixes: 00db8189d984 ("This patch adds a PHY Abstraction Layer to the Linux Kernel, enabling ethernet drivers to remain as ignorant as is reasonable of the connected PHY's design and operation details.")
Signed-off-by: Aleksei Sviridkin <f@lex.la>
---
Both exits matter: phy_detach() for a generic driver that bound and is
being released, and phy_attach_direct()'s error_module_put label for a
generic probe that failed, which never calls phy_detach().

Which buses and MAC drivers the bus interrupt table covers, which other
paths to PHY_POLL the guard also restores and why none of them is
harmed, and the sysfs unbind case this does not cover, are worked
through under v2:
https://lore.kernel.org/netdev/20260824024029.41310-3-f@lex.la/
 drivers/net/phy/phy_device.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 94b2e85e00a3..be4c35db8de9 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1734,6 +1734,18 @@ static bool phy_drv_supports_irq(const struct phy_driver *phydrv)
 	return phydrv->config_intr && phydrv->handle_interrupt;
 }
 
+/* Give back the interrupt phy_probe() parked when a driver with no interrupt
+ * callbacks bound. mdiobus_alloc() defaults bus->irq[] to PHY_POLL and the
+ * bind cycle does not touch the table, so whatever the bus recorded there
+ * still stands. Only the parking is undone: any other value the PHY carries
+ * was put there by someone else.
+ */
+static void phy_restore_genphy_irq(struct phy_device *phydev)
+{
+	if (phydev->irq == PHY_POLL)
+		phydev->irq = phydev->mdio.bus->irq[phydev->mdio.addr];
+}
+
 /**
  * phy_attach_direct - attach a network device to a given PHY device pointer
  * @dev: network device to attach
@@ -1896,6 +1908,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 error_module_put:
 	module_put(d->driver->owner);
+	phy_restore_genphy_irq(phydev);
 	phydev->is_genphy_driven = 0;
 	d->driver = NULL;
 error_put_device:
@@ -1965,6 +1978,7 @@ void phy_detach(struct phy_device *phydev)
 	 * real driver could be loaded
 	 */
 	if (phydev->is_genphy_driven) {
+		phy_restore_genphy_irq(phydev);
 		device_release_driver(&phydev->mdio.dev);
 		phydev->is_genphy_driven = 0;
 	}
-- 
2.55.0