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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] media: memory corruption via active crop/compose modification during streaming
- [High] pm: hardware power leak due to async pm_runtime_put() in stop_streaming
- [High] media: out-of-bounds DMA stride due to incorrect clamp and align ordering
- [High] media: image size violation due to invalid scaler output pixel calculation
- [High] media: dead queue caused by lockless destruction of shared run_work
- [Medium] media: registration race exposes incomplete media graph to userspace
- [Medium] media: data race on Gamma LUT cache state during context destruction
- [Low] media: build failure when CONFIG_MEDIA_CONTROLLER is disabled
--

commit 22118f1287a95c24fecdb6655e65ff6e1e0a42f2
Author: Loic Poulain <loic.poulain@oss.qualcomm.com>
Subject: media: qcom: camss: Add CAMSS Offline Processing Engine driver

Adds an image processing driver for the Qualcomm Offline Processing Engine
(OPE), an ISP block converting raw Bayer frames to YUV. Exposes three video
nodes per pipeline instance.

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
> --- a/drivers/media/platform/qcom/camss/ope/core.c
> +++ b/drivers/media/platform/qcom/camss/ope/core.c
[ ... ]
> +static unsigned int ope_calc_h_scale(unsigned int src, unsigned int dst,
> +				     unsigned int nstripes, unsigned int c_hsub,
> +				     unsigned int sw_align)
> +{
[ ... ]
> +	/* Find the smallest h_scale denominator >= dst such that the
> +	 * last-stripe adjustment can satisfy both Y and C pixel counts.
> +	 */
> +	for (denom = dst; denom <= dst + 16 * nstripes; denom++) {
> +		unsigned int hs = DS_Q21(src, denom);
> +		unsigned int tw = (nstripes - 1) * DS_OUTPUT_PIX(OPE_STRIPE_MAX_W, 0, hs);

[Severity: High]
Does this calculate the total output pixels correctly? 

It looks like the calculation assumes all non-final stripes are exactly 
OPE_STRIPE_MAX_W pixels wide. However, ope_gen_stripes() might reduce the 
actual penultimate stripe width to satisfy the minimum width requirement for 
the final stripe. 

If the penultimate stripe is shrunk, would this cause a mismatch in the 
output pixel count and trigger an OPE_BUS_WR_INPUT_IF_IRQ_STATUS_0_IMG_SZ_VIOL 
hardware image size violation?

[ ... ]
> +static void ope_gen_stripes(struct ope_ctx *ctx, dma_addr_t src, dma_addr_t dst)
> +{
[ ... ]
> +		stripe->src.pattern = src_hw ? src_hw->pattern : 0;
> +		stripe->src.format = src_hw ? src_hw->unpacker : 0;
> +
> +		/* Ensure last stripe is wide enough */
> +		if (width > OPE_STRIPE_MAX_W && width < OPE_STRIPE_MAX_W + OPE_STRIPE_MIN_W)
> +			stripe->src.width -= OPE_STRIPE_MIN_W * 2;

[Severity: High]
This appears to be the location where the penultimate stripe width is reduced.

Should the scaling coefficient calculator be updated to account for this 
reduction so we don't encounter the image size constraint violation at 
runtime?

[ ... ]
> +static void ope_sched_cancel(struct ope_ctx *ctx)
> +{
> +	struct ope_dev *ope = ctx->ope;
> +	bool running;
> +
> +	disable_work_sync(&ope->run_work);

[Severity: High]
Could this disable_work_sync() cause a deadlock if called locklessly during 
context destruction? 

If another process concurrently opens the device and starts streaming, its 
call to schedule_work() might get silently dropped because the globally shared 
work item is disabled here. When the workqueue is later re-enabled, the 
dropped jobs wouldn't be restarted, potentially leaving the new stream 
permanently stalled.

[ ... ]
> +static const struct ope_fmt *ope_fmt_try(struct ope_dev *ope, bool is_output,
> +					 struct v4l2_pix_format_mplane *pix)
> +{
[ ... ]
> +	if (is_output && fmt->fourcc != V4L2_PIX_FMT_GREY)
> +		align = 8 * fmt->c_hsub;
> +	else
> +		align = 1;
> +
> +	bytesperline = min(bytesperline, ALIGN_DOWN(OPE_MAX_STRIDE, align));
> +	bytesperline = ALIGN(bytesperline, align);

[Severity: High]
Is the order of clamping and alignment correct here? 

If bytesperline is clamped to ALIGN_DOWN(OPE_MAX_STRIDE, align) and happens 
to not be a multiple of the alignment, the subsequent ALIGN() operation will 
round it up. 

Could this cause the final bytesperline to exceed OPE_MAX_STRIDE, resulting 
in an out-of-bounds DMA transfer when the hardware stride register is 
programmed?

[ ... ]
> +static void ope_prog_gamma(struct ope_ctx *ctx, bool force)
> +{
[ ... ]
> +	} else {
> +		/* Dirty, or miss in both banks: upload into the LRU bank. */
> +		bank = !ope->glut_mru_bank;
> +		ope_glut_upload(ctx, bank);
> +	}
> +
> +	ope->glut_mru_bank = bank;

[Severity: Medium]
Is it safe to update the global gamma LUT cache state here while 
ope_ctx_destroy() might be concurrently invalidating the cache locklessly? 

It looks like ope_glut_cache_invalidate() modifies the current and MRU bank 
variables without a lock, which could race with this assignment and lead to 
incorrect hardware bank selection for concurrent streams.

[ ... ]
> +static void ope_stop_streaming(struct vb2_queue *q)
> +{
[ ... ]
> +	ope_sched_cancel(ctx);
> +	ope_queue_drain(ctx, idx, VB2_BUF_STATE_ERROR);
> +	ope_sched_resume(ctx);
> +
> +	pm_runtime_put(ctx->ope->dev);

[Severity: High]
Does this asynchronous pm_runtime_put() leave the device vulnerable to a 
power leak during driver unbind? 

If userspace stops the stream or closes the device, and the device is 
immediately unbound, the devres cleanup will call pm_runtime_disable(). This 
cancels any pending async suspend requests, leaving the power domains and 
clocks permanently enabled. 

Should this be pm_runtime_put_sync() instead?

[ ... ]
> +static int ope_proc_set_fmt(struct v4l2_subdev *sd,
> +			    struct v4l2_subdev_state *state,
> +			    struct v4l2_subdev_format *fmt)
> +{
> +	if (fmt->pad == OPE_PROC_PAD_SINK_PAR)
> +		return v4l2_subdev_get_fmt(sd, state, fmt);
> +
> +	if (fmt->pad == OPE_PROC_PAD_SINK_IN) {
> +		ope_proc_set_sink_fmt(state, &fmt->format);
> +		return 0;
> +	}

[Severity: Critical]
Can the active format be changed here while the pipeline is currently 
streaming? 

If userspace dynamically increases the format size during an active stream, 
ope_run_work() might fetch this new active state using 
v4l2_subdev_lock_and_get_active_state() without validating it against the 
previously allocated DMA buffer size. This could result in the hardware 
writing past the end of the DMA buffer.

[ ... ]
> +static int ope_proc_set_selection(struct v4l2_subdev *sd,
> +				  struct v4l2_subdev_state *state,
> +				  struct v4l2_subdev_selection *sel)
> +{
> +	if (sel->pad == OPE_PROC_PAD_SINK_IN) {
> +		if (sel->target != V4L2_SEL_TGT_CROP)
> +			return -EINVAL;
> +		ope_proc_set_sink_crop(state, &sel->r);
> +		return 0;
> +	}

[Severity: Critical]
Similar to the format change above, can this active crop rectangle be 
modified mid-stream? 

Altering the crop size during streaming might increase the active compose 
size beyond what the video node buffer was allocated for, potentially causing 
memory corruption.

[ ... ]
> +static void ope_ctx_destroy(struct ope_ctx *ctx)
> +{
[ ... ]
> +	ope_sched_resume(ctx);
> +
> +	/* Do not leave the LUT cache pointing at a context about to be freed */
> +	ope_glut_cache_invalidate(ctx->ope);

[Severity: Medium]
As mentioned earlier, mutating the global cache state here without holding 
any locks seems to race against other contexts running ope_prog_gamma(). 

Could this cause incorrect gamma LUT application for concurrent streams?

[ ... ]
> +static int ope_release(struct file *file)
> +{
[ ... ]
> +	scoped_guard(mutex, &ope->mutex) {
> +		if (--ope->open_count != 0)
> +			return 0;
> +		ope->shared_ctx = NULL;
> +		scoped_guard(spinlock, &ope->ctx_lock)
> +			list_del(&ctx->list);
> +	}
> +
> +	ope_ctx_destroy(ctx);

[Severity: High]
Calling ope_ctx_destroy() outside of the main device mutex appears to be 
what leads to the lockless workqueue disable and LUT cache invalidation 
races described above. 

Is there a reason context destruction is performed concurrently rather than 
under the mutex?

[ ... ]
> +static int ope_v4l2_register(struct ope_dev *ope)
> +{
[ ... ]
> +	ret = v4l2_device_register(ope->dev, &ope->v4l2_dev);
> +	if (ret) {
> +		/* No v4l2_device reference exists yet, so release @ope directly */
> +		ope_v4l2_dev_release(&ope->v4l2_dev);
> +		return ret;
> +	}
> +
> +	ret = media_device_register(&ope->mdev);
> +	if (ret)
> +		goto err_v4l2;
> +
> +	ope->pipeline = camss_isp_pipeline_alloc(OPE_ENTITY_COUNT);

[Severity: Medium]
Is the media device being registered and exposed to userspace too early? 

It looks like media_device_register() is called before the pipeline entities 
are allocated and the internal subdev links are created via 
media_create_pad_link(). If udev triggers an application launch upon device 
creation, the application might fail because the media graph is incomplete.

[ ... ]
> +static void ope_remove(struct platform_device *pdev)
> +{
> +	struct ope_dev *ope = platform_get_drvdata(pdev);
> +
> +	disable_irq(ope->irq);
> +	ope_unregister(ope);
> +}

[Severity: High]
Does this remove path safely handle device suspension? 

Because the stop_streaming path uses an asynchronous pm_runtime_put(), 
removing the driver immediately without explicitly forcing the device to 
suspend might disable devres PM before the async suspend completes, causing 
a power leak.

> diff --git a/drivers/media/platform/qcom/camss/ope/pipeline.c b/drivers/media/platform/qcom/camss/ope/pipeline.c
> --- a/drivers/media/platform/qcom/camss/ope/pipeline.c
> +++ b/drivers/media/platform/qcom/camss/ope/pipeline.c
> @@ ... @@
> +#if !IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
> +static inline int media_entity_pads_init(struct media_entity *e, u16 n,
> +					 struct media_pad *p) { return 0; }
> +static inline void media_entity_remove_links(struct media_entity *e) {}
> +static inline int media_create_pad_link(struct media_entity *src, u16 sp,
> +					struct media_entity *sink, u16 dp,
> +					u32 flags) { return 0; }
> +#endif

[Severity: Low]
Does this cause a compilation error when CONFIG_MEDIA_CONTROLLER is disabled? 

The function media_entity_pads_init() is already unconditionally declared as 
a non-static function in include/media/media-entity.h. Providing a static 
inline fallback here creates a static vs non-static declaration conflict.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923-camss-isp-ope-v9-0-86a75dc18b83@oss.qualcomm.com?part=6