Re: [PATCH v8 6/9] media: qcom: camss: Add CAMSS Offline Processing Engine driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] media: qcom: camss: Unaligned crop and compose rectangles break MIPI 10-bit unpacking and UV plane
- [High] media: qcom: camss: Closing file descriptor bypasses vb2 queue release permanently leaking buffers
- [High] media: qcom: camss: TOCTOU race in ope_try_schedule() and missing state cleanup permanently stalls pipeline
- [Medium] media: qcom: camss: Missing media_entity_cleanup() for video and base entities leaks links array
- [Medium] media: qcom: camss: Missing video_device_pipeline_stop() causes media_pipeline memory leak
- [Medium] media: qcom: camss: Asynchronous pm_runtime_put() in ope_stop_streaming() leaks hardware clocks
--

commit e9f7b71037eb9bb411187989ce0d6e02a54a0cc6
Author: Loic Poulain <loic.poulain@oss.qualcomm.com>

    media: qcom: camss: Add CAMSS Offline Processing Engine driver

    Add an image processing driver for the Qualcomm Offline Processing Engine
    (OPE). OPE is a memory-to-memory ISP block that converts raw Bayer
    frames to YUV, performing white balance, demosaic, chroma enhancement,
    color correction and downscaling.

    Link: https://github.com/jwrdegoede/libcamera/commits/camss_pipeline_v2.1/
    Link: https://github.com/loicpoulain/camss-isp-m2m-test

> diff --git a/drivers/media/platform/qcom/camss/ope/core.c b/drivers/media/platform/qcom/camss/ope/core.c
> index 0000000000000..90fec886bebff 100644
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/ope/core.c

> [ ... ]

