Re: [PATCH net-next v17 05/12] net: phylink: support PCS provider release

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net: phylink: data race on pl->link_config.advertising in pcs_provider_notify()
- [Medium] net: phylink: manual ethtool advertising mask is destroyed on PCS hotplug
--

commit 54f6a8bb94be99e3cfebd46ca4bf449e65e57693
Author: Christian Marangi <ansuelsmth@gmail.com>

net: phylink: support PCS provider release

This commit adds support for releasing fwnode PCS from a PCS provider using a
global notifier. The notifier is handled by pcs_provider_notify(), which
iterates over phylink instances, deletes the matching PCS, and recalculates
capabilities.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 9441afe372c33..019ce3d89da04 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -1939,11 +1964,95 @@ static int phylink_fill_available_pcs(struct phylink *pl,
>  	}
>  
>  out:
> +	mutex_unlock(&pl->state_mutex);
> +
>  	kfree(pcss);
>  
>  	return ret;
>  }
>  
> +static void phylink_del_pcs(struct phylink *pl, struct phylink_pcs *pcs)
> +{
[ ... ]
> +static int pcs_provider_notify(struct notifier_block *self,
> +			       unsigned long val, void *data)
> +{
> +	struct phylink *pl = container_of(self, struct phylink, fwnode_pcs_nb);
> +	struct fwnode_pcs_provider *pp = data;
> +	struct phylink_pcs *pcs, *tmp;
> +	bool resolve = false;
> +
> +	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;
> +
> +		phylink_del_pcs(pl, pcs);
> +		resolve = true;
> +	}
> +
> +	/* Exit early if nothing has changed */
> +	if (!resolve) {
> +		mutex_unlock(&pl->state_mutex);
> +		rtnl_unlock();
> +		return NOTIFY_DONE;
> +	}
> +
> +	/* 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);
> +
> +	mutex_unlock(&pl->state_mutex);
> +
> +	/* Recalculate capabilities */
> +	linkmode_fill(pl->supported);
> +	linkmode_fill(pl->link_config.advertising);

[Severity: High]
Does this introduce a data race on pl->link_config.advertising?

pcs_provider_notify() drops pl->state_mutex before modifying
pl->link_config.advertising via linkmode_fill().

However, phylink_resolve() can execute concurrently and reads pl->link_config
by performing a full struct copy while holding only pl->state_mutex:

phylink_resolve() {
    ...
    mutex_lock(&pl->state_mutex);
    if (pl->phylink_disable_state) {
        ...
    } else if (pl->link_failed) {
        link_state = pl->link_config;
    ...
}

Could phylink_resolve() observe a torn state where all capability bits are
temporarily set to 1, causing an invalid configuration to be sent to the
MAC/PCS hardware?

[Severity: Medium]
Does unconditionally filling pl->link_config.advertising overwrite manual
ethtool settings?

When a PCS notification matches, pl->link_config.advertising is completely
reset using linkmode_fill(). This appears to discard any user-configured
manual speed or link advertisement limits applied previously via ethtool,
reverting the link to advertise the maximum supported capabilities.

Should hotplugging an unused or secondary PCS destructively alter the manual
configuration state of the active link?

> +	phylink_validate_mask(pl, NULL, pl->supported, &pl->link_config,
> +			      pl->supported_interfaces);
> +
> +	rtnl_unlock();
> +
> +	phylink_run_resolve(pl);
> +
> +	return NOTIFY_OK;
> +}
> +

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