[PATCH net v9 0/4] net: phy: keep a PHY interrupt across a generic bind cycle

[PATCH net v9 0/4] net: phy: keep a PHY interrupt across a generic bind cycle

[Aleksei Sviridkin]

On an MT7981B board with an MT7531 switch, the Airoha EN8811H behind
lan4 has its PHY driver as a module on the root filesystem. The switch
brings its user ports up before that filesystem is mounted, so the PHY
attaches to the generic driver first. phy_probe() replaces phydev->irq
with PHY_POLL because genphy has no interrupt callbacks, nothing puts
the number back, and the PHY polls for the rest of the uptime once its
real driver takes over. The devicetree describes a working interrupt for
it and the number is never used again. On this particular board the port
does not come up at all while that happens, because phylink rejects
2500base-x against the generic driver and DSA drops the port; it is the
PHY that carries the lost number across the cycle.

Patch 3 takes the number back from mdiobus->irq[] at the end of the bind
cycle. Patch 4 covers the other end of the same cycle: a genphy bind
that fails inside phy_attach_direct() unwinds on a label that does not
reach phy_detach(). Patches 1 and 2 put two USB drivers' interrupt
numbers into the bus table, so that there is something to take back for
them as well.

On the board above the number reaches that table through
fwnode_mdiobus_phy_device_register(), which writes phydev->irq and
mdiobus->irq[addr] together when the PHY node carries an interrupt; the
EN8811H hangs off the SoC MDIO bus, at the devicetree node
/soc/ethernet@15100000/mdio-bus/ethernet-phy@d. A driver that owns its
bus can fill the table itself instead, and several do - mt7530 writes
irq_create_mapping() results into ds->user_mii_bus->irq[] before
registering the bus, which is where the switch ports' own numbers in the
notes to patch 3 come from, and mlxbf_gige writes an ACPI GPIO interrupt
into its bus table the same way.

Andrew asked [2] for the full set of drivers that keep the interrupt
outside the bus, "so that the bus is the source of truth" [3]. Going
through that list against net/main, in his grouping:

  - lan78xx and smsc95xx write a live interrupt into phydev->irq only.
    Those are patches 1 and 2.
  - ucc_geth never writes the field at all; its single use is a read on
    the WoL path, so nothing to change, as he said.
  - ixp4xx_eth, ax88796c and emac-mac force PHY_POLL into phydev->irq
    around their connect, and each keeps doing it. ixp4xx and ax88796c
    store it in probe just after the connect succeeds, and their only
    detaches are their own probe error path and remove. emac-mac stores
    it early in emac_mac_up(), on the line before the connect, and that
    runs on every bringup. No attach in any of the three follows
    a detach without the driver forcing PHY_POLL again, so a restore in
    between cannot cost them anything.
  - stmmac_mdio already writes mdiobus->irq[] next to phydev->irq, so
    it is on the right side of this. The block is also unreachable in
    tree: it is guarded by probed_phy_irq > 0 and nothing sets that
    field, in stmmac or in sxgbe's copy of it.
  - mlxbf_gige likewise writes the bus table, from ACPI.
  - bcmasp_intf, bcmmii and tsnep set PHY_MAC_INTERRUPT, and genphy does
    not touch it: phy_interrupt_is_valid() is false for both PHY_POLL
    and PHY_MAC_INTERRUPT, and it guards the substitution, so the
    generic driver never demotes a MAC-served interrupt. They also set
    it after connect - tsnep unconditionally, bcmasp and genet for their
    internal PHY only - so what a restore hands back is replaced again.
    icplus sets it from ip175c_read_status() for its switch ports and is
    safe for the same reason.

The v7 commit message also named sxgbe as losing the interrupt. That
was wrong - the same dead probed_phy_irq guard - and it is gone.

The review [4] asked how this sits with Documentation/networking/phy.rst
telling MAC drivers to set phydev->irq directly. That contract is
per-connect: set it "before you call phy_start". The restore happens in
phy_detach(), between connections, so the value a MAC installs after a
connect still stands when phy_start() runs.

One behaviour change worth naming. phy_request_interrupt() falls back to
PHY_POLL when request_threaded_irq() fails, and that fallback no longer
survives a detach. A board with a broken interrupt line still ends up
polling, exactly as before, but it now says so once per attach instead
of once per boot.

Longer term the substitution itself is what wants removing. There are
two of them and they are identical - phy_probe() and phy_attach_direct()
both do "if the bound driver has no interrupt callbacks and the number
looks valid, replace it with PHY_POLL" - so phy_interrupt_is_valid()
could ask whether the bound driver can service the interrupt and neither
site would need to write anything. That reaches every caller of the
helper, so it belongs in net-next and not here.

v7 1/2 ("net: phylink: unwind the PHY binding when bringup fails late")
has nothing to do with the interrupt and is posted for net on its own
alongside this series, which is why the patch count changed.

