Re: [RFC PATCH net-next v8 03/12] net: phylink: add phylink_release_pcs() to externally release a PCS

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unconditional and unbalanced `phylink_link_down()` in `phylink_release_pcs()` causes duplicate `mac_link_down()` callbacks and corrupts the internal phylink state machine.
--

commit 2b29c68aa2ea3d8d4cc5979668a733bbdab7236c
Author: Christian Marangi <ansuelsmth@gmail.com>

net: phylink: add phylink_release_pcs() to externally release a PCS

This commit introduces phylink_release_pcs() to allow removing a PCS from a
phylink instance. When the released PCS is the active one, the link is
brought down and a major config is forced to trigger a reconfiguration.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index c38bcd43b8c8b..064d6f5a06da4 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c

[ ... ]

> @@ -918,6 +920,60 @@ static void phylink_resolve_an_pause(struct phylink_link_state *state)

[ ... ]

> +	/*
> +	 * Check if we are removing the PCS currently
> +	 * in use by phylink. If this is the case, tear down
> +	 * the link, force phylink resolve to reconfigure the
> +	 * interface mode, disable the current PCS and set the
> +	 * phylink PCS to NULL.
> +	 */
> +	if (pl->pcs == pcs) {
> +		phylink_link_down(pl);
> +		phylink_pcs_disable(pl->pcs);
> +
> +		pl->force_major_config = true;

[Severity: High]
Does this unconditional call to phylink_link_down() cause unbalanced
mac_link_down() callbacks if the link is already down?

Also, if the link was up, does pl->old_link_state need to be cleared here?

For PHYLINK_DEV instances that lack a netdev, phylink_link_is_up(pl) relies
entirely on pl->old_link_state. Because pl->old_link_state remains true, the
scheduled phylink_resolve() evaluates cur_link_state as true.

Inside phylink_resolve():

1. The force_major_config block sees cur_link_state == true and calls
phylink_link_down() a second time, then sets cur_link_state = false.

2. Later, the state update block compares link_state.link != cur_link_state
(false != false), evaluating to false. As a result, pl->old_link_state is
NOT updated and remains true permanently.

3. On the next phylink_resolve() run, cur_link_state again evaluates to
true. The state update block compares link_state.link != cur_link_state
(false != true), evaluating to true, and calls phylink_link_down(pl) a third
time.

Can this state machine corruption result in multiple unbalanced
mac_link_down() calls and cause clock underflows, regulator warnings, or
hardware lockups in MAC drivers?

> +		pl->pcs = NULL;
> +	}
> +
> +	mutex_unlock(&pl->state_mutex);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618125752.1223-1-ansuelsmth@gmail.com?part=3