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

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

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    | 7 +++----
 drivers/net/usb/smsc95xx.c   | 1 +
 3 files changed, 9 insertions(+), 4 deletions(-)


base-commit: 3b95a04eb5f95bf6a016a1bb9ff37d3eee48de63
-- 
2.53.0

[PATCH net v8 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 where the bus keeps it and take phydev->irq from there. What the
PHY is attached with does not change.

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.
    
    The else branch that forced PHY_POLL is dropped rather than moved to the
    table, for two reasons. It cannot run: lan78xx_setup_irq_domain() returns
    -EINVAL when it cannot create the mapping, lan78xx_bind() gives up on that
    return, and lan78xx_phy_init() only runs afterwards, so phyirq is always
    positive by the time this code is reached. And if it could run it would be
    wrong in the other direction: this bus is registered with
    of_mdiobus_register(), so the table may already hold an interrupt the
    devicetree described, and forcing PHY_POLL over it would invert what
    patches 3 and 4 are for.
    
    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 | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index cb782d81d84f..d02e4ffcbbc1 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2892,11 +2892,10 @@ 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;
+		dev->mdiobus->irq[phydev->mdio.addr] = dev->domain_data.phyirq;
+
+	phydev->irq = dev->mdiobus->irq[phydev->mdio.addr];
 	netdev_dbg(dev->net, "phydev->irq = %d\n", phydev->irq);
 
 	ret = phylink_connect_phy(dev->phylink, phydev);
-- 
2.53.0

[PATCH net v8 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 registered a few lines earlier, so
record the number there 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.
    
    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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 42e4048b574b..130a460c1f54 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1252,6 +1252,7 @@ static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf)
 		goto unregister_mdio;
 	}
 
+	pdata->mdiobus->irq[pdata->phydev->mdio.addr] = phy_irq;
 	pdata->phydev->irq = phy_irq;
 	pdata->phydev->is_internal = pdata->is_internal_phy;
 
-- 
2.53.0

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

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

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 v8 1/4] net: usb: lan78xx: register the PHY interrupt with the MDIO bus

[Andrew Lunn]

