Re: [PATCH net v7 2/2] net: phy: take the interrupt back from the bus on detach

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: phy: take the interrupt back from the bus on detach

This adds a single store to phy_detach() that re-reads phydev->irq from
mdiobus->irq[phydev->mdio.addr], so that a PHY which spent part of its life
bound to the generic driver does not keep PHY_POLL forever. The intent is to
undo the substitutions made by phy_probe() and phy_attach_direct() without
either of them having to save the previous value.

No Link: tags are present in the commit header.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 94b2e85e00a37..84e2da81dbd3d 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1969,6 +1969,11 @@ void phy_detach(struct phy_device *phydev)
>  		phydev->is_genphy_driven = 0;
>  	}
>  
> +	/* Whatever this attachment did to the interrupt, the bus that
> +	 * described it still knows the number. Take it back from there.
> +	 */

[Severity: Medium]
Is the premise in this comment true for all buses?  mdiobus_alloc_size()
fills the table with PHY_POLL:

drivers/net/phy/mdio_bus_provider.c:mdiobus_alloc_size() {
	...
	for (i = 0; i < PHY_MAX_ADDR; i++) {
		bus->irq[i] = PHY_POLL;
	...
}

Only the OF/fwnode path keeps the two in sync:

drivers/net/mdio/fwnode_mdio.c:fwnode_mdiobus_phy_device_register() {
	...
	if (rc > 0) {
		phy->irq = rc;
		mdio->irq[addr] = rc;
	} else {
		phy->irq = mdio->irq[addr];
	}
	...
}

For a bus that never populates the table, is this a restore or an
overwrite?  Documentation/networking/phy.rst tells MAC drivers to select
interrupt behaviour by assigning phydev->irq directly, and drivers do
exactly that:

drivers/net/usb/lan78xx.c:lan78xx_phy_init() {
	...
	if (dev->domain_data.phyirq > 0)
		phydev->irq = dev->domain_data.phyirq;
	...
}

drivers/net/usb/smsc95xx.c:smsc95xx_bind() {
	...
	pdata->phydev->irq = phy_irq;
	...
}

In those cases phy_detach() now stores PHY_POLL over the number the driver
supplied, so the polling-forever behaviour the commit targets is not fixed
there but the driver's own value is lost.  The reverse direction exists as
well, where a driver deliberately declines the interrupt only in
phydev->irq while its MDIO bus is OF-registered:

drivers/net/ethernet/xscale/ixp4xx_eth.c:ixp4xx_eth_probe() {
	...
	phydev->irq = PHY_POLL;
	...
}

drivers/net/ethernet/asix/ax88796c_main.c does the same.  Would a later
attach then re-impose an interrupt the driver refused?

Would saving and restoring the value that the substitution actually
overwrote, or restoring only at the sites where phylib itself wrote
PHY_POLL, avoid depending on a table that most buses leave at the default?

Checked candidates that do not appear to lose anything today: sxgbe never
assigns a real number (plat->mdio_bus_data has no ->irqs or
->probed_phy_irq set in sxgbe_platform.c, so both phy->irq and
mdio_bus->irq[] stay PHY_POLL), and emac-mac, bcmmii, bcmasp_intf and
tsnep re-establish their value on every attach.  lan78xx and smsc95xx
attach once per bind, so the loss there is latent rather than observable.

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

[Severity: Medium]
The commit message says:

    Detach is the end of every bind cycle, so this covers the two
    substitutions phy_attach_direct() makes as well as the one in
    phy_probe()

Is that accurate?  phy_attach_direct() binds the generic driver by hand:

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

d->driver->probe is phy_probe(), which writes PHY_POLL before its own
failure points (phydrv->get_features(), which is genphy_read_abilities()
for genphy_driver and propagates MDIO read errors), and
device_bind_driver() can return -ENOMEM from driver_sysfs_add().  The
label that failure lands on does not detach:

drivers/net/phy/phy_device.c:phy_attach_direct() {
	...
error_module_put:
	module_put(d->driver->owner);
	phydev->is_genphy_driven = 0;
	d->driver = NULL;
error_put_device:
	put_device(d);
	...
}

So this bind cycle ends with phydev->irq == PHY_POLL and no restore, while
the other failure label in the same function (error:) does call
phy_detach().  On a retried attach phy_interrupt_is_valid(phydev) is now
false, nothing re-derives the number, and the PHY polls permanently.
Should the restore also cover this path?

[Severity: Medium]
Can this store race a concurrent phy_probe()?  It runs after
device_release_driver(&phydev->mdio.dev) has already made the mdio device
bindable again, and the device lock is not held across the return:

drivers/base/dd.c:device_release_driver_internal() {
	...
	__device_driver_lock(dev, parent);

	if (!drv || drv == dev->driver)
		__device_release_driver(dev, parent);

	__device_driver_unlock(dev, parent);
	...
}

phy_detach() runs on the rtnl side and holds nothing against the driver
core, while phy_probe() is the other reader and writer of the same field
and is serialized only by device_lock():

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

If a real PHY driver is registered on another CPU inside that window
(phy_driver_register() -> driver_attach() -> really_probe() -> phy_probe()),
two outcomes look possible.  A driver without config_intr/handle_interrupt
demotes to PHY_POLL and this store immediately republishes a valid number,
leaving phy_interrupt_is_valid() true for a driver that cannot service it.
An interrupt-capable driver that binds in the window reads the stale
PHY_POLL and makes a permanent probe-time decision from it:

drivers/net/phy/realtek/realtek_main.c:rtl8211f_probe() {
	...
	if (device_property_read_bool(dev, "wakeup-source") &&
	    phy_interrupt_is_valid(phydev)) {
		device_set_wakeup_capable(dev, true);
		devm_pm_set_wake_irq(dev, phydev->irq);
	}
	...
}

after which the restore leaves an apparently valid runtime IRQ with no wake
IRQ configured.  Would placing the restore before device_release_driver(),
under the same device lock that serializes the next probe, order these?

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

[Severity: Low]
Should this carry a Fixes: tag and a stable annotation?  The defect chain is
reproducible in the tree: phy_device_create() seeds dev->irq =
bus->irq[addr], phy_probe() then demotes it with

	if (!phy_drv_supports_irq(phydrv) && phy_interrupt_is_valid(phydev))
		phydev->irq = PHY_POLL;

and genphy_driver has no config_intr or handle_interrupt, so the demotion
always happens for a PHY with a real IRQ.  Nothing re-derives the number
afterwards, since phy_attach_direct() only ever demotes.  The commit that
introduced the phy_probe() substitution looks like the natural Fixes:
target; without it the backport range is undefined.
-- 
pw-bot: cr