[PATCH net-next v2 2/2] net: phy: restore the interrupt after a generic-driver bind cycle

A PHY with no specific driver available at attach time gets the generic
one, and phy_probe() parks it in polling mode because that driver has
no interrupt callbacks. phy_detach() releases the generic driver so a
real one can bind later, but the interrupt is not given back, and a
generic probe that fails leaves the PHY the same way without reaching
phy_detach() at all. The specific driver then attaches with
irq == PHY_POLL and the PHY stays polled for the rest of the uptime,
with no warning on that path.

Take it back from the MDIO bus interrupt table on both exits from the
cycle: the cycle does not touch that table, so whatever the bus
recorded there still stands. A bus that never filled it has nothing to
give back and its PHY stays polled. Restore only where the cycle left
PHY_POLL, so that an interrupt mode installed on the attached PHY
afterwards is not reset.

Signed-off-by: Aleksei Sviridkin <[email protected]>
---
The cycle is easy to hit on a DSA switch that probes before the rootfs
is mounted: the switch connects its user ports during setup, the PHY
driver is still a module on that rootfs, so the generic driver binds
and is released again when the connect fails. The real driver binds at
ifup and gets irq == PHY_POLL.

Both exits from the cycle need the restore: phy_detach() for a generic
driver that bound and is being released, and phy_attach_direct()'s
error_module_put label for a generic probe that failed, which never
calls phy_detach() at all.

What the table covers and what it does not. Nothing writes
mii_bus->irq[] after the bus is registered except stmmac_mdio.c and
mlxbf_gige_main.c, and both assign the same value to phydev->irq in the
same breath, so a PHY whose interrupt came from firmware is fixed here.
A PHY handed its interrupt by its MAC driver is not: lan78xx and
smsc95xx write phydev->irq after registration and leave the table
alone, so they keep polling after a generic cycle exactly as they do
today, and the set of drivers that write only phydev->irq is larger
than those two. Which raises a question I would rather ask than settle
alone: if bus->irq[] is the per-address registry for a bus, should
those drivers be mirroring into it the way mlxbf_gige and stmmac
already do? If that is the intent I am happy to send it as a follow-up.

What the guard distinguishes and what it does not. It preserves an
interrupt mode installed on the attached PHY after connect, so
PHY_MAC_INTERRUPT from genet, tsnep or bcmasp survives the detach. It
cannot tell phy_probe()'s parking from the other ways phydev->irq
reaches PHY_POLL, so a PHY parked by PHY_F_NO_IRQ, by a failed
phy_request_interrupt(), or by a MAC taking the phy.rst advice to set
PHY_POLL, is restored here as well. That is harmless for the drivers
that do so today: phy_attach_direct() applies PHY_F_NO_IRQ again on the
next attach, a failed request is simply retried, and every MAC that
forces PHY_POLL does so in the same function that connects the PHY, so
a restored value is overwritten before anything can act on it.

The guard is not only tidiness. Without it a PHY that a MAC had put in
PHY_MAC_INTERRUPT mode would get a real interrupt number back at
detach, the next phy_connect_direct() would request it, and
phy_disconnect() would then skip phy_free_interrupt(), because the MAC
overwrites phydev->irq again right after connect and
phy_interrupt_is_valid() is false by the time the interrupt would be
freed. That asymmetry between phy_connect_direct() and
phy_disconnect() is not new, and it bites any such MAC whose PHY has an
interrupt to request; the guard keeps a generic-driver cycle from
walking a PHY into it.

The same interrupt is also lost on a plain sysfs unbind and rebind of a
PHY driver, and this patch does not cover that. A restore in
phy_remove() would cover both paths, but is_genphy_driven is what keeps
the intent narrow here, so I would rather not widen the fix on a guess.

No Fixes: tag on this one. The behaviour predates what I can bisect in
this tree; if someone can name the commit I will add it.
 drivers/net/phy/phy_device.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 94b2e85e00a3..8c93d35e1c94 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1734,6 +1734,18 @@ static bool phy_drv_supports_irq(const struct phy_driver *phydrv)
 	return phydrv->config_intr && phydrv->handle_interrupt;
 }
 
+/* Give back the interrupt phy_probe() parked when a driver with no interrupt
+ * callbacks bound. The bind cycle does not touch the bus interrupt table, so
+ * whatever the bus recorded there still stands; a bus that never filled it has
+ * nothing to give back. Only the parking is undone: any other value the PHY
+ * carries was put there by someone else.
+ */
+static void phy_restore_genphy_irq(struct phy_device *phydev)
+{
+	if (phydev->irq == PHY_POLL)
+		phydev->irq = phydev->mdio.bus->irq[phydev->mdio.addr];
+}
+
 /**
  * phy_attach_direct - attach a network device to a given PHY device pointer
  * @dev: network device to attach
@@ -1896,6 +1908,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 error_module_put:
 	module_put(d->driver->owner);
+	phy_restore_genphy_irq(phydev);
 	phydev->is_genphy_driven = 0;
 	d->driver = NULL;
 error_put_device:
@@ -1965,6 +1978,7 @@ void phy_detach(struct phy_device *phydev)
 	 * real driver could be loaded
 	 */
 	if (phydev->is_genphy_driven) {
+		phy_restore_genphy_irq(phydev);
 		device_release_driver(&phydev->mdio.dev);
 		phydev->is_genphy_driven = 0;
 	}
-- 
2.55.0