Re: [PATCH net-next v15 01/12] net: phylink: keep and use MAC supported_interfaces in phylink struct

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Destructive masking of `pl->sfp_interfaces` permanently loses SFP capabilities, breaking dynamic PCS addition.
- [High] Dynamic PCS addition does not update `pl->supported` ethtool linkmodes, causing userspace to see stale capabilities.
--

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

net: phylink: keep and use MAC supported_interfaces in phylink struct

This patch adds a copy of supported_interfaces from phylink_config into the
phylink struct and uses it instead of relying on the phylink_config value.
This prepares the driver to support internal PCS handling, where a PCS can
be added or removed dynamically after the phylink is created.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 3ec3bb4391095..7f7a7328124f2 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -1904,6 +1909,9 @@ struct phylink *phylink_create(struct phylink_config *config,
>  	__set_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state);
>  	timer_setup(&pl->link_poll, phylink_fixed_poll, 0);
>  
> +	phy_interface_copy(pl->supported_interfaces,
> +			   pl->config->supported_interfaces);
> +
>  	linkmode_fill(pl->supported);
>  	linkmode_copy(pl->link_config.advertising, pl->supported);
>  	phylink_validate(pl, pl->supported, &pl->link_config);

[Severity: High]
Since the goal of this patch is to support adding a PCS after phylink
creation, does this leave pl->supported with stale capabilities?

When a PCS is dynamically added, pl->supported_interfaces is updated,
but it doesn't appear that pl->supported is recalculated here or during
the update. Would this cause userspace ethtool to see incorrect or stale
supported linkmodes for the network device?

[ ... ]
> @@ -3758,14 +3766,14 @@ static int phylink_sfp_config_optical(struct phylink *pl)
>  
>  	phylink_dbg(pl, "optical SFP: interfaces=[mac=%*pbl, sfp=%*pbl]\n",
>  		    (int)PHY_INTERFACE_MODE_MAX,
> -		    pl->config->supported_interfaces,
> +		    pl->supported_interfaces,
>  		    (int)PHY_INTERFACE_MODE_MAX,
>  		    pl->sfp_interfaces);
>  
>  	/* Find the union of the supported interfaces by the PCS/MAC and
>  	 * the SFP module.
>  	 */
> -	phy_interface_and(pl->sfp_interfaces, pl->config->supported_interfaces,
> +	phy_interface_and(pl->sfp_interfaces, pl->supported_interfaces,
>  			  pl->sfp_interfaces);

[Severity: High]
Does this destructively mask pl->sfp_interfaces against the current
(potentially pre-PCS) supported_interfaces?

If an SFP module is probed and inserted before the dynamic PCS provider is
attached, this operation will permanently drop capabilities from
pl->sfp_interfaces. When the PCS is later added and supported_interfaces
expands, the SFP module will have already lost those capabilities, which
might cause it to fail to link or be restricted to a suboptimal interface.

[ ... ]
> @@ -3936,7 +3944,7 @@ static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
>  
>  	/* Set the PHY's host supported interfaces */
>  	phy_interface_and(phy->host_interfaces, phylink_sfp_interfaces,
> -			  pl->config->supported_interfaces);
> +			  pl->supported_interfaces);

[Severity: High]
Similarly, does this irreversibly limit the PHY's host_interfaces based
on the current supported_interfaces before the PCS is attached?

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