Re: [PATCH net-next v16 06/12] net: phylink: support late PCS provider attach

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] phylink: missing pl->supported update on late PCS attach
- [Medium] phylink: data race on pl->supported_interfaces during PCS hotplug
--

commit 09779c4042c55422798d0cff65f1f25068da26a9
Author: Christian Marangi <ansuelsmth@gmail.com>
Subject: net: phylink: support late PCS provider attach

This commit adds support for late PCS provider attachment to a phylink
instance. It introduces a FWNODE_PCS_PROVIDER_ADD event that allows a
PCS provider to be linked and supported interfaces to be refreshed if the
provider is added after the instance is created. If a link was previously
down, it can be re-established when the PCS returns.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 88570a59ba9e7..fb3fb522769b6 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -2006,27 +2027,57 @@ 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();
> +	/*
> +	 * On PCS provider deletion hold rtnl lock as one of
> +	 * PCS can be currently in use by the phylink instance
> +	 * and ethtool OPs can reference it.
> +	 */
> +	if (val == FWNODE_PCS_PROVIDER_DEL)
> +		rtnl_lock();

[Severity: Medium]
Is it safe to conditionally skip rtnl_lock() on the FWNODE_PCS_PROVIDER_ADD
path?

Later in this function, pl->supported_interfaces is modified:

	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);

Functions like phylink_validate_mask() read this multi-word bitmap
word-by-word while holding only rtnl_lock, without acquiring pl->state_mutex.
Could this lead to a torn read, causing concurrent ethtool operations to see
an invalid interface mask and reject valid configurations?

[ ... ]
>  	/* Exit early if nothing has changed */
>  	if (!resolve) {
>  		mutex_unlock(&pl->state_mutex);
> -		rtnl_unlock();
> +
> +		if (val == FWNODE_PCS_PROVIDER_DEL)
> +			rtnl_unlock();
> +
>  		return NOTIFY_DONE;
>  	}
>  
> @@ -2040,7 +2091,8 @@ static int pcs_provider_notify(struct notifier_block *self,
>  
>  	mutex_unlock(&pl->state_mutex);
>  
> -	rtnl_unlock();
> +	if (val == FWNODE_PCS_PROVIDER_DEL)
> +		rtnl_unlock();
>  
>  	phylink_run_resolve(pl);

[Severity: Medium]
When a late PCS is attached, does pl->supported need to be recalculated
alongside pl->supported_interfaces?

If a late PCS provides new interface capabilities (such as new 10G link
modes), updating pl->supported_interfaces without also recalculating
pl->supported (via phylink_validate()) could leave the ethtool capabilities
stale. Could this result in phylink_ethtool_ksettings_set() rejecting valid
configurations?

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