Changes since v8 [5]:

 - Patch 1 fills the lan78xx bus table before the bus is registered
   rather than one entry after the scan, and drops the write to
   phydev->irq that phylib now does from the table itself [6]. The
   netdev_dbg() that printed the field goes with it -
   phylink_bringup_phy() prints the same number, at info level, as this
   driver connects.
 - Patch 2 does the same for smsc95xx, and drops its phydev->irq write
   [7].

Changes since v7 [1]:

 - The restore moved above device_release_driver(). After that call the
   mdio device is bindable and the device lock is dropped, so a
   phy_probe() on another CPU writes the same field. Holding the lock
   across both, as the review [4] asked, is not available:
   device_release_driver_internal() takes it itself. Ordering the store
   ahead of the release gives the same guarantee, since the generic
   driver is still bound there and a driver registering meanwhile is
   turned away with -EBUSY before it reaches phy_probe().
 - New patch 4 for the phy_attach_direct() failure path. The v7 commit
   message claimed detach covered every substitution; it does not cover
   that one.
 - v7 2/2 carried no Fixes: tag at all. It does now, and patch 4 has
   its own.
 - New patches 1 and 2, for lan78xx and smsc95xx.
 - The sxgbe claim dropped.
 - The phylink patch split out, see above.

Patch 3 is measured on that board. The one condition arranged for the
run is that the PHY driver module loads after the root filesystem
instead of from the early boot list the distribution normally uses -
that early list is also why a shipped image does not trip over this.
The distribution's own late-PHY handling was also removed, that being the
one patch which could have changed the outcome; upstream has nothing like
it. The unwind block of the phylink patch posted alongside this series is
in the kernel too and is not reached on either path: the -EINVAL failure
returns from phylink_bringup_phy() at its validate call, before
pl->phydev is assigned, and the -EIO failure returns from
phy_attach_direct() before phylink_bringup_phy() runs. The kernel is
still a distribution one and its remaining patches to phylink and
phy_device do run on these paths; none of them writes phydev->irq. The
generic driver then binds at 1.87 s and the real one between 13.4 and
13.6 s depending on the boot, and phydev->irq afterwards reads -1 without
the patch and 15 with it, 15 being the number the devicetree gave that
PHY.

Patch 4 needs a generic probe that fails, which the board does not
produce on its own, so it went through a debug-only module parameter
that fails it once for one address. The connect then ends in -EIO rather
than the -EINVAL of the validation path, and the unwind takes the label
that patch touches; the same reading is -1 with patch 3 alone and 15
with both.

Patches 1 and 2 are compile-tested only - I have no LAN78xx or LAN95xx
device, and a Tested-by from someone who has one would be welcome.

[1] https://lore.kernel.org/r/20260909204306.2374562-1-f@lex.la/
[2] https://lore.kernel.org/r/8f67d3ba-ce25-49bf-8378-c76d748879a9@lunn.ch/
[3] https://lore.kernel.org/r/a2a2a8fb-97c3-498f-9bf4-e0c44be2eff6@lunn.ch/
[4] https://lore.kernel.org/r/20260915005946.823736-1-kuba@kernel.org/
[5] https://lore.kernel.org/r/20260918015029.2518425-1-f@lex.la/
[6] https://lore.kernel.org/r/df5d4af1-a86a-4820-9aeb-b1449a60f37e@lunn.ch/
[7] https://lore.kernel.org/r/6e14d6b7-2a5a-40e2-920e-fc69a6e85173@lunn.ch/

Aleksei Sviridkin (4):
  net: usb: lan78xx: register the PHY interrupt with the MDIO bus
  net: usb: smsc95xx: register the PHY interrupt with the MDIO bus
  net: phy: take the interrupt back from the bus on detach
  net: phy: restore the interrupt when the generic bind cycle fails

 drivers/net/phy/phy_device.c |  5 +++++
 drivers/net/usb/lan78xx.c    | 12 +++++-------
 drivers/net/usb/smsc95xx.c   |  6 ++++--
 3 files changed, 14 insertions(+), 9 deletions(-)


base-commit: 3b95a04eb5f95bf6a016a1bb9ff37d3eee48de63
-- 
2.53.0

[PATCH net v9 1/4] net: usb: lan78xx: register the PHY interrupt with the MDIO bus

[Aleksei Sviridkin]

The interrupt this driver maps for its PHY is written only into
phydev->irq, while the bus table mdiobus->irq[] keeps reading PHY_POLL
for the same address. That table is where phylib records what the bus
described - phy_device_create() seeds phydev->irq from it - so the
number lives only as long as nothing else writes that one field.

Put it in the table before the bus is registered, so that the PHY the
scan creates is born with the number, and drop the write to phydev->irq
that phylib then makes by itself. The address is not known that early,
so the whole table gets it. A devicetree PHY node still overrides that.

