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

[PATCH net v6 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,
so a later disconnect cannot detach the same PHY twice. Unchanged since
v3, where it got a Reviewed-by.

Patch 2 gives back the interrupt phy_probe() replaced with PHY_POLL. v5
kept the number in a new struct phy_device field. Andrew asked for
mdiobus->irq[] to be the source of truth instead, so v6 reads it from
there and the field is gone; the patch is now four lines of logic and no
new state.

That leaves buses whose driver writes only phydev->irq and never the
table. On those the table holds PHY_POLL and there is nothing to give
back, so the fix does not reach them. Three are in that position:
lan78xx and smsc95xx, which Andrew named, and sxgbe_mdio, which writes
its own irqlist and the phydev but not the bus. Converting them is their
maintainers' call and I have left them alone here rather than send a
series across four subsystems; say the word and they follow.

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. The cycle
repeats once a second and every pass on both builds reads the same.
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.

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

Aleksei Sviridkin (2):
  net: phylink: unwind the PHY binding when bringup fails late
  net: phy: restore the interrupt the bus gave a PHY

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


base-commit: 38b6be101006d3e7af972999f45d4f1e8250587a
-- 
2.53.0

[PATCH net v6 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")
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 v6 2/2] net: phy: restore the interrupt the bus gave a PHY

[Aleksei Sviridkin]

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

Put it back from mdiobus->irq[], which is where the number came from:
phy_device_create() seeds phydev->irq out of that table, so the bus that
described the interrupt still holds it. Restore at the three points the
bind cycle can end, phy_remove(), phy_probe()'s own error exit and the
unwind in phy_attach_direct(), so a bind that is undone by any path
leaves the PHY as it was found.

Skip it while phy_link_change marks an attached consumer. That consumer
called phy_request_interrupt() on the PHY_POLL it saw and would free an
interrupt it never requested.

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
give back; lan78xx, smsc95xx and sxgbe are in that position today and
registering the interrupt with the bus is theirs to do.

This needs commit e0d1c55501d3 ("net: phy: fix
phy_uses_state_machine()") to be of any use: without it the mark this
skips on is never cleared once a consumer has attached, and the restore
never runs.

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

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 94b2e85e00a3..b55402569300 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1734,6 +1734,19 @@ static bool phy_drv_supports_irq(const struct phy_driver *phydrv)
 	return phydrv->config_intr && phydrv->handle_interrupt;
 }
 
+/* Give back what phy_probe() took, from the bus that owns the number, but
+ * not while phy_link_change marks a consumer: it skipped
+ * phy_request_interrupt() on the value it saw, so phy_disconnect() would
+ * free an interrupt nobody requested.
+ */
+static void phy_restore_probe_irq(struct phy_device *phydev)
+{
+	if (phydev->phy_link_change || phydev->irq != PHY_POLL)
+		return;
+
+	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 +1909,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 error_module_put:
 	module_put(d->driver->owner);
+	phy_restore_probe_irq(phydev);
 	phydev->is_genphy_driven = 0;
 	d->driver = NULL;
 error_put_device:
@@ -3820,6 +3834,8 @@ static int phy_probe(struct device *dev)
 	if (!phydev->is_on_sfp_module)
 		phy_led_triggers_unregister(phydev);
 
+	phy_restore_probe_irq(phydev);
+
 	/* Re-assert the reset signal on error */
 	phy_device_reset(phydev, 1);
 
@@ -3848,6 +3864,8 @@ static int phy_remove(struct device *dev)
 	if (phydev->drv && phydev->drv->remove)
 		phydev->drv->remove(phydev);
 
+	phy_restore_probe_irq(phydev);
+
 	/* Assert the reset signal */
 	phy_device_reset(phydev, 1);
 
-- 
2.53.0

Re: [PATCH net v6 2/2] net: phy: restore the interrupt the bus gave a PHY

[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

- [Medium] phy_restore_probe_irq() bails out whenever
  phydev->phy_link_change is non-NULL and records no pending…
- [Medium] phy_restore_probe_irq() performs an unsynchronized check-then-
  act on state owned by the rtnl-side attach/detach paths.
- [Low] No Fixes: tag (and no target-tree indication) on a patch whose
  changelog describes a concrete, lasting user-visible defect.

--

net: phy: restore the interrupt the bus gave a PHY

phy_probe() demotes phydev->irq to PHY_POLL when the binding driver has no
interrupt support, and nothing restores it afterwards.  This patch adds a
helper that copies the number back out of mdiobus->irq[], and calls it from
phy_remove(), phy_probe()'s error exit and the phy_attach_direct() unwind.
The helper skips the restore while phydev->phy_link_change marks an attached
consumer.

>     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
>     give back; lan78xx, smsc95xx and sxgbe are in that position today and
>     registering the interrupt with the bus is theirs to do.
>
>     This needs commit e0d1c55501d3 ("net: phy: fix
>     phy_uses_state_machine()") to be of any use: without it the mark this
>     skips on is never cleared once a consumer has attached, and the restore
>     never runs.
>
>     Assisted-by: LLM
>     Signed-off-by: Aleksei Sviridkin <f@lex.la>

[Severity: Low]
The changelog describes a lasting user-visible defect ("polls for the rest
of the uptime once the real driver takes over"), but the tag block carries
no Fixes: line and the subject has no net/net-next scoping.

The assignment being undone, in phy_probe():

	if (!phy_drv_supports_irq(phydrv) && phy_interrupt_is_valid(phydev))
		phydev->irq = PHY_POLL;

is the natural blame target.  Could a Fixes: tag pointing at it (or an
explicit statement that this targets net-next as a behavioural change) be
added?

This matters more than usual given the stated dependency on e0d1c55501d3,
which is what clears phydev->phy_link_change in phy_detach().  On a tree
without that commit the new restore in phy_remove() is a silent no-op, so a
backport applies cleanly and changes nothing.  Should that boundary be
spelled out for stable?

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 94b2e85e00a3..b55402569300 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1734,6 +1734,19 @@ static bool phy_drv_supports_irq(const struct phy_driver *phydrv)
>  	return phydrv->config_intr && phydrv->handle_interrupt;
>  }
>
> +/* Give back what phy_probe() took, from the bus that owns the number, but
> + * not while phy_link_change marks a consumer: it skipped
> + * phy_request_interrupt() on the value it saw, so phy_disconnect() would
> + * free an interrupt nobody requested.
> + */
> +static void phy_restore_probe_irq(struct phy_device *phydev)
> +{
> +	if (phydev->phy_link_change || phydev->irq != PHY_POLL)
> +		return;
> +
> +	phydev->irq = phydev->mdio.bus->irq[phydev->mdio.addr];
> +}

[Severity: Medium]
Can this check-then-act run concurrently with the attach/detach paths that
own phydev->phy_link_change and phydev->irq?

The helper reads phy_link_change and irq, then writes irq, running from
phy_probe()/phy_remove() under the mdio device lock only.  The writers of
that same state take neither the mdio device lock nor phydev->lock:

phy_attach_direct()
	phydev->phy_link_change = phy_link_change;
	...
	if (phydev->dev_flags & PHY_F_NO_IRQ)
		phydev->irq = PHY_POLL;

	if (!phy_drv_supports_irq(phydev->drv) && phy_interrupt_is_valid(phydev))
		phydev->irq = PHY_POLL;

phy_detach()
	phydev->phy_link_change = NULL;

phy_driver_register() leaves suppress_bind_attrs unset, so a root sysfs
unbind can enter phy_remove() at any time, and __device_release_driver()
clears dev->driver only after ->remove() returns, so a concurrent
phy_attach_direct() still sees d->driver != NULL and proceeds.

One interleaving:

CPU0 phy_remove() -> phy_restore_probe_irq()
	reads phy_link_change == NULL, irq == PHY_POLL
	(preempted before the store)

CPU1 phy_attach_direct()
	phydev->phy_link_change = phy_link_change;
	phydev->irq = PHY_POLL;		/* no phy_request_interrupt() */

CPU0 resumes
	phydev->irq = phydev->mdio.bus->irq[phydev->mdio.addr];

Does that leave a valid IRQ published for a consumer that requested
nothing, so that teardown hits:

phy_disconnect()
	if (phy_interrupt_is_valid(phydev))
		phy_free_interrupt(phydev);

and free_irq(phydev->irq, phydev) warns about freeing an already-free IRQ?
phy_polling_mode() is also false at that point, so link changes would be
neither polled nor interrupt driven, which is the situation the new comment
says it avoids.

The reverse order gives the opposite outcome: phy_remove() observes a still
non-NULL phy_link_change just before phy_detach() clears it, and the PHY is
left unbound with irq == PHY_POLL.

Would making the individual accesses atomic help here, or does the decision
plus the store need to be serialized against the whole attach/detach and
interrupt request/free sequence, for example by driving the restore from
phy_detach() under rtnl?

> @@ -3848,6 +3864,8 @@ static int phy_remove(struct device *dev)
>  	if (phydev->drv && phydev->drv->remove)
>  		phydev->drv->remove(phydev);
>
> +	phy_restore_probe_irq(phydev);
> +

[Severity: Medium]
When phy_restore_probe_irq() takes the phy_link_change early return here, is
the interrupt number ever restored later?

The changelog says restoring at the three bind-cycle exits means "a bind
that is undone by any path leaves the PHY as it was found", but nothing
records a pending restore for the skipped case.

A driver unbind while a consumer is still attached:

unbind_store()
	if (dev && dev->driver == drv) {
		device_driver_detach(dev);

reaches phy_remove() with phy_link_change still set by phy_attach_direct(),
so the helper returns early and phy_remove() then clears phydev->drv.

When the consumer disconnects afterwards, phy_detach() clears the marker but
its release is gated:

	phydev->phy_link_change = NULL;
	...
	if (phydev->is_genphy_driven) {
		device_release_driver(&phydev->mdio.dev);
		phydev->is_genphy_driven = 0;
	}

and for an already-unbound device device_release_driver() is a no-op, so
phy_remove() does not run again and no restore happens.  A later
interrupt-capable driver then sees phydev->irq == PHY_POLL in phy_probe(),
phy_interrupt_is_valid() is false, and polling continues.

The early return itself looks necessary, since restoring under an attached
consumer is what would make phy_disconnect() free an unrequested IRQ.  Could
the skipped restore instead be completed in phy_detach() right after
phy_link_change is cleared?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908155025.4155289-1-f%40lex.la