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

[Andrew Lunn]

On Sun, Sep 27, 2026 at 02:50:23AM +0300, Aleksei Sviridkin wrote:
> 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 a Keenetic KN-1012 stands,
> with an Airoha EN8811H behind an MT7531 port and its driver on the root
> filesystem: the devicetree interrupt of the PHY maps to irq 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. Under the generic driver a PHY_MAC_INTERRUPT written into
> phydev->irq alone is still replaced; bcmasp, genet and tsnep, the MACs
> that write it that way, write it again after each connect.
> 
> 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; before it, the generic driver is bound and the driver core turns
> such a probe away with -EBUSY. That is ordering, not exclusion: nothing
> on this side holds the device lock. Nor does the ordering hold after a
> sysfs unbind of the generic driver under a consumer, which leaves
> is_genphy_driven set with no driver bound, so the store can race or
> follow the probe of a driver that binds meanwhile. The next
> phy_attach_direct() applies the substitution again for whichever driver
> it finds bound, so the number its callers request still matches that
> driver.
> 
> 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@xxxxxx>
> ---
> 
> Notes:
>     Found and measured on a Keenetic KN-1012 (MT7981B, MT7531 switch) with 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_fwnode_phy_connect(), which DSA reaches through
>     phylink_of_phy_connect() for this port, 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.

This is still way too much text, which no human is going to read it.

I actually suggest you write this by hand. If you cannot do that, you
don't understand the code sufficiently to actually submit it with your
Signed-of-by.

    Andrew

---
pw-bot: cr