Re: [PATCH net-next v10 02/11] net: phylink: introduce internal phylink PCS handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] phylink_validate_mac_and_pcs() incorrectly validates unsupported interfaces when a required PCS is missing.
--

--- Patch [2]: [PATCH net-next v10 02/11] net: phylink: introduce internal phylink PCS handling ---
commit 523b36754a7978492b9869c650925bee50a978c5
Author: Christian Marangi <ansuelsmth@gmail.com>

net: phylink: introduce internal phylink PCS handling

Introduce internal handling of PCS for phylink. This is an alternative
way to .mac_select_pcs that moves the selection logic of the PCS entirely
to phylink with the usage of the supported_interface value in the PCS
struct.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 3464e3ec56997..a5e2d342a92e1 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: Medium]
If the interface requires a dedicated PCS (the test_bit check is true)
but no matching PCS is found in the list, pcs_found will remain false.
Does this inadvertently skip PCS validation and fall through to MAC
validation?

It appears this allows phylink to incorrectly report that an interface
mode is supported even if the mandatory PCS is missing. Any subsequent
attempt to configure the link with this mode will then fail at runtime
in phylink_major_config(), which correctly aborts if the required PCS
isn't found. 

Should phylink_validate_mac_and_pcs() return -EINVAL when a required 
PCS is missing?

>  		/* The PCS, if present, must be setup before phylink_create()
>  		 * has been called. If the ops is not initialised, print an
>  		 * error and backtrace rather than oopsing the kernel.

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