Found going through the drivers that keep a PHY interrupt outside the
bus table, so that the restore on detach later in this series has a
number to hand back here as well.

Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
---

Notes:
    Compile-tested only; I have no LAN78xx device.
    
    No Fixes: tag on this one. On its own it fixes nothing - nothing reads the
    bus table back until patch 3 - which is also why it sorts ahead of that
    patch rather than after it.
    
    lan78xx_setup_irq_domain() runs before lan78xx_mdio_init() in
    lan78xx_bind(), so the number is already mapped where the table is filled.
    Which address the PHY answers on is not known until of_mdiobus_register()
    has scanned, hence the whole table; mdio-moxart.c fills its own the same
    way.
    
    The fill is a default rather than an override. For a PHY node that
    describes an interrupt, fwnode_mdiobus_phy_device_register() writes the
    devicetree number over the table entry, and into phydev->irq, once the
    device exists. That inverts the old order, where the driver's own number
    was written last and won. Neither in-tree lan78xx PHY node carries an
    interrupts property, so nothing in tree changes, but a devicetree that
    described one would now be believed.
    
    Teardown order keeps the number live for as long as it is read:
    lan78xx_disconnect() detaches the PHY through phylink_disconnect_phy(), and
    lan78xx_unbind() calls lan78xx_remove_irq_domain() only afterwards.

 drivers/net/usb/lan78xx.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index cb782d81d84f..d7472d894c8d 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2092,6 +2092,7 @@ static int lan78xx_mdio_init(struct lan78xx_net *dev)
 {
 	struct device_node *node;
 	int ret;
+	int i;
 
 	dev->mdiobus = mdiobus_alloc();
 	if (!dev->mdiobus) {
@@ -2118,6 +2119,10 @@ static int lan78xx_mdio_init(struct lan78xx_net *dev)
 		break;
 	}
 
+	if (dev->domain_data.phyirq > 0)
+		for (i = 0; i < PHY_MAX_ADDR; i++)
+			dev->mdiobus->irq[i] = dev->domain_data.phyirq;
+
 	node = of_get_child_by_name(dev->udev->dev.of_node, "mdio");
 	ret = of_mdiobus_register(dev->mdiobus, node);
 	of_node_put(node);
@@ -2892,13 +2897,6 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 		return 0;
 	}
 
-	/* 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);
 	if (ret) {
 		netdev_err(dev->net, "can't attach PHY to %s, error %pe\n",
-- 
2.53.0

[PATCH net v9 2/4] net: usb: smsc95xx: register the PHY interrupt with the MDIO bus

[Aleksei Sviridkin]

The interrupt this driver maps for its PHY is written only into
phydev->irq, while the bus table mdiobus->irq[] keeps reading PHY_POLL
for the same address. That table is where phylib records what the bus
described - phy_device_create() seeds phydev->irq from it - so the
number lives only as long as nothing else writes that one field.

The bus is the one this function is about to register, so put the number
in its table first and let the scan seed the PHY from there. Which
address answers is not known that early, so the whole table gets it.

Found going through the drivers that keep a PHY interrupt outside the
bus table, so that the restore on detach later in this series has a
number to hand back here as well.

Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
---

Notes:
    Compile-tested only; I have no LAN95xx device.
    
    No Fixes: tag, for the same reason as patch 1: the write has no reader until
    patch 3 lands.
    
    The mapping is created well before mdiobus_alloc(), so the number is in hand
    where the table is filled, and mdiobus_alloc_size() is the only thing that
    writes PHY_POLL into that table - a fill placed after the alloc and before
    mdiobus_register() is not undone by the scan. Which address the PHY answers
    on is what phy_find_first() goes looking for afterwards, hence the whole
    table; mdio-moxart.c fills its own the same way.
    
    Teardown order keeps the number live for as long as it is read:
    smsc95xx_unbind() disconnects the PHY before it disposes the interrupt
    mapping.

 drivers/net/usb/smsc95xx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 42e4048b574b..ad65d2b9cec7 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1148,7 +1148,7 @@ static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf)
 {
 	struct smsc95xx_priv *pdata;
 	char usb_path[64];
-	int ret, phy_irq;
+	int ret, phy_irq, i;
 	u32 val;
 
 	ret = usbnet_get_endpoints(dev, intf);
@@ -1239,6 +1239,9 @@ static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf)
 	snprintf(pdata->mdiobus->id, ARRAY_SIZE(pdata->mdiobus->id),
 		 "usb-%03d:%03d", dev->udev->bus->busnum, dev->udev->devnum);
 
+	for (i = 0; i < PHY_MAX_ADDR; i++)
+		pdata->mdiobus->irq[i] = phy_irq;
+
 	ret = mdiobus_register(pdata->mdiobus);
 	if (ret) {
 		netdev_err(dev->net, "Could not register MDIO bus\n");
@@ -1252,7 +1255,6 @@ static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf)
 		goto unregister_mdio;
 	}
 
-	pdata->phydev->irq = phy_irq;
 	pdata->phydev->is_internal = pdata->is_internal_phy;
 
 	/* detect device revision as different features may be available */