> @@ -845,6 +845,15 @@ static void ope_gen_stripe_yuv_dst(struct ope_ctx *ctx, struct ope_stripe *stripe,
>  		x_init = prev->dst[OPE_WR_CLIENT_DISP_C].x_init +
>  			 prev->dst[OPE_WR_CLIENT_DISP_C].width;
>  	else
>  		x_init = compose->left * 2 / fo->fmt->c_hsub;
>  
>  	stripe->dst[OPE_WR_CLIENT_DISP_C].addr   = base;
>  	stripe->dst[OPE_WR_CLIENT_DISP_C].x_init = x_init;

[Severity: High]
Does an unaligned compose->left swap the U and V components during memory 
writes?

When userspace sets an unaligned compose rectangle via the V4L2 selection API, 
an unaligned compose->left for sub-sampled formats (like NV12 with c_hsub=2) 
results in an odd x_init for the C plane. This could cause the hardware to 
start writing at an odd byte offset, swapping the interleaved U and V chroma 
components.

> [ ... ]

> @@ -882,5 +882,12 @@ static void ope_gen_stripes(struct ope_ctx *ctx, dma_addr_t src, dma_addr_t dst)
>  	compose = v4l2_subdev_state_get_compose(sd_state, OPE_PROC_PAD_SOURCE);
>  
>  	/* Advance source pointer to the crop origin */
>  	src += crop->top * fi->bytesperline + crop->left * fi->fmt->depth / 8;
>  	width = crop->width;
>  	num_stripes = DIV_ROUND_UP(crop->width, OPE_STRIPE_MAX_W);

[Severity: High]
Does this misalign the DMA address when crop->left is not a multiple of 4 for
10-bit formats?

Userspace can trigger this by setting an unaligned crop rectangle using the
V4L2 selection API. For 10-bit MIPI packed formats, an unaligned crop->left
results in a byte offset that breaks the 5-byte/4-pixel grouping required by 
the hardware unpacker, shifting all subsequent bits. 

Additionally, an odd crop->top or crop->left flips the Bayer phase (e.g., 
RGGB becomes GBRG), but the driver blindly applies 
stripe->src.pattern = src_hw->pattern.

> [ ... ]

> @@ -1630,5 +1630,12 @@ static void ope_run_work(struct work_struct *work)
>  		ctx = ope->curr_ctx;
>  		if (!ctx || !ctx->queued || ope->hw_running)
>  			return;
>  		if (!ope_job_ready(ctx))
>  			return;
>  		/* The registers still hold the configuration of @hw_ctx */

[Severity: High]
Does returning early here without clearing the scheduler state variables
(ctx->queued and ope->curr_ctx) permanently stall the pipeline?

> [ ... ]

> @@ -1643,6 +1643,11 @@ static void ope_try_schedule(struct ope_ctx *ctx)
>  {
>  	struct ope_dev *ope = ctx->ope;
>  
>  	if (!ope_job_ready(ctx))
>  		return;
>  
>  	/* Claim the job slot. */
> 	scoped_guard(spinlock_irqsave, &ope->sched_lock) {
> 		if (ctx->queued || ope->curr_ctx)
> 			return;

[Severity: High]
Is it possible for a TOCTOU race here to permanently stall the pipeline,
preventing any future STREAMON or QBUF operations?

If a hardware interrupt finishes a job on CPU A concurrently with a userspace
STREAMOFF ioctl on CPU B, the ope_job_ready check happens outside the lock.
CPU A might evaluate it as true, race with CPU B processing a STREAMOFF 
(which drains the queue and clears ctx->queued), and then proceed to claim the 
job slot. 

Later, when ope_run_work executes, ope_job_ready evaluates to false,
taking the early return mentioned above without clearing ctx->queued and
ope->curr_ctx.

> [ ... ]

> @@ -2046,5 +2046,9 @@ static void ope_stop_streaming(struct vb2_queue *q)
>  
>  	pm_runtime_put(ctx->ope->dev);
>  
>  	if (ope_hw_available(ctx->ope))
>  		video_device_pipeline_stop(vdev);
>  }

[Severity: Medium]
Does this asynchronous pm_runtime_put leak hardware clocks?

If the device is unbound while streaming, the driver core's devm teardown
path will call pm_runtime_disable shortly after remove returns. Because
pm_runtime_disable cancels any pending asynchronous idle or suspend work,
the actual runtime suspend callback is never invoked, leaving hardware
clocks and power domains permanently powered.

[Severity: Medium]
Also, does skipping video_device_pipeline_stop cause a permanent memory
leak of the media_pipeline object?

If the driver is unbound, v4l2_device_unregister clears the V4L2 device
pointer, causing ope_hw_available to return false. When userspace later
closes the open file descriptor, the pipeline stop is skipped. The
stop function only manages software reference counts and memory, so it
is safe and necessary to call even after hardware removal.

> [ ... ]

> @@ -2580,4 +2580,8 @@ static void ope_sd_adjust_crop_rect(struct v4l2_rect *crop, const struct v4l2_re
>  {
>  	v4l2_rect_set_min_size(crop, &ope_sd_min_rect);
>  	v4l2_rect_map_inside(crop, bounds);
>  }

[Severity: High]
Does this fail to enforce the required pixel alignments for the crop rectangle?

While v4l2_rect_map_inside is used to validate rectangles, it only clamps
coordinates and does not enforce pixel alignment. The selection rectangles
likely need to be aligned via v4l_bound_align_image to prevent the DMA
and chroma offset corruptions mentioned earlier.

> [ ... ]

> @@ -3225,5 +3225,9 @@ static int ope_release(struct file *file)
>  	scoped_guard(mutex, &ope->mutex) {
>  		if (--ope->open_count != 0)
>  			return 0;
>  		ope->shared_ctx = NULL;

[Severity: High]
Does this early return bypass the vb2 queue release and permanently leak
buffers?

If one process allocates buffers and keeps the queue owned, then closes
its file descriptor while a second process keeps the device open, this
early return skips ope_ctx_destroy where vb2_queue_release is called.

The buffers allocated by the first process remain leaked until the last
fd is closed, and any other open file descriptor is locked out because
the queue owner points to a freed v4l2_fh.

> [ ... ]

> diff --git a/drivers/media/platform/qcom/camss/ope/pipeline.c b/drivers/media/platform/qcom/camss/ope/pipeline.c
> index 0000000000000..bbb076717ef77 100644
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/ope/pipeline.c

> [ ... ]

> @@ -148,5 +148,8 @@ void camss_isp_pipeline_free(struct camss_isp_pipeline *pipeline)
>  		struct camss_isp_pipeline_entity *slot = &pipeline->entities[i];
>  
>  		if (slot->obj_type == MEDIA_ENTITY_TYPE_V4L2_SUBDEV)
>  			v4l2_subdev_cleanup(&slot->subdev);
>  
>  		kfree(slot->pads);

[Severity: Medium]
Does this fail to clean up the video and base entities, leaking their
dynamically allocated links arrays?

When unloading the module or failing probe after links are created,
media_entity_cleanup is never called for MEDIA_ENTITY_TYPE_VIDEO_DEVICE
or MEDIA_ENTITY_TYPE_BASE entities. Since media_create_pad_link dynamically
allocates the links array on the entity during registration, this guarantees
a memory leak of the links array every time the driver is unbound.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921-camss-isp-ope-v8-0-dd1c86a3c8a0@oss.qualcomm.com?part=6