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

[PATCH net v5 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 binds a driver with no interrupt callbacks loses
the interrupt number the firmware node declared. The specific driver
that binds afterwards never sees it, and unless its consumer installs
one itself the PHY is polled from then on.

Patch 2 takes a different approach from v4 and does not carry Andrew's
Reviewed-by, which I asked him to hold [2]. v4 read the number back
from bus->irq[]. The Sashiko bot pointed out that a bus installing the
interrupt only on the phy_device never writes that table, and it is
right: on smsc95xx and lan78xx the restore read PHY_POLL back out and
did nothing. v5 saves the number where phy_probe() takes it, so the
source cannot be a table that never held it, and nothing has to guess
whether a PHY_POLL came from the bind.

Two neighbouring problems are deliberately left alone:

 - phy_attach_direct() clobbers the interrupt a second time, at the
   attach rather than at the bind, and that loses a number a consumer
   installed after probe. It is phylib's own rule about a driver
   without interrupt callbacks, not the PHY_F_NO_IRQ line above it, and
   it is re-applied on every attach, so undoing it is a different
   change with a different owner.
 - phy_remove() leaves phydev->drv NULL while a consumer can still hold
   the PHY, and phy_disconnect() dereferences it through
   phy_config_interrupt(). That is reachable today for a PHY that has
   an interrupt number and a driver that supports interrupts. Patch 2
   refuses to restore while a consumer holds the PHY, so it does not
   hand a number back to a PHY that phy_probe() had put in polling
   mode, and does not widen that path.

The Fixes tag reaches 2005, but the guard leans on phy_detach() clearing
phy_link_change, which is only true since commit e0d1c55501d3 ("net:
phy: fix phy_uses_state_machine()") in v6.17. Older trees never clear
the mark, so the restore would be refused forever and the patch would be
a silent no-op there. Patch 2 says so in its own message, since that is
what travels into a backport. Those trees also lack the is_genphy_driven
context the unwind hunk needs, so the patch will not apply to them
unaided in any case.

One case the patch does not cover, for the same reason. Unbind a driver
through sysfs while a consumer holds the PHY and the restore is
refused, correctly; the consumer's later detach clears the mark but
reaches no second remove, so the number waits in irq_saved and the next
driver to bind still starts polled. It comes back at that driver's
remove. Closing it would mean restoring from phy_detach() again, which
is the shape this version exists to leave behind.

No hardware measurement of the restore is offered, and the reason is
worth stating rather than hiding. The cycle this fixes needs a generic
driver bound at the PHY before the specific one - which needs the
specific driver or its firmware to be unreadable when the MDIO bus is
scanned. The board I develop on cannot produce that: its rootfs and
firmware are present at boot, so the specific driver binds directly and
the generic one never probes. phydev->irq has no observable outside the
phy_attached_info() line, and that needs a consumer to attach, which on
this board only ever meets the specific driver. So the three restore
sites are argued from the code, not run: phy_remove(), reached from a
sysfs unbind and from phy_detach(); phy_probe()'s own error exit, which
the driver core does not follow with a remove; and phy_attach_direct()'s
unwind of a generic bind that failed after probe, where
device_bind_driver() fails only in driver_sysfs_add().

[2] https://lore.kernel.org/netdev/20260905000722.422652-1-f@lex.la/

Changes in v5:
 - patch 2 changes approach: the number is saved in phy_probe() where
   it is taken and restored in phy_remove() and on phy_probe()'s own
   error exit, rather than read back from bus->irq[] in phy_detach()
 - patch 2: a generic bind that fails after its probe succeeded is a
   third exit with no restore, so phy_attach_direct()'s unwind gets one
   too, as it did in v4
 - patch 2: the restore is refused while a consumer holds the PHY, so
   an unbind or an rmmod under a live consumer cannot hand a number
   back to a phy_disconnect() that never requested one. The mark is
   phy_link_change rather than attached_dev, because a DSA shared port
   attaches its PHY with no netdev and leaves attached_dev NULL
 - patch 1 is unchanged apart from the Assisted-by trailer, which it
   should have carried from the start
 - v4: https://lore.kernel.org/netdev/20260902080511.2211261-1-f@lex.la/

Aleksei Sviridkin (2):
  net: phylink: unwind the PHY binding when bringup fails late
  net: phy: restore the interrupt phy_probe() replaced with PHY_POLL

 drivers/net/phy/phy_device.c | 23 ++++++++++++++++++++++-
 drivers/net/phy/phylink.c    | 29 ++++++++++++++++++++---------
 include/linux/phy.h          |  3 +++
 3 files changed, 45 insertions(+), 10 deletions(-)


base-commit: 6262acad9db197b5ed12e3b245d2e6d0c80fb960
-- 
2.53.0

[PATCH net v5 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 v5 2/2] net: phy: restore the interrupt phy_probe() replaced with PHY_POLL

[Aleksei Sviridkin]

phy_probe() sets phydev->irq to PHY_POLL when the driver it is binding
has no interrupt callbacks, and nothing puts the number back. The
driver that binds afterwards therefore starts polled, and unless its
consumer installs the interrupt again the PHY stays that way for the
rest of the uptime, with no trace beyond an informational irq=POLL
beside that driver's name. A DSA switch that connects its user ports
before the rootfs holding the PHY driver module is mounted hits this on
every boot.

Save the number where it is taken away and put it back in phy_remove(),
in phy_probe()'s own error path, which the driver core does not follow
with a remove, and in phy_attach_direct()'s unwind of a generic bind
that failed after its probe succeeded. Only a probe that took a number
away arms the restore, and phy_link_change suppresses it when a
consumer holds the PHY, since that consumer skipped requesting an
interrupt on the value it saw. phy_detach() clears phy_link_change
before it releases the driver, so the case above still restores.

That last part arrived in commit e0d1c55501d3 ("net: phy: fix
phy_uses_state_machine()"). Without it the mark is never cleared once a
consumer has attached, so the restore this patch exists for never fires;
a backport needs that commit first.

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.")
Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
---
 drivers/net/phy/phy_device.c | 23 ++++++++++++++++++++++-
 include/linux/phy.h          |  3 +++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 94b2e85e00a3..ba117147721a 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -770,6 +770,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 	mdiodev->device_remove = phy_mdio_device_remove;
 	mdiodev->reset_state = -1;
 
+	dev->irq_saved = PHY_POLL;
 	dev->speed = SPEED_UNKNOWN;
 	dev->duplex = DUPLEX_UNKNOWN;
 	dev->pause = false;
@@ -1734,6 +1735,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, 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_saved == PHY_POLL)
+		return;
+
+	phydev->irq = phydev->irq_saved;
+	phydev->irq_saved = PHY_POLL;
+}
+
 /**
  * phy_attach_direct - attach a network device to a given PHY device pointer
  * @dev: network device to attach
@@ -1896,6 +1910,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:
@@ -3694,8 +3709,10 @@ static int phy_probe(struct device *dev)
 	/* Disable the interrupt if the PHY doesn't support it
 	 * but the interrupt is still a valid one
 	 */
-	if (!phy_drv_supports_irq(phydrv) && phy_interrupt_is_valid(phydev))
+	if (!phy_drv_supports_irq(phydrv) && phy_interrupt_is_valid(phydev)) {
+		phydev->irq_saved = phydev->irq;
 		phydev->irq = PHY_POLL;
+	}
 
 	if (phydrv->flags & PHY_IS_INTERNAL)
 		phydev->is_internal = true;
@@ -3820,6 +3837,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 +3867,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);
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 5f8d65868e0f..d492417572a0 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -591,6 +591,8 @@ struct phy_oatc14_sqi_capability {
  *      - Bits [31:24] are reserved for defining generic
  *        PHY driver behavior.
  * @irq: IRQ number of the PHY's interrupt (-1 if none)
+ * @irq_saved: @irq as it was before phy_probe() replaced it with PHY_POLL,
+ *             or PHY_POLL when there is nothing to restore
  * @phylink: Pointer to phylink instance for this PHY
  * @sfp_bus_attached: Flag indicating whether the SFP bus has been attached
  * @sfp_bus: SFP bus attached to this PHY's fiber port
@@ -761,6 +763,7 @@ struct phy_device {
 	 * -1 means no interrupt
 	 */
 	int irq;
+	int irq_saved;
 
 	/* private data pointer */
 	/* For use by PHYs to maintain extra state */
-- 
2.53.0

Re: [PATCH net v5 2/2] net: phy: restore the interrupt phy_probe() replaced with PHY_POLL

[Andrew Lunn]

> @@ -591,6 +591,8 @@ struct phy_oatc14_sqi_capability {
>   *      - Bits [31:24] are reserved for defining generic
>   *        PHY driver behavior.
>   * @irq: IRQ number of the PHY's interrupt (-1 if none)
> + * @irq_saved: @irq as it was before phy_probe() replaced it with PHY_POLL,
> + *             or PHY_POLL when there is nothing to restore

I don't really like this. We should bite the bullet and make
mdiobus->irq[] the source of truth. It does not look too bad:

usb/lan78xx.c has an mdiobus.

       /* if phyirq is not set, use polling mode in phylib */
        if (dev->domain_data.phyirq > 0)
                phydev->irq = dev->domain_data.phyirq;
        else
                phydev->irq = PHY_POLL;
        netdev_dbg(dev->net, "phydev->irq = %d\n", phydev->irq);

        ret = phylink_connect_phy(dev->phylink, phydev);

This can be changed to set the value in the mdio bus structure.

usb/smsc95xx.c: You have direct access to the mdiobus, the same
function just registered it.

ethernet/freescale/ucc_geth.c: This is about WoL, so no change needed.

ethernet/xscale/ixp4xx_eth.c: Appears to be pointless
ethernet/qualcomm/emac/emac-mac.c: Also pointless
ethernet/asix/ax88796c_main.c: More pointlessness
ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c: Pointless in a different way

ethernet/broadcom/asp2/bcmasp_intf.c,
ethernet/broadcom/genet/bcmmii.c, ethernet/engleder/tsnep_main.c all
set PHY_MAC_INTERRUPT. genphy should not need to touch that.

ethernet/stmicro/stmmac/stmmac_mdio.c: One of the previous types of pointless.

So please work on these.

   Andrew

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] Commit message misstates the impact of the bug being fixed on
  the SFP path.

--

net: phylink: unwind the PHY binding when bringup fails late

phylink_bringup_phy() stores the PHY in pl->phydev before calling
phy_eee_rx_clock_stop(), which can fail on a MAC implementing LPI, leaving
a stale pl->phydev behind after the callers unwind with phy_detach().  The
patch clears the binding on that failure path and factors the clearing out
of phylink_disconnect_phy() into a new phylink_clear_phydev() helper.

The code change looks right to me; the question below is only about the
description of the bug being fixed.

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

[Severity: Medium]
Is the -EBUSY description accurate for the SFP path?  On that path the
phy_device is created by sfp_sm_probe_phy() and destroyed when the connect
fails, so the stale pl->phydev looks like it points at freed memory rather
than just blocking a later attach.

phylink_sfp_config_phy() propagates the bringup error:

	ret = phylink_bringup_phy(pl, phy, config.interface);
	if (ret < 0) {
		phy_detach(phy);
		return ret;
	}

That error travels back through sfp_add_phy() (ops->connect_phy) into
sfp_sm_probe_phy():

	err = sfp_add_phy(sfp->sfp_bus, phy);
	if (err) {
		phy_device_remove(phy);
		phy_device_free(phy);
		dev_err(sfp->dev, "sfp_add_phy failed: %pe\n", ERR_PTR(err));
		return err;
	}

phy_detach() drops the reference taken by phy_attach_direct(), then
phy_device_free() -> put_device() drops the last one and
phy_device_release() does kfree(to_phy_device(dev)).  sfp->mod_phy is never
assigned in this path either, so no later disconnect_phy callback would
clear pl->phydev.

Wouldn't pre-patch consumers of pl->phydev then touch freed memory, for
instance phylink_resolve():

	mutex_lock(&pl->phydev_mutex);
	phy = pl->phydev;
	if (phy)
		mutex_lock(&phy->lock);

along with the ethtool ksettings/EEE helpers that test pl->phydev, and a
subsequent phylink_disconnect_phy() -> phy_disconnect()?

If that reading is correct, could the commit message describe the SFP path
as a dangling pl->phydev pointing at a freed phy_device instead of a
permanent -EBUSY, since that affects how the fix is triaged for stable?

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

Re: [PATCH net v5 2/2] net: phy: restore the interrupt phy_probe() replaced with PHY_POLL

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 2 · Low: 0

- [High] Incomplete fix: only the `phydev->irq = PHY_POLL` clobber inside
  phy_probe() (drivers/net/phy/phy_device.c:3712-3715) records the…
- [Medium] phy_restore_probe_irq()
  (drivers/net/phy/phy_device.c:1742-1749) returns early when…
- [Medium] phy_restore_probe_irq()
  (drivers/net/phy/phy_device.c:1742-1749) decides whether it may…

--

net: phy: restore the interrupt phy_probe() replaced with PHY_POLL

The patch records the PHY interrupt number that phy_probe() replaces with
PHY_POLL in a new phy_device field, irq_saved, and puts it back from
phy_remove(), from the phy_probe() error path and from the
phy_attach_direct() unwind of a failed generic bind.  A new helper,
phy_restore_probe_irq(), performs the restore and skips it while
phydev->phy_link_change indicates an attached consumer.

A few questions about the new helper and its call sites below.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 94b2e85e00a37..ba117147721ad 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -1734,6 +1735,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, 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_saved == PHY_POLL)
> +		return;
> +
> +	phydev->irq = phydev->irq_saved;
> +	phydev->irq_saved = PHY_POLL;
> +}

