Re: [PATCH net-next v11 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_validate_mac_and_pcs() fails to return an error when a required PCS is missing, leading to subsequent fatal errors.
- [Medium] Accidental deletion of MAC supported_interfaces validation in phylink_create().
--

--- Patch [2]: [PATCH net-next v11 02/12] net: phylink: introduce internal phylink PCS handling ---
commit 73e85e119d9228a55571bdc2d1bafc5c5ee4b1fa
Author: Christian Marangi <[email protected]>

net: phylink: introduce internal phylink PCS handling
    
This commit introduces internal handling of PCS for phylink using a
linked list of available PCS instead of relying solely on the
.mac_select_pcs callback. It updates major configuration and validation
paths to search the available PCS list when the MAC requires a dedicated
PCS.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index b7180ae76cc84..9decd7333c702 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c

[ ... ]

> @@ -530,9 +552,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: High]
If a required PCS is not found in the list, pcs_found remains false,
but doesn't phylink_validate_mac_and_pcs() then fall through and continue
to validate MAC capabilities instead of immediately returning an error like
-EINVAL?

Later in phylink_major_config(), trying to apply this configuration
could fail fatally and leave the link in a broken state.

[ ... ]

> @@ -1854,13 +1964,17 @@ struct phylink *phylink_create(struct phylink_config *config,
>  			       phy_interface_t iface,
>  			       const struct phylink_mac_ops *mac_ops)
>  {
> +	struct phylink_pcs *tmp, *pcs;
>  	struct phylink *pl;
>  	int ret;
>  
> -	/* Validate the supplied configuration */
> -	if (phy_interface_empty(config->supported_interfaces)) {
> +	/*
> +	 * Make sure either PCS internal validation or .mac_select_pcs
> +	 * is used. Return error if both are defined.
> +	 */
> +	if (config->num_possible_pcs && mac_ops->mac_select_pcs) {
>  		dev_err(config->dev,
> -			"phylink: error: empty supported_interfaces\n");
> +			"phylink: error: either phylink_config .num_possible_pcs or .mac_select_pcs must be used\n");
>  		return ERR_PTR(-EINVAL);
>  	}

[Severity: Medium]
Was the validation check for an empty supported_interfaces mask intentionally
removed here?

Without it, it looks like misconfigured MAC drivers might initialize
successfully but fail silently later.

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