Re: [PATCH v7 2/8] media: subdev: Add media_async_register_subdev() helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Uninitialized `v4l2_fwnode_endpoint` struct causes unpredictable parsing failures.
- [High] Mixing goto-based error handling with `__free()` cleanup causes a dangling pointer and potential double-free.
- [Low] Build failure due to undefined macro `fwnode_graph_for_each_endpoint_scoped`.
- [Low] Redundant and useless notifier cleanup in `media_async_subdev_cleanup`.
- [Low] Misleading error message when an endpoint is unsupported.
--

commit 9d4b9d1706779f41c6021852dce23778f72cf70b
Author: Frank Li <Frank.Li@nxp.com>
Subject: media: subdev: Add media_async_register_subdev() helper

This commit adds a helper to register a V4L2 sub-device with the asynchronous
sub-device framework. It centralizes firmware graph parsing, media pad creation,
and notifier registration to reduce duplicated code in V4L2 drivers.

> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 62a3a452f7884..4e15adc1af065 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
[ ... ]
> @@ -1302,6 +1303,174 @@ int __v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd, struct module *m
[ ... ]
> +static int
> +v4l2_async_nf_parse_fwnode(struct device *dev, struct media_pad *pads, u32 pads_count,
> +			   struct v4l2_async_notifier *notifier)
> +{
> +	struct v4l2_subdev *sd = notifier->sd;
> +	struct v4l2_async_connection *asd;
> +	struct v4l2_fwnode_endpoint vep;

[Severity: High]
Should vep be initialized before use?

When passed to v4l2_fwnode_endpoint_parse(), uninitialized fields like
vep.bus_type might contain stack garbage. Could this potentially confuse the
parser into assuming a specific bus type was requested, leading to parsing
failures?

> +	struct media_pad *pad;
> +	int ret;
> +
> +	if (!sd->internal_ops || !sd->internal_ops->set_pad_by_ep)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Missed set_pad_by_ep() callback\n");
> +	pad = pads;
> +
> +	fwnode_graph_for_each_endpoint_scoped(dev_fwnode(dev), ep) {

[Severity: Low]
Does this macro exist in the kernel? It looks like
fwnode_graph_for_each_endpoint_scoped is not defined yet, which might cause a
build failure here.

> +		u32 flags;
> +
> +		ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "failed to parse endpoint\n");
> +
> +		if (vep.base.port >= pads_count)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "port number bigger than pad number\n");
> +
> +		pad = pads + vep.base.port;
> +
> +		ret = sd->internal_ops->set_pad_by_ep(sd, pad, &vep);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret, "Can support endpoint\n");

[Severity: Low]
Is there a typo in this error message? It says "Can support endpoint" when
it is on the failure path, which might be confusing during debugging.

[ ... ]
> +void media_async_subdev_cleanup(struct v4l2_subdev *sd)
> +{
> +	v4l2_async_unregister_subdev(sd);
> +	v4l2_subdev_cleanup(sd);
> +	media_entity_cleanup(&sd->entity);
> +	v4l2_async_nf_unregister(sd->subdev_notifier);
> +	v4l2_async_nf_cleanup(sd->subdev_notifier);

[Severity: Low]
Are these notifier cleanup calls necessary here?

v4l2_async_unregister_subdev() already unregisters and cleans up the
notifier, then sets sd->subdev_notifier to NULL. These subsequent calls
appear to operate on a NULL pointer.

> +	kfree(sd->entity.pads);
> +}
> +EXPORT_SYMBOL_GPL(media_async_subdev_cleanup);
> +
> +int __media_async_register_subdev(struct v4l2_subdev *sd, struct module *module)
> +{
> +	struct device *dev = sd->dev;
> +	u32 ep_count;
> +	int ret;
> +
> +	if (WARN_ON(!sd->dev))
> +		return -ENODEV;
> +
> +	struct v4l2_async_notifier *notifier __free(kfree) = kzalloc_obj(*notifier);
> +	if (!notifier)
> +		return -ENOMEM;
> +
> +	v4l2_async_subdev_nf_init(notifier, sd);
> +
> +	ep_count = fwnode_graph_get_endpoint_count(dev_fwnode(dev),
> +						   FWNODE_GRAPH_DEVICE_DISABLED);
> +	if (!ep_count)
> +		return dev_err_probe(dev, -EINVAL, "No connected endpoints\n");
> +
> +	struct media_pad *pads __free(kfree) = kzalloc_objs(struct media_pad, ep_count);
> +	if (!pads)
> +		return -ENOMEM;
> +
> +	ret = v4l2_async_nf_parse_fwnode(dev, pads, ep_count, notifier);
> +	if (ret < 0)
> +		goto out_cleanup;
> +
> +	ret = media_entity_pads_init(&sd->entity, ep_count, pads);
> +	if (ret)
> +		goto out_cleanup;
> +
> +	ret = v4l2_async_nf_register(notifier);
> +	if (ret < 0)
> +		goto out_cleanup;
> +
> +	ret = v4l2_subdev_init_finalize(sd);
> +	if (ret)
> +		goto out_unregister;
> +
> +	ret = __v4l2_async_register_subdev(sd, module);
> +	if (ret < 0)
> +		goto out_unregister;
> +
> +	sd->subdev_notifier = no_free_ptr(notifier);
> +	retain_and_null_ptr(pads);
> +
> +	return 0;
> +
> +out_unregister:
> +	v4l2_async_nf_unregister(notifier);
> +	v4l2_subdev_cleanup(sd);
> +out_cleanup:
> +	v4l2_async_nf_cleanup(notifier);
> +
> +	return ret;
> +}

[Severity: High]
Could this goto-based error path lead to a dangling pointer?

Earlier in the function, media_entity_pads_init() stores the local pads
pointer into sd->entity.pads. When jumping to out_cleanup, the
compiler-inserted cleanup implicitly frees pads upon returning, but
sd->entity.pads is never reset to NULL.

If the driver handles the failure by calling media_async_subdev_cleanup(),
might this result in a double-free on sd->entity.pads? The cleanup
subsystem guidelines prohibit mixing goto labels with scope-based
cleanup annotations for this reason.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-imx8qxp_pcam-v7-0-b47d9e363400@nxp.com?part=2