-- 
2.53.0

[PATCH net v9 3/4] 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. That is where an MT7981B board here
stands, with an Airoha EN8811H behind an MT7531 port and its driver on
the root filesystem: the devicetree gives the PHY interrupt 15, and
once the real driver has taken over the field reads -1.

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.

Do it before device_release_driver() rather than after. That call
returns with the mdio device bindable and the device lock dropped, so
from then on a phy_probe() on another CPU is the other writer of this
field. Ahead of it the generic driver is still bound, and a driver
registering meanwhile is turned away with -EBUSY before it can reach
phy_probe().

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

Notes:
    Found and measured on an MT7981B board with an MT7531 switch and an Airoha
    EN8811H behind lan4, whose interrupt the devicetree describes and whose
    driver is a module.
    
    The one condition arranged for the run is that the PHY driver module loads
    after the root filesystem rather than from the early boot list this
    distribution normally puts it in. The distribution's own late-PHY handling
    was also removed, that being the one patch which could have changed the
    outcome; upstream has nothing like it. The kernel is still a distribution
    one and its remaining patches to phylink and phy_device do run on these
    paths - none of them writes phydev->irq.
    
    DSA then sets the port up at 1.87 s, the generic driver is bound by hand,
    phy_probe() replaces the interrupt with PHY_POLL, and phylink rejects
    2500base-x against it:
    
      lan4 (uninitialized): validation of 2500base-x ... failed: -EINVAL
      lan4 (uninitialized): failed to connect to PHY: -EINVAL
    
    The real driver arrives between 13.4 and 13.6 s depending on the boot, and
    binds. phydev->irq then reads -1 without this patch and 15 with it, 15
    being what the devicetree gave that PHY. The three switch ports alongside
    read 79, 80 and 81 in both runs, so the reading distinguishes rather than
    printing one answer. The field has no sysfs attribute of its own, so it was
    read with a debug-only module parameter that walks the MDIO bus and prints
    it.

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

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 94b2e85e00a3..8e6b399f95d6 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1959,6 +1959,9 @@ void phy_detach(struct phy_device *phydev)
 	if (phydev->mdio.dev.driver)
 		module_put(phydev->mdio.dev.driver->owner);
 
+	/* The release below can hand this field to a probe on another CPU. */
+	phydev->irq = phydev->mdio.bus->irq[phydev->mdio.addr];
+
 	/* If the device had no specific driver before (i.e. - it
 	 * was using the generic driver), we unbind the device
 	 * from the generic driver so that there's a chance a
-- 
2.53.0

[PATCH net v9 4/4] net: phy: restore the interrupt when the generic bind cycle fails

[Aleksei Sviridkin]

phy_attach_direct() binds the generic driver by hand, and the probe it
calls is phy_probe(), which replaces phydev->irq with PHY_POLL before
either of the points it can fail at. That failure unwinds on a label of
its own, which does not go through phy_detach(), so the substitution
outlives a bind cycle that never completed and a later attach finds a
PHY that can only be polled. Found while placing the restore of the
previous patch, as the other exit of the same bind cycle.

Take the number back on that label as well, before it clears d->driver.
That store is what reopens the device to the driver core: until it runs,
a driver registering on another CPU is turned away with -EBUSY and
phy_probe() cannot be the second writer of this field.

Fixes: 6d9f66ac7fec ("net: phy: Fix PHY module checks and NULL deref in phy_attach_direct()")
Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
---

Notes:
    Both points the hand-bind can fail at are reachable. phy_probe() reaches
    genphy_read_abilities() through genphy_driver's .get_features, and that
    returns the error from phy_read(phydev, MII_BMSR); device_bind_driver()
    returns whatever driver_sysfs_add() got from sysfs_create_link().
    
    A failed genphy bind leaves the device with no driver bound at all, so the
    next driver to arrive binds directly and never goes through phy_detach().
    That is why patch 3 cannot cover this path, and why the Fixes: tag here is
    6d9f66ac7fec rather than the one patch 3 carries. That commit did not
    introduce the lost number - the substitution is far older - it created this
    second exit from the bind cycle, splitting the failure off the label that
    calls phy_detach(). Before it, patch 3 alone would have covered this, so
    that is where the backport range for this one starts.
    
    Exercised on the board described in patch 3, with a debug-only module
    parameter that fails the hand-bound generic probe once for one MDIO
    address. The connect then ends in -EIO rather than the -EINVAL of the
    validation path, so the unwind takes the label this patch touches.
    phydev->irq afterwards reads -1 with patch 3 alone and 15 with this one.
    
    One difference between the injector and a real failure, since it does not
    affect what was measured but should not be implied away: a genuine error
    inside phy_probe() leaves through its out: label, which re-asserts the PHY
    reset before returning, while the injector returns earlier than that.
    Neither path touches phydev->irq.

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

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 8e6b399f95d6..69d8911ea7f6 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1896,6 +1896,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 error_module_put:
 	module_put(d->driver->owner);
+	/* Before the NULL below, which lets another probe reach this field. */
+	phydev->irq = bus->irq[phydev->mdio.addr];
 	phydev->is_genphy_driven = 0;
 	d->driver = NULL;
 error_put_device:
