[PATCH net] net: phylink: allow stopping a suspended instance

[PATCH net] net: phylink: allow stopping a suspended instance

[James Hilliard]

After a failed MAC resume, a driver must be able to stop phylink without
resuming it and reconfiguring the failed MAC.

For MAC WoL, finish the deferred link-down and clear the WoL disable bit.
Otherwise, avoid repeating PHY, SFP and PCS shutdown, but suspend a PHY
that prepare_resume() powered up for the MAC reset clock.

Retain the saved link state if MAC WoL is suspended again after a failed
resume. For example, fbnic can fail to allocate IRQs on resume and call
phylink_suspend() again on the next system suspend. The carrier is already
off, so taking another snapshot loses the deferred mac_link_down(). Keep
the original snapshot until resume or stop completes, allowing the next
suspend cycle to capture the new link state.

Restore any PHY advertisement reduced by suspend. Track that reduction
separately from explicit driver speed-down requests, so stopping a
suspended instance does not undo a driver's close-time power saving.

Fixes: f97493657c63 ("net: phylink: add suspend/resume support")
Fixes: 4c8925cb9db1 ("net: phylink: fix suspend/resume with WoL enabled and link down")
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/net/phy/phylink.c | 56 +++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 52 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 1bbcf46c8356..08f06ae5213f 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -78,6 +78,7 @@ struct phylink {
 
 	bool link_failed;
 	bool suspend_link_up;
+	bool suspend_speed_down;
 	bool force_major_config;
 	bool major_config_failed;
 	bool mac_supports_eee_ops;
@@ -2498,6 +2499,14 @@ void phylink_start(struct phylink *pl)
 }
 EXPORT_SYMBOL_GPL(phylink_start);
 
+static void phylink_restore_suspend_speed(struct phylink *pl)
+{
+	if (pl->suspend_speed_down) {
+		phylink_speed_up(pl);
+		pl->suspend_speed_down = false;
+	}
+}
+
 /**
  * phylink_stop() - stop a phylink instance
  * @pl: a pointer to a &struct phylink returned from phylink_create()
@@ -2509,11 +2518,30 @@ EXPORT_SYMBOL_GPL(phylink_start);
  *
  * This will synchronously bring down the link if the link is not already
  * down (in other words, it will trigger a mac_link_down() method call.)
+ * A suspended instance may be stopped without first calling phylink_resume().
+ * In particular, closing a device after a failed resume must not restart the
+ * link or reconfigure the MAC just to finish shutting it down.
+ * Any PHY advertisement reduced by phylink_suspend() is restored as part
+ * of this transition.
+ * If phylink_prepare_resume() powered up an already stopped PHY, suspend
+ * it again when Wake-on-LAN permits.
  */
 void phylink_stop(struct phylink *pl)
 {
 	ASSERT_RTNL();
 
+	/* Also undo PHY speed control when terminating a suspended instance. */
+	phylink_restore_suspend_speed(pl);
+
+	if (test_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state)) {
+		/* A failed MAC resume may have called phylink_prepare_resume()
+		 * and powered the stopped PHY back up to supply its RX clock.
+		 */
+		if (pl->phydev)
+			phy_suspend(pl->phydev);
+		return;
+	}
+
 	if (pl->sfp_bus)
 		sfp_upstream_stop(pl->sfp_bus);
 	if (pl->phydev)
@@ -2526,6 +2554,16 @@ void phylink_stop(struct phylink *pl)
 
 	phylink_run_resolve_and_disable(pl, PHYLINK_DISABLE_STOPPED);
 
+	if (test_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state)) {
+		/* Finish the link-down deferred by MAC WoL, without restarting. */
+		flush_work(&pl->resolve);
+		mutex_lock(&pl->state_mutex);
+		if (pl->suspend_link_up)
+			phylink_link_down(pl);
+		__clear_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state);
+		mutex_unlock(&pl->state_mutex);
+	}
+
 	pl->pcs_state = PCS_STATE_DOWN;
 
 	phylink_pcs_disable(pl->pcs);
@@ -2635,10 +2673,13 @@ void phylink_suspend(struct phylink *pl, bool mac_wol)
 		/* Wake-on-Lan enabled, MAC handling */
 		mutex_lock(&pl->state_mutex);
 
+		/* Preserve the pending link-down if a previous resume failed. */
+		if (!test_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state))
+			pl->suspend_link_up = phylink_link_is_up(pl);
+
 		/* Stop the resolver bringing the link up */
 		__set_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state);
 
