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

[PATCH net v10 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 too, though after registering it and after
phy_find_first(), the way stmmac does.

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 in
    ucc_geth_open() feeding device_set_wakeup_capable(), 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 for its internal
    PHY and genet for an internal PHY that is not on v5 - 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.

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 v9 [8]:

 - Patch 3 restores under is_genphy_driven rather than above the block.
   Unconditionally it also wrote a field this series has no claim on: a
   MAC installing PHY_MAC_INTERRUPT writes phydev->irq alone, and a
   phy_request_interrupt() that failed left PHY_POLL there while the bus
   table still held the number that could not be requested.
 - The v9 cover named a behaviour change, a PHY_POLL fallback from a
   failed phy_request_interrupt() no longer surviving a detach. The
   scoping takes it back, and that paragraph with it:
   phy_request_interrupt() runs behind phy_interrupt_is_valid(), which
   is false for as long as the generic driver is bound, so the fallback
   never meets the restore.
 - The -EBUSY sentence in patches 3 and 4 claimed more than the code
   gives. The prober's half holds - __driver_probe_device() returns
   -EBUSY while dev->driver is set - but neither the hand-bind in
   phy_attach_direct() nor the store in phy_detach() holds the device
   lock that device_bind_driver() asks its callers for, so what is there
   is statement ordering. Both bodies say that now. Taking the lock is a
   separate series.
 - Patches 1 and 2 keep their code. Both changelogs now give the real
   reason for filling the whole bus table, which is that a loop is
   smaller than a second branch on the chip id; phy_mask already pins the
   address to one entry everywhere except lan78xx's 7801 and smsc95xx's
   external PHY.
 - Patch 2 also puts the declarations in smsc95xx_bind() back in longest
   to shortest order: the i it adds had made the int line longer than
   the char line above it.
 - The question whether lan78xx's fill of the bus table overrides a
   per-PHY interrupt from the devicetree was against the v8 shape, which
   wrote the entry in lan78xx_phy_init() after the bus was registered.
   Since v9 it is written in lan78xx_mdio_init() ahead of
   of_mdiobus_register(), and
   fwnode_mdiobus_phy_device_register() then overwrites it from the PHY
   node.

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 puts it where the generic driver is still bound
   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/
[8] https://lore.kernel.org/r/20260919015326.499479-1-f@lex.la/

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


base-commit: 3b95a04eb5f95bf6a016a1bb9ff37d3eee48de63
-- 
2.53.0

[PATCH net v10 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. Fill the whole table rather than one
entry: for 7801 the address is not known until the scan, and for the
other two phy_mask leaves only address 1 readable, so a loop costs less
than a second switch on the chip id. 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.
    The fill covers the whole table because only the 7801 case leaves the
    address open until of_mdiobus_register() has scanned; 7800 and 7850 set
    phy_mask a few lines above, so every entry but address 1 is unreachable
    and writing them costs nothing.
    
    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 v10 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. The whole
table gets it: with an external PHY the address is not known until the
scan, and with the internal one phy_mask has already left a single
reachable entry, so a loop costs less than a branch on which case this
is.

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 before mdiobus_alloc(), so the number is in hand
    where the table is filled, and mdiobus_alloc_size() is the only thing in the
    tree that writes PHY_POLL into that table - a fill placed after the alloc
    and before mdiobus_register() is not undone by the scan. The fill covers the
    whole table because the external-PHY case leaves the address to
    phy_find_first() afterwards; on the internal path phy_mask has already
    reduced it to one entry.
    
    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..b629092b94c2 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1147,8 +1147,8 @@ static void smsc95xx_handle_link_change(struct net_device *net)
 static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf)
 {
 	struct smsc95xx_priv *pdata;
+	int ret, phy_irq, i;
 	char usb_path[64];
-	int ret, phy_irq;
 	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 v10 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. Only under is_genphy_driven,
since that is the substitution being undone: elsewhere the field belongs
to whoever wrote it, a MAC installing PHY_MAC_INTERRUPT writes
phydev->irq alone, and a phy_request_interrupt() that failed leaves
PHY_POLL there while the bus table still holds the number that could not
be requested.

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. Until it runs the generic driver is still bound and the driver
core turns a probe away with -EBUSY. That is ordering, not exclusion:
nothing on this side holds the device lock.

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.
    
    The reading predates the is_genphy_driven guard the store now sits under,
    which should not be implied away. On the measured path the flag is set:
    phylink_connect_phy() calls phy_detach() from its own failure check, and
    the three writes of that flag are all in phy_device.c, none of them between
    the hand-bind and that call. So the guard passes and the store is the one
    the reading came from. Its other side, a detach with a real driver bound,
    was not measured.

 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 94b2e85e00a3..a9c71a286118 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1965,6 +1965,8 @@ void phy_detach(struct phy_device *phydev)
 	 * real driver could be loaded
 	 */
 	if (phydev->is_genphy_driven) {
+		/* The release below lets phy_probe() write this field. */
+		phydev->irq = phydev->mdio.bus->irq[phydev->mdio.addr];
 		device_release_driver(&phydev->mdio.dev);
 		phydev->is_genphy_driven = 0;
 	}
-- 
2.53.0

[PATCH net v10 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,
the core turns a probe away with -EBUSY. That is ordering, not
exclusion - this bind and its unwind run without the device lock
device_bind_driver() asks its callers to hold.

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 either of its two
    sysfs_create_link() calls or from the coredump attribute.
    
    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 a9c71a286118..06161a73fe3f 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);
+	/* The NULL below lets phy_probe() write 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 v10 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 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 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 restore added to phy_detach()
  (drivers/net/phy/phy_device.c:1967-1971) is keyed on…
- [Low] Concern as filed: the new store in phy_detach()
  (phy_device.c:1967-1971) is gated only by phydev->is_genphy_driven…

--

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

phy_probe() overwrites phydev->irq with PHY_POLL when the bound driver has
no interrupt support, and nothing restored it, so a PHY that first gets the
generic driver keeps polling after its real driver binds. The patch restores
phydev->irq from mdiobus->irq[] in phy_detach(), before
device_release_driver(), gated on phydev->is_genphy_driven.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 94b2e85e00a37..a9c71a2861186 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1965,6 +1965,8 @@ void phy_detach(struct phy_device *phydev)
>  	 * real driver could be loaded
>  	 */
>  	if (phydev->is_genphy_driven) {
> +		/* The release below lets phy_probe() write this field. */
> +		phydev->irq = phydev->mdio.bus->irq[phydev->mdio.addr];

[Severity: Medium]
Is the restore complete if it is placed here rather than next to the write
it undoes?

The store being undone is in phy_probe() and does not look at
is_genphy_driven at all:

drivers/net/phy/phy_device.c:phy_probe() {
	/* 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))
		phydev->irq = PHY_POLL;
	...
}

So it runs for whatever driver the driver core binds, including a real PHY
driver without interrupt support. The matching unbind goes
device_release_driver() -> phy_remove(), and phy_remove() contains no
phydev->irq write:

drivers/net/phy/phy_device.c:phy_remove() {
	...
	if (phydev->drv && phydev->drv->remove)
		phydev->drv->remove(phydev);

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

	phydev->drv = NULL;

	return 0;
}

Two paths then still end with PHY_POLL and no restore site:

- genphy_driver is registered without suppress_bind_attrs, so a bind through
  /sys/bus/mdio_bus/drivers/Generic PHY/bind leaves is_genphy_driven at 0,
  and the unbind reaches phy_remove() only;
- rmmod of an interrupt-less PHY driver, after which a later driver that does
  support interrupts inherits PHY_POLL while mdiobus->irq[addr] still holds
  the number.

Would saving the pre-substitution value in phy_probe() and restoring it in
phy_remove() cover those as well as the path this patch covers?

For the record, the sibling gap on the error_module_put label in
phy_attach_direct() is closed by the later patch in this series, "net: phy:
restore the interrupt when the generic bind cycle fails", which adds the same
store there. The phy_remove() and sysfs-unbind paths above are still
uncovered at the end of the series.

[Severity: Low]
Can is_genphy_driven be stale at this point?

There are only three writes of that flag, one set in phy_attach_direct() and
two clears, one on the error_module_put label and the one added just below
this store. phy_remove() does not clear it. After a sysfs unbind of "Generic
PHY" on an attached phydev the flag stays 1 while phydev->mdio.dev.driver is
NULL, so a subsequent bind of a real driver followed by phy_detach() takes
this branch and writes mdiobus->irq[addr] over the PHY_POLL that phy_probe()
wrote on purpose for a driver without interrupt support.

The effect looks limited: both phy_request_interrupt() call sites,
phy_connect_direct() and phylink_bringup_phy(), run right after
phy_attach_direct(), which re-applies the same substitution against the
driver bound at that moment:

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

so the wrong value only lives in the detached window. Would it still be
worth clearing is_genphy_driven from phy_remove() so the flag tracks the
binding it names?

The commit message says of the placement before device_release_driver():

    That is ordering, not exclusion: nothing on this side holds the device
    lock.

With a stale flag, is phy_probe() on another CPU also a writer of this field
before device_release_driver() is reached, rather than only after it?

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

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

- [High] The new unconditional store `phydev->irq =
  bus->irq[phydev->mdio.addr]` at drivers/net/phy/phy_device.c:1900 is…
- [Medium] The correctness argument the commit message gives for the new
  store ("Take the number back on that label as well, before it clears…

Pre-existing issues:
- [High] The `error_module_put` unwind in `phy_attach_direct()`
  (drivers/net/phy/phy_device.c:1897-1902), which this patch extends…

--

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

The hand-rolled generic driver bind in phy_attach_direct() unwinds on the
error_module_put label, which never runs phy_detach(), so the PHY_POLL
value that phy_probe() writes into phydev->irq used to survive the failed
bind.  This patch adds a store of bus->irq[phydev->mdio.addr] back into
phydev->irq on that label, before d->driver is cleared.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index a9c71a2861186..06161a73fe3f9 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);
> +	/* The NULL below lets phy_probe() write this field. */
> +	phydev->irq = bus->irq[phydev->mdio.addr];

[Severity: High]
Can this store land on a PHY that is still attached to another netdev?

phydev->is_genphy_driven is set when the hand-bind happens and is cleared
only by phy_detach() or by this label, so a PHY that attached with the
generic driver keeps is_genphy_driven == 1 and d->driver ==
&genphy_driver.mdiodrv.driver for its whole attached lifetime.  On a second
phy_attach_direct() for that same phydev, the "if (!d->driver)" assignment
is skipped, but the bind block still runs, and it runs before the guard
that is supposed to reject this case:

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

		if (err)
			goto error_module_put;
	}

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

device_bind_driver() -> driver_sysfs_add() -> sysfs_create_link() returns
-EEXIST for an already bound device, so error_module_put is reached with no
hardware error involved, and the new store then writes bus->irq[addr] into
the live attachment's phydev->irq.

For a DT PHY with an interrupt, that value is a real IRQ number rather than
PHY_POLL:

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

Does the still-working netdev then stop noticing link changes?  No handler
was ever requested, because the first attach saw PHY_POLL, and after the
clobber the state machine no longer re-queues itself:

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

And on teardown, does phy_disconnect() call free_irq() for an interrupt
that was never requested?

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

Separately, is reconstructing the value from bus->irq[] the same thing as
restoring what phydev->irq held on entry?  Some MACs install phydev->irq
themselves without ever touching bus->irq[]:

drivers/net/ethernet/broadcom/genet/bcmmii.c:bcmgenet_mii_probe() {
	...
	if (priv->internal_phy && !GENET_IS_V5(priv))
		dev->phydev->irq = PHY_MAC_INTERRUPT;
	...
}

Would saving the entry value in a local and writing it back on the label be
more faithful than recomputing it from the bus array?

[Severity: Medium]
The commit message says:

  "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,
   the core turns a probe away with -EBUSY. That is ordering, not
   exclusion - this bind and its unwind run without the device lock
   device_bind_driver() asks its callers to hold."

Is that ordering actually enforced anywhere?  phydev->irq and d->driver are
non-aliasing plain stores, so the compiler may sink the irq store past the
d->driver = NULL, and on a weakly ordered CPU another core may observe
d->driver == NULL before the irq store becomes visible.

There is also no lock in common with the driver core, which reads and
writes dev->driver only under the device lock:

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

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

So can the restore land after a competing bind has already completed?

CPU0 phy_attach_direct()
   device_bind_driver() fails
   d->driver = NULL becomes visible

CPU1 driver_attach() -> really_probe() -> mdio_probe() -> phy_probe()
   takes device_lock(), sees dev->driver == NULL, binds
   drivers/net/phy/phy_device.c:phy_probe() {
	if (!phy_drv_supports_irq(phydrv) && phy_interrupt_is_valid(phydev))
		phydev->irq = PHY_POLL;
   }

CPU0
   phydev->irq = bus->irq[phydev->mdio.addr];   /* pending store lands */

leaving the PHY marked interrupt-driven for a driver that cannot service
interrupts, which is the inverse of what the patch intends?  And in the
mirror interleaving, where CPU1 reads phydev->irq before CPU0's store lands,
is the restore simply lost?  There are drivers keying off that value during
config:

drivers/net/phy/smsc.c:smsc_phy_config_init() {
	...
	if (!priv->edpd_mode_set_by_user && phydev->irq != PHY_POLL)
		priv->edpd_enable = false;
	...
}

The detach side does not have this problem, since the equivalent restore is
immediately followed by device_release_driver(), which takes the device lock
internally:

drivers/net/phy/phy_device.c:phy_detach() {
	if (phydev->is_genphy_driven) {
		/* The release below lets phy_probe() write this field. */
		phydev->irq = phydev->mdio.bus->irq[phydev->mdio.addr];
		device_release_driver(&phydev->mdio.dev);
		phydev->is_genphy_driven = 0;
	}
}

Would holding device_lock(d) across the hand-bind and its unwind, or using
device_driver_attach(), be preferable to relying on source order here?

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

[Severity: High]
This isn't a bug introduced by this patch, but since the patch extends this
label with another per-field restore, is a per-field restore enough here?

The label is also reached when phy_probe() succeeded and only
device_bind_driver() 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;
	}

In that case neither phy_probe()'s own out: unwind nor phy_remove() runs,
so does everything phy_probe() set up stay behind?  phy_probe() registers
LED triggers and sets up ports:

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

drivers/net/phy/phy_led_triggers.c:phy_led_triggers_register() {
	...
	phy->led_link_trigger = kzalloc_obj(*phy->led_link_trigger);
	...
	phy->phy_led_triggers = kzalloc_objs(struct phy_led_trigger,
					     phy->phy_num_led_triggers);
	...
}

and only phy_remove() undoes it:

drivers/net/phy/phy_device.c:phy_remove() {
	phy_led_triggers_unregister(phydev);
	...
	phy_cleanup_ports(phydev);
	...
	phy_device_reset(phydev, 1);

	phydev->drv = NULL;
}

Is the leak permanent?  Destroying the phydev does not unregister the
triggers or clean the ports:

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

Is the missing re-assert of the reset also not self-correcting, given that
the level is cached?

drivers/net/phy/mdio_device.c:mdio_device_reset() {
	...
	if (mdiodev->reset_state == value)
		return;
	...
}

A later bind's de-assert would then be a no-op, so the PHY never gets the
reset pulse the bind sequence provides.

And does phydev->drv stay pointing at genphy_driver while d->driver is set
to NULL on this label, leaving phylib and the driver core disagreeing about
whether a driver is bound?  phy_attach_direct() returns err straight to
phy_connect_direct()/phy_attach(), which propagate it without calling
phy_detach().

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