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

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

[Aleksei Sviridkin]

Two fixes on the same path: a PHY whose driver is a module on a rootfs
that is not mounted when the MAC probes.

Patch 1 clears a stale pl->phydev when bringup fails after recording it.
The code is unchanged since v3, where it got a Reviewed-by. The
changelog is not. It used to say the SFP path turns the failure into a
permanent -EBUSY, and that understates it: sfp_sm_probe_phy() frees the
phy_device on that error and assigns sfp->mod_phy only past the return,
so pl->phydev is left pointing at freed memory. That matters for stable,
so the wording gets a version of its own.

Patch 2 gives back the interrupt phy_probe() replaced with PHY_POLL. It
is five lines in phy_detach() and nothing else.

v5 kept the number in a new phy_device field. v6 dropped the field for
mdiobus->irq[], which is what Andrew asked for, but kept the restore in
phy_remove() behind a phydev->phy_link_change test. Both are gone here.

Recording at one clobber site was never enough: phy_attach_direct()
substitutes PHY_POLL in two more places, so a number lost there was
never given back. Taking the value from the bus at detach covers all
three, because detach ends every bind cycle and the bus is where the
number came from.

The phy_link_change test could not be a guard. phy_remove() reads it
under the device lock, phy_attach_direct() sets it on the rtnl side and
takes no device lock at all, so by the time the restore acted on that
test the test could already be stale. phy_detach() has one writer on one
side and the question goes away.

The case that test was there for is a sysfs unbind reaching a PHY that
still has a consumer. I ran it on the board. It takes the box down in
phy_polling_mode(), which reads phydev->drv->update_stats with no NULL
check, well before the interrupt number matters.

A bus whose driver writes only phydev->irq and never the table is not
covered: the table holds PHY_POLL and there is nothing to take back.
lan78xx, smsc95xx and sxgbe are in that position today, and registering
the interrupt with the bus is theirs to do.

Measured on an MT7981B board, an MT7531 switch port with an Airoha
EN8811H whose driver is a module. The generic driver binds first, and
the number is read out either side of the detach that releases it:

  with patch 2:     bound: irq -1   after detach: irq 15
  without patch 2:  bound: irq -1   after detach: irq -1

-1 is PHY_POLL, 15 is what the device tree gives that PHY. Reaching that
state needs a kernel that lets the generic driver bind where this board
would normally refuse it, so both numbers come from a modified poller.
The patch under test is the only difference between the two builds. 485
passes of that cycle under ifdown/ifup and sysfs churn read the same,
with no warning and no free_irq complaint.

The restore runs on every ordinary detach as well, so I measured it
there too, with nothing modified: twenty unbind and rebind rounds of the
switch driver, each one a real teardown and setup of four ports. The
EN8811H came back with irq 15 every time, the three internal PHYs with
79, 80 and 81, and ethtool -r moved the counter in /proc/interrupts
afterwards, so the number that comes back is a live interrupt. That
needed two local fixes to the switch driver's remove path, which crashes
on unbind on this chip. They are not part of this series.

Previous posting:
https://lore.kernel.org/netdev/20260908155025.4155289-1-f@lex.la/

Aleksei Sviridkin (2):
  net: phylink: unwind the PHY binding when bringup fails late
  net: phy: take the interrupt back from the bus on detach

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


base-commit: e0554c6276da957b6e72849520c70a97404cd1ae
-- 
2.53.0

[PATCH net v7 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() goes through phylink_attach_phy(), which refuses
to attach while pl->phydev is set, turning a transient MDIO error into
a permanent -EBUSY. The SFP path is worse than that: sfp_sm_probe_phy()
answers the failure with phy_device_remove() and phy_device_free(), and
it assigns sfp->mod_phy only past that error return, so nothing clears
pl->phydev and it is left pointing at a freed phy_device that
phylink_resolve() and the ethtool helpers go on reading.
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")
Assisted-by: LLM
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 3ec3bb439109..6a92fac58f25 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.53.0

[PATCH net v7 2/2] net: phy: take the interrupt back from the bus on detach

[Aleksei Sviridkin]

A PHY whose own driver is a module on a filesystem that is not mounted
when the MAC probes gets the generic driver first. phy_probe() replaces
phydev->irq with PHY_POLL because that driver has no interrupt support,
nothing puts it back, and the PHY polls for the rest of the uptime once
its real driver takes over.

Take the number back in phy_detach(), from mdiobus->irq[], which is
where phy_device_create() seeded phydev->irq from and where the bus that
described the interrupt still holds it. Detach is the end of every bind
cycle, so this covers the two substitutions phy_attach_direct() makes as
well as the one in phy_probe(), without any of them having to record
anything.

Doing it here rather than from phy_remove() keeps a single writer on the
rtnl side. phy_attach_direct() is what reads the number back and decides
whether to request an interrupt, and it holds no lock against the driver
core, so a restore driven by an unbind would be racing that decision
rather than ordered against it.

A bus whose driver writes only phydev->irq and never the table is not
covered, because the table then holds PHY_POLL and there is nothing to
take back; lan78xx, smsc95xx and sxgbe are in that position today and
registering the interrupt with the bus is theirs to do.

Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
---
 drivers/net/phy/phy_device.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 94b2e85e00a3..84e2da81dbd3 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1969,6 +1969,11 @@ void phy_detach(struct phy_device *phydev)
 		phydev->is_genphy_driven = 0;
 	}
 
+	/* Whatever this attachment did to the interrupt, the bus that
+	 * described it still knows the number. Take it back from there.
+	 */
+	phydev->irq = phydev->mdio.bus->irq[phydev->mdio.addr];
+
 	/* Assert the reset signal */
 	phy_device_reset(phydev, 1);
 
-- 
2.53.0