-- 
2.53.0

Re: [PATCH net v9 0/4] net: phy: keep a PHY interrupt across a generic bind cycle

[Aleksei Sviridkin]

Andrew, please don't spend time on v9. A v10 follows with one code
change.

The store in phy_detach() moves inside the generic-driver branch. As
posted it runs on every detach, so it also takes the value from the
drivers you listed that set PHY_MAC_INTERRUPT in phydev->irq alone,
which is what you said genphy should not touch.

There is a second case for the same fix. After a failed request
phy_request_interrupt() leaves PHY_POLL in phydev->irq while the bus
table still holds the number that failed, so an unconditional restore
hands that number back on the next cycle.

pw-bot: cr

Re: [PATCH net v9 1/4] net: usb: lan78xx: register the PHY interrupt with the MDIO bus

[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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Commit message hides a user-visible precedence inversion and
  is factually wrong about it.

--

net: usb: lan78xx: register the PHY interrupt with the MDIO bus

The driver's mapped PHY interrupt number is now written into every entry of
mdiobus->irq[] in lan78xx_mdio_init() before of_mdiobus_register(), and the
later assignment to phydev->irq in lan78xx_phy_init() is removed so that
phy_device_create() seeds the number from the bus table instead.

> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index cb782d81d84f..d7472d894c8d 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -2092,6 +2092,7 @@ static int lan78xx_mdio_init(struct lan78xx_net *dev)
>  {
>  	struct device_node *node;
>  	int ret;
> +	int i;
>  
>  	dev->mdiobus = mdiobus_alloc();
>  	if (!dev->mdiobus) {
> @@ -2118,6 +2119,10 @@ static int lan78xx_mdio_init(struct lan78xx_net *dev)
>  		break;
>  	}
>  
> +	if (dev->domain_data.phyirq > 0)
> +		for (i = 0; i < PHY_MAX_ADDR; i++)
> +			dev->mdiobus->irq[i] = dev->domain_data.phyirq;
> +
>  	node = of_get_child_by_name(dev->udev->dev.of_node, "mdio");
>  	ret = of_mdiobus_register(dev->mdiobus, node);
>  	of_node_put(node);

[ ... ]

> @@ -2892,13 +2897,6 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>  		return 0;
>  	}
>  
> -	/* 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);

[Severity: Medium]
Does moving the number into mdiobus->irq[] change which value ends up in
phydev->irq for LAN7800/LAN7850?

Before this patch the assignment above ran after of_mdiobus_register() had
created and probed the PHY, so the driver's number was written last. Now
the number is present at scan time, and phy_probe() runs after it:

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

The lan88xx entry in drivers/net/phy/microchip.c deliberately defines no
interrupt callbacks:

	/* Interrupt handling is broken, do not define related
	 * functions to force polling.
	 */

So for the internal PHY at address 1 (phy_mask = ~(1 << 1)) phydev->irq is
seeded from the table, phy_probe() sees a valid interrupt with a driver that
does not support one, and demotes it back to PHY_POLL. The old late write in
lan78xx_phy_init() happened after phy_probe() and defeated that demotion.
Does this move LAN7515 boards such as Raspberry Pi 3B+/4 from the chip's
INT_EP_PHY interrupt to phylib polling?

If so, the post-commit note "Neither in-tree lan78xx PHY node carries an
interrupts property, so nothing in tree changes" does not cover this path,
since the change here comes from phy_probe() rather than from devicetree.

The commit message also says:

	A devicetree PHY node still overrides that.

Is "still" accurate? Previously fwnode_mdiobus_phy_device_register() wrote a
devicetree interrupt into both phy->irq and mdio->irq[addr]:

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

and lan78xx_phy_init() then overwrote phy->irq with the driver's number, so
the devicetree value was discarded. After this patch the devicetree value
wins. That is a precedence inversion rather than unchanged behaviour, so
could the commit body state it directly?

The explanation of all of this currently lives in the notes below the ---
line, which is dropped when the patch is applied. Could the permanent commit
message carry both behaviour changes, and could the "On its own it fixes
nothing" reasoning for omitting a Fixes tag be revisited given that
LAN7800/LAN7850 interrupt handling changes with this patch alone?

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

Re: [PATCH net v9 3/4] net: phy: take the interrupt back from the bus on detach

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The new store in phy_detach()
  (drivers/net/phy/phy_device.c:1963) is placed before, and outside, the…
- [Medium] The restore of phydev->irq is not paired with the site that
  clobbers it.
- [Medium] phy_detach() writes phydev->irq
  (drivers/net/phy/phy_device.c:1963) with no lock held on the mdio…
- [Low] Concern that the patch's premise (mdiobus->irq[addr] is always
  where the board's PHY interrupt is recorded) is violated by sxgbe,…
- [Low] The added comment "The release below can hand this field to a
  probe on another CPU." (drivers/net/phy/phy_device.c:1962) ties the…
- [Low] Documentation/networking/phy.rst:216-223 tells MAC drivers that
  phydev->irq is theirs to set directly ("just set phydev->irq to…

--

net: phy: take the interrupt back from the bus on detach

phy_detach() now re-seeds phydev->irq from mdiobus->irq[addr] so that a PHY
which was temporarily bound to the generic driver does not keep the PHY_POLL
value that phy_probe() installed. The store is placed just before the
device_release_driver() call that unbinds the generic driver.

A few questions about the placement and the scope of the new store.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 94b2e85e00a3..8e6b399f95d6 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1959,6 +1959,9 @@ void phy_detach(struct phy_device *phydev)
>  	if (phydev->mdio.dev.driver)
>  		module_put(phydev->mdio.dev.driver->owner);
>  
> +	/* The release below can hand this field to a probe on another CPU. */
> +	phydev->irq = phydev->mdio.bus->irq[phydev->mdio.addr];
> +

[Severity: High]

This store is outside the "if (phydev->is_genphy_driven)" block below, so it
runs on every detach, including detaches where a real driver stays bound and
no phy_probe() will re-derive the field.

Can that duplicate an irqaction for MAC drivers that install
PHY_MAC_INTERRUPT after connecting?  tsnep_phy_open() in
drivers/net/ethernet/engleder/tsnep_main.c does:

	retval = phy_connect_direct(...);	/* requests the PHY interrupt */
	...
	adapter->phydev->irq = PHY_MAC_INTERRUPT;
	phy_start(adapter->phydev);

On close, phy_disconnect() skips the free because the field no longer looks
like a valid irq:

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

	phydev->adjust_link = NULL;

	phy_detach(phydev);

so the request_threaded_irq() action stays installed, and phy_detach() now
writes the real interrupt number back into phydev->irq.  On the next ifup,
phy_connect_direct() -> phy_request_interrupt() sees a valid irq and calls
request_threaded_irq() again for the same irq with the same dev_id (phydev),
so a second action is appended for every down/up cycle, and a later
free_irq() removes only one of the two identical actions.

If the trigger types disagree instead, the second request fails and
phy_request_interrupt() takes this path on every ifup:

	phydev_warn(phydev, "Error %d requesting IRQ %d, falling back to polling\n",
		    err, phydev->irq);
	phydev->irq = PHY_POLL;

phy_attach_direct() only re-sanitizes the two cases it knows about:

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

which recomputes neither PHY_MAC_INTERRUPT nor the polling fallback above.
Would moving the store inside the is_genphy_driven branch, the only case
where a later phy_probe() reads the field, avoid this side effect?

[Severity: Medium]

Is this store synchronized against the other writer of the field?
phy_probe() runs from really_probe() with device_lock(&phydev->mdio.dev)
held, while phy_detach() holds no lock on the mdio device here, so the
lockset intersection for phydev->irq is empty.

The commit message says:

    Ahead of it the generic driver is still bound, and a driver
    registering meanwhile is turned away with -EBUSY before it can reach
    phy_probe().

The -EBUSY does exist, but it comes from __driver_probe_device():

	if (dev->driver)
		return -EBUSY;

and it is evaluated under the device lock that phy_detach() does not hold.
It also does not cover an independent unbind on another CPU:

	unbind_store() -> device_driver_detach() -> device_release_driver()

which clears mdio.dev.driver and makes the device bindable before
phy_detach() reaches its own device_release_driver().  And because the store
is unconditional, it also runs when is_genphy_driven is 0, where phy_detach()
never touches the binding at all and no exclusion of any kind applies.

Would doing the restore under device_lock(&phydev->mdio.dev), or from
phy_remove() which already runs under it, be a better fit than ordering it
before device_release_driver()?

[Severity: Medium]

The restore is not placed at the counterpart of the site that clobbers the
field.  phy_probe() writes PHY_POLL on every bind of a driver without
config_intr/handle_interrupt:

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

but phy_remove(), which is what undoes phy_probe(), is untouched.  Does the
downgrade then survive a PHY driver swap performed while the interface stays
up?

	echo dev > /sys/bus/mdio_bus/drivers/<no-irq driver>/unbind
	echo dev > /sys/bus/mdio_bus/drivers/<irq-capable driver>/bind

unbind_store() -> device_release_driver() -> phy_remove() does not restore
the field, so the new driver's phy_probe() sees phy_interrupt_is_valid() ==
false and keeps PHY_POLL.  With no phy_detach() afterwards the PHY polls for
the rest of the uptime, which is the same outcome the commit message
describes as "nothing puts it back".  The same holds for rmmod of an
interrupt-less PHY driver followed by modprobe of an interrupt-capable one.

Worth noting that simply moving the restore into phy_remove() is not a
drop-in replacement either: restoring while the PHY is still attached makes
phy_interrupt_is_valid() true for a connection that never requested the irq,
so phy_disconnect() would call phy_free_interrupt() on a line that was never
requested.  Could the field's saved value be tracked explicitly instead of
adding one restore per unwind path?

For context, the follow-on commit in this series ("net: phy: restore the
interrupt when the generic bind cycle fails") adds the same restore to the
error_module_put label in phy_attach_direct(), so the restore is now
duplicated on two caller paths rather than living at the counterpart of the
clobber.

[Severity: Low]

Is mdiobus->irq[addr] authoritative for every bus?  sxgbe_mdio_register()
records a probed PHY interrupt directly in phydev->irq:

	if ((mdio_data->irqs == NULL) &&
	    (mdio_data->probed_phy_irq > 0)) {
		irqlist[phy_addr] = mdio_data->probed_phy_irq;
		phy->irq = mdio_data->probed_phy_irq;
	}

If a bus does that, the unconditional restore here would replace it with the
bus array value.

This particular branch looks unreachable in-tree: the only instantiation of
struct sxgbe_mdio_bus_data is the devm_kzalloc() in sxgbe_probe_config_dt(),
so probed_phy_irq is always 0, and the store above is a no-op for sxgbe.
Is the assumption that no bus keeps the number only in phydev->irq something
that should be spelled out in the commit message?

[Severity: Low]

The comment says "The release below can hand this field to a probe on
another CPU", but the release it refers to is conditional while the store is
not:

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

For a phydev with a real driver bound there is no release at all, yet the
field is still overwritten.  Could the comment cover that case, or the store
move inside the is_genphy_driven branch?

[Severity: Low]

Documentation/networking/phy.rst currently tells MAC drivers the field is
theirs to set:

    also handles PHY status changes, just set phydev->irq to PHY_MAC_INTERRUPT
    before you call phy_start and use phy_mac_interrupt() from the network
    driver. If you don't want to use interrupts, set phydev->irq to PHY_POLL.

with no mention of mdiobus->irq[].  After this change, mdiobus->irq[addr]
becomes the authoritative store and a MAC-written value only survives until
the next detach.  Should phy.rst and the phy_detach() kernel-doc, which
currently only says it detaches the phy from its network device and driver
and drops the reference taken in phy_attach_direct(), mention the reset?

>  	/* If the device had no specific driver before (i.e. - it
>  	 * was using the generic driver), we unbind the device
>  	 * from the generic driver so that there's a chance a

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

Re: [PATCH net v9 4/4] net: phy: restore the interrupt when the generic bind cycle fails

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The added restore reads bus->irq[phydev->mdio.addr] instead of
  the value phydev->irq actually held on entry to phy_attach_direct().
- [Medium] The correctness argument the patch states for the new store is
  not backed by any lock or barrier.

Pre-existing issues:
- [Medium] The error_module_put label extended by this patch performs no
  phy_remove()-equivalent rollback when d->driver->probe(d)…

--

net: phy: restore the interrupt when the generic bind cycle fails

The patch adds a restore of phydev->irq on the error_module_put label of
phy_attach_direct(), so that the PHY_POLL substitution done by phy_probe()
does not outlive a hand-bind of the generic driver that failed.  The value
is taken from bus->irq[phydev->mdio.addr] and stored before d->driver is
cleared.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 8e6b399f95d6..69d8911ea7f6 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1896,6 +1896,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  
>  error_module_put:
>  	module_put(d->driver->owner);
> +	/* Before the NULL below, which lets another probe reach this field. */
> +	phydev->irq = bus->irq[phydev->mdio.addr];

[Severity: Medium]

Is bus->irq[phydev->mdio.addr] always the value phydev->irq held on entry
to phy_attach_direct()?  The array is seeded with PHY_POLL for every
address:

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

Some producers do keep the two in sync, which is why the restore works for
DT buses:

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

Others write only phydev->irq.  In sxgbe_mdio_register() the probed
interrupt goes into irqlist[] - which is mdio_data->irqs or the private
priv->mii_irq, never mdio_bus->irq[] - and into phy->irq:

drivers/net/ethernet/samsung/sxgbe/sxgbe_mdio.c:sxgbe_mdio_register() {
	...
			if ((mdio_data->irqs == NULL) &&
			    (mdio_data->probed_phy_irq > 0)) {
				irqlist[phy_addr] = mdio_data->probed_phy_irq;
				phy->irq = mdio_data->probed_phy_irq;
			}
	...
}

On a bus like that, doesn't the new store put PHY_POLL back into
phydev->irq?  That would leave exactly the "a later attach finds a PHY that
can only be polled" state the changelog describes, since sxgbe assigns
phy->irq once at mdiobus_register() time and a later phy_connect() sees the
clobbered value.

The opposite mismatch looks possible too.  A MAC that deliberately forces
PHY_POLL before attaching:

drivers/net/ethernet/qualcomm/emac/emac-mac.c:emac_mac_up() {
	...
	adpt->phydev->irq = PHY_POLL;
	ret = phy_connect_direct(netdev, adpt->phydev, emac_adjust_link,
				 PHY_INTERFACE_MODE_SGMII);
	...
}

If the DT for that bus declares the PHY interrupt, the new line replaces the
caller's PHY_POLL with the bus number on the error path (transient for emac,
which re-sets it before every connect), and in that case phy_probe() never
substituted anything to begin with.

Would saving phydev->irq into a local before d->driver->probe(d) and
restoring that local instead be correct for every producer?

>  	phydev->is_genphy_driven = 0;
>  	d->driver = NULL;

[Severity: Medium]

The changelog and the new comment rest on this claim:

  "That store is what reopens the device to the driver core: until it runs,
  a driver registering on another CPU is turned away with -EBUSY and
  phy_probe() cannot be the second writer of this field."

Which lock makes that exclusion hold?  The -EBUSY gate is a plain load taken
under device_lock(dev):

drivers/base/dd.c:__driver_probe_device() {
	...
	if (dev->driver)
		return -EBUSY;
	...
}

phy_attach_direct() never takes device_lock(d).  It writes d->driver in the
hand-bind:

	if (!d->driver) {
		if (phydev->is_c45)
			d->driver = &genphy_c45_driver.mdiodrv.driver;
		else
			d->driver = &genphy_driver.mdiodrv.driver;

		phydev->is_genphy_driven = 1;
	}

and clears it on this label, and calls device_bind_driver() in between,
whose kerneldoc states:

drivers/base/dd.c:
 * This function must be called with the device lock held.

With no lock shared between the two sides, can a concurrent
driver_register() -> driver_attach() -> device_driver_attach() ->
__driver_probe_device() observe d->driver == NULL, pass the gate, and run
really_probe() -> phy_probe(), which writes the field again?

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

That is the "PHY driver module arrives later" case the series is about.

Separately, the intended "restore, then reopen" ordering is two plain stores
to different addresses with no smp_store_release() or smp_wmb(), and the
reader side is a plain load, so on a weakly ordered CPU can the NULL become
visible before the restored irq?

Would performing the hand-bind and this unbind under device_lock(d), as
device_bind_driver() requires, make both the exclusion and the ordering
real?

> error_put_device:

[Severity: Medium]

This isn't a bug introduced by this patch - the label came in with
6d9f66ac7fec - but since the label is being extended, what unwinds
phy_probe() when d->driver->probe(d) succeeded and device_bind_driver()
then failed?

	if (phydev->is_genphy_driven) {
		err = d->driver->probe(d);
		if (err >= 0)
			err = device_bind_driver(d);

		if (err)
			goto error_module_put;
	}

device_bind_driver() returns driver_sysfs_add()'s error and does not call
bus->remove, so the caller owns the rollback, but the label only does
module_put(), the new irq store, is_genphy_driven = 0 and d->driver = NULL.

What phy_probe() established stays live: the kzalloc'd
phydev->led_link_trigger and phy_led_triggers array plus the triggers
registered with the LED core in phy_led_triggers_register(), the phy_port
objects from phy_setup_ports(), the de-asserted reset

drivers/net/phy/phy_device.c:phy_probe() {
	...
	/* Deassert the reset signal */
	phy_device_reset(phydev, 0);
	...
}

and phydev->drv still pointing at genphy_driver while d->driver is NULL.

Because the label clears is_genphy_driven and d->driver, can any later
phy_detach() -> device_release_driver() -> phy_remove() still run the
counterpart teardown?

drivers/net/phy/phy_device.c:phy_remove() {
	...
	if (!phydev->is_on_sfp_module)
		phy_led_triggers_unregister(phydev);
	...
	phy_cleanup_ports(phydev);
	...
	/* Assert the reset signal */
	phy_device_reset(phydev, 1);

	phydev->drv = NULL;
	...
}

The leaked triggers also keep their names registered, so a later
phy_led_triggers_register() for the same MDIO address returns -EEXIST.
phy_probe() ignores that return value, so the next bind still succeeds, just
without LED triggers for that PHY.

One reachable trigger for the sysfs link failure: the bind block keys on the
persistent phydev->is_genphy_driven and runs before the

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

check, so attaching the same phydev twice can make sysfs_create_link()
return -EEXIST.

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