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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In this patch alone ("net: phy: take the interrupt back from the
  bus on detach"), phydev->irq is restored only in phy_detach().

--

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

When a PHY is first bound to the generic driver, phy_probe() replaces
phydev->irq with PHY_POLL, and the value is never put back. After that,
the PHY stays in polling mode even once the real driver binds. This patch
restores phydev->irq from mdiobus->irq[] in phy_detach() before the
genphy driver is released.

> 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: Low]
phy_attach_direct() has a second way to undo the genphy bind, and that
path doesn't restore phydev->irq.

This patch only adds the restore to phy_detach(). phy_attach_direct() can
also back out of the genphy bind:

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;
	}
    ...
error_module_put:
	module_put(d->driver->owner);
	phydev->is_genphy_driven = 0;
	d->driver = NULL;
    ...
}

phy_probe() sets phydev->irq = PHY_POLL before steps that can still fail.
Two examples are an MDIO read error in genphy_read_abilities() and
-ENOMEM in phy_setup_ports(). device_bind_driver() can also fail.

In those cases the error_module_put path clears is_genphy_driven and
d->driver, but phydev->irq keeps the PHY_POLL value. If the PHY device
survives and a real driver binds later, the PHY would still be polled.

The next patch in the series, "net: phy: restore the interrupt when the
generic bind cycle fails", closes this gap. It saves phydev->irq in a local
at the start of phy_attach_direct() and writes it back under
error_module_put. So the gap only exists between these two commits.

Would it be worth folding that change into this patch, so both genphy
unbind paths restore phydev->irq in the same commit?

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

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