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

[PATCH net v4 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 v4:
 - patch 2: reword the phy_restore_genphy_irq() comment to drop the
   undefined "parking" wording (Andrew Lunn); the comment is the only
   change since v3, no code changed
 - v3: https://lore.kernel.org/netdev/20260827211638.63395-1-f@lex.la/

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 | 13 +++++++++++++
 drivers/net/phy/phylink.c    | 29 ++++++++++++++++++++---------
 2 files changed, 33 insertions(+), 9 deletions(-)

-- 
2.53.0

[PATCH net v4 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.53.0

[PATCH net v4 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/

v4: reword the phy_restore_genphy_irq() comment; no code change.

 drivers/net/phy/phy_device.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 94b2e85e00a3..20fc29355f60 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1734,6 +1734,17 @@ static bool phy_drv_supports_irq(const struct phy_driver *phydrv)
 	return phydrv->config_intr && phydrv->handle_interrupt;
 }
 
+/* Undo the PHY_POLL that phy_probe() sets when a driver without
+ * interrupt callbacks binds. The bind cycle never writes bus->irq[],
+ * so the table still holds the pre-bind value; anything else the PHY
+ * carries did not come from the bind and must stand.
+ */
+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 +1907,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 +1977,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.53.0

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

[Andrew Lunn]

On Wed, Sep 02, 2026 at 08:05:11AM +0000, Aleksei Sviridkin wrote:
> 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>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

[netdev-bot+sashiko]

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

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

- [High] phy_restore_genphy_irq() in drivers/net/phy/phy_device.c uses
  bus->irq[phydev->mdio.addr] as the recovery source and its new comment…
- [Medium] phy_restore_genphy_irq()
  (drivers/net/phy/phy_device.c:1742-1746) infers "the bind cycle…
- [Medium] The value is destroyed by phy_probe() (the driver-core ->probe
  callback, drivers/net/phy/phy_device.c:3710-3711) for ANY driver where…
- [Medium] The new phy_restore_genphy_irq() call added at
  phy_attach_direct()'s error_module_put label…

Pre-existing issues:
- [Medium] Pre-existing defect on the error label this patch extends
  (reported only because validation was requested; it is not part of…

--

net: phy: restore the interrupt after a generic-driver bind cycle

A PHY that gets the generic driver at attach time has phydev->irq forced to
PHY_POLL by phy_probe(), and nothing puts it back when the generic driver is
released or when its probe fails.  This patch adds a helper that copies
bus->irq[addr] back into phydev->irq, called from phy_detach() and from the
error_module_put label in phy_attach_direct().

Reference given below the --- separator:
https://lore.kernel.org/netdev/20260824024029.41310-3-f@lex.la/

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 94b2e85e00a37..20fc29355f60b 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1734,6 +1734,17 @@ static bool phy_drv_supports_irq(const struct phy_driver *phydrv)
>  	return phydrv->config_intr && phydrv->handle_interrupt;
>  }
>  
> +/* Undo the PHY_POLL that phy_probe() sets when a driver without
> + * interrupt callbacks binds. The bind cycle never writes bus->irq[],
> + * so the table still holds the pre-bind value; anything else the PHY
> + * carries did not come from the bind and must stand.
> + */
> +static void phy_restore_genphy_irq(struct phy_device *phydev)
> +{
> +	if (phydev->irq == PHY_POLL)
> +		phydev->irq = phydev->mdio.bus->irq[phydev->mdio.addr];
> +}

[Severity: High]
The comment states that "the table still holds the pre-bind value".  Is that
true for buses whose consumers install the PHY interrupt only into
phydev->irq and never touch the table?

mdiobus_alloc_size() fills every entry 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;
	...
}

smsc95xx_bind() allocates its bus with mdiobus_alloc() and then only does:

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

lan78xx does the same with the irq_domain mapping:

drivers/net/usb/lan78xx.c {
	...
		phydev->irq = dev->domain_data.phyirq;
	...
}

For those setups the sequence the changelog describes seems to survive
unchanged: phy_probe() writes PHY_POLL, phy_restore_genphy_irq() reads
PHY_POLL back out of the table, and the specific driver that binds later
still starts polled.  Only producers that mirror the value into the table
(fwnode_mdio.c, stmmac_mdio.c, mlxbf_gige_main.c) are recovered.

Would saving the pre-bind phydev->irq itself, rather than deriving it from
bus->irq[], cover every source?  As it stands, the in-tree comment documents
an invariant that MAC drivers are not currently required to maintain, and
nothing next to mii_bus::irq mentions the new expectation.

[Severity: Medium]
The comment also claims that "anything else the PHY carries did not come from
the bind and must stand", and the changelog says the restore happens "only
where the cycle left PHY_POLL".  Since no pre-bind value or provenance is
recorded, can the equality test really distinguish a bind-installed PHY_POLL
from a caller-installed one?

Other in-tree writers of PHY_POLL, none of which come from the bind cycle:

drivers/net/phy/phy_device.c:phy_attach_direct() {
	...
	if (phydev->dev_flags & PHY_F_NO_IRQ)
		phydev->irq = PHY_POLL;
	...
}

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

drivers/net/ethernet/asix/ax88796c_main.c:ax88796c_probe() {
	...
	ax_local->phydev->irq = PHY_POLL;	/* after phy_connect() */
	...
}

drivers/net/ethernet/qualcomm/emac/emac-mac.c also does this.

For such a PHY on a bus that does mirror a live interrupt into bus->irq[],
phy_detach() now hands back the interrupt number that the board or MAC had
deliberately taken out of service.  The next phy_connect_direct() acts on it:

drivers/net/phy/phy_device.c:phy_connect_direct() {
	...
	if (phy_interrupt_is_valid(phydev))
		phy_request_interrupt(phydev);
	...
}

and because those MACs re-assert PHY_POLL after connect, phy_disconnect()
skips phy_free_interrupt(), which is the requested-and-never-freed asymmetry
the changelog says the guard avoids.

There is one more case in the same direction: phy_probe() only overwrites the
irq when phy_interrupt_is_valid() is true:

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

so a PHY that entered the generic bind already polling is untouched by the
probe, yet it is still "restored" on the way out.  Could the guard be keyed on
state the bind cycle actually records, or the comment reworded as a heuristic?

[Severity: Medium]
The value is destroyed in phy_probe(), which is the driver core ->probe
callback, for any driver where !phy_drv_supports_irq(phydrv).  Why is the undo
placed in the phylib attach/detach pair rather than in phy_remove(), the
matching ->remove callback installed by phy_driver_register()?

phy_remove() already reverts other probe-time state (clears phydev->drv,
re-asserts the reset, unregisters the LED triggers) but does not touch
phydev->irq, and it is reachable without going through phy_detach():

drivers/base/bus.c:unbind_store() {
	...
	if (dev && bus_rescan_devices_helper(dev, NULL) == 0) {
		device_driver_detach(dev);
	...
}

After a sysfs unbind of the generic driver, does phydev->irq stay at
PHY_POLL, so that a real interrupt-capable driver binding afterwards inherits
it and the PHY is polled for the rest of the uptime again?

The same question applies to a real, non-generic PHY driver that lacks
config_intr/handle_interrupt: phy_probe() clobbers phydev->irq for it too, but
phydev->is_genphy_driven is 0, so neither new call site restores anything.

Restoring in phy_remove() from a value saved in phy_probe() would need neither
the irq == PHY_POLL heuristic nor the assumption about bus->irq[].

The changelog says the change covers "both exits" of the bind cycle; the sysfs
unbind exit is mentioned only in the lore link below the --- separator, so it
will not be in git history.

>  
>  /**
>   * phy_attach_direct - attach a network device to a given PHY device pointer
>   * @dev: network device to attach
> @@ -1896,6 +1907,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:

[Severity: Medium]
Is this label reached only for "a generic probe that fails", as the changelog
says?  phydev->is_genphy_driven is a persistent bit on the phy_device, and the
probe/bind block in phy_attach_direct() is gated on that bit and runs before
the already-attached check:

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;
	}

	if (phydev->attached_dev) {
		dev_err(&dev->dev, "PHY already attached\n");
		err = -EBUSY;
		goto error;
	}
	...
}

So a second phy_attach_direct() on a phydev that is already attached and
genphy-driven (for instance two MACs whose phy-handle resolves to the same DT
node) re-runs probe and device_bind_driver() on an already bound device.
device_bind_driver() -> driver_sysfs_add() -> sysfs_create_link() returns
-EEXIST for the links that already exist, and control lands here.

Does that then rewrite the live PHY's irq?  The first successful attach left
it at PHY_POLL, and on a DT/fwnode bus bus->irq[addr] holds a real number.
Once phydev->irq is no longer PHY_POLL the state machine stops re-arming:

drivers/net/phy/phy.c:_phy_state_machine() {
	...
	if (phy_polling_mode(phydev) && phy_is_started(phydev))
		phy_queue_state_machine(phydev,
					phy_get_next_update_time(phydev));
	...
}

and no interrupt was ever requested for the restored number, so link changes
on the still-attached PHY would go undetected with no message.

[Severity: Medium]
This isn't a bug introduced by this patch, but the label being extended here
does not unwind a successful phy_probe().  When probe() returns 0 and
device_bind_driver() then fails, the device is not bound, so the driver core
never calls phy_remove() for this attempt, and this label only does
module_put(), clears is_genphy_driven, clears d->driver and puts the device.

Left behind from phy_probe():

drivers/net/phy/phy_device.c:phy_probe() {
	...
	/* Deassert the reset signal */
	phy_device_reset(phydev, 0);
	...
	if (!phydev->is_on_sfp_module)
		phy_led_triggers_register(phydev);
	...
}

The phydev is not freed here (the mdio bus owns it), so
phydev->phy_led_triggers keeps pointing at the array allocated by
phy_led_triggers_register(), the ports set up by phy_setup_ports() and the sfp
upstream registration stay live, and the PHY is left with its reset
de-asserted.  Should this label also assert the reset and drop those
allocations, or call into a shared unwind with phy_remove()?

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