[PATCH net-next 1/3] net: phylink: unwind the PHY binding when bringup fails late

Aleksei Sviridkin <[email protected]>
Newsgroups org.kernel.vger.netdev,org.kernel.vger.linux-kernel
Message-ID <[email protected]>
phylink_bringup_phy() records the PHY in pl->phydev before its last
fallible step: on a MAC whose phylink ops implement LPI,
phy_eee_rx_clock_stop() can fail with a real MDIO error. The callers
unwind with phy_detach(), which knows nothing about pl->phydev, so a
pointer to a PHY that is no longer attached outlives the failed
connect.

What that costs depends on how the caller got here.
phylink_connect_phy() and the SFP path go through
phylink_attach_phy(), which refuses to attach while pl->phydev is set
and turns a transient MDIO error into a permanent -EBUSY.
phylink_fwnode_phy_connect() has no such check, so a later connect
overwrites the stale pointer and hides the problem. A disconnect does
not: phylink_disconnect_phy() hands that pointer to phy_disconnect(),
and the second phy_detach() on the same PHY drops a device reference
and two module references that were only ever taken once.

Clear the binding on the failure path, the same three fields
phylink_disconnect_phy() clears, under the same locks. The PHY-side
fields are left to phy_detach(), which every caller already runs on
this path.

Signed-off-by: Aleksei Sviridkin <[email protected]>
---
Reachability

The failing step needs pl->mac_supports_eee_ops, i.e. a MAC whose
phylink ops implement the LPI callbacks; mt7530 is one, and on the
board I tested ethtool --show-eee returns -EOPNOTSUPP, which is what
phylink reports when mac_supports_eee_ops is set and mac_supports_eee
is not, so that tail runs on every bringup there. The error itself is
an MDIO transaction failure inside phy_eee_rx_clock_stop(), which
cannot be produced deliberately, so this patch is compile-tested and
the series it belongs to ran on hardware with it in place.

The double-detach path needs a port that outlives a failed connect,
which is what patch 3 introduces; before that, DSA destroyed the port
immediately and the stale pointer went with it.
 drivers/net/phy/phylink.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 5b8e95690..9d403ff1b 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2197,6 +2197,18 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 	if (ret == 0 && phy_interrupt_is_valid(phy))
 		phy_request_interrupt(phy);
 
+	if (ret) {
+		mutex_lock(&pl->phydev_mutex);
+		mutex_lock(&phy->lock);
+		mutex_lock(&pl->state_mutex);
+		pl->phydev = NULL;
+		pl->phy_enable_tx_lpi = false;
+		pl->mac_tx_clk_stop = false;
+		mutex_unlock(&pl->state_mutex);
+		mutex_unlock(&phy->lock);
+		mutex_unlock(&pl->phydev_mutex);
+	}
+
 	return ret;
 }
 
-- 
2.43.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.