> -	/* 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;
> +		dev->mdiobus->irq[phydev->mdio.addr] = dev->domain_data.phyirq;
> +
> +	phydev->irq = dev->mdiobus->irq[phydev->mdio.addr];

This line is redundant, given how phylib works.

>  	netdev_dbg(dev->net, "phydev->irq = %d\n", phydev->irq);

So maybe change this line?

	Andrew

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

[Andrew Lunn]

On Fri, Sep 18, 2026 at 04:50:27AM +0300, Aleksei Sviridkin wrote:
> 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 registered a few lines earlier, so
> record the number there 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.
>     
>     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 | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> index 42e4048b574b..130a460c1f54 100644
> --- a/drivers/net/usb/smsc95xx.c
> +++ b/drivers/net/usb/smsc95xx.c
> @@ -1252,6 +1252,7 @@ static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf)
>  		goto unregister_mdio;
>  	}
>  
> +	pdata->mdiobus->irq[pdata->phydev->mdio.addr] = phy_irq;
>  	pdata->phydev->irq = phy_irq;

Looking at just the information here it looks like this line can be deleted?

	Andrew

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

[Aleksei Sviridkin]

> > +	phydev->irq = dev->mdiobus->irq[phydev->mdio.addr];
>
> This line is redundant, given how phylib works.

It is once the table is filled before the bus is registered, and here
it is not yet: this patch writes the table in lan78xx_phy_init(), after
the scan, so phy_device_create() had already seeded phydev->irq from a
table that still read PHY_POLL.

The number is known before the bus exists, lan78xx_setup_irq_domain()
runs ahead of lan78xx_mdio_init(), so v9 fills the table there, before
of_mdiobus_register(), and drops this assignment together with the
if/else above it. On a 7801 the PHY address is not known until the scan,
so every entry gets the number, the way mdio-moxart does it. A
devicetree mdio node still wins: fwnode_mdio writes both fields after
the device is created.

> >  	netdev_dbg(dev->net, "phydev->irq = %d\n", phydev->irq);
>
> So maybe change this line?

It goes too. phy_attached_print() reports the irq at connect.

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

[Aleksei Sviridkin]

> > +	pdata->mdiobus->irq[pdata->phydev->mdio.addr] = phy_irq;
> >  	pdata->phydev->irq = phy_irq;
>
> Looking at just the information here it looks like this line can be
> deleted?

Yes, once the table is filled before mdiobus_register() instead of
after phy_find_first(): phy_device_create() seeds phydev->irq from the
table at scan time, and as written that still happens with PHY_POLL in
it. phy_irq exists before the bus is even allocated, so v9 moves the
fill ahead of the registration, for every address since the PHY's
address is not known until the scan, and deletes this line. Same shape
in 1/4.

Re: [PATCH net v8 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 (1)

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] In lan78xx_phy_init() (drivers/net/usb/lan78xx.c:2895-2898)
  the new store `dev->mdiobus->irq[phydev->mdio.addr] =…

Pre-existing issues:
- [Low] The kernel-doc for lan78xx_get_phy()
  (drivers/net/usb/lan78xx.c:2640-2648) states "If no PHY is found and…

--

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

The patch moves the chip-domain PHY interrupt that lan78xx maps for itself
into the mii_bus IRQ table and then reads phydev->irq back out of that
table, replacing the previous direct assignment to phydev->irq and dropping
the PHY_POLL else branch.

> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index cb782d81d84f..d02e4ffcbbc1 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -2892,11 +2892,10 @@ 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;
> +		dev->mdiobus->irq[phydev->mdio.addr] = dev->domain_data.phyirq;
> +
> +	phydev->irq = dev->mdiobus->irq[phydev->mdio.addr];

[Severity: Medium]
Can this store discard an interrupt that devicetree described for this PHY
address?

lan78xx_mdio_init() registers the bus through OF:

	node = of_get_child_by_name(dev->udev->dev.of_node, "mdio");
	ret = of_mdiobus_register(dev->mdiobus, node);

and for a PHY child with an interrupts property the MDIO core records the
firmware interrupt in both places, in fwnode_mdiobus_phy_device_register():

	if (rc > 0) {
		phy->irq = rc;
		mdio->irq[addr] = rc;
	} else {
		phy->irq = mdio->irq[addr];
	}

lan78xx_phy_init() then writes dev->domain_data.phyirq over that slot. Is
the guard dev->domain_data.phyirq > 0 ever false here? lan78xx_setup_irq_domain()
returns -EINVAL when the mapping cannot be created and lan78xx_bind() gives
up on that return, which the commit message itself notes, so the store looks
unconditional in practice and the devicetree value is gone from the table
with no way to get it back.

The interrupt being written is the chip-internal virq driven from the
LAN78xx endpoint status bit in intr_complete():

	if (intdata & INT_ENP_PHY_INT) {
		...
		if (dev->domain_data.phyirq > 0)
			generic_handle_irq_safe(dev->domain_data.phyirq);

On LAN7801 with an external RGMII PHY whose nINT line is wired to the host
and described in devicetree, that is a different source from the line the
table held. Does this leave the table unable to distinguish "firmware
described an interrupt" from "driver's own chip-domain virq"?

That distinction is what the later patches in the series depend on: with
"net: phy: take the interrupt back from the bus on detach" and "net: phy:
restore the interrupt when the generic bind cycle fails" applied,
phy_detach() and the failed-bind path read bus->irq[addr] back, so from
there on they hand the chip-internal virq to phydev->irq rather than the
devicetree line.

There is also a mismatch with the changelog, which justifies deleting the
else branch this way:

  "this bus is registered with of_mdiobus_register(), so the table may
  already hold an interrupt the devicetree described, and forcing PHY_POLL
  over it would invert what patches 3 and 4 are for"

Does the line immediately above it perform that same inversion for a real
devicetree interrupt, only losing the original value instead of masking it?

Would it work to seed the table only when it still reads PHY_POLL, or to
state the precedence between the devicetree line and the chip-domain virq
explicitly in this patch?

>  	netdev_dbg(dev->net, "phydev->irq = %d\n", phydev->irq);
>  
>  	ret = phylink_connect_phy(dev->phylink, phydev);

[Severity: Low]
This isn't a bug introduced by this patch, but the kernel-doc above
lan78xx_get_phy() does not match what the function does, and the mismatch
touches the code being changed here. It says:

  * This function attempts to find a PHY on the MDIO bus. If no PHY is found
  * and the chip is LAN7801, it registers a fixed PHY as fallback. It also
  * sets dev->interface based on chip ID and detected PHY type.
  *
  * Return: a valid PHY device pointer, or ERR_PTR() on failure.

No fixed PHY device is registered there, and the ID_REV_CHIP_ID_7801_ case
returns plain NULL:

	dev->interface = PHY_INTERFACE_MODE_RGMII;
	/* No PHY found – fallback to fixed PHY (e.g. KSZ switch board) */
	return NULL;

