Re: [PATCH net-next v20 04/15] net: pcs: implement Firmware node support for PCS driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] net: pcs: fwnode_pcs_get_from_pcsspec() clobbers
  fwnode_xlate() error codes
- [Low] net: pcs: dead name lookup in fwnode_parse_pcsspec() uses
  pcs-names
--

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

net: pcs: implement Firmware node support for PCS driver

This commit introduces firmware node provider and consumer helpers for PCS
drivers, allowing MAC drivers to look up and parse PCS instances referenced
by "pcs-handle" properties.

> 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 int fwnode_parse_pcsspec(const struct fwnode_handle *fwnode,
> +				int index, const char *name,
> +				struct fwnode_reference_args *out_args)
> +{
> +	if (!fwnode)
> +		return -EINVAL;
> +
> +	if (name) {
> +		index = fwnode_property_match_string(fwnode, "pcs-names",
> +						     name);
> +		if (index < 0)
> +			return index;
> +	}
> +
> +	return fwnode_property_get_reference_args(fwnode, "pcs-handle",
> +						  "#pcs-cells", 0, index,
> +						  out_args);
> +}

[Severity: Low]
Should this match "pcs-handle-names" (as defined in
Documentation/devicetree/bindings/net/ethernet-controller.yaml) instead of
"pcs-names", or can the name lookup be dropped since __fwnode_pcs_get() is
only called with con_id set to NULL?

> +
> +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;
> +	}
> +	mutex_unlock(&fwnode_pcs_mutex);
> +
> +	return !IS_ERR_OR_NULL(pcs) ? pcs : ERR_PTR(-ENODEV);
> +}

[Severity: Medium]
Does fwnode_pcs_get_from_pcsspec() overwrite errors returned by
provider->fwnode_xlate() with ERR_PTR(-ENODEV)?

When the matching provider's fwnode_xlate() returns an error pointer (for
example ERR_PTR(-EINVAL) on invalid #pcs-cells arguments), !IS_ERR(pcs) is
false so the loop keeps iterating, and the return statement turns any
ERR_PTR(pcs) into ERR_PTR(-ENODEV).  Because fwnode_phylink_pcs_parse()
treats -ENODEV as an unprobed provider and skips it via continue, invalid
PCS specifiers are silently ignored instead of failing parsing.

Checking provider->fwnode == pcsspec->fwnode directly in the loop and
returning provider->fwnode_xlate(pcsspec, provider->data) (or
ERR_PTR(-ENODEV) if no provider matched) would preserve the error code.

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