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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Type mismatch in `fwnode_parse_pcsspec()` where a negative error code is assigned to an unsigned integer, making the error check dead code.
- [Medium] `fwnode_phylink_pcs_parse()` incorrectly aborts on out-of-bounds instead of returning the parsed count, violating its 'up to num_pcs' contract.
- [Low] The fallback stub for `fwnode_pcs_get()` uses an incorrect parameter type.
--

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

net: pcs: implement Firmware node support for PCS driver

This commit implements the foundation of Firmware node support for PCS
drivers. It introduces a simple Provider API where a PCS driver can expose
multiple PCS and consumers can access them using fwnode_pcs_get().

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

[ ... ]

> +static int fwnode_parse_pcsspec(const struct fwnode_handle *fwnode,
> +				unsigned int index, const char *name,
> +				struct fwnode_reference_args *out_args)
> +{
> +	int ret;
> +
> +	if (!fwnode)
> +		return -EINVAL;
> +
> +	if (name) {
> +		index = fwnode_property_match_string(fwnode, "pcs-names",
> +						     name);
> +		if (index < 0)
> +			return index;

[Severity: Medium]
Does this error check work as intended? fwnode_property_match_string()
returns negative error codes like -ENODATA, but it is assigned to index
which is an unsigned int. The check for index < 0 will be dead code,
potentially leading to an out-of-bounds query that returns -ENOENT.

> +	}
> +
> +	ret = fwnode_property_get_reference_args(fwnode, "pcs-handle",
> +						 "#pcs-cells",
> +						 -1, index, out_args);

[ ... ]

> +int fwnode_phylink_pcs_parse(struct fwnode_handle *fwnode,
> +			     struct phylink_pcs **available_pcs,
> +			     unsigned int num_pcs)
> +{
> +	unsigned int i, found = 0;
> +
> +	if (!available_pcs)
> +		return -EINVAL;
> +
> +	if (!fwnode_property_present(fwnode, "pcs-handle"))
> +		return -ENODEV;
> +
> +	for (i = 0; i < num_pcs; i++) {
> +		struct phylink_pcs *pcs;
> +
> +		pcs = fwnode_pcs_get(fwnode, i);
> +		if (IS_ERR(pcs)) {
> +			/*
> +			 * Ignore -ENODEV error for PCS that still
> +			 * needs to probe.
> +			 */
> +			if (PTR_ERR(pcs) == -ENODEV)
> +				continue;
> +
> +			return PTR_ERR(pcs);

[Severity: Medium]
If the firmware node contains fewer pcs-handle properties than num_pcs,
won't fwnode_pcs_get() eventually return -ENOENT? Since this loop only
ignores -ENODEV, does this cause the function to abort entirely and return
an error instead of returning the successfully parsed count?

> +		}
> +
> +		available_pcs[found] = pcs;
> +		found++;
> +	}
> +
> +	return found;
> +}
> +EXPORT_SYMBOL_GPL(fwnode_phylink_pcs_parse);

[ ... ]

> diff --git a/include/linux/pcs/pcs.h b/include/linux/pcs/pcs.h
> new file mode 100644
> --- /dev/null
> +++ b/include/linux/pcs/pcs.h

[ ... ]

> +#else
> +static inline struct phylink_pcs *fwnode_pcs_get(struct fwnode_handle *fwnode,
> +						 int index)

[Severity: Low]
Is this intentional? The fallback stub for fwnode_pcs_get() uses int index,
but the active declaration and implementation use unsigned int index.

> +{
> +	return ERR_PTR(-ENOENT);
> +}

[ ... ]

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