Re: [PATCH net-next v20 09/15] net: phylink: support late PCS provider attach

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] net: phylink: broken MLO_AN_FIXED and MLO_AN_INBAND recovery in
  phylink_resolve()
- [Medium] net: phylink: capability mask corruption in
  phylink_calculate_capabilities()
- [Medium] net: phylink: data race on linkmode bitmaps in
  phylink_create()
- [Low] net: pcs: inaccurate Returns kernel-doc for
  fwnode_pcs_get_from_provider()
--

commit 483dcebdc3997073778f57fa52e0f5ffb3f2eaf9
Author: Christian Marangi <ansuelsmth@gmail.com>

net: phylink: support late PCS provider attach

This commit adds the FWNODE_PCS_PROVIDER_ADD notifier event and
fwnode_pcs_get_from_provider() helper so phylink instances can attach
PCS providers that register after phylink creation and re-trigger link
resolution.

> diff --git a/drivers/net/pcs/pcs.c b/drivers/net/pcs/pcs.c
> index 25b3248e8b93b..fef560eddf2a9 100644
> --- a/drivers/net/pcs/pcs.c
> +++ b/drivers/net/pcs/pcs.c
> @@ -69,6 +69,10 @@ fwnode_pcs_add_provider(struct fwnode_handle *fwnode,
>  
>  	mutex_unlock(&fwnode_pcs_mutex);
>  
> +	blocking_notifier_call_chain(&fwnode_pcs_notify_list,
> +				     FWNODE_PCS_PROVIDER_ADD,
> +				     pp);
> +
>  	pr_debug("Added pcs provider from %pfwf\n", fwnode);
>  
>  	return pp;

[Severity: Medium]
Can firing FWNODE_PCS_PROVIDER_ADD here cause a data race on
pl->supported, pl->link_config.advertising, and pl->supported_interfaces
if a PCS provider registers while phylink_create() or
phylink_validate_phy() is running on another CPU?

phylink_create() registers pcs_provider_notify() before setup is finished
and releases pl->state_mutex before calling phylink_parse_mode() and
phylink_parse_fixedlink() (and phylink_create() is called without
rtnl_lock):

CPU0                                  CPU1
phylink_create()
  register_fwnode_pcs_notifier()
  ...
  mutex_unlock(&pl->state_mutex)
                                      fwnode_pcs_add_provider()
                                        ...
                                        pcs_provider_notify()
                                          mutex_lock(&pl->state_mutex)
                                          phylink_calculate_capabilities()
                                            linkmode_fill(pl->supported)
  phylink_parse_mode()
    phylink_set(pl->supported, ...)  <-- data race

phylink_validate_phy() similarly reads pl->supported_interfaces outside
pl->state_mutex before phylink_bringup_phy() acquires the lock, racing
with pcs_provider_notify() updating pl->supported_interfaces.

[ ... ]

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index e807fcccb5de9..b2d5ced432091 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c

[ ... ]

> @@ -2127,20 +2151,42 @@ static int pcs_provider_notify(struct notifier_block *self,
>  	struct fwnode_pcs_provider *pp = data;
>  	struct phylink_pcs *pcs, *tmp;
>  	bool resolve = false;
> +	int count, i;
>  
>  	rtnl_lock();
>  	mutex_lock(&pl->state_mutex);
>  
> -	/*
> -	 * Loop all the PCS for phylink instance and check if
> -	 * this notification is relevant for some of them.
> -	 */
> -	list_for_each_entry_safe(pcs, tmp, &pl->pcs_list, list) {
> -		if (!fwnode_pcs_matches_provider(pp, pl->fwnode, pcs))
> -			continue;
> +	switch (val) {
> +	case FWNODE_PCS_PROVIDER_ADD:
> +		count = fwnode_phylink_pcs_count(pl->fwnode);
> +		for (i = 0; i < count; i++) {
> +			pcs = fwnode_pcs_get_from_provider(pp, pl->fwnode, i);
> +			if (IS_ERR_OR_NULL(pcs))
> +				continue;
> +
> +			/* Trigger a resolve only if a PCS is actually added */
> +			if (phylink_add_pcs(pl, pcs))
> +				resolve = true;
> +		}
> +
> +		/* Force an interface reconfig if major config fail */
> +		if (resolve && pl->major_config_failed)
> +			pl->force_major_config = true;

[Severity: High]
Does setting pl->force_major_config here actually recover MLO_AN_FIXED and
MLO_AN_INBAND links when phylink_major_config() previously failed during
phylink_start()?

When phylink_start() runs before the PCS provider is registered,
phylink_major_config() fails and returns before phylink_pcs_neg_mode()
can set pl->act_link_an_mode, leaving pl->act_link_an_mode at 0
(MLO_AN_PHY):

drivers/net/phy/phylink.c:phylink_major_config() {
	...
		if (!pcs) {
			phylink_err(pl,
				    "couldn't find a PCS for %s\n",
				    phy_modes(state->interface));

			pl->major_config_failed = true;
			return;
		}
	}

	pcs_changed = pl->pcs != pcs;

	phylink_pcs_neg_mode(pl, pcs, state->interface, state->advertising);
	...
}

When the PCS provider later attaches and phylink_resolve() runs,
phylink_resolve() selects the link_state source using pl->act_link_an_mode
before calling phylink_major_config():

drivers/net/phy/phylink.c:phylink_resolve() {
	...
	} else if (pl->act_link_an_mode == MLO_AN_FIXED) {
		phylink_get_fixed_state(pl, &link_state);
		mac_config = link_state.link;
	} else if (pl->act_link_an_mode == MLO_AN_PHY) {
		link_state = pl->phy_state;
		mac_config = link_state.link;
	} else {
		phylink_mac_pcs_get_state(pl, &link_state);
	...
	if ((mac_config && link_state.interface != pl->link_config.interface) ||
	    pl->force_major_config) {
		...
		phylink_major_config(pl, false, &link_state);
		pl->link_config.interface = link_state.interface;
		pl->force_major_config = false;
	}
}

Because pl->act_link_an_mode is still MLO_AN_PHY, phylink_resolve() copies
the unpopulated pl->phy_state (where link is false and advertising is
zero) into link_state.  For MLO_AN_INBAND 802.3z links without a PHY,
phylink_pcs_neg_mode() sees ETHTOOL_LINK_MODE_Autoneg_BIT cleared in
state->advertising, selects PHYLINK_PCS_NEG_INBAND_DISABLED, and programs
an empty advertisement into the PCS.  For MLO_AN_FIXED links without
polling or a link GPIO interrupt, link_state.link is false,
pl->force_major_config is cleared, and the fixed link stays down forever.

Even when pl->act_link_an_mode is already MLO_AN_INBAND (such as after
FWNODE_PCS_PROVIDER_DEL followed by FWNODE_PCS_PROVIDER_ADD),
phylink_resolve() calls phylink_mac_pcs_get_state() while pl->pcs is still
NULL, before phylink_major_config() attaches and enables the new PCS, so a
non-polled PCS is never queried for its link state after attachment.

> -		phylink_del_pcs(pl, pcs);
> -		resolve = true;
> +		break;
> +	case FWNODE_PCS_PROVIDER_DEL:
> +		/*
> +		 * Loop all the PCS for phylink instance and check if
> +		 * this notification is relevant for some of them.
> +		 */
> +		list_for_each_entry_safe(pcs, tmp, &pl->pcs_list, list) {
> +			if (!fwnode_pcs_matches_provider(pp, pl->fwnode, pcs))
> +				continue;
> +
> +			phylink_del_pcs(pl, pcs);
> +			resolve = true;
> +		}
> +		break;
>  	}
>  
>  	/* Exit early if nothing has changed */

[Severity: Medium]
Does calling phylink_calculate_capabilities(pl) below this check in
pcs_provider_notify() overwrite the mode-, PHY-, and SFP-restricted
capability masks in pl->supported and pl->link_config.advertising?

After phylink_calculate_capabilities() runs in phylink_create(),
phylink_parse_mode(), phylink_parse_fixedlink(), phylink_bringup_phy(),
and phylink_sfp_set_config() narrow pl->supported and
pl->link_config.advertising to the active link mode, attached PHY, or SFP
module without updating pl->advertising.  When pcs_provider_notify() later
calls phylink_calculate_capabilities(), it refills pl->supported with all
ones and validates only against the raw MAC and PCS capabilities, wiping
out those restrictions:

drivers/net/phy/phylink.c:phylink_calculate_capabilities() {
	...
	/* Get user-disabled advertising modes */
	linkmode_xor(disabled_advertising, pl->advertising,
		     pl->link_config.advertising);

	/* Calculate the full supported advertisement */
	linkmode_fill(pl->supported);
	linkmode_copy(pl->link_config.advertising, pl->supported);
	__phylink_validate(pl, pl->supported, &pl->link_config);
	...
}

In addition, if pl->link_config.interface is only supported by a PCS that
has not attached yet (or was just removed), __phylink_validate() returns
-EINVAL early when test_bit(state->interface, interfaces) fails, leaving
pl->supported and pl->advertising filled with all ones and causing
phylink_parse_mode() or phylink_validate_phy() to fail with -EINVAL before
the PCS provider can attach.

[ ... ]

> diff --git a/include/linux/pcs/pcs.h b/include/linux/pcs/pcs.h
> index 83e0a2ad4b099..00a636c82c65f 100644
> --- a/include/linux/pcs/pcs.h
> +++ b/include/linux/pcs/pcs.h

[ ... ]

> +/**
> + * fwnode_pcs_get_from_provider() - Retrieve a PCS from a specific provider
> + * @provider: PCS provider to use
> + * @fwnode: firmware node
> + * @index: index fwnode PCS handle in firmware node
> + *
> + * Get a PCS from the firmware node at index specifically provided by
> + * passed PCS provider.
> + *
> + * Unlike fwnode_pcs_get(), this function does not search the global list of
> + * PCS providers. The caller must provide the provider responsible for the
> + * referenced PCS.
> + *
> + * Returns: a pointer to the phylink_pcs or a negative error pointer. Can
> + * return -ENODEV if the PCS is not present in global providers list (either
> + * due to driver still needs to be probed or it failed to probe/removed) or
> + * can return -EINVAL if the PCS provider doesn't expose any PCS for the fwnode.
> + */

[Severity: Low]
This isn't a bug, but the Returns description says
fwnode_pcs_get_from_provider() can return -ENODEV if the PCS is not
present in the global providers list, which contradicts the preceding
paragraph and the implementation in drivers/net/pcs/pcs.c (which only
checks the passed provider and never searches fwnode_pcs_providers).

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