Re: [PATCH net-next v20 03/15] net: phylink: introduce internal phylink PCS handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] net: phylink: missing error return in
  phylink_validate_mac_and_pcs()
- [Medium] net: phylink: broken PCS interface filtering in
  phylink_create()
--

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

net: phylink: introduce internal phylink PCS handling

This commit introduces internal PCS tracking and selection in phylink via a
per-instance PCS list populated through .fill_available_pcs, as an
alternative to the .mac_select_pcs callback.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 51c147eb8c2b6..35bfdffc86ba8 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -535,6 +554,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;
> +			}
> +		}
>  	}

[Severity: Medium]
Should phylink_validate_mac_and_pcs() return -EINVAL when state->interface is
set in pl->config->pcs_interfaces but no matching PCS is found in
pl->pcs_list?

When no PCS in pl->pcs_list supports state->interface, pcs remains NULL and
phylink_validate_mac_and_pcs() skips the if (pcs) block, validating the mode
against MAC capabilities alone and returning 0.  Unsupported PCS-dependent
interface modes then pass validation during phylink_create() or SFP/PHY
setup, only to fail later in phylink_major_config() when !pcs sets
pl->major_config_failed = true.

Also, this isn't a bug, but the pl->pcs_list lookup loop is duplicated across
phylink_validate_mac_and_pcs(), phylink_inband_caps(), and
phylink_major_config() and could be moved into a helper.

[ ... ]

> @@ -2096,16 +2220,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)
> +		rcu_assign_pointer(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]
As reported by Maxime Chevallier on v7 [1], does ORing
pcs->supported_interfaces into pl->supported_interfaces leave unsupported
PCS modes enabled while adding modes the MAC itself does not support?

Any mode in config->pcs_interfaces that is also in
config->supported_interfaces stays set in pl->supported_interfaces even when
no PCS in pl->pcs_list supports it.  Conversely, if a PCS advertises an
interface mode in pcs->supported_interfaces that the MAC does not support in
config->supported_interfaces or config->pcs_interfaces, phy_interface_or()
adds it to pl->supported_interfaces, and phylink then treats it as a native
MAC mode requiring no PCS.  Callers inspecting pl->supported_interfaces
such as phylink_validate_phy() and SFP interface selection will therefore
see unsupported modes as valid.

[1] https://lore.kernel.org/r/3bbacda3-4225-4536-a4b4-3aa31a47a3aa@bootlin.com

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