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

Critical: 0 · High: 0 · Medium: 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 restore added to phy_detach()
  (drivers/net/phy/phy_device.c:1967-1971) is keyed on…
- [Low] Concern as filed: the new store in phy_detach()
  (phy_device.c:1967-1971) is gated only by phydev->is_genphy_driven…

--

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

phy_probe() overwrites phydev->irq with PHY_POLL when the bound driver has
no interrupt support, and nothing restored it, so a PHY that first gets the
generic driver keeps polling after its real driver binds. The patch restores
phydev->irq from mdiobus->irq[] in phy_detach(), before
device_release_driver(), gated on phydev->is_genphy_driven.

> 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: Medium]
Is the restore complete if it is placed here rather than next to the write
it undoes?

The store being undone is in phy_probe() and does not look at
is_genphy_driven at all:

drivers/net/phy/phy_device.c:phy_probe() {
	/* Disable the interrupt if the PHY doesn't support it
	 * but the interrupt is still a valid one
	 */
	if (!phy_drv_supports_irq(phydrv) && phy_interrupt_is_valid(phydev))
		phydev->irq = PHY_POLL;
	...
}

So it runs for whatever driver the driver core binds, including a real PHY
driver without interrupt support. The matching unbind goes
device_release_driver() -> phy_remove(), and phy_remove() contains no
phydev->irq write:

drivers/net/phy/phy_device.c:phy_remove() {
	...
	if (phydev->drv && phydev->drv->remove)
		phydev->drv->remove(phydev);

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

	phydev->drv = NULL;

	return 0;
}

Two paths then still end with PHY_POLL and no restore site:

- genphy_driver is registered without suppress_bind_attrs, so a bind through
  /sys/bus/mdio_bus/drivers/Generic PHY/bind leaves is_genphy_driven at 0,
  and the unbind reaches phy_remove() only;
- rmmod of an interrupt-less PHY driver, after which a later driver that does
  support interrupts inherits PHY_POLL while mdiobus->irq[addr] still holds
  the number.

Would saving the pre-substitution value in phy_probe() and restoring it in
phy_remove() cover those as well as the path this patch covers?

For the record, the sibling gap on the error_module_put label in
phy_attach_direct() is closed by the later patch in this series, "net: phy:
restore the interrupt when the generic bind cycle fails", which adds the same
store there. The phy_remove() and sysfs-unbind paths above are still
uncovered at the end of the series.

[Severity: Low]
Can is_genphy_driven be stale at this point?

There are only three writes of that flag, one set in phy_attach_direct() and
two clears, one on the error_module_put label and the one added just below
this store. phy_remove() does not clear it. After a sysfs unbind of "Generic
PHY" on an attached phydev the flag stays 1 while phydev->mdio.dev.driver is
NULL, so a subsequent bind of a real driver followed by phy_detach() takes
this branch and writes mdiobus->irq[addr] over the PHY_POLL that phy_probe()
wrote on purpose for a driver without interrupt support.

The effect looks limited: both phy_request_interrupt() call sites,
phy_connect_direct() and phylink_bringup_phy(), run right after
phy_attach_direct(), which re-applies the same substitution against the
driver bound at that moment:

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

so the wrong value only lives in the detached window. Would it still be
worth clearing is_genphy_driven from phy_remove() so the flag tracks the
binding it names?

The commit message says of the placement before device_release_driver():

    That is ordering, not exclusion: nothing on this side holds the device
    lock.

With a stale flag, is phy_probe() on another CPU also a writer of this field
before device_release_driver() is reached, rather than only after it?

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