[Severity: Medium]
When the phy_link_change test takes the early return, the restore is
neither performed later nor disarmed.  Does that leave two loose ends?

First, is the restore simply dropped?  PHY drivers do not set
suppress_bind_attrs, so a sysfs unbind can reach phy_remove() while a
consumer is still attached.  phy_restore_probe_irq() returns, phydev->irq
stays PHY_POLL, and phy_detach() only clears phy_link_change:

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

For a normally bound specific driver is_genphy_driven is 0, so nothing
runs the helper afterwards and no further phy_remove() happens for that
bind.  The next interrupt-capable driver's phy_probe() also cannot re-arm
anything, because its condition requires phy_interrupt_is_valid(), which
is false while irq == PHY_POLL.  Is this not the same end state the patch
sets out to remove?

Second, does irq_saved go stale and get applied to an unrelated driver?
After a suppressed restore, irq_saved still holds the old number, and the
next phy_remove() or phy_probe() error path that runs with no consumer
attached writes it into phydev->irq on behalf of a driver that never took
an interrupt away.  That seems to conflict with the helper's own comment
("Give back what phy_probe() took") and with the new kernel-doc for
@irq_saved ("or PHY_POLL when there is nothing to restore"), and it can
overwrite a value another party installed in the meantime, for example
PHY_MAC_INTERRUPT as set by bcmgenet_mii_probe(), tsnep, bcmasp_intf or
icplus, or the deliberate PHY_POLL fallback in phy_request_interrupt().

