Thread (4 messages) flat view 4 messages, 2 authors, 1h ago
HOTtoday

Revision v5 of 4 in this series.

Revisions (4)
  1. v2 [diff vs current]
  2. v3 [diff vs current]
  3. v4 [diff vs current]
  4. v5 current

[PATCH net v5 0/2] net: fix a stale phylink PHY pointer and a lost PHY interrupt

From: Aleksei Sviridkin <hidden>
Date: 2026-09-06 17:46:47
Also in: lkml

Two independent fixes, both found while chasing a PHY whose driver is a
module on a rootfs that is not mounted yet when a DSA switch probes.
Neither one depends on that setup, and neither depends on the other.

Patch 1: phylink_bringup_phy() records the PHY in pl->phydev before its
last fallible step, so a failure there leaves a pointer to a PHY the
caller has already detached. A later phylink_disconnect_phy() detaches
it a second time and drops references the first detach already
released.

Patch 2: a PHY that binds a driver with no interrupt callbacks loses
the interrupt number the firmware node declared. The specific driver
that binds afterwards never sees it, and unless its consumer installs
one itself the PHY is polled from then on.

Patch 2 takes a different approach from v4 and does not carry Andrew's
Reviewed-by, which I asked him to hold [2]. v4 read the number back
from bus->irq[]. The Sashiko bot pointed out that a bus installing the
interrupt only on the phy_device never writes that table, and it is
right: on smsc95xx and lan78xx the restore read PHY_POLL back out and
did nothing. v5 saves the number where phy_probe() takes it, so the
source cannot be a table that never held it, and nothing has to guess
whether a PHY_POLL came from the bind.

Two neighbouring problems are deliberately left alone:

 - phy_attach_direct() clobbers the interrupt a second time, at the
   attach rather than at the bind, and that loses a number a consumer
   installed after probe. It is phylib's own rule about a driver
   without interrupt callbacks, not the PHY_F_NO_IRQ line above it, and
   it is re-applied on every attach, so undoing it is a different
   change with a different owner.
 - phy_remove() leaves phydev->drv NULL while a consumer can still hold
   the PHY, and phy_disconnect() dereferences it through
   phy_config_interrupt(). That is reachable today for a PHY that has
   an interrupt number and a driver that supports interrupts. Patch 2
   refuses to restore while a consumer holds the PHY, so it does not
   hand a number back to a PHY that phy_probe() had put in polling
   mode, and does not widen that path.

The Fixes tag reaches 2005, but the guard leans on phy_detach() clearing
phy_link_change, which is only true since commit e0d1c55501d3 ("net:
phy: fix phy_uses_state_machine()") in v6.17. Older trees never clear
the mark, so the restore would be refused forever and the patch would be
a silent no-op there. Patch 2 says so in its own message, since that is
what travels into a backport. Those trees also lack the is_genphy_driven
context the unwind hunk needs, so the patch will not apply to them
unaided in any case.

One case the patch does not cover, for the same reason. Unbind a driver
through sysfs while a consumer holds the PHY and the restore is
refused, correctly; the consumer's later detach clears the mark but
reaches no second remove, so the number waits in irq_saved and the next
driver to bind still starts polled. It comes back at that driver's
remove. Closing it would mean restoring from phy_detach() again, which
is the shape this version exists to leave behind.

No hardware measurement of the restore is offered, and the reason is
worth stating rather than hiding. The cycle this fixes needs a generic
driver bound at the PHY before the specific one - which needs the
specific driver or its firmware to be unreadable when the MDIO bus is
scanned. The board I develop on cannot produce that: its rootfs and
firmware are present at boot, so the specific driver binds directly and
the generic one never probes. phydev->irq has no observable outside the
phy_attached_info() line, and that needs a consumer to attach, which on
this board only ever meets the specific driver. So the three restore
sites are argued from the code, not run: phy_remove(), reached from a
sysfs unbind and from phy_detach(); phy_probe()'s own error exit, which
the driver core does not follow with a remove; and phy_attach_direct()'s
unwind of a generic bind that failed after probe, where
device_bind_driver() fails only in driver_sysfs_add().

[2] https://lore.kernel.org/netdev/20260905000722.422652-1-f@lex.la/ (local)

Changes in v5:
 - patch 2 changes approach: the number is saved in phy_probe() where
   it is taken and restored in phy_remove() and on phy_probe()'s own
   error exit, rather than read back from bus->irq[] in phy_detach()
 - patch 2: a generic bind that fails after its probe succeeded is a
   third exit with no restore, so phy_attach_direct()'s unwind gets one
   too, as it did in v4
 - patch 2: the restore is refused while a consumer holds the PHY, so
   an unbind or an rmmod under a live consumer cannot hand a number
   back to a phy_disconnect() that never requested one. The mark is
   phy_link_change rather than attached_dev, because a DSA shared port
   attaches its PHY with no netdev and leaves attached_dev NULL
 - patch 1 is unchanged apart from the Assisted-by trailer, which it
   should have carried from the start
 - v4: https://lore.kernel.org/netdev/20260902080511.2211261-1-f@lex.la/ (local)

Aleksei Sviridkin (2):
  net: phylink: unwind the PHY binding when bringup fails late
  net: phy: restore the interrupt phy_probe() replaced with PHY_POLL

 drivers/net/phy/phy_device.c | 23 ++++++++++++++++++++++-
 drivers/net/phy/phylink.c    | 29 ++++++++++++++++++++---------
 include/linux/phy.h          |  3 +++
 3 files changed, 45 insertions(+), 10 deletions(-)


base-commit: 6262acad9db197b5ed12e3b245d2e6d0c80fb960
-- 
2.53.0
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help