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

[Andrew Lunn]

On Sun, Sep 27, 2026 at 02:50:24AM +0300, Aleksei Sviridkin wrote:
> 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 point the hand-bind 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 on a Keenetic KN-1012 while placing
> the restore of the previous patch, as the other exit of the same bind
> cycle.
> 
> Save phydev->irq on entry and put it back on that label. The unwind runs
> inside the call that made the substitution, so the value from before it
> is known exactly. The bus table the previous patch reads from would be
> wrong here twice over: it does not hold a PHY_MAC_INTERRUPT that a MAC
> wrote into phydev->irq alone, and the label is also reached when a
> second attach of an attached PHY fails its bind, where the field is
> live.
> 
> The store is not ordered against a concurrent bind: this unwind, like
> the hand-bind it undoes, runs without the device lock that
> 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@xxxxxx>
> ---
> 
> 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.
>     Measured again for this version, since the value now comes from the
>     local: two images of the distribution's 6.18.52 kernel differing only
>     by this patch, injected failure at 2.0 s, real driver bound at 6.4 s.
>     phydev->irq afterwards reads -1 with patch 3 alone and 15 with this
>     one; the three switch ports read 79, 80 and 81 in both.
>     
>     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.

Again, way too much text.

	Andrew