Re: [PATCH net v8 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: 0 · Medium: 1 (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 exclusion the commit message relies on does not exist as a
  lock.
- [Low] The new comment added at drivers/net/phy/phy_device.c:1899
  ("Before the NULL below, which lets another probe reach this field.")…

Pre-existing issues:
- [Medium] The error_module_put label that this patch extends
  (drivers/net/phy/phy_device.c:1897-1903) unwinds only the module…

--

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

This adds a restore of phydev->irq from bus->irq[phydev->mdio.addr] on the
error_module_put label in phy_attach_direct(), so that the PHY_POLL value
installed by phy_probe() does not survive a hand-bind of the generic driver
that never completed. The restore is placed ahead of the d->driver = NULL
store, and the commit message argues that this ordering keeps phy_probe()
from being a second writer of phydev->irq.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 8e6b399f95d6..69d8911ea7f6 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);
> +	/* Before the NULL below, which lets another probe reach this field. */

[Severity: Low]
Does the code really establish the ordering this comment describes?  The two
stores are plain and touch distinct fields, with no data dependency and no
barrier between them:

drivers/net/phy/phy_device.c:phy_attach_direct() {
	...
	phydev->irq = bus->irq[phydev->mdio.addr];
	phydev->is_genphy_driven = 0;
	d->driver = NULL;
	...
}

Can the compiler, or a weakly ordered CPU, publish the NULL driver pointer
before the IRQ restore becomes visible?  The admission check the commit
message refers to reads the field with no acquire pairing:

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

so a PHY driver registering on another CPU could pass that check and reach
phy_probe(), which writes the same field:

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

and then the delayed store from the unwind path overwrites PHY_POLL with the
bus interrupt number.  The attach path re-forces PHY_POLL on the next attach
in phy_attach_direct(), so the observable effect looks limited to the stated
guarantee not matching what the stores do.  Would either a real ordering
primitive, or dropping the claim from the comment and the changelog, be more
accurate here?  The same wording was added to the phy_detach() restore by the
preceding commit in the series ("net: phy: take the interrupt back from the
bus on detach"), so both would need the same treatment.

> +	phydev->irq = bus->irq[phydev->mdio.addr];

[Severity: Medium]
Is the exclusion the changelog relies on backed by a lock anywhere?  The
changelog says:

    "That store is what reopens the device to the driver core: until it runs,
    a driver registering on another CPU is turned away with -EBUSY and
    phy_probe() cannot be the second writer of this field."

phy_attach_direct() installs the generic driver, calls its probe and calls
device_bind_driver(d), and later clears d->driver on this label, without ever
taking device_lock(&phydev->mdio.dev).  The driver core documents the
opposite requirement:

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

and every driver-core access to dev->driver happens under that mutex, taken
by __device_driver_lock() on the driver_register() -> driver_attach() ->
__driver_attach() -> device_driver_attach() -> driver_probe_device() path.
With an empty lockset intersection, does the -EBUSY check actually serialize
the two paths?

There also seems to be a wider window than the one this patch reasons about:
if a concurrent __driver_probe_device() observes dev->driver == NULL just
before phy_attach_direct() stores the genphy pointer into it, does anything
prevent really_probe()/device_set_driver() from running while phylib is in
device_bind_driver() -> driver_sysfs_add()/driver_bound() for the same mdio
device?  Would doing the generic bind and unbind under device_lock(d), or via
device_driver_attach()/device_release_driver(), remove the need for the
ordering-by-convention argument altogether?

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

[Severity: Medium]
This isn't a bug introduced by this patch, but while this label is being
extended: does it leak what phy_probe() allocated when the bind fails at the
second point?

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

On that path phy_probe() already returned 0, so it has de-asserted the PHY
reset, created the port objects via phy_setup_ports() and registered the LED
triggers via phy_led_triggers_register().  device_bind_driver() returns the
driver_sysfs_add() error without calling the driver's remove callback:

drivers/base/dd.c:device_bind_driver() {
	ret = driver_sysfs_add(dev);
	if (!ret) {
		device_links_force_bind(dev);
		driver_bound(dev);
	}
	else
		bus_notify(dev, BUS_NOTIFY_DRIVER_NOT_BOUND);
	return ret;
}

Since driver_bound() never ran, the driver core will not call phy_remove(),
which is the only place that undoes those steps:

drivers/net/phy/phy_device.c:phy_remove() {
	phy_cleanup_ports(phydev);

	if (phydev->drv && phydev->drv->remove)
		phydev->drv->remove(phydev);

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

	phydev->drv = NULL;
	...
}

and the release function is a plain kfree():

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

Does that leak the port objects and the phy_led_triggers/led_link_trigger
allocations, leave the triggers registered with the LED core for a PHY with
no driver bound, and leave the reset de-asserted?  phydev->drv also stays
pointing at genphy_driver while d->driver is cleared, so does a later
phy_attach_direct() re-run phy_setup_ports() and phy_led_triggers_register()
on top of that state, overwriting and leaking the previous trigger array?

This path was split onto its own label by 6d9f66ac7fec, the commit named in
the Fixes: tag, so it predates this change and the changelog does not claim
to address it.  Would it be worth routing this label through the same cleanup
as phy_remove(), or noting in the changelog that the label is still an
incomplete unwind?

> error_put_device:

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