-		pl->suspend_link_up = phylink_link_is_up(pl);
 		if (pl->suspend_link_up) {
 			/* Disable the carrier, to prevent transmit timeouts,
 			 * but one would hope all packets have been sent. This
@@ -2657,8 +2698,10 @@ void phylink_suspend(struct phylink *pl, bool mac_wol)
 		phylink_stop(pl);
 	}
 
-	if (phylink_phy_pm_speed_ctrl(pl))
+	if (phylink_phy_pm_speed_ctrl(pl)) {
 		phylink_speed_down(pl, false);
+		pl->suspend_speed_down = true;
+	}
 }
 EXPORT_SYMBOL_GPL(phylink_suspend);
 
@@ -2698,8 +2741,7 @@ void phylink_resume(struct phylink *pl)
 {
 	ASSERT_RTNL();
 
-	if (phylink_phy_pm_speed_ctrl(pl))
-		phylink_speed_up(pl);
+	phylink_restore_suspend_speed(pl);
 
 	if (test_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state)) {
 		/* Wake-on-Lan enabled, MAC handling */
@@ -3616,6 +3658,12 @@ int phylink_speed_down(struct phylink *pl, bool sync)
 
 	ASSERT_RTNL();
 
+	/* An explicit request takes over from suspend-time speed control.
+	 * Restore the original advertisement before saving it again, so a
+	 * repeated speed-down cannot replace it with the reduced advertisement.
+	 */
+	phylink_restore_suspend_speed(pl);
+
 	if (!pl->sfp_bus && pl->phydev)
 		ret = phy_speed_down(pl->phydev, sync);
 

---
base-commit: 7375d38364a9aa66fb31716bcefef38aecad75d8
change-id: 20261001-submit-phylink-suspended-stop-v1-09f9b63e6f43

Best regards,
--  
James Hilliard <james.hilliard1@gmail.com>

Re: [PATCH net] net: phylink: allow stopping a suspended instance

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net] net: phylink: allow stopping a suspended instance

[Andrew Lunn]

On Thu, Oct 01, 2026 at 01:24:53AM -0600, James Hilliard wrote:
> After a failed MAC resume, a driver must be able to stop phylink without
> resuming it and reconfiguring the failed MAC.

This is adding a lot of complexity, for an edge case, which i don't
particularly like.

In your case, why is resume failing?

   Andrew

Re: [PATCH net] net: phylink: allow stopping a suspended instance

[James Hilliard]

On Sun, Oct 4, 2026 at 7:54 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Thu, Oct 01, 2026 at 01:24:53AM -0600, James Hilliard wrote:
> > After a failed MAC resume, a driver must be able to stop phylink without
> > resuming it and reconfiguring the failed MAC.
>
> This is adding a lot of complexity, for an edge case, which i don't
> particularly like.
>
> In your case, why is resume failing?

This is a prerequisite for the stmmac recovery fixes, rather than a
fix for the underlying cause of a particular resume failure.

stmmac_resume() already returns an error if stmmac_hw_setup() fails,
for example when the DMA reset does not complete. That return is
before phylink_resume(), leaving phylink suspended.

The stmmac recovery work preserves the interface's administrative
state after that failure, but leaves the datapath detached and
quiescent. An ordinary down/up must then be able to release the
retained resources and attempt a fresh open. Closing the interface
therefore needs to terminate the suspended phylink instance.

Calling phylink_resume() merely to make phylink_stop() usable would
reconfigure the MAC and restart link resolution, even though MAC
setup has just failed.

The additional bookkeeping handles the two different suspend paths:
with MAC WoL, stop must finish the deferred link-down and clear the
WoL disable state; without MAC WoL, suspend has already stopped the
instance, so stop must avoid repeating that shutdown. Suspend-owned
PHY speed changes also need unwinding so the next start does not
retain the reduced advertisement.

Those are phylink-owned transitions which stmmac cannot complete
itself. The driver still owns the recovery policy and any fix for
the underlying resume failure; this supplies the cleanup transition
needed when restoration fails.

>
>    Andrew

Re: [PATCH net] net: phylink: allow stopping a suspended instance

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch does not handle the state where
  PHYLINK_DISABLE_STOPPED and PHYLINK_DISABLE_MAC_WOL are both set.

--

net: phylink: allow stopping a suspended instance

This patch lets drivers call phylink_stop() on a suspended phylink
instance after a failed MAC resume. For MAC WoL it finishes the deferred
link-down and clears PHYLINK_DISABLE_MAC_WOL. It also keeps the saved
link state when MAC WoL suspend is repeated, and tracks suspend-time PHY
speed-down separately.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 1bbcf46c83566..08f06ae5213fa 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c

[ ... ]

