[PATCH net v2] net: phylink: unwind the PHY binding when bringup fails late

[PATCH net v2] net: phylink: unwind the PHY binding when bringup fails late

[Aleksei Sviridkin]

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() goes through phylink_attach_phy(), which refuses
to attach while pl->phydev is set, turning a transient MDIO error into
a permanent -EBUSY. The SFP path is worse than that: sfp_sm_probe_phy()
answers the failure with phy_device_remove() and phy_device_free(), and
it assigns sfp->mod_phy only past that error return, so nothing clears
pl->phydev and it is left pointing at a freed phy_device that
phylink_resolve() and the ethtool helpers go on reading.
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 references the first
one already released.

Found while making a DSA port survive a PHY whose driver arrives after
the switch probes: keeping the port across a failed connect and
retrying is what makes this window reachable.

Clear the binding on the failure path. This is the same operation
phylink_disconnect_phy() performs, so both now share a helper. The
PHY-side fields are left to phy_detach(), which every caller already
runs on this path.

Fixes: 03abf2a7c654 ("net: phylink: add EEE management")
Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---

Notes:
    v2: dropped the comment above phylink_clear_phydev(); phylink states no
    lock requirements in comments and leaves its static helpers uncommented,
    as Andrew pointed out. Added a sentence on how the defect was found. The
    code is identical to v1, so the Reviewed-by is kept.
    v1: https://lore.kernel.org/netdev/20260918015109.2518797-1-f@lex.la/
    
    Split out of the [PATCH net v7 0/2] pair (20260909204306.2374562-1-f@lex.la):
    2/2 is being reworked per review, and this patch has nothing left to wait
    for.
    
    Reviewed-by was given on v2 of the pair (net-next v2 1/2), lore message-id
    006ad9d6-b51b-46ed-8c5d-3649b70a4f62@lunn.ch.

 drivers/net/phy/phylink.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index a1458da8111b..0e59b7ce8b7a 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2085,6 +2085,17 @@ static int phylink_validate_phy(struct phylink *pl, struct phy_device *phy,
 	return phylink_validate(pl, supported, state);
 }
 
+static void phylink_clear_phydev(struct phylink *pl, struct phy_device *phy)
+{
+	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);
+}
+
 static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 			       phy_interface_t interface)
 {
@@ -2199,6 +2210,12 @@ 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);
+		phylink_clear_phydev(pl, phy);
+		mutex_unlock(&pl->phydev_mutex);
+	}
+
 	return ret;
 }
 
@@ -2349,15 +2366,8 @@ void phylink_disconnect_phy(struct phylink *pl)
 
 	mutex_lock(&pl->phydev_mutex);
 	phy = pl->phydev;
