Re: [PATCH v14 02/12] net: phylink: introduce internal phylink PCS handling

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] phylink_create() unconditionally expands the MAC's supported interfaces with all PCS-supported interfaces, enabling unsupported MAC hardware modes.
- [Medium] phylink_validate_mac_and_pcs() fails to reject interfaces that require a dedicated PCS when no matching PCS is found.
--

--- Patch [2]: [PATCH v14 02/12] net: phylink: introduce internal phylink PCS handling ---
commit 6086d011ffb72e143c6102bee959c780b432bd7a
Author: Christian Marangi <[email protected]>

net: phylink: introduce internal phylink PCS handling

This commit introduces internal handling of PCS for phylink by allowing
MACs to provide a list of available PCS via phylink_config. It parses
this array and links the PCS list to the phylink instance. phylink
validation and configuration functions are updated to use this PCS list
instead of relying exclusively on .mac_select_pcs.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index e1e9f07f51bc6..6204e3bba16d7 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c

[ ... ]

> @@ -530,9 +550,24 @@ 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);
> +
> +		pcs_found = !!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)) {
> +		list_for_each_entry(pcs, &pl->pcs_list, list) {
> +			if (!phylink_validate_pcs_interface(pcs, state->interface)) {
> +				pcs_found = true;
> +				break;
> +			}
> +		}
>  	}
>  
> -	if (pcs) {
> +	if (pcs_found) {

[Severity: Medium]
Should there be an explicit error return here if a dedicated PCS is
required but none is found?

If pcs_found remains false after the loop in phylink_validate_mac_and_pcs(),
the code skips the validation block without returning -EINVAL.

This allows the unsupported mode to pass MAC validation, which later fails
during phylink_major_config().

[ ... ]

> @@ -1908,16 +2028,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: High]
Does this bitwise OR operation inadvertently expand the system's supported
interfaces beyond the MAC's physical capabilities?

By injecting all PCS-supported interfaces into pl->supported_interfaces,
generic PCS modules might expose high-speed modes that the attached MAC
does not actually support. Could this lead to the MAC driver receiving
configuration requests for unhandled interface types?

A similar capability expansion also appears to happen dynamically in
pcs_provider_notify() when a PCS provider is added:

    /* 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);

Is this intended?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2