>  /**
>   * phy_attach_direct - attach a network device to a given PHY device pointer
>   * @dev: network device to attach

[ ... ]

> @@ -1896,6 +1910,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:

[ ... ]

> @@ -3694,8 +3709,10 @@ static int phy_probe(struct device *dev)
>  	/* Disable the interrupt if the PHY doesn't support it
>  	 * but the interrupt is still a valid one
>  	 */
> -	if (!phy_drv_supports_irq(phydrv) && phy_interrupt_is_valid(phydev))
> +	if (!phy_drv_supports_irq(phydrv) && phy_interrupt_is_valid(phydev)) {
> +		phydev->irq_saved = phydev->irq;
>  		phydev->irq = PHY_POLL;
> +	}

[Severity: High]
This is the only place that records the replaced number, but
phy_attach_direct() overwrites phydev->irq in two more places, one of them
with the identical condition:

	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;

Neither writes irq_saved, so irq_saved stays PHY_POLL and
phy_restore_probe_irq() returns at its irq_saved == PHY_POLL test at all
three new call sites.  Is the number then still lost for the remaining
life of the phy_device, exactly as described in the commit message ("the
PHY stays that way for the rest of the uptime")?

There are in-tree orders where phy_probe() never sees a valid number and
phy_attach_direct() does:

stmmac_mdio_register()
    of_mdiobus_register()      /* binds and probes the PHY driver,
                                  bus->irq[addr] is still PHY_POLL */
    phydev->irq = mdio_bus_data->probed_phy_irq;
  ... later ...
    phy_attach_direct()        /* driver has no config_intr/handle_interrupt,
                                  so irq = PHY_POLL, unrecorded */

mlxbf_gige_probe() does the same shape, assigning phydev->irq = phy_irq
right before phy_connect_direct().

phydev->irq is seeded from the bus array only once, in phy_device_create():

	dev->irq = bus->irq[addr];

so nothing re-reads bus->irq[addr] afterwards.  Should the two
phy_attach_direct() sites save the number too?

[ ... ]

> @@ -3820,6 +3837,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 +3867,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);
>  

