Re: [PATCH net-next v16 03/12] net: pcs: implement Firmware node support for PCS driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: pcs: fwnode_pcs_get clobbers xlate errors breaking probe deferral
--

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

net: pcs: implement Firmware node support for PCS driver

Implements the foundation of Firmware node support for PCS drivers. This
provides a simple Provider API where a PCS driver can expose multiple PCS
with a fwnode_xlate function.

> diff --git a/drivers/net/pcs/pcs.c b/drivers/net/pcs/pcs.c
> new file mode 100644
> index 0000000000000..39550d9a12a2a
> --- /dev/null
> +++ b/drivers/net/pcs/pcs.c

[ ... ]

> +static struct phylink_pcs *
> +__fwnode_pcs_get_from_pcsspec_provider(struct fwnode_reference_args *pcsspec,
> +				       struct fwnode_pcs_provider *provider)
> +{
> +	if (provider->fwnode != pcsspec->fwnode)
> +		return ERR_PTR(-EINVAL);
> +
> +	return provider->fwnode_xlate(pcsspec, provider->data);
> +}
> +
> +static struct phylink_pcs *
> +fwnode_pcs_get_from_pcsspec(struct fwnode_reference_args *pcsspec)
> +{
> +	struct fwnode_pcs_provider *provider;
> +	struct phylink_pcs *pcs = NULL;
> +
> +	if (!pcsspec)
> +		return ERR_PTR(-EINVAL);
> +
> +	mutex_lock(&fwnode_pcs_mutex);
> +	list_for_each_entry(provider, &fwnode_pcs_providers, link) {
> +		pcs = __fwnode_pcs_get_from_pcsspec_provider(pcsspec, provider);
> +		if (!IS_ERR(pcs))
> +			break;
> +	}

[Severity: High]
Does this loop clobber errors returned by the matching provider's xlate
function?

Looking at __fwnode_pcs_get_from_pcsspec_provider(), it returns -EINVAL if
the fwnode doesn't match, or the result of fwnode_xlate() if it does. If
fwnode_xlate() returns an error like -EPROBE_DEFER, !IS_ERR(pcs) evaluates
to false, causing the loop to continue.

The original error is then overwritten by -EINVAL from subsequent
non-matching providers. Should the loop break if the provider matches the
fwnode, regardless of whether the returned pcs is an error?

> +	mutex_unlock(&fwnode_pcs_mutex);
> +
> +	return !IS_ERR_OR_NULL(pcs) ? pcs : ERR_PTR(-ENODEV);

[Severity: High]
Does this unconditional return of -ENODEV squash probe deferrals?

If pcs holds an error pointer (like -EPROBE_DEFER) at the end of the loop,
this statement masks the original error code by always returning -ENODEV.

This breaks module load ordering and prevents MAC drivers from correctly
deferring probe until their PCS is ready.

> +}

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