Re: [PATCH net v6 2/2] net: phy: restore the interrupt the bus gave a PHY

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 2 · Low: 1

- [Medium] phy_restore_probe_irq() bails out whenever
  phydev->phy_link_change is non-NULL and records no pending…
- [Medium] phy_restore_probe_irq() performs an unsynchronized check-then-
  act on state owned by the rtnl-side attach/detach paths.
- [Low] No Fixes: tag (and no target-tree indication) on a patch whose
  changelog describes a concrete, lasting user-visible defect.

--

net: phy: restore the interrupt the bus gave a PHY

phy_probe() demotes phydev->irq to PHY_POLL when the binding driver has no
interrupt support, and nothing restores it afterwards.  This patch adds a
helper that copies the number back out of mdiobus->irq[], and calls it from
phy_remove(), phy_probe()'s error exit and the phy_attach_direct() unwind.
The helper skips the restore while phydev->phy_link_change marks an attached
consumer.

>     A bus whose driver writes only phydev->irq and never the table is not
>     covered, because the table then holds PHY_POLL and there is nothing to
>     give back; lan78xx, smsc95xx and sxgbe are in that position today and
>     registering the interrupt with the bus is theirs to do.
>
>     This needs commit e0d1c55501d3 ("net: phy: fix
>     phy_uses_state_machine()") to be of any use: without it the mark this
>     skips on is never cleared once a consumer has attached, and the restore
>     never runs.
>
>     Assisted-by: LLM
>     Signed-off-by: Aleksei Sviridkin <f@lex.la>

[Severity: Low]
The changelog describes a lasting user-visible defect ("polls for the rest
of the uptime once the real driver takes over"), but the tag block carries
no Fixes: line and the subject has no net/net-next scoping.

The assignment being undone, in phy_probe():

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

is the natural blame target.  Could a Fixes: tag pointing at it (or an
explicit statement that this targets net-next as a behavioural change) be
added?

This matters more than usual given the stated dependency on e0d1c55501d3,
which is what clears phydev->phy_link_change in phy_detach().  On a tree
without that commit the new restore in phy_remove() is a silent no-op, so a
backport applies cleanly and changes nothing.  Should that boundary be
spelled out for stable?

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 94b2e85e00a3..b55402569300 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1734,6 +1734,19 @@ static bool phy_drv_supports_irq(const struct phy_driver *phydrv)
>  	return phydrv->config_intr && phydrv->handle_interrupt;
>  }
>
> +/* Give back what phy_probe() took, from the bus that owns the number, but
> + * not while phy_link_change marks a consumer: it skipped
> + * phy_request_interrupt() on the value it saw, so phy_disconnect() would
> + * free an interrupt nobody requested.
> + */
> +static void phy_restore_probe_irq(struct phy_device *phydev)
> +{
> +	if (phydev->phy_link_change || phydev->irq != PHY_POLL)
> +		return;
> +
> +	phydev->irq = phydev->mdio.bus->irq[phydev->mdio.addr];
> +}

[Severity: Medium]
Can this check-then-act run concurrently with the attach/detach paths that
own phydev->phy_link_change and phydev->irq?

The helper reads phy_link_change and irq, then writes irq, running from
phy_probe()/phy_remove() under the mdio device lock only.  The writers of
that same state take neither the mdio device lock nor phydev->lock:

phy_attach_direct()
	phydev->phy_link_change = phy_link_change;
	...
	if (phydev->dev_flags & PHY_F_NO_IRQ)
		phydev->irq = PHY_POLL;

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

phy_detach()
	phydev->phy_link_change = NULL;

phy_driver_register() leaves suppress_bind_attrs unset, so a root sysfs
unbind can enter phy_remove() at any time, and __device_release_driver()
clears dev->driver only after ->remove() returns, so a concurrent
phy_attach_direct() still sees d->driver != NULL and proceeds.

One interleaving:

CPU0 phy_remove() -> phy_restore_probe_irq()
	reads phy_link_change == NULL, irq == PHY_POLL
	(preempted before the store)

CPU1 phy_attach_direct()
	phydev->phy_link_change = phy_link_change;
	phydev->irq = PHY_POLL;		/* no phy_request_interrupt() */

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

Does that leave a valid IRQ published for a consumer that requested
nothing, so that teardown hits:

phy_disconnect()
	if (phy_interrupt_is_valid(phydev))
		phy_free_interrupt(phydev);

and free_irq(phydev->irq, phydev) warns about freeing an already-free IRQ?
phy_polling_mode() is also false at that point, so link changes would be
neither polled nor interrupt driven, which is the situation the new comment
says it avoids.

The reverse order gives the opposite outcome: phy_remove() observes a still
non-NULL phy_link_change just before phy_detach() clears it, and the PHY is
left unbound with irq == PHY_POLL.

Would making the individual accesses atomic help here, or does the decision
plus the store need to be serialized against the whole attach/detach and
interrupt request/free sequence, for example by driving the restore from
phy_detach() under rtnl?

> @@ -3848,6 +3864,8 @@ static int phy_remove(struct device *dev)
>  	if (phydev->drv && phydev->drv->remove)
>  		phydev->drv->remove(phydev);
>
> +	phy_restore_probe_irq(phydev);
> +

[Severity: Medium]
When phy_restore_probe_irq() takes the phy_link_change early return here, is
the interrupt number ever restored later?

The changelog says restoring at the three bind-cycle exits means "a bind
that is undone by any path leaves the PHY as it was found", but nothing
records a pending restore for the skipped case.

A driver unbind while a consumer is still attached:

unbind_store()
	if (dev && dev->driver == drv) {
		device_driver_detach(dev);

reaches phy_remove() with phy_link_change still set by phy_attach_direct(),
so the helper returns early and phy_remove() then clears phydev->drv.

When the consumer disconnects afterwards, phy_detach() clears the marker but
its release is gated:

	phydev->phy_link_change = NULL;
	...
	if (phydev->is_genphy_driven) {
		device_release_driver(&phydev->mdio.dev);
		phydev->is_genphy_driven = 0;
	}

and for an already-unbound device device_release_driver() is a no-op, so
phy_remove() does not run again and no restore happens.  A later
interrupt-capable driver then sees phydev->irq == PHY_POLL in phy_probe(),
phy_interrupt_is_valid() is false, and polling continues.

The early return itself looks necessary, since restoring under an attached
consumer is what would make phy_disconnect() free an unrequested IRQ.  Could
the skipped restore instead be completed in phy_detach() right after
phy_link_change is cleared?

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