[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

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

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: phy: take the interrupt back from the bus on detach

This adds a single store to phy_detach() that re-reads phydev->irq from
mdiobus->irq[phydev->mdio.addr], so that a PHY which spent part of its life
bound to the generic driver does not keep PHY_POLL forever. The intent is to
undo the substitutions made by phy_probe() and phy_attach_direct() without
either of them having to save the previous value.

No Link: tags are present in the commit header.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 94b2e85e00a37..84e2da81dbd3d 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.
> +	 */

[Severity: Medium]
Is the premise in this comment true for all buses?  mdiobus_alloc_size()
fills the table with PHY_POLL:

drivers/net/phy/mdio_bus_provider.c:mdiobus_alloc_size() {
	...
	for (i = 0; i < PHY_MAX_ADDR; i++) {
		bus->irq[i] = PHY_POLL;
	...
}

Only the OF/fwnode path keeps the two in sync:

drivers/net/mdio/fwnode_mdio.c:fwnode_mdiobus_phy_device_register() {
	...
	if (rc > 0) {
		phy->irq = rc;
		mdio->irq[addr] = rc;
	} else {
		phy->irq = mdio->irq[addr];
	}
	...
}

For a bus that never populates the table, is this a restore or an
overwrite?  Documentation/networking/phy.rst tells MAC drivers to select
interrupt behaviour by assigning phydev->irq directly, and drivers do
exactly that:

drivers/net/usb/lan78xx.c:lan78xx_phy_init() {
	...
	if (dev->domain_data.phyirq > 0)
		phydev->irq = dev->domain_data.phyirq;
	...
}

drivers/net/usb/smsc95xx.c:smsc95xx_bind() {
	...
	pdata->phydev->irq = phy_irq;
	...
}

In those cases phy_detach() now stores PHY_POLL over the number the driver
supplied, so the polling-forever behaviour the commit targets is not fixed
there but the driver's own value is lost.  The reverse direction exists as
well, where a driver deliberately declines the interrupt only in
phydev->irq while its MDIO bus is OF-registered:

drivers/net/ethernet/xscale/ixp4xx_eth.c:ixp4xx_eth_probe() {
	...
	phydev->irq = PHY_POLL;
	...
}

drivers/net/ethernet/asix/ax88796c_main.c does the same.  Would a later
attach then re-impose an interrupt the driver refused?

Would saving and restoring the value that the substitution actually
overwrote, or restoring only at the sites where phylib itself wrote
PHY_POLL, avoid depending on a table that most buses leave at the default?

Checked candidates that do not appear to lose anything today: sxgbe never
assigns a real number (plat->mdio_bus_data has no ->irqs or
->probed_phy_irq set in sxgbe_platform.c, so both phy->irq and
mdio_bus->irq[] stay PHY_POLL), and emac-mac, bcmmii, bcmasp_intf and
tsnep re-establish their value on every attach.  lan78xx and smsc95xx
attach once per bind, so the loss there is latent rather than observable.

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

[Severity: Medium]
The commit message says:

    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()

Is that accurate?  phy_attach_direct() binds the generic driver by hand:

drivers/net/phy/phy_device.c:phy_attach_direct() {
	...
	if (phydev->is_genphy_driven) {
		err = d->driver->probe(d);
		if (err >= 0)
			err = device_bind_driver(d);

		if (err)
			goto error_module_put;
	}
	...
}

d->driver->probe is phy_probe(), which writes PHY_POLL before its own
failure points (phydrv->get_features(), which is genphy_read_abilities()
for genphy_driver and propagates MDIO read errors), and
device_bind_driver() can return -ENOMEM from driver_sysfs_add().  The
label that failure lands on does not detach:

drivers/net/phy/phy_device.c:phy_attach_direct() {
	...
error_module_put:
	module_put(d->driver->owner);
	phydev->is_genphy_driven = 0;
	d->driver = NULL;
error_put_device:
	put_device(d);
	...
}

So this bind cycle ends with phydev->irq == PHY_POLL and no restore, while
the other failure label in the same function (error:) does call
phy_detach().  On a retried attach phy_interrupt_is_valid(phydev) is now
false, nothing re-derives the number, and the PHY polls permanently.
Should the restore also cover this path?

[Severity: Medium]
Can this store race a concurrent phy_probe()?  It runs after
device_release_driver(&phydev->mdio.dev) has already made the mdio device
bindable again, and the device lock is not held across the return:

drivers/base/dd.c:device_release_driver_internal() {
	...
	__device_driver_lock(dev, parent);

	if (!drv || drv == dev->driver)
		__device_release_driver(dev, parent);

	__device_driver_unlock(dev, parent);
	...
}

phy_detach() runs on the rtnl side and holds nothing against the driver
core, while phy_probe() is the other reader and writer of the same field
and is serialized only by device_lock():

drivers/net/phy/phy_device.c:phy_probe() {
	...
	if (!phy_drv_supports_irq(phydrv) && phy_interrupt_is_valid(phydev))
		phydev->irq = PHY_POLL;
	...
}

If a real PHY driver is registered on another CPU inside that window
(phy_driver_register() -> driver_attach() -> really_probe() -> phy_probe()),
two outcomes look possible.  A driver without config_intr/handle_interrupt
demotes to PHY_POLL and this store immediately republishes a valid number,
leaving phy_interrupt_is_valid() true for a driver that cannot service it.
An interrupt-capable driver that binds in the window reads the stale
PHY_POLL and makes a permanent probe-time decision from it:

drivers/net/phy/realtek/realtek_main.c:rtl8211f_probe() {
	...
	if (device_property_read_bool(dev, "wakeup-source") &&
	    phy_interrupt_is_valid(phydev)) {
		device_set_wakeup_capable(dev, true);
		devm_pm_set_wake_irq(dev, phydev->irq);
	}
	...
}

after which the restore leaves an apparently valid runtime IRQ with no wake
IRQ configured.  Would placing the restore before device_release_driver(),
under the same device lock that serializes the next probe, order these?

>  
>  	/* Assert the reset signal */
>  	phy_device_reset(phydev, 1);
>  

[Severity: Low]
Should this carry a Fixes: tag and a stable annotation?  The defect chain is
reproducible in the tree: phy_device_create() seeds dev->irq =
bus->irq[addr], phy_probe() then demotes it with

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

and genphy_driver has no config_intr or handle_interrupt, so the demotion
always happens for a PHY with a real IRQ.  Nothing re-derives the number
afterwards, since phy_attach_direct() only ever demotes.  The commit that
introduced the phy_probe() substitution looks like the natural Fixes:
target; without it the backport range is undefined.
-- 
pw-bot: cr

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

[Andrew Lunn]

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

Maybe i was not clear. I listed all the drivers i thought you needed
to look at and patch, so that the bus is the source of truth. This is
also what the AI was pointing out as a problem.

    Andrew

---
pw-bot: cr