Re: [PATCH net v2 1/2] net: phylink: allow stopping a suspended instance

[netdev-bot+sashiko@kernel.org]

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] phylink_stop() (drivers/net/phy/phylink.c:2506-2508) now
  returns early when PHYLINK_DISABLE_STOPPED is already set.

--

net: phylink: allow stopping a suspended instance

This commit lets phylink_stop() handle an instance that is still
suspended by phylink_suspend(), for example after a driver's resume
failed. Without MAC WoL it returns early, since phylink_suspend() already
called phylink_stop(). With MAC WoL it finishes the deferred
mac_link_down() and clears PHYLINK_DISABLE_MAC_WOL.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index a1458da8111b6..df520d77fdedd 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -2495,11 +2495,18 @@ EXPORT_SYMBOL_GPL(phylink_start);

[ ... ]

>  void phylink_stop(struct phylink *pl)
>  {
>  	ASSERT_RTNL();
>  
> +	/* phylink_suspend() already stops the link without MAC WoL. */
> +	if (test_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state))
> +		return;
> +

[Severity: Medium]
Does this early return leave the PHY speed-down from phylink_suspend() in
place?

phylink_suspend() does this after its own phylink_stop():

drivers/net/phy/phylink.c:phylink_suspend() {
    ...
	} else {
		phylink_stop(pl);
	}

	if (phylink_phy_pm_speed_ctrl(pl))
		phylink_speed_down(pl, false);
}

In phylink, only the phylink_speed_up() call in phylink_resume() undoes
this. With this patch, stopping a suspended instance leaves
phydev->advertising reduced and the saved adv_old still pending. Phylink
still treats the instance as cleanly stopped.

With stmmac, which sets wol_phy_speed_ctrl, this looks reachable with
PHY-only WoL (wolopts_mac == 0 and phy_may_wakeup() true):

1. stmmac_resume() fails before it reaches phylink_resume(). Examples are
   a failure in plat->resume, stmmac_legacy_serdes_power_up() or
   stmmac_hw_setup().

2. The interface is closed while device_may_wakeup(priv->device) is true.
   stmmac_release() then calls phylink_speed_down() a second time:

	if (device_may_wakeup(priv->device))
		phylink_speed_down(priv->phylink, false);

   phy_speed_down() always overwrites adv_old, before it checks whether
   anything changed:

	linkmode_copy(adv_tmp, phydev->advertising);
	ret = phy_speed_down_core(phydev);
	...
	linkmode_copy(phydev->adv_old, adv_tmp);

   This replaces the saved full advertisement with the reduced one.
   __stmmac_release() then calls phylink_stop(), which returns early here.
   phylink_disconnect_phy() runs next.

3. On reopen, phylink_bringup_phy() copies the reduced phy->advertising
   into pl->link_config.advertising. The phylink_speed_up() call in
   stmmac_open() can only restore the reduced adv_old.

Would the link then negotiate only the lowest common speed, for example
10 Mbps instead of 1 Gbps? This would last through later close/open and
suspend/resume cycles until the advertisement is reset with ethtool -s.

The commit message says:

  the driver still decides whether to retry resume or wait for an ordinary
  administrative down/up cycle.

In this case the down/up cycle does not appear to restore the speed. If
device_may_wakeup() is false, the phylink_speed_up() call in stmmac_open()
does restore the advertisement. That fix comes from the driver, though,
and not from the new suspend-to-stop transition in phylink.

Should the suspend-to-stop path in phylink_stop() also call
phylink_speed_up() when phylink_phy_pm_speed_ctrl(pl) is true?

>  	if (pl->sfp_bus)
>  		sfp_upstream_stop(pl->sfp_bus);
>  	if (pl->phydev)

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-submit-stmmac-reset-fixes-v1-v2-0-d92e609be996%40gmail.com