Re: [PATCH net-next v16 02/12] net: phylink: introduce internal phylink PCS handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] net: phylink_validate_mac_and_pcs() fails to return error when required PCS is missing
- [Medium] net: phylink_create() unsafely expands MAC supported interfaces with generic PCS capabilities
--

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

net: phylink: introduce internal phylink PCS handling

This commit introduces internal handling of PCS for phylink, moving the PCS
selection logic entirely to phylink using the supported_interface value in
the PCS struct. This provides an alternative to the .mac_select_pcs callback.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index d67a5184c524f..417d2f1e54b72 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -530,6 +549,21 @@ static int phylink_validate_mac_and_pcs(struct phylink *pl,
>  		pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface);
>  		if (IS_ERR(pcs))
>  			return PTR_ERR(pcs);
> +	/*
> +	 * Find a PCS in available PCS list for the requested interface.
> +	 *
> +	 * Skip searching if the MAC doesn't require a dedicated PCS for
> +	 * the requested interface.
> +	 */
> +	} else if (test_bit(state->interface, pl->config->pcs_interfaces)) {
> +		struct phylink_pcs *tmp;
> +
> +		list_for_each_entry(tmp, &pl->pcs_list, list) {
> +			if (!phylink_validate_pcs_interface(tmp, state->interface)) {
> +				pcs = tmp;
> +				break;
> +			}
> +		}
>  	}
>  
>  	if (pcs) {

[Severity: Medium]
If the requested interface requires a dedicated PCS (the pcs_interfaces bit
is set), but the loop finishes without finding a matching PCS in the
pcs_list, pcs will remain NULL. Should this code return an error here
rather than falling through?

If it doesn't return an error, phylink might falsely advertise unsupported
link modes during auto-negotiation, and later phylink_major_config() could
attempt to configure this mode, fail to find the required PCS, and fatally
abort configuration.

[ ... ]

> @@ -2031,16 +2150,31 @@ 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);
>  
> +	/* Fill the PCS list with available PCS from phylink config */
> +	ret = phylink_fill_available_pcs(pl, config);
> +	if (ret < 0)
> +		goto free_pl;
> +
> +	/* Link available PCS to phylink */
> +	list_for_each_entry(pcs, &pl->pcs_list, list)
> +		pcs->phylink = pl;
> +
>  	phy_interface_copy(pl->supported_interfaces,
>  			   pl->config->supported_interfaces);
>  
> +	/* Update supported interfaces */
> +	list_for_each_entry(pcs, &pl->pcs_list, list)
> +		phy_interface_or(pl->supported_interfaces,
> +				 pl->supported_interfaces,
> +				 pcs->supported_interfaces);
> +

[Severity: Medium]
Does this safely handle MAC capabilities, or could it incorrectly expand
the MAC's supported interfaces with generic PCS capabilities?

Generic PCS blocks often support interface modes that the specific MAC or
board trace routing does not. By ORing in all interfaces supported by the
attached PCS modules, this might cause phylink_validate() to incorrectly
accept unsupported interface modes (since MAC drivers often only check
speed/duplex in their mac_capabilities).

>  	linkmode_fill(pl->supported);
>  	linkmode_copy(pl->link_config.advertising, pl->supported);
>  	phylink_validate(pl, pl->supported, &pl->link_config);

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