Re: [PATCH v9 3/3] media: nxp: Add i.MX95 CSI pixel formatter v4l2 driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Unchecked pointer from `v4l2_subdev_state_get_format()` is dereferenced due to missing stream ID validation.
- [High] The driver incorrectly passes and iterates over the source stream mask to query sink-side formats and frame descriptors.
- [High] `csi_formatter_stop_stream()` clears the wrong hardware register by using the stream index instead of the Virtual Channel (VC) ID.
- [High] The driver leaks the PM runtime usage counter and fails to clean up hardware state if removed while streams are active.
- [High] Error handling in `csi_formatter_subdev_disable_streams()` desynchronizes state with the V4L2 core, causing PM usage counter underflows.
- [Medium] Mixing scope-based cleanup helpers (`__free()` and `guard()`) with `goto` error handling.
- [Medium] Stream start and stop routines inappropriately limit V4L2 stream ID iterations to `CSI_FORMATTER_VC_NUM` (8).
- [Medium] The driver inappropriately returns `-EINVAL` when setting a format on an unrouted sink stream.
--

commit 519af3c273f2310baca55e801417774f1157eba3
Author: Guoniu Zhou <guoniu.zhou@nxp.com>
Date:   Tue May 26 08:57:22 2026 +0000

    media: nxp: Add i.MX95 CSI pixel formatter v4l2 driver
    
    The CSI pixel formatter is a module found on i.MX95 used to reformat
    packet info, pixel and non-pixel data from CSI-2 host controller to
    match Pixel Link(PL) definition.

> diff --git a/drivers/media/platform/nxp/imx95-csi-formatter.c b/drivers/media/platform/nxp/imx95-csi-formatter.c
> new file mode 100644
> index 0000000000000..9dee38d85839c
> --- /dev/null
> +++ b/drivers/media/platform/nxp/imx95-csi-formatter.c

[ ... ]

