Re: [PATCH net-next v10 04/11] 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:
- [High] fwnode_phylink_pcs_parse() ignores missing PCS providers, breaking probe deferral and shifting array indices.
- [High] The PCS framework returns raw pointers to consumers without taking a module reference or establishing a device link, allowing the provider to be unloaded while in use.
- [Low] Missing dummy stubs for the provider API in pcs-provider.h when CONFIG_FWNODE_PCS is disabled.
--

--- Patch [4]: [PATCH net-next v10 04/11] net: pcs: implement Firmware node support for PCS driver ---
commit d0c8f24c0baae704d5686e9b3a0c34423583e32b
Author: Christian Marangi <ansuelsmth@gmail.com>

net: pcs: implement Firmware node support for PCS driver

Implement the foundation of Firmware node support for PCS driver.
To support this, implement a simple Provider API where a PCS driver can
expose multiple PCS with an xlate .get function.

> diff --git a/drivers/net/pcs/pcs.c b/drivers/net/pcs/pcs.c
> new file mode 100644
> index 0000000000000..fa8ab67076e24
> --- /dev/null
> +++ b/drivers/net/pcs/pcs.c
[ ... ]
> +static struct phylink_pcs *
> +fwnode_pcs_get_from_pcsspec(struct fwnode_reference_args *pcsspec)
> +{
> +	struct fwnode_pcs_provider *provider;
> +	struct phylink_pcs *pcs = ERR_PTR(-ENODEV);
> +
> +	if (!pcsspec)
> +		return ERR_PTR(-EINVAL);
> +
> +	mutex_lock(&fwnode_pcs_mutex);
> +	list_for_each_entry(provider, &fwnode_pcs_providers, link) {
> +		if (provider->fwnode == pcsspec->fwnode) {
> +			pcs = provider->get(pcsspec, provider->data);
> +			if (!IS_ERR(pcs))
> +				break;

[Severity: High]
Does this safely manage the lifetime of the provider module?

When a consumer gets the PCS pointer here, there is no try_module_get()
or similar call to pin the provider module in memory. Because fw_devlink
doesn't automatically parse the pcs-handle property, no automatic device 
links are created to enforce unbind ordering either.

Can the provider module be cleanly unloaded while the consumer is still
actively using the returned struct phylink_pcs, leading to a 
use-after-free?

> +		}
> +	}
> +	mutex_unlock(&fwnode_pcs_mutex);
> +
> +	return pcs;
> +}
[ ... ]
> +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)) {
> +			/* Exit early if no PCS remain.*/
> +			if (PTR_ERR(pcs) == -ENOENT)
> +				break;
> +
> +			/*
> +			 * Ignore -ENODEV error for PCS that still
> +			 * needs to probe.
> +			 */
> +			if (PTR_ERR(pcs) == -ENODEV)
> +				continue;

[Severity: High]
Does this break the standard probe deferral mechanism?

If a consumer driver calls fwnode_phylink_pcs_parse() during its probe
sequence and the PCS provider hasn't registered yet, skipping the -ENODEV
error allows the consumer to successfully complete probing despite missing
hardware.

Also, because the loop continues without incrementing the `found` index,
the available_pcs array is compacted. If pcs-handle specifies multiple
PCS components that map positionally to specific hardware lanes, doesn't
skipping an unprobed PCS shift the array indices and associate the wrong
PCS with a given hardware lane?

Should this code return -EPROBE_DEFER instead of dropping the error?

> +
> +			return PTR_ERR(pcs);
> +		}
> +
> +		available_pcs[found] = pcs;
> +		found++;
> +	}
> +
> +	return found;
> +}
> +EXPORT_SYMBOL_GPL(fwnode_phylink_pcs_parse);
> diff --git a/include/linux/pcs/pcs-provider.h b/include/linux/pcs/pcs-provider.h
> new file mode 100644
> index 0000000000000..ae51c108147e3
> --- /dev/null
> +++ b/include/linux/pcs/pcs-provider.h
[ ... ]
> +int fwnode_pcs_add_provider(struct fwnode_handle *fwnode,
> +			    struct phylink_pcs *(*get)(struct fwnode_reference_args *pcsspec,
> +						       void *data),
> +			    void *data);
> +
> +/**
> + * fwnode_pcs_del_provider - Removes a PCS provider
> + * @fwnode: Firmware node
> + */
> +void fwnode_pcs_del_provider(struct fwnode_handle *fwnode);
> +
> +#endif /* __LINUX_PCS_PROVIDER_H */

[Severity: Low]
Are static inline fallback stubs needed here for when CONFIG_FWNODE_PCS
is disabled?

While include/linux/pcs/pcs.h correctly provides dummy stubs for the
consumer API, the provider API declarations lack an #else block. If a
driver conditionally exposes a PCS and includes pcs-provider.h in a
configuration where CONFIG_FWNODE_PCS=n, it could fail to build at link
time due to unresolved symbols for these functions.

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