Re: [PATCH net 1/3] 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() is newly documented and implemented as a legal
  replacement for phylink_resume() after phylink_suspend(), but it does…

--

net: phylink: allow stopping a suspended instance

This makes phylink_stop() usable directly on a suspended phylink instance,
so a MAC that cannot restore its hardware after system sleep can close the
netdev instead of calling phylink_resume(). An already stopped instance
returns early, and the MAC Wake-on-LAN case drains the resolve work,
completes the deferred link-down and clears PHYLINK_DISABLE_MAC_WOL.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index a1458da8111b6..8f9278112623b 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -2495,11 +2495,17 @@ 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.)
> + * It may also be called after phylink_suspend() if the MAC cannot resume
> + * and the network device must be closed instead.
>   */
>  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 new exit path leave the PHY advertisement clamped to the lowest
speed?

phylink_suspend() performs two things when PHY based power management speed
control is in use:

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

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

The only place in phylink that reverses the second part is
phylink_resume():

drivers/net/phy/phylink.c:phylink_resume() {
	ASSERT_RTNL();

	if (phylink_phy_pm_speed_ctrl(pl))
		phylink_speed_up(pl);
	...
}

Both new exits added here, this early return and the MAC WoL completion
block below, return without calling phylink_speed_up().  So a MAC that
follows the new kernel-doc ("It may also be called after phylink_suspend()
if the MAC cannot resume and the network device must be closed instead")
ends up with the PHY still advertising only the lowest common speed, and
with the original advertisement left in phydev->adv_old where phylink will
never consume it.

Should the new documentation state that the MAC has to call
phylink_speed_up() itself in this case, or should phylink_stop() reverse
the speed-down on these paths?

The only in-tree user of wol_phy_speed_ctrl is stmmac, and stmmac_open()
calls phylink_speed_up() unconditionally, so the next open repairs the
residue in the common case.  The residue looks like it can become permanent
if another speed-down runs first, since stmmac_release() also calls
phylink_speed_down() when device_may_wakeup(priv->device) is true:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_release() {
	...
	if (device_may_wakeup(priv->device))
		phylink_speed_down(priv->phylink, false);
	...
}

and phy_speed_down() overwrites the saved advertisement each time:

drivers/net/phy/phy.c:phy_speed_down() {
	...
	linkmode_copy(phydev->adv_old, adv_tmp);
	...
}

With no intervening phylink_speed_up(), would the second phy_speed_down()
store the already reduced set into phydev->adv_old, so the later
phylink_speed_up() restores the reduced advertisement rather than the
configured one?  Note device_may_wakeup(priv->device) can be true while
pl->wolopts_mac is zero, since stmmac calls
device_set_wakeup_capable(priv->device, 1) when plat->pmt is set.

>  	if (pl->sfp_bus)
>  		sfp_upstream_stop(pl->sfp_bus);
>  	if (pl->phydev)
> @@ -2512,6 +2518,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);
> +	}
> +

[ ... ]

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