Re: [PATCH net-next v9 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 4 potential issue(s) to consider:
- [High] Unconditional call to phylink_link_down() without updating old_link_state leads to multiple mac_link_down() calls and state machine desynchronization.
- [Medium] The phylink instance's supported ethtool link modes and advertising mask are not updated after a PCS is removed.
- [Medium] Polling timer is not stopped when the active polling PCS is released, causing a runaway timer.
- [Low] Reusing the function parameter 'pcs' as a loop iterator variable violates C scoping best practices and kernel coding standards.
--

--- Patch [3]: [PATCH net-next v9 03/12] net: phylink: add phylink_release_pcs() to externally release a PCS ---
commit 3d6c63e283b60421054ef189e8d1a2deb1f29536
Author: Christian Marangi <ansuelsmth@gmail.com>

net: phylink: add phylink_release_pcs() to externally release a PCS
    
Add phylink_release_pcs() to externally release a PCS from a phylink
instance. This can be used to handle case when a single PCS needs to be
removed and the phylink instance needs to be refreshed.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index d96941d4c5235..986e0c1e3d678 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -917,6 +919,60 @@ static void phylink_resolve_an_pause(struct phylink_link_state *state)
[ ... ]
> +void phylink_release_pcs(struct phylink_pcs *pcs)
> +{
> +	struct phylink *pl;
> +
> +	ASSERT_RTNL();
> +
> +	pl = pcs->phylink;
> +	if (!pl)
> +		return;
> +
> +	mutex_lock(&pl->state_mutex);
> +
> +	list_del(&pcs->list);
> +	pcs->phylink = NULL;
> +
> +	/*
> +	 * 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;
> +		pl->pcs = NULL;
> +	}

[Severity: High]
Does calling phylink_link_down() here without also updating pl->old_link_state
cause state machine desynchronization?

If pl->old_link_state is true when the PCS is removed (such as for DSA switch
ports where there is no netdev), the link is logically brought down, but
pl->old_link_state remains true. This could cause subsequent link-up events to
be ignored because phylink thinks the link is already up, leaving the
interface permanently dead.

[Severity: Medium]
When the active PCS has polling enabled (pcs->poll is true), does clearing
pl->pcs and triggering a resolve leave a runaway timer?

If phylink_major_config() is called by the resolve task and no replacement PCS
is found, it may return early on error and bypass the phylink_pcs_poll_stop()
call. Could this cause the original timer to continue firing and re-arming
itself indefinitely?

> +
> +	mutex_unlock(&pl->state_mutex);
> +
> +	/* Refresh supported interfaces */
> +	phy_interface_copy(pl->supported_interfaces,
> +			   pl->config->supported_interfaces);
> +	list_for_each_entry(pcs, &pl->pcs_list, list)
> +		phy_interface_or(pl->supported_interfaces,
> +				 pl->supported_interfaces,
> +				 pcs->supported_interfaces);

[Severity: Low]
This isn't a bug, but does reusing the function parameter pcs as the list
iterator variable overwrite its pointer value?

When the loop terminates, pcs will point to an invalid structure offset
derived from the list head. Using a separate iterator variable would avoid
corrupting the parameter.

[Severity: Medium]
While pl->supported_interfaces is updated here, is it also necessary to
update the phylink instance's supported ethtool link modes and advertising
mask?

Without calling phylink_validate() or similar to update pl->supported and
pl->link_config.advertising, these masks might continue to advertise the
capabilities of the removed PCS. Could this lead to runtime connection
failures if userspace or the PHY attempts to negotiate these now-unsupported
modes?

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