Re: [PATCH net v4 03/15] net: phylink: allow stopping a suspended instance

[netdev-bot+sashiko]

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

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

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() now returns early whenever
  PHYLINK_DISABLE_STOPPED is set (drivers/net/phy/phylink.c:2536-2543).
- [Medium] The new MAC-WoL completion in phylink_stop()
  (phylink.c:2557-2565) calls phylink_link_down() only when…
- [Low] The only Fixes: tag is f97493657c63 ("net: phylink: add
  suspend/resume support", 2021).

--

net: phylink: allow stopping a suspended instance

This patch lets phylink_stop() shut down an instance that is still
suspended after a driver resume failed, without going through
phylink_resume(). When the instance is already stopped, it no longer
repeats PHY/SFP/PCS shutdown. It also completes the link-down that MAC WoL
deferred, and it tracks and undoes the PHY speed-down done at suspend time.

> Fixes: f97493657c63 ("net: phylink: add suspend/resume support")

[Severity: Low]
Is this Fixes: tag complete? Several of the things this patch fixes were
added after f97493657c63.

phylink_phy_pm_speed_ctrl() and the phylink_speed_down() call in
phylink_suspend() came from dc1a2a9ce5b2c8. The new suspend_speed_down
tracking and phylink_restore_suspend_speed() fix that commit.

The new MAC WoL block in phylink_stop() depends on suspend_link_up
handling, which came from 4c8925cb9db158.

The commit message gives phylink_prepare_resume() as the reason for the
new phy_suspend() call, and phylink_prepare_resume() came from
367f1854d442b3.

Should there be at least one more Fixes: tag, for dc1a2a9ce5b2c8? With
only f97493657c63, stable tooling may try to backport this to trees that
lack the state and functions the patch modifies.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 1bbcf46c83566..b7d30ca340316 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.
> + * 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;
> +	}

[Severity: Medium]
Is PHYLINK_DISABLE_STOPPED specific enough to identify a suspended
instance? phylink_create() also sets it:

drivers/net/phy/phylink.c:phylink_create() {
    ...
	__set_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state);
    ...
}

Every completed phylink_stop() also leaves it set. So this branch also
runs for an instance that was never started and for a second, unbalanced
phylink_stop(). In both cases it calls phy_suspend() without any warning.

Before this patch, a second phylink_stop() reached phy_stop() with the PHY
in PHY_HALTED, which hit:

	WARN(1, "called from state %s\n", phy_state_to_str(phydev->state));

Is it intended that phylink users lose this warning for unbalanced
start/stop?

The new kernel-doc says the PHY is suspended again only "If
phylink_prepare_resume() powered up an already stopped PHY". The code does
it for any stopped instance whose PHY is not suspended. That includes a
PHY powered up by phy_attach_direct() on an instance that was never
started.

A later patch in this series ("net: stmmac: track datapath and power
ownership across failed reopening") already relies on this broader
behaviour. Its __stmmac_open() calls phylink_stop() twice on an instance
that phylink_start() never started.

Should the commit message and kernel-doc describe this wider contract?

The early return also skips the new MAC WoL cleanup further down. If
phylink_suspend(pl, true) runs on an instance that is already stopped,
both bits end up set:

phylink_suspend(pl, true)
    __set_bit(PHYLINK_DISABLE_MAC_WOL, ...)
phylink_stop(pl)
    STOPPED already set -> phy_suspend(); return;
phylink_start(pl)
    phylink_enable_and_run_resolve(pl, PHYLINK_DISABLE_STOPPED)
        MAC_WOL is still set, so the resolver never runs

Can the link still come up after that phylink_start()? At this commit,
stmmac seems able to reach this sequence:

  - an MTU change reopen fails, leaving the interface up with phylink
    stopped
  - a MAC WoL system suspend
  - a failed resume
  - close, then open

> +
>  	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]
Does this still complete the deferred link-down if phylink_suspend() was
called twice with no phylink_resume() in between?

phylink_suspend() recomputes the flag on every MAC WoL suspend:

drivers/net/phy/phylink.c:phylink_suspend() {
    ...
		pl->suspend_link_up = phylink_link_is_up(pl);
		if (pl->suspend_link_up) {
			...
			if (pl->netdev)
				netif_carrier_off(pl->netdev);
			pl->old_link_state = false;
		}
    ...
}

The first suspend turns the carrier off and clears old_link_state, but
does not call mac_link_down(). While PHYLINK_DISABLE_MAC_WOL is set, the
resolver cannot turn the carrier back on.

Now suppose resume fails before phylink_resume() and the device stays
running. The next system suspend calls phylink_suspend(pl, true) again.

stmmac_suspend() at this commit does this, because it only checks
netif_running(). ucc_geth_suspend() does it after ucc_geth_init_mac()
fails in ucc_geth_resume().

That second call sets suspend_link_up to false, although mac_link_down()
has still not been called.

A later close reaches this block and skips phylink_link_down(), so neither
mac_link_down() nor phylink_deactivate_lpi() runs. PHYLINK_DISABLE_MAC_WOL
is still cleared.

Wouldn't that leave the earlier mac_link_up() without a matching
mac_link_down(), with pl->mac_enable_tx_lpi still true? The next
phylink_start() could then call mac_link_up() again with no link-down in
between.

[ ... ]

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