[net,v4,03/15] net: phylink: allow stopping a suspended instance
Commit Message
If a network driver's system resume fails before phylink_resume(), the
network device can remain administratively up with phylink suspended.
Closing the interface then needs to terminate that suspended instance.
Calling phylink_resume() merely to make phylink_stop() work is not a
safe substitute: resume reconfigures the MAC and restarts link
resolution, although the driver has not successfully restored the MAC.
This is a missing suspend-to-stop transition, independent of the reason
hardware restoration failed. No MAC recovery policy belongs in phylink;
the driver still decides whether to retry resume or wait for an ordinary
administrative down/up cycle.
Without MAC Wake-on-LAN, phylink_suspend() has already called
phylink_stop(). Do not repeat PHY, SFP and PCS shutdown. However,
phylink_prepare_resume() may have powered that stopped PHY back up to
provide the receive clock for MAC reset. Suspend it again if WoL
permits, without repeating phy_stop() on a PHY which is already halted.
With MAC Wake-on-LAN, suspend deliberately defers mac_link_down() and
sets PHYLINK_DISABLE_MAC_WOL. Finish that deferred link-down, drain
resolution work and clear the WoL disable bit while retaining
PHYLINK_DISABLE_STOPPED. Otherwise a subsequent start cannot resolve the
link.
Also undo PHY speed control performed by phylink_suspend(). As Andrew
Lunn pointed out, phylink_start() does not restore the advertised speeds
that phylink_resume() normally restores. Track suspend-owned speed
control and restore the saved advertisement from either resume or
suspended stop, including when PHY shutdown has already completed.
An explicit driver speed-down request, such as stmmac's close-time power
saving, must remain in effect until its matching speed-up. Restore any
suspend-owned reduction before applying that request, so it cannot save
the reduced advertisement over the original one. The following stop must
not undo the driver's new reduction.
Document that a suspended instance can be stopped directly. This neither
resumes the PHY nor reconfigures or brings up the MAC.
Fixes: f97493657c63 ("net: phylink: add suspend/resume support")
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
drivers/net/phy/phylink.c | 51 ++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 48 insertions(+), 3 deletions(-)
Comments
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.
[ ... ]
@@ -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);
@@ -2657,8 +2695,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 +2738,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 +3655,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);