The fixed link is configured later by phylink_set_fixed_link() in
lan78xx_set_fixed_link(). The undocumented NULL return is what the
if (!phydev) early return in lan78xx_phy_init() keys off, and that early
return is what keeps the new phydev->mdio.addr accesses safe. Could the
kernel-doc be updated to mention the NULL return and drop the fixed PHY
registration claim?

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

Re: [PATCH net v8 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 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 2 · 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).

- [High] The new store in phy_detach()
  (drivers/net/phy/phy_device.c:1962-1963) sits above and is not guarded…
- [Medium] Incomplete fix / restore placed at the wrong layer: the
  destructive write lives in phy_probe() (`if…
- [Medium] The new store `phydev->irq =
  phydev->mdio.bus->irq[phydev->mdio.addr]` in phy_detach()…

--

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

phy_detach() now re-seeds phydev->irq from mdiobus->irq[phydev->mdio.addr]
just before the generic driver is released, so that a PHY which was bound to
the generic driver first (because its own driver was a module not yet
available) does not keep the PHY_POLL value phy_probe() installed once the
real driver binds.

> 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 sits above the is_genphy_driven test, so it runs on every
phy_detach() and not only on the generic-driver case the changelog
describes.  Should it be inside that branch?

As written, mii_bus->irq[] becomes the authoritative value for all PHYs, and
any phydev->irq that was never mirrored into the bus array is discarded.
Three cases that seem affected:

First, phylib's own permanent fallback in phy_request_interrupt():

  drivers/net/phy/phy.c:phy_request_interrupt() {
	...
	if (err) {
		phydev_warn(phydev, "Error %d requesting IRQ %d, falling back to polling\n",
			    err, phydev->irq);
		phydev->irq = PHY_POLL;
	...
  }

bus->irq[addr] still holds the number whose request just failed.  After the
restore, does the next connect re-request the known-bad interrupt and print
the warning again, once per cycle?

Second, MAC drivers that set PHY_MAC_INTERRUPT only in phydev->irq, which is
what Documentation/networking/phy.rst tells them to do:

  "If the MAC interrupt of your network driver also handles PHY status
   changes, just set phydev->irq to PHY_MAC_INTERRUPT before you call
   phy_start ... If you don't want to use interrupts, set phydev->irq to
   PHY_POLL."

tsnep_phy_open() does this after phy_connect_direct() has already requested
the bus interrupt:

  drivers/net/ethernet/engleder/tsnep_main.c:tsnep_phy_open() {
	...
	adapter->phydev->irq = PHY_MAC_INTERRUPT;
	phy_start(adapter->phydev);
  }

bcmgenet_mii_probe() and bcmasp_intf.c do the same for internal PHYs.  On
teardown, phy_disconnect() cannot free that registration:

  drivers/net/phy/phy_device.c:phy_disconnect() {
	...
	if (phy_interrupt_is_valid(phydev))
		phy_free_interrupt(phydev);
	...
	phy_detach(phydev);
  }

because phy_interrupt_is_valid(PHY_MAC_INTERRUPT) is false.  With the
restore, the next open calls phy_request_interrupt() again on a DT-described
interrupt that of_mdiobus_register() mirrored into bus->irq[addr].  Does
this leak one irqaction and IRQ thread per ifdown/ifup, where before the
patch the PHY_POLL substitution persisted and bounded it to one?

Third, the reverse direction for buses that record the number only in
phydev->irq plus a private array:

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

bus->irq[] is left at the mdiobus_alloc_size() default:

  drivers/net/phy/mdio_bus_provider.c:mdiobus_alloc_size() {
	/* Initialise the interrupts to polling and 64-bit seqcounts */
	for (i = 0; i < PHY_MAX_ADDR; i++) {
		bus->irq[i] = PHY_POLL;
	...
  }

so does an ordinary close/open on sxgbe now silently downgrade a working
interrupt to polling?  stmmac_mdio.c mirrors into new_bus->irq[] and would
not be affected, which suggests the assumption that bus->irq[] always holds
the truth is not tree-wide.

[Severity: Medium]

The destructive write lives in phy_probe():

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

but the restore is added to a caller of the bind/unbind sequence rather than
to phy_probe()'s counterpart phy_remove().  Would phy_remove() be the more
complete place for it?

phy_remove() is the hook the driver core runs for every unbind, and it
writes phydev->drv = NULL without touching phydev->irq.  An unbind that does
not come through phy_detach() therefore leaves PHY_POLL in place - for
example a sysfs unbind of the bound non-IRQ-capable driver on the mdio
device, which is reachable because nothing in drivers/net/phy sets
suppress_bind_attrs.  A later-loading interrupt-capable driver then inherits
PHY_POLL for the rest of the session.

phy_probe()'s own out: label also returns with the field already replaced and
never restores it.

The sibling unwind path in phy_attach_direct():

  error_module_put:
	module_put(d->driver->owner);
	phydev->is_genphy_driven = 0;
	d->driver = NULL;

is left with the same gap by this patch and is only closed by the later
patch in this series, "net: phy: restore the interrupt when the generic bind
cycle fails".  With one clobber site and now two hand-placed restores, and
phy_remove() still uncovered, is the caller-side approach the right layer?

[Severity: Medium]

The new store runs with neither device_lock(&phydev->mdio.dev) nor
phydev->lock held, while the other writer of the same field, phy_probe(),
always runs from really_probe() under the mdio device lock.  Is there a
common lock for phydev->irq here?

The comment reasons about ordering against device_release_driver(), but the
binding state is never sampled under the device lock - phy_detach() consults
its own phydev->is_genphy_driven bit, and the generic driver is installed by
a plain assignment in phy_attach_direct():

  drivers/net/phy/phy_device.c:phy_attach_direct() {
	...
	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;
	}
	...
  }

A concurrent sysfs unbind clears dev->driver and makes the device bindable
independently of this path:

  drivers/base/bus.c:unbind_store() {
	...
	dev = bus_find_device_by_name(bus, NULL, buf);
	if (dev && dev->driver == drv) {
		device_driver_detach(dev);
	...
  }

If that lands while phy_detach() is between its module_put() and this store,
a phy_probe() on another CPU becomes a second unsynchronised writer of the
same int.  Can the resulting lost update drop either the restore this patch
adds or phy_probe()'s PHY_POLL for a driver without interrupt support?

Would doing the restore in phy_remove() under the device lock, or wrapping
it in device_lock()/device_unlock(), close this?  For the
is_genphy_driven == 0 case the changelog offers no exclusion argument at
all, since the pairing with the device mutex only exists on the genphy path.

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

Re: [PATCH net v8 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: 1 (1) · Low: 1

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 exclusion the commit message relies on does not exist as a
  lock.
- [Low] The new comment added at drivers/net/phy/phy_device.c:1899
  ("Before the NULL below, which lets another probe reach this field.")…

Pre-existing issues:
- [Medium] The error_module_put label that this patch extends
  (drivers/net/phy/phy_device.c:1897-1903) unwinds only the module…

--

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

This adds a restore of phydev->irq from bus->irq[phydev->mdio.addr] on the
error_module_put label in phy_attach_direct(), so that the PHY_POLL value
installed by phy_probe() does not survive a hand-bind of the generic driver
that never completed. The restore is placed ahead of the d->driver = NULL
store, and the commit message argues that this ordering keeps phy_probe()
from being a second writer of phydev->irq.

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

[Severity: Low]
Does the code really establish the ordering this comment describes?  The two
stores are plain and touch distinct fields, with no data dependency and no
barrier between them:

drivers/net/phy/phy_device.c:phy_attach_direct() {
	...
	phydev->irq = bus->irq[phydev->mdio.addr];
	phydev->is_genphy_driven = 0;
	d->driver = NULL;
	...
}

Can the compiler, or a weakly ordered CPU, publish the NULL driver pointer
before the IRQ restore becomes visible?  The admission check the commit
message refers to reads the field with no acquire pairing:

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

so a PHY driver registering on another CPU could pass that check and reach
phy_probe(), which writes the same field:

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

and then the delayed store from the unwind path overwrites PHY_POLL with the
bus interrupt number.  The attach path re-forces PHY_POLL on the next attach
in phy_attach_direct(), so the observable effect looks limited to the stated
guarantee not matching what the stores do.  Would either a real ordering
primitive, or dropping the claim from the comment and the changelog, be more
accurate here?  The same wording was added to the phy_detach() restore by the
preceding commit in the series ("net: phy: take the interrupt back from the
bus on detach"), so both would need the same treatment.

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

[Severity: Medium]
Is the exclusion the changelog relies on backed by a lock anywhere?  The
changelog says:

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

phy_attach_direct() installs the generic driver, calls its probe and calls
device_bind_driver(d), and later clears d->driver on this label, without ever
taking device_lock(&phydev->mdio.dev).  The driver core documents the
opposite requirement:

drivers/base/dd.c, kernel-doc for device_bind_driver():
 * This function must be called with the device lock held.
 *
 * Callers should prefer to use device_driver_attach() instead.

and every driver-core access to dev->driver happens under that mutex, taken
by __device_driver_lock() on the driver_register() -> driver_attach() ->
__driver_attach() -> device_driver_attach() -> driver_probe_device() path.
With an empty lockset intersection, does the -EBUSY check actually serialize
the two paths?

There also seems to be a wider window than the one this patch reasons about:
if a concurrent __driver_probe_device() observes dev->driver == NULL just
before phy_attach_direct() stores the genphy pointer into it, does anything
prevent really_probe()/device_set_driver() from running while phylib is in
device_bind_driver() -> driver_sysfs_add()/driver_bound() for the same mdio
device?  Would doing the generic bind and unbind under device_lock(d), or via
device_driver_attach()/device_release_driver(), remove the need for the
ordering-by-convention argument altogether?

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

[Severity: Medium]
This isn't a bug introduced by this patch, but while this label is being
extended: does it leak what phy_probe() allocated when the bind fails at the
second point?

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

On that path phy_probe() already returned 0, so it has de-asserted the PHY
reset, created the port objects via phy_setup_ports() and registered the LED
triggers via phy_led_triggers_register().  device_bind_driver() returns the
driver_sysfs_add() error without calling the driver's remove callback:

drivers/base/dd.c:device_bind_driver() {
	ret = driver_sysfs_add(dev);
	if (!ret) {
		device_links_force_bind(dev);
		driver_bound(dev);
	}
	else
		bus_notify(dev, BUS_NOTIFY_DRIVER_NOT_BOUND);
	return ret;
}

Since driver_bound() never ran, the driver core will not call phy_remove(),
which is the only place that undoes those steps:

drivers/net/phy/phy_device.c:phy_remove() {
	phy_cleanup_ports(phydev);

	if (phydev->drv && phydev->drv->remove)
		phydev->drv->remove(phydev);

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

	phydev->drv = NULL;
	...
}

and the release function is a plain kfree():

drivers/net/phy/phy_device.c:phy_device_release() {
	fwnode_handle_put(dev->fwnode);
	kfree(to_phy_device(dev));
}

Does that leak the port objects and the phy_led_triggers/led_link_trigger
allocations, leave the triggers registered with the LED core for a PHY with
no driver bound, and leave the reset de-asserted?  phydev->drv also stays
pointing at genphy_driver while d->driver is cleared, so does a later
phy_attach_direct() re-run phy_setup_ports() and phy_led_triggers_register()
on top of that state, overwriting and leaking the previous trigger array?

This path was split onto its own label by 6d9f66ac7fec, the commit named in
the Fixes: tag, so it predates this change and the changelog does not claim
to address it.  Would it be worth routing this label through the same cleanup
as phy_remove(), or noting in the changelog that the label is still an
incomplete unwind?

> error_put_device:

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