> +static int csi_formatter_subdev_enum_mbus_code(struct v4l2_subdev *sd,
> +					       struct v4l2_subdev_state *sd_state,
> +					       struct v4l2_subdev_mbus_code_enum *code)
> +{
> +	if (code->pad == CSI_FORMATTER_PAD_SOURCE) {
> +		struct v4l2_mbus_framefmt *fmt;
> +
> +		if (code->index > 0)
> +			return -EINVAL;
> +
> +		fmt = v4l2_subdev_state_get_format(sd_state, code->pad,
> +						   code->stream);
> +		code->code = fmt->code;

[Severity: High]
Is there a missing check for fmt being NULL here? 

If userspace provides an invalid or non-existent stream ID,
v4l2_subdev_state_get_format() will return NULL, which appears to be
unconditionally dereferenced here.

> +		return 0;
> +	}
> +
> +	if (code->index >= ARRAY_SIZE(formats))
> +		return -EINVAL;
> +
> +	code->code = formats[code->index].code;
> +
> +	return 0;
> +}
> +
> +static int csi_formatter_subdev_set_fmt(struct v4l2_subdev *sd,
> +					struct v4l2_subdev_state *sd_state,
> +					struct v4l2_subdev_format *sdformat)
> +{
> +	struct csi_formatter_pix_format const *format;
> +	struct v4l2_mbus_framefmt *fmt;
> +
> +	if (sdformat->pad == CSI_FORMATTER_PAD_SOURCE)
> +		return v4l2_subdev_get_fmt(sd, sd_state, sdformat);
> +
> +	format = csi_formatter_find_format(sdformat->format.code);
> +	if (!format)
> +		format = &formats[0];
> +
> +	v4l_bound_align_image(&sdformat->format.width, 1, 0xffff, 2,
> +			      &sdformat->format.height, 1, 0xffff, 0, 0);
> +
> +	fmt = v4l2_subdev_state_get_format(sd_state, sdformat->pad,
> +					   sdformat->stream);
> +	*fmt = sdformat->format;

[Severity: High]
Could fmt be NULL here as well if an invalid stream ID is provided?

> +
> +	/* Set default code if user set an invalid value */
> +	fmt->code = format->code;
> +
> +	/* Propagate the format from sink stream to source stream */
> +	fmt = v4l2_subdev_state_get_opposite_stream_format(sd_state, sdformat->pad,
> +							   sdformat->stream);
> +	if (!fmt)
> +		return -EINVAL;

[Severity: Medium]
Should this return -EINVAL if the stream is unrouted? 

The V4L2 specification requires format changes on unrouted streams to succeed
(they just shouldn't propagate), so returning an error here might break
compliant userspace applications configuring unrouted pad formats.

> +
> +	*fmt = sdformat->format;
> +
> +	return 0;
> +}

[ ... ]

> +static void csi_formatter_stop_stream(struct csi_formatter *formatter,
> +				      u64 stream_mask)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < CSI_FORMATTER_VC_NUM; ++i) {

[Severity: Medium]
Is it safe to limit V4L2 stream ID iterations to CSI_FORMATTER_VC_NUM (8)?

V4L2 stream IDs are arbitrary and can range from 0 to 63, so any
configured stream with an ID >= 8 would be silently ignored. Could
this use for_each_set_bit() to iterate over the entire mask instead?

> +		if (!(stream_mask & BIT(i)))
> +			continue;
> +
> +		csi_formatter_write(formatter, CSI_VC_PIXEL_DATA_TYPE(i), 0);

[Severity: High]
Does this write to the correct hardware register?

csi_formatter_start_stream() correctly resolves the hardware Virtual
Channel (VC) from the remote frame descriptor, but here we seem to be
writing 0 to CSI_VC_PIXEL_DATA_TYPE(i) where i is the V4L2 stream index.

If the stream index and VC ID differ, could this clear the wrong register
and corrupt another running VC's state?

> +	}
> +}
> +
> +static int csi_formatter_start_stream(struct csi_formatter *formatter,
> +				      struct v4l2_subdev_state *state,
> +				      u64 stream_mask)
> +{
> +	const struct csi_formatter_pix_format *pix_fmt;
> +	struct v4l2_mbus_framefmt *fmt;
> +	struct v4l2_mbus_frame_desc fd = {};
> +	u64 configured_streams = 0;
> +	unsigned int i;
> +	u32 val;
> +	int vc;
> +	int ret;
> +
> +	ret = v4l2_subdev_call(formatter->csi_sd, pad, get_frame_desc,
> +			       formatter->remote_pad, &fd);
> +	if (ret < 0 && ret != -ENOIOCTLCMD) {
> +		dev_err(formatter->dev, "Failed to get frame desc: %d\n", ret);
> +		return ret;
> +	}
> +
> +	for (i = 0; i < CSI_FORMATTER_VC_NUM; ++i) {
> +		if (!(stream_mask & BIT(i)))
> +			continue;
> +
> +		fmt = v4l2_subdev_state_get_format(state,
> +						   CSI_FORMATTER_PAD_SINK, i);
> +
> +		pix_fmt = csi_formatter_find_format(fmt->code);

[Severity: High]
In csi_formatter_subdev_enable_streams(), stream_mask originates from the
source pad, but it is passed here and used as the stream index (i) to fetch
the sink-side format.

If the routing maps a different sink stream ID to this source stream ID,
will v4l2_subdev_state_get_format() return NULL for the non-existent sink
stream i and cause a NULL pointer dereference when accessing fmt->code?

> +
> +		val = BIT(csi_formatter_get_index_by_dt(pix_fmt->data_type));
> +
> +		if (ret == -ENOIOCTLCMD) {
> +			/*
> +			 * Source doesn't implement get_frame_desc, use
> +			 * default VC 0
> +			 */
> +			vc = 0;
> +		} else {
> +			vc = csi_formatter_get_vc(formatter, &fd, i);
> +			if (vc < 0) {
> +				ret = vc;
> +				goto err_cleanup;
> +			}
> +		}
> +
> +		csi_formatter_write(formatter, CSI_VC_PIXEL_DATA_TYPE(vc), val);
> +		configured_streams |= BIT(i);
> +	}
> +
> +	return 0;
> +
> +err_cleanup:
> +	csi_formatter_stop_stream(formatter, configured_streams);
> +	return ret;
> +}
> +
> +static int csi_formatter_subdev_enable_streams(struct v4l2_subdev *sd,
> +					       struct v4l2_subdev_state *state,
> +					       u32 pad, u64 streams_mask)
> +{
> +	struct csi_formatter *formatter = sd_to_formatter(sd);
> +	struct device *dev = formatter->dev;
> +	u64 sink_streams;
> +	int ret;
> +
> +	sink_streams = v4l2_subdev_state_xlate_streams(state,
> +						       CSI_FORMATTER_PAD_SOURCE,
> +						       CSI_FORMATTER_PAD_SINK,
> +						       &streams_mask);
> +	if (!sink_streams || !streams_mask)
> +		return -EINVAL;
> +
> +	guard(mutex)(&formatter->lock);

[Severity: Medium]
Is it intended to mix scope-based cleanup helpers (guard) with goto error
handling here? 

The kernel cleanup subsystem guidelines mandate that goto-based error handling
and scope-based cleanups should never be mixed within the same function.

> +
> +	if (!formatter->enabled_streams) {
> +		ret = pm_runtime_resume_and_get(formatter->dev);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to resume runtime PM: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	ret = csi_formatter_start_stream(formatter, state, streams_mask);
> +	if (ret)
> +		goto err_runtime_put;
> +
> +	ret = v4l2_subdev_enable_streams(formatter->csi_sd,
> +					 formatter->remote_pad,
> +					 sink_streams);
> +	if (ret)
> +		goto err_stop_stream;
> +
> +	formatter->enabled_streams |= streams_mask;
> +
> +	return 0;
> +
> +err_stop_stream:
> +	csi_formatter_stop_stream(formatter, streams_mask);
> +err_runtime_put:
> +	if (!formatter->enabled_streams)
> +		pm_runtime_put(formatter->dev);
> +	return ret;
> +}
> +
> +static int csi_formatter_subdev_disable_streams(struct v4l2_subdev *sd,
> +						struct v4l2_subdev_state *state,
> +						u32 pad, u64 streams_mask)
> +{
> +	struct csi_formatter *formatter = sd_to_formatter(sd);
> +	u64 sink_streams;
> +	int ret;
> +
> +	sink_streams = v4l2_subdev_state_xlate_streams(state,
> +						       CSI_FORMATTER_PAD_SOURCE,
> +						       CSI_FORMATTER_PAD_SINK,
> +						       &streams_mask);
> +	if (!sink_streams || !streams_mask)
> +		return -EINVAL;
> +
> +	guard(mutex)(&formatter->lock);
> +
> +	ret = v4l2_subdev_disable_streams(formatter->csi_sd, formatter->remote_pad,
> +					  sink_streams);
> +	if (ret)
> +		dev_err(formatter->dev, "Failed to disable streams: %d\n", ret);
> +
> +	csi_formatter_stop_stream(formatter, streams_mask);
> +
> +	formatter->enabled_streams &= ~streams_mask;
> +
> +	if (!formatter->enabled_streams)
> +		pm_runtime_put(formatter->dev);

[Severity: High]
If v4l2_subdev_disable_streams() fails and returns an error, the V4L2 core keeps
its internal stream state as "enabled", but this function proceeds to clear
formatter->enabled_streams and calls pm_runtime_put().

If the V4L2 core later retries disabling the stream, will it trigger an
unbalanced, secondary pm_runtime_put() call resulting in a PM usage counter
underflow since enabled_streams is already 0?

> +
> +	return ret;
> +}

[ ... ]

> +static int csi_formatter_async_register(struct csi_formatter *formatter)
> +{
> +	struct device *dev = formatter->dev;
> +	struct v4l2_async_connection *asc;
> +	int ret;
> +
> +	struct fwnode_handle *ep __free(fwnode_handle) =
> +		fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0,
> +						FWNODE_GRAPH_ENDPOINT_NEXT);

[Severity: Medium]
Does this mix __free(fwnode_handle) with goto error handling below?

Similar to csi_formatter_subdev_enable_streams(), kernel guidelines state that
scope-based cleanup helpers and goto should not be used in the same function.

> +	if (!ep)
> +		return -ENOTCONN;
> +
> +	v4l2_async_subdev_nf_init(&formatter->notifier, &formatter->sd);
> +
> +	asc = v4l2_async_nf_add_fwnode_remote(&formatter->notifier, ep,
> +					      struct v4l2_async_connection);
> +	if (IS_ERR(asc)) {
> +		ret = PTR_ERR(asc);
> +		goto err_cleanup_notifier;
> +	}
> +
> +	formatter->notifier.ops = &formatter_notify_ops;
> +
> +	ret = v4l2_async_nf_register(&formatter->notifier);
> +	if (ret)
> +		goto err_cleanup_notifier;
> +
> +	ret = v4l2_async_register_subdev(&formatter->sd);
> +	if (ret)
> +		goto err_unregister_notifier;
> +
> +	return 0;
> +
> +err_unregister_notifier:
> +	v4l2_async_nf_unregister(&formatter->notifier);
> +err_cleanup_notifier:
> +	v4l2_async_nf_cleanup(&formatter->notifier);
> +	return ret;
> +}

[ ... ]

> +static void csi_formatter_remove(struct platform_device *pdev)
> +{
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct csi_formatter *formatter = sd_to_formatter(sd);
> +
> +	csi_formatter_async_unregister(formatter);
> +
> +	v4l2_subdev_cleanup(&formatter->sd);
> +	media_entity_cleanup(&formatter->sd.entity);
> +}

[Severity: High]
If the subdevice is unbound via sysfs while streams are active, will this leak
the PM usage counter and leave the hardware in an active state?

It looks like csi_formatter_remove() unconditionally unregisters the subdevice
but doesn't check if hardware streams need to be stopped or if the PM usage
counter needs to be dropped.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526-csi_formatter-v9-0-ca3d8c334c39@oss.nxp.com?part=3