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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [High] Unchecked return value from v4l2_subdev_state_get_format() causes a kernel panic on invalid stream IDs.
- [High] Missing .unbind callback leaves a dangling pointer to the upstream subdevice, leading to Use-After-Free.
- [High] Global format caching breaks multiplexed multi-stream configurations.
- [High] Register overwrite on shared virtual channels disables concurrent streams.
- [High] Querying upstream frame descriptor after disabling it leads to resource leaks.
- [High] Unprotected format modification and missing streaming check in formatter_subdev_set_fmt().
- [High] Double pm_runtime_put() on stream disable failure leads to unbalanced PM usage counter.
- [Medium] Mixing of goto-based error handling and scope-based cleanup helpers violates subsystem guidelines.
- [Medium] Uninitialized stack variable `source_fd` in get_vc() risks out-of-bounds memory reads.
- [Medium] Stream bitmasks > 7 are silently ignored during start/stop.
- [Medium] formatter_subdev_set_fmt() breaks unrouted streams by improperly returning -EINVAL.
--

commit 4d8a1be6e9f473bbb1ccaf942fe57cfe4870bd41
Author: Guoniu Zhou <guoniu.zhou@nxp.com>

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. Add data formatting support.

> 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..45240b7422b47
> --- /dev/null
> +++ b/drivers/media/platform/nxp/imx95-csi-formatter.c
> @@ -0,0 +1,776 @@
[ ... ]
> +static int 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;

Is it possible for v4l2_subdev_state_get_format() to return NULL here?
If userspace passes an unrouted stream ID, this might result in a NULL
pointer dereference.

> +		return 0;
> +	}
[ ... ]
> +static int formatter_subdev_set_fmt(struct v4l2_subdev *sd,
> +				    struct v4l2_subdev_state *sd_state,
> +				    struct v4l2_subdev_format *sdformat)
> +{
> +	struct csi_formatter *formatter = sd_to_formatter(sd);
> +	struct 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);
> +
> +	/*
> +	 * Validate the media bus code and clamp and align the size.
> +	 *
> +	 * The total number of bits per line must be a multiple of 8. We thus
> +	 * need to align the width for formats that are not multiples of 8
> +	 * bits.
> +	 */
> +	format = find_csi_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;

Can v4l2_subdev_state_get_format() return NULL for an unrouted stream here
as well?