[Severity: Medium]
This adds a writer of phydev->irq from the driver core path, which holds
only device_lock(&phydev->mdio.dev).  The code that sets and clears
phy_link_change and samples phydev->irq runs under rtnl and does not take
the device lock: phy_attach_direct() sets phydev->phy_link_change and then
reads phydev->irq, phy_connect_direct() gates phy_request_interrupt() on
phy_interrupt_is_valid(), phylink_bringup_phy() does the same, and
phy_detach() clears phy_link_change before device_release_driver().  With
no common lock and no READ_ONCE/WRITE_ONCE pairing, is the phy_link_change
test in phy_restore_probe_irq() a check-then-act window rather than a
guard?

One interleaving:

CPU0 (sysfs unbind, device_lock)      CPU1 (rtnl, attach)
phy_remove()
  phy_restore_probe_irq()
    reads phy_link_change == NULL
                                      phy_attach_direct()
                                        phydev->phy_link_change = ...
                                        reads irq == PHY_POLL,
                                        skips phy_request_interrupt()
    phydev->irq = phydev->irq_saved;

The attached consumer now sees a valid IRQ nobody requested,
phy_polling_mode() turns false so link state stops being polled, and the
later phy_disconnect() takes the phy_interrupt_is_valid() branch into
phy_free_interrupt() -> phy_disable_interrupts(), where
phy_config_interrupt() dereferences phydev->drv:

