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

Critical: 0 · High: 1 · Medium: 0 (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] When a genphy-driven PHY is attached a second time, the
  error_module_put unwind clears the state of the live first attachment.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the
  error_module_put unwind in phy_attach_direct() does not undo a…

--

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

This commit saves phydev->irq on entry to phy_attach_direct() and writes it
back on the error_module_put path. The goal is that a failed genphy
probe/bind no longer leaves the PHY in PHY_POLL.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index a9c71a2861186..8bfb154402adc 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1755,6 +1755,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  	struct mii_bus *bus = phydev->mdio.bus;
>  	struct device *d = &phydev->mdio.dev;
>  	struct module *ndev_owner = NULL;
> +	int irq = phydev->irq;
>  	int err;
>  
>  	/* For Ethernet device drivers that register their own MDIO bus, we

[ ... ]

> @@ -1896,6 +1897,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  
>  error_module_put:
>  	module_put(d->driver->owner);
> +	phydev->irq = irq;
>  	phydev->is_genphy_driven = 0;
>  	d->driver = NULL;

[Severity: High]
The commit message says irq is saved because "the same label is reached
when a second attach of an already attached PHY fails, and there the field
is live". Writing irq back on that path does no harm. What about the other
two writes under this label?

Take a second phy_attach_direct() on a genphy-driven PHY that is still
attached. d->driver is non-NULL, but is_genphy_driven is still 1 from the
first attach. So this block runs again, before the attached_dev check:

    if (phydev->is_genphy_driven) {
        err = d->driver->probe(d);
        if (err >= 0)
            err = device_bind_driver(d);

        if (err)
            goto error_module_put;
    }

phy_probe() runs again on the live PHY. device_bind_driver() then fails
with -EEXIST in driver_sysfs_add(), because the sysfs links from the first
bind are still there.

That sends control to error_module_put. It clears is_genphy_driven and
d->driver on a PHY that is still bound in the driver core and still
attached to the first net_device.

Later the first owner calls phy_detach(), and both of these branches are
skipped:

    if (phydev->mdio.dev.driver)
        module_put(phydev->mdio.dev.driver->owner);
    ...
    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;
    }

As a result:

  - The first attach's driver module reference leaks.
  - device_release_driver() and phy_remove() never run.
  - irq is never restored from the bus table.

Would every later attach then fail with -EEXIST, in device_bind_driver()
for genphy or in really_probe() for a real PHY driver? phydev->irq would
stay at PHY_POLL for good, and the phy_device would leak through its klist
reference.

That is the end state this patch tries to prevent, reached through the
path the commit message names.

The clearing itself is older than this patch. It comes from 6d9f66ac7fec
and from the persistent is_genphy_driven flag added in 2796ff1e3dcae7.
Since this patch depends on that path, could genphy be probed, bound and
unwound only when this call assigned d->driver (for example with a local
flag)? Another option would be to check attached_dev before the genphy
probe/bind.

[Severity: Medium]
This is a pre-existing issue, not one this patch introduces. When
phy_probe() succeeds and device_bind_driver() then fails, does
error_module_put leak what phy_probe() set up?

A successful phy_probe() leaves behind:

  - phydev->drv set
  - the reset line released by phy_device_reset(phydev, 0)
  - ports allocated by phy_setup_ports()
  - LED triggers allocated and registered by phy_led_triggers_register()

device_bind_driver() does not call the driver's remove callback when it
fails:

    ret = driver_sysfs_add(dev);
    if (!ret) {
        device_links_force_bind(dev);
        driver_bound(dev);
    }
    else
        bus_notify(dev, BUS_NOTIFY_DRIVER_NOT_BOUND);

error_module_put only restores irq, is_genphy_driven and d->driver. The
teardown that phy_remove() would do never happens:

  - phy_led_triggers_unregister() is not called
  - phy_cleanup_ports() is not called
  - phy_device_reset(phydev, 1) is not called
  - phydev->drv is not cleared

On a retried attach, phy_led_triggers_register() overwrites
phydev->led_link_trigger and phydev->phy_led_triggers. The duplicate
registrations fail with -EEXIST, so the original triggers stay registered
and leak. The PHY is also left out of reset with no driver bound.

This path is reached when driver_sysfs_add() fails on a first attach. It
is also reached on every attach after the second-attach case above.

>  error_put_device:

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