> +
> +	/* 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;

Since the V4L2 Streams API permits setting formats on unrouted streams,
should this skip propagation instead of returning -EINVAL?

> +
> +	*fmt = sdformat->format;
> +
> +	/* Store the CSIS format descriptor for active formats. */
> +	if (sdformat->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> +		formatter->fmt = format;

Does this modification of formatter->fmt require formatter->lock protection
to avoid a race with enable_streams()?

Also, could userspace modify the format concurrently with an active stream
since there is no media_entity_is_streaming() check here?

> +
> +	return 0;
> +}
[ ... ]
> +static int get_vc(struct csi_formatter *formatter, unsigned int stream)
> +{
> +	struct v4l2_mbus_frame_desc source_fd;
> +	struct v4l2_mbus_frame_desc_entry *entry = NULL;
> +	unsigned int i;
> +	int vc;
> +	int ret;
> +
> +	/*
> +	 * Return virtual channel 0 as default value when remote subdev
> +	 * don't implement .get_frame_desc subdev callback
> +	 */
> +	ret = v4l2_subdev_call(formatter->csi_sd, pad, get_frame_desc,
> +			       formatter->remote_pad, &source_fd);

Could source_fd be left uninitialized if the upstream subdev returns 0
but fails to fully populate it? This might lead to an out-of-bounds read
when iterating over source_fd.num_entries.

> +	if (ret < 0)
> +		return (ret == -ENOIOCTLCMD) ? 0 : ret;
[ ... ]
> +static void csi_formatter_stop_stream(struct csi_formatter *formatter,
> +				     u64 stream_mask)
> +{
> +	unsigned int i;
> +	int ret;
> +	int vc;
> +
> +	for (i = 0; i < V4L2_FRAME_DESC_ENTRY_MAX; ++i) {
> +		if (!(stream_mask & BIT(i)))
> +			continue;

Are stream IDs >= V4L2_FRAME_DESC_ENTRY_MAX supported? If the mask
contains bits higher than this limit, they seem to be silently ignored.

> +
> +		ret = get_vc(formatter, i);
> +		if (WARN_ON(ret < 0)) {
> +			dev_err(formatter->dev,
> +				"Failed to get VC for stream %d: %d\n", i, ret);
> +			continue;
> +		}
> +
> +		vc = ret;
> +
> +		formatter_write(formatter, CSI_VC_PIXEL_DATA_TYPE(vc), 0);

Does overwriting this register completely break other interleaved streams
sharing the same Virtual Channel? A read-modify-write might be necessary
here since the register uses a bitmask.

> +	}
> +}
> +
> +static int csi_formatter_start_stream(struct csi_formatter *formatter,
> +				      u64 stream_mask)
> +{
> +	const struct formatter_pix_format *fmt = formatter->fmt;
> +	u64 configured_streams = 0;
> +	unsigned int i;
> +	u32 val;
> +	int ret;
> +	int vc;
> +
> +	for (i = 0; i < V4L2_FRAME_DESC_ENTRY_MAX; ++i) {
> +		if (!(stream_mask & BIT(i)))
> +			continue;
> +
> +		val = BIT(get_index_by_dt(fmt->data_type));

Will this break if userspace configures a multi-stream pipeline with
different formats per stream, since it applies the same global data type
to all enabled streams?

> +
> +		ret = get_vc(formatter, i);
> +		if (ret < 0)
> +			goto err_cleanup;
> +
> +		vc = ret;
> +
> +		formatter_write(formatter, CSI_VC_PIXEL_DATA_TYPE(vc), val);
> +		configured_streams |= BIT(i);
> +	}
[ ... ]
> +static int 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;
> +
> +	dev_dbg(dev, "Enable streams: pad=%u sink=0x%llx source=0x%llx\n",
> +		formatter->remote_pad, sink_streams, streams_mask);
> +
> +	if (!formatter->csi_sd) {
> +		dev_err(dev, "CSI controller not linked with formatter\n");
> +		return -EPIPE;
> +	}
> +
> +	guard(mutex)(&formatter->lock);
> +
> +	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, streams_mask);
> +	if (ret)
> +		goto err_runtime_put;

Does mixing guard() with goto labels create confusing ownership semantics
here?

> +
> +	ret = v4l2_subdev_enable_streams(formatter->csi_sd,
> +					 formatter->remote_pad,
> +					 sink_streams);
> +	if (ret)
> +		goto err_stop_stream;
[ ... ]
> +static int 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);

Since the upstream subdevice is disabled first, will its get_frame_desc
callback fail when csi_formatter_stop_stream() calls get_vc()? This could
cause the driver to skip disabling the local hardware stream.

> +
> +	formatter->enabled_streams &= ~streams_mask;
> +
> +	if (!formatter->enabled_streams)
> +		pm_runtime_put(formatter->dev);
> +
> +	return ret;

If v4l2_subdev_disable_streams() fails, the V4L2 core retains the stream
as enabled, but the driver clears enabled_streams and calls pm_runtime_put().
Would a subsequent retry to disable the stream cause a second
pm_runtime_put() without a matching get, unbalancing the usage counter?

> +}
[ ... ]
> +static const struct v4l2_async_notifier_operations formatter_notify_ops = {
> +	.bound = csi_formatter_notify_bound,
> +};

Is an .unbind callback missing here? If the upstream subdevice is unbound,
formatter->csi_sd would be left dangling, potentially leading to a
use-after-free during subsequent V4L2 operations.

> +
> +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);
> +	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;

This mixes __free() and goto-based error handling. Could this lead to
unexpected resource leaks or ownership issues?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518-csi_formatter-v7-0-562b750557e3@oss.nxp.com?part=2