drivers/net/phy/phy.c:phy_config_interrupt() {
	if (phydev->drv->config_intr)
	...
}

phy_remove() sets phydev->drv = NULL just after the restore, and free_irq()
would be called for an interrupt that was never requested.  Can this
happen?

A second interleaving loses the restore instead: phy_detach() clears
phy_link_change before calling device_release_driver(), so a concurrent
unbind that wins the device lock first still observes phy_link_change !=
NULL, skips the restore, and by the time phy_detach() reaches
device_release_driver() the driver is already unbound, so phy_remove()
never runs again.  Would it be better to serialize the guard test, the
phy_link_change transition and the irq update under one lock rather than
adding a second unlocked writer of phydev->irq?

> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 5f8d65868e0ff..d492417572a0f 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -591,6 +591,8 @@ struct phy_oatc14_sqi_capability {
>   *      - Bits [31:24] are reserved for defining generic
>   *        PHY driver behavior.
>   * @irq: IRQ number of the PHY's interrupt (-1 if none)
> + * @irq_saved: @irq as it was before phy_probe() replaced it with PHY_POLL,
> + *             or PHY_POLL when there is nothing to restore
>   * @phylink: Pointer to phylink instance for this PHY
>   * @sfp_bus_attached: Flag indicating whether the SFP bus has been attached
>   * @sfp_bus: SFP bus attached to this PHY's fiber port

[ ... ]

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