-	if (phy) {
-		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);
-	}
+	if (phy)
+		phylink_clear_phydev(pl, phy);
 	mutex_unlock(&pl->phydev_mutex);
 
 	if (phy) {
-- 
2.53.0

Re: [PATCH net v2] net: phylink: unwind the PHY binding when bringup fails late

[Andrew Lunn]

On Sat, Sep 19, 2026 at 04:53:38AM +0300, Aleksei Sviridkin wrote:
> 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() goes through phylink_attach_phy(), which refuses
> to attach while pl->phydev is set, turning a transient MDIO error into
> a permanent -EBUSY. The SFP path is worse than that: sfp_sm_probe_phy()
> answers the failure with phy_device_remove() and phy_device_free(), and
> it assigns sfp->mod_phy only past that error return, so nothing clears
> pl->phydev and it is left pointing at a freed phy_device that
> phylink_resolve() and the ethtool helpers go on reading.
> 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 references the first
> one already released.
> 
> Found while making a DSA port survive a PHY whose driver arrives after
> the switch probes: keeping the port across a failed connect and
> retrying is what makes this window reachable.
> 
> Clear the binding on the failure path. This is the same operation
> phylink_disconnect_phy() performs, so both now share a helper. The
> PHY-side fields are left to phy_detach(), which every caller already
> runs on this path.

What is missing here is an explanation why you cannot record the PHY
in pl->phydev later, once all calls which can fail have been
performed. That seems like a simpler and more logical fix. But maybe
i'm missing something.

	Andrew

Re: [PATCH net v2] net: phylink: unwind the PHY binding when bringup fails late

[Aleksei Sviridkin]

Yes, and v3 does it that way.

Nothing between the assignment and phy_eee_rx_clock_stop() reads
pl->phydev, and a NULL pl->phydev already means "no PHY" to the
readers. phy_request_interrupt() comes after it and cannot fail, it
falls back to polling itself. So the unwind is gone.

The fallible call itself stays where it is. phy_support_eee() can
reach drv->disable_autonomous_eee, which writes PHY registers, so
lifting phy_eee_rx_clock_stop() above the EEE block would reorder
MDIO. Only the assignment moves.

Tested on an MT7981 board whose MAC implements the LPI ops, so that
call sits on the bringup path of every port. With an MDIO error
injected there the late attach fails with -EIO and the retry brings
the port up. Without the change the box dies instead:

  Unable to handle kernel execute from non-executable memory at virtual address 0000000000000000
  Workqueue: events_power_efficient phy_state_machine
  pc : 0x0
  lr : phy_check_link_status+0xc4/0xf0
  Call trace:
   0x0
   _phy_start_aneg+0x4c/0xa0
   _phy_state_machine+0x16c/0x300
   phy_state_machine+0x28/0x80

The caller detached the PHY after the failure, so phydev->drv is NULL,
but phylink still had the pointer and started it again.

[PATCH net v3] net: phylink: record the PHY only once bringup cannot fail

[Aleksei Sviridkin]

phylink_bringup_phy() stores 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() goes through phylink_attach_phy(), which refuses
to attach while pl->phydev is set, turning a transient MDIO error into
a permanent -EBUSY. The SFP path is worse than that: sfp_sm_probe_phy()
answers the failure with phy_device_remove() and phy_device_free(), and
it assigns sfp->mod_phy only past that error return, so nothing clears
pl->phydev and it is left pointing at a freed phy_device that
phylink_resolve() and the ethtool helpers go on reading.
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 references the first
one already released.

Found while making a DSA port survive a PHY whose driver arrives after
the switch probes: keeping the port across a failed connect and
retrying is what makes this window reachable.

Publish the pointer after the last call that can fail instead of
unwinding it afterwards. Nothing between the two points reads
pl->phydev, and the registration that follows cannot fail:
phy_request_interrupt() falls back to polling on its own. The PHY-side
state keeps the order it had, so no MDIO operation moves relative to
another.

Fixes: 03abf2a7c654 ("net: phylink: add EEE management")
Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
---
Changes since v2:
- Record the PHY after the last call that can fail, as Andrew suggested,
  which removes the unwind and the helper it shared with
  phylink_disconnect_phy(). The fallible call keeps its place so no MDIO
  operation moves.
- Hardware evidence for the failure this prevents is in the reply to v2.
---
 drivers/net/phy/phylink.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index a1458da8111b..1bbcf46c8356 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2129,7 +2129,6 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 	mutex_lock(&pl->phydev_mutex);
 	mutex_lock(&phy->lock);
 	mutex_lock(&pl->state_mutex);
-	pl->phydev = phy;
 	pl->phy_state.interface = interface;
 	pl->phy_state.pause = MLO_PAUSE_NONE;
 	pl->phy_state.speed = SPEED_UNKNOWN;
@@ -2196,10 +2195,25 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 			ret = 0;
 	}
 
-	if (ret == 0 && phy_interrupt_is_valid(phy))
+	if (ret)
+		return ret;
+
+	/* Nothing below can fail, so the PHY can be recorded now. Doing it
+	 * here rather than above keeps a failed bringup from leaving
+	 * pl->phydev pointing at a PHY the caller is about to detach.
+	 */
+	mutex_lock(&pl->phydev_mutex);
+	mutex_lock(&phy->lock);
+	mutex_lock(&pl->state_mutex);
+	pl->phydev = phy;
+	mutex_unlock(&pl->state_mutex);
+	mutex_unlock(&phy->lock);
+	mutex_unlock(&pl->phydev_mutex);
+
+	if (phy_interrupt_is_valid(phy))
 		phy_request_interrupt(phy);
 
-	return ret;
+	return 0;
 }
 
 static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
-- 
2.53.0

Re: [PATCH net v3] net: phylink: record the PHY only once bringup cannot fail

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 21 Sep 2026 01:20:44 +0300 you wrote:
> phylink_bringup_phy() stores 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.
> 
> [...]

Here is the summary with links:
  - [net,v3] net: phylink: record the PHY only once bringup cannot fail
    https://git.kernel.org/netdev/net/c/a940003f44e7

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html