Re: [PATCH v13 10/17] media: ti: j721e-csi2rx: add support for processing virtual channels

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The check for an unmatched stream in `ti_csi2rx_get_vc_and_dt` is incorrectly placed inside the loop, rendering it dead code and causing a false success return.
- [High] Error path bypasses `video_device_pipeline_stop()`, leaking a media pipeline reference if `ti_csi2rx_get_vc_and_dt` fails.
--

commit a246af785a58954c2b9c8219f7853322d7ba7a14
Author: Jai Luthra <jai.luthra@linux.dev>

media: ti: j721e-csi2rx: add support for processing virtual channels

This commit adds support for virtual channels by using get_frame_desc()
to retrieve the frame descriptor from the connected source. It falls
back to virtual channel 0 if the source does not support
get_frame_desc().

> diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> index 26a8eaa98b3da..d0a681ba78ebf 100644
> --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c

[ ... ]

> @@ -881,12 +887,46 @@ static void ti_csi2rx_buffer_queue(struct vb2_buffer *vb)
> +static int ti_csi2rx_get_vc_and_dt(struct ti_csi2rx_ctx *ctx)
> +{
[ ... ]
> +	for (i = 0; i < fd.num_entries; i++) {
> +		if (ctx->stream == fd.entry[i].stream) {
> +			ctx->vc = fd.entry[i].bus.csi2.vc;
> +			ctx->dt = fd.entry[i].bus.csi2.dt;
> +			break;
> +		}
> +
> +		/* Return error if no matching stream found */
> +		if (i == fd.num_entries)
> +			return -EINVAL;
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Is it possible for the i == fd.num_entries condition to ever be true here?

Because it is inside the for loop body where the condition i < fd.num_entries
is strictly enforced, i can never equal fd.num_entries.

If no matching stream is found, the loop will exit normally and the function
will silently return 0 instead of returning an error. This leaves ctx->vc and
ctx->dt uninitialized or with stale values, and bypasses the fallback logic.

[ ... ]

> @@ -901,6 +941,15 @@ static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count)
>  	if (ret)
>  		goto err;
>  
> +	ret = ti_csi2rx_get_vc_and_dt(ctx);
> +	if (ret == -ENOIOCTLCMD) {
> +		ctx->vc = 0;
> +		fmt = find_format_by_fourcc(ctx->v_fmt.fmt.pix.pixelformat);
> +		ctx->dt = fmt->csi_dt;
> +	} else if (ret < 0) {
> +		goto err;
> +	}

[Severity: High]
Does this error path leak the media pipeline reference?

By jumping to the err label, the execution bypasses the err_pipeline label
which calls video_device_pipeline_stop(). This would leave the pipeline
permanently in an active state and block future pipeline operations
with -EBUSY.

I understand this issue is fixed in a later commit ("media: ti: j721e-csi2rx:
add multistream support"), but could we fix it here to ensure this commit
doesn't introduce regressions during bisection?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520120022.539913-1-r-donadkar@ti.com?part=10