> @@ -2509,11 +2518,30 @@ EXPORT_SYMBOL_GPL(phylink_start);
>   *
>   * This will synchronously bring down the link if the link is not already
>   * down (in other words, it will trigger a mac_link_down() method call.)
> + * A suspended instance may be stopped without first calling phylink_resume().
> + * In particular, closing a device after a failed resume must not restart the
> + * link or reconfigure the MAC just to finish shutting it down.

[ ... ]

>  void phylink_stop(struct phylink *pl)
>  {
>  	ASSERT_RTNL();
>  
> +	/* Also undo PHY speed control when terminating a suspended instance. */
> +	phylink_restore_suspend_speed(pl);
> +
> +	if (test_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state)) {
> +		/* A failed MAC resume may have called phylink_prepare_resume()
> +		 * and powered the stopped PHY back up to supply its RX clock.
> +		 */
> +		if (pl->phydev)
> +			phy_suspend(pl->phydev);
> +		return;
> +	}
> +
>  	if (pl->sfp_bus)
>  		sfp_upstream_stop(pl->sfp_bus);
>  	if (pl->phydev)
> @@ -2526,6 +2554,16 @@ void phylink_stop(struct phylink *pl)
>  
>  	phylink_run_resolve_and_disable(pl, PHYLINK_DISABLE_STOPPED);
>  
> +	if (test_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state)) {
> +		/* Finish the link-down deferred by MAC WoL, without restarting. */
> +		flush_work(&pl->resolve);
> +		mutex_lock(&pl->state_mutex);
> +		if (pl->suspend_link_up)
> +			phylink_link_down(pl);
> +		__clear_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state);
> +		mutex_unlock(&pl->state_mutex);
> +	}
> +

[Severity: Medium]
What happens when PHYLINK_DISABLE_STOPPED and PHYLINK_DISABLE_MAC_WOL
are both set?

phylink_suspend() takes the MAC WoL branch without checking
PHYLINK_DISABLE_STOPPED, so this sequence looks possible:

  phylink_suspend(pl, false)   /* non-WoL, phylink_stop() sets STOPPED */
  <MAC resume fails before phylink_resume() is called>
  phylink_suspend(pl, true)    /* MAC WoL, sets MAC_WOL */

At that point suspend_link_up is false, because the carrier is already
off.

If phylink_stop() is called next, the new STOPPED early return runs
before the MAC_WOL block above. That block is the only place that clears
PHYLINK_DISABLE_MAC_WOL, so the bit survives the stop. The next
phylink_start() only clears STOPPED:

  phylink_start()
    phylink_enable_and_run_resolve(pl, PHYLINK_DISABLE_STOPPED)
      phylink_run_resolve()
        if (!pl->phylink_disable_state)
            queue_work(system_power_efficient_wq, &pl->resolve);

Does this leave the resolver gated, so the link stays down until another
stop/start cycle?

If phylink_resume() is called instead, it takes the MAC_WOL branch. It
calls phylink_mac_initial_config() and clears only MAC_WOL.
phylink_start() never runs, so STOPPED stays set, phy_start() and
sfp_upstream_start() are skipped, and the resolver stays gated. This
patch doesn't change that part of phylink_resume(). Still, the patch is
meant to make re-suspend and stop after a failed resume behave
correctly.

fbnic looks like it can reach this. fbnic_stop() passes a firmware
derived value as mac_wol:

  phylink_suspend(fbn->phylink, fbnic_bmc_present(fbn->fbd));

The BMC presence is re-read from firmware during resume, before
__fbnic_open() runs. So the mode can change between a failed resume (for
example, fbnic_mac_request_irq() failing) and the next suspend. Since
fbnic only uses phylink_suspend() and phylink_resume(), the link could
stay down across open/close cycles while fbnic_bmc_present() stays true.

For stmmac (stmmac_resume() failing in stmmac_hw_setup()) and ucc_geth,
ethtool rejects WoL changes with -ENODEV while the device is detached. So
the mode change seems harder to hit on those drivers.

The new kernel-doc says "A suspended instance may be stopped without
first calling phylink_resume()". The commit message says "For MAC WoL,
finish the deferred link-down and clear the WoL disable bit".

Should the STOPPED early return also handle a pending
PHYLINK_DISABLE_MAC_WOL? Separately, should phylink_suspend() avoid
setting MAC_WOL on an instance that is already stopped, or should
phylink_resume() fall back to phylink_start() in that case?

>  	pl->pcs_state = PCS_STATE_DOWN;
>  
>  	phylink_pcs_disable(pl->pcs);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001-submit-phylink-suspended-stop-v1-v1-1-0669fdb4a93b%40gmail.com