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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 15 potential issue(s) to consider:
- [Critical] Missing locking in V4L2 ioctls allows format state tearing, leading to out-of-bounds DMA access.
- [High] Concurrent job execution during streamoff causes DMA Use-After-Free.
- [High] Probe always fails with a timeout due to a masked reset interrupt, and unbounded wait hangs the kernel during teardown.
- [High] IRQ handler does not abort after triggering a software reset, leading to double-completion of buffers and starting hardware mid-reset.
- [High] Hardware configuration is not fully restored after a software reset, leading to corrupted frames.
- [High] Downscaler can produce a 0 output width for the last stripe under high downscale ratios, leading to hardware violation.
- [High] Concurrent list_add and list_del on ope->ctx_list causes list corruption due to unlocked access.
- [High] Repeatedly opening and closing the device node causes a kernel memory leak.
- [High] The main `ope_dev` structure is freed via devres upon device unbind, causing a Use-After-Free if a file descriptor remains open.
- [High] Driver removal does not stop the hardware or disable the IRQ before canceling the workqueue, leading to a Use-After-Free.
- [Medium] Missing `pm_ptr()` wrapper for `dev_pm_ops` assignment creates dead code when PM is disabled.
- [Medium] Asynchronous Runtime PM put during probe error path causes a power leak.
- [Medium] Missing phase accumulator programming causes scaling discontinuities (seams).
- [Medium] OPE_ENTITY_DISP does not propagate sink format changes to its source pad.
- [Medium] Hardware pipeline stalls indefinitely after encountering a processing error because the scheduler is not triggered.
--

--- Patch [6]: [PATCH v4 6/7] media: qcom: camss: Add CAMSS Offline Processing Engine driver ---
commit 4b6f83336240d059ff0d3b87339ee9f1c8085fee
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.

> diff --git a/drivers/media/platform/qcom/camss/camss-ope.c b/drivers/media/platform/qcom/camss/camss-ope.c
> new file mode 100644
> index 0000000000000..2c0d68cf1a637
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-ope.c

[ ... ]

> +static void ope_gen_stripe_dsc(struct ope_ctx *ctx, struct ope_stripe *stripe,
> +			       unsigned int h_scale, unsigned int v_scale)
> +{

[ ... ]

> +	dsc_y->input_height = stripe->src.height;
> +
> +	/* Clip the output so the WE always receives the expected count */
> +	dsc_y->output_width  = DS_OUTPUT_PIX(sw, 0, h_scale);

[Severity: High]
Does this downscaler formula truncate to 0 under high downscale ratios?
For large downscale ratios, ope_calc_h_scale() bounds the last stripe's
input width to OPE_STRIPE_MIN_W (16). The integer division in DS_OUTPUT_PIX
with a large h_scale would yield 0, which gets programmed into the Write Engine
and triggers an IMG_SZ_VIOL or hardware hang.

[ ... ]

> +static void ope_job_finish(struct ope_ctx *ctx, enum vb2_buffer_state state)
> +{

[ ... ]

> +	dev_dbg(ctx->ope->dev, "done ctx=%p state=%u\n", ctx, state);
> +
> +	if (state == VB2_BUF_STATE_DONE)
> +		ope_try_schedule(ctx);
> +}

[Severity: Medium]
Will this indefinitely stall the hardware pipeline after a processing error?
If a hardware violation triggers RST_DONE, ope_job_finish() completes the
buffer with VB2_BUF_STATE_ERROR. Since ope_try_schedule() is skipped,
subsequent queued buffers are ignored and the hardware remains idle.

[ ... ]

> +static void ope_prog_stripe(struct ope_ctx *ctx, struct ope_stripe *stripe)
> +{

[ ... ]

> +		ope_write_wr(ope, OPE_BUS_WR_CLIENT_ADDR_IMAGE(i),
> +			     stripe->dst[i].addr);
> +		ope_write_wr(ope, OPE_BUS_WR_CLIENT_IMAGE_CFG_0(i),
> +			     (stripe->dst[i].height << 16) | stripe->dst[i].width);

[Severity: High]
Is it possible to program the hardware Write Engine with a size of 0 here?
If the downscaler computed an output_width of 0 for the last stripe,
stripe->dst[i].width would be 0, leading to a constraint violation.

[ ... ]

> +		ope_write_pp(ope, OPE_PP_CLC_DOWNSCALE_MN_DS_IMAGE_SIZE_CFG(base),
> +			     ((dsc->input_width - 1) << 16) | (dsc->input_height - 1));
> +		ope_write_pp(ope, OPE_PP_CLC_DOWNSCALE_MN_DS_MN_H_CFG(base),
> +			     dsc->phase_step_h);
> +		ope_write_pp(ope, OPE_PP_CLC_DOWNSCALE_MN_DS_MN_V_CFG(base),
> +			     dsc->phase_step_v);

[Severity: Medium]
Are the initial phase offsets missing here?
The driver programs the phase steps but does not program the initial phase
offsets (OPE_PP_CLC_DOWNSCALE_MN_DS_MN_H_PHASE_CFG and V_PHASE_CFG).
This resets the phase to 0 at the start of every 336-pixel boundary,
which can cause visible scaling discontinuities or seams.

[ ... ]

> +static void ope_sched_cancel(struct ope_ctx *ctx)
> +{

[ ... ]

> +	if (running) {
> +		reinit_completion(&ope->reset_complete);
> +		ope_write(ope, OPE_TOP_RESET_CMD, OPE_TOP_RESET_CMD_SW);
> +		wait_for_completion(&ope->reset_complete);
> +	}
> +}

[Severity: High]
Could this result in an unbounded wait?
wait_for_completion() will hang the process permanently if the hardware
fails to respond or if the interrupt is lost. Should this use a timeout?

> +
> +static void ope_run_work(struct work_struct *work)
> +{

[ ... ]

> +		if (!ope_job_ready(ctx))
> +			return;
> +		ctx_changed = (ope->prev_ctx != ctx);
> +		ope->prev_ctx = ctx;
> +		ope->curr_ctx = ctx;
> +	}
> +
> +	ope_run_job(ctx, ctx_changed);
> +}

[Severity: High]
Can ctx_changed incorrectly evaluate to false after a software reset?
If a hardware error resets the engine, the RST_DONE IRQ calls
ope_job_finish() which clears ope->curr_ctx but leaves ope->prev_ctx
intact. On the next job, this computes ctx_changed as false, preventing
the Image Quality modules from being properly re-enabled, leading
to corrupted frames.

> +
> +static void ope_try_schedule(struct ope_ctx *ctx)
> +{
> +	struct ope_dev *ope = ctx->ope;
> +
> +	if (!ope_job_ready(ctx))
> +		return;
> +
> +	scoped_guard(spinlock_irqsave, &ope->sched_lock) {
> +		if (ctx->queued || ope->curr_ctx)
> +			return;
> +		ctx->queued = true;
> +	}
> +
> +	schedule_work(&ope->run_work);
> +}

[Severity: High]
Could this reschedule the workqueue asynchronously after a streamoff?
Calling VIDIOC_STREAMOFF on the OPE_QUEUE_DISP_OUT queue leaves
ctx->started true (it is only cleared for FRAME_IN). If a hardware completion
interrupt fires concurrently after the software reset, the IRQ handler calls
ope_try_schedule(), which evaluates ctx->started as true and reschedules
run_work. This can run after cancel_work_sync() has returned, resulting in a
DMA Use-After-Free when it programs the hardware with freed buffer pointers.

[ ... ]

> +static irqreturn_t ope_irq(int irq, void *dev_id)
> +{

[ ... ]

> +	if (status & OPE_TOP_IRQ_STATUS_WE)
> +		ope_we_irq(ope, ctx);
> +
> +	if ((status & OPE_TOP_IRQ_STATUS_IDLE) && ctx) {
> +		struct ope_stripe *stripe = ope_current_stripe(ctx);

[Severity: High]
Should this return early if a hardware reset was triggered?
If ope_we_irq() detects a constraint violation, it issues a full hardware
reset (OPE_TOP_RESET_CMD_SW). However, execution continues here to evaluate
the IDLE status. If IDLE is set, it calls ope_job_finish(), dequeues the
buffer, and calls ope_try_schedule() which issues a new GO_CMD while the
hardware is mid-reset, potentially causing a bus hang.

[ ... ]

> +static void ope_adjust_power(struct ope_dev *ope)
> +{
> +	unsigned int loadavg = 0, loadpeak = 0, loadconfig = 0;
> +	unsigned long pixclk = 0;
> +	struct dev_pm_opp *opp;
> +	struct ope_ctx *ctx;
> +	int ret;
> +
> +	list_for_each_entry(ctx, &ope->ctx_list, list) {
> +		unsigned int fps;

[Severity: High]
Is it safe to iterate ope->ctx_list without acquiring ope->mutex?
Concurrent additions and deletions to ctx_list via ope_open() and
ope_release() can cause list corruption and kernel crashes.

[ ... ]

> +static void ope_stop_streaming(struct vb2_queue *q)
> +{

[ ... ]

> +	dev_dbg(ctx->ope->dev, "streaming stop ctx=%p queue=%u\n", ctx, idx);
> +
> +	if (idx == OPE_QUEUE_FRAME_IN) {
> +		ctx->started = false;
> +		ope_adjust_power(ctx->ope);
> +	}
> +
> +	ope_sched_cancel(ctx);

[Severity: High]
Does this safely stop the pipeline for the DISP_OUT queue?
It only clears ctx->started if the FRAME_IN queue is stopped. Calling
streamoff on DISP_OUT ignores this state update. As noted earlier, this can
lead to DMA Use-After-Free if a concurrent IRQ reschedules the workqueue.

[ ... ]

> +static int ope_init_vq(struct ope_ctx *ctx, unsigned int idx)
> +{
> +	struct ope_dev *ope = ctx->ope;
> +	struct ope_vq_priv *qpriv;
> +	struct vb2_queue *q = &ctx->queues[idx].vq;
> +
> +	qpriv = devm_kzalloc(ope->dev, sizeof(*qpriv), GFP_KERNEL);

[Severity: High]
Does this cause a memory leak on every open/close cycle?
Using devm_kzalloc() ties the lifetime of qpriv to the device binding rather
than the file descriptor. Repeatedly opening and closing the device node will
exhaust kernel memory since the context is destroyed in ope_release() but
the devm allocations are not freed.

[ ... ]

> +static int ope_s_fmt_vid_out(struct file *file, void *priv, struct v4l2_format *f)
> +{

[ ... ]

> +	fmt = ope_fmt_try(ctx->ope, false, &f->fmt.pix_mp);
> +	ctx->fmt_in.fmt		    = fmt;
> +	ctx->fmt_in.width	    = f->fmt.pix_mp.width;
> +	ctx->fmt_in.height	    = f->fmt.pix_mp.height;
> +	ctx->fmt_in.bytesperline    = f->fmt.pix_mp.plane_fmt[0].bytesperline;
> +	ctx->fmt_in.sizeimage	    = f->fmt.pix_mp.plane_fmt[0].sizeimage;

[Severity: Critical]
Is this missing synchronization?
This modifies multi-component state locklessly. Concurrent S_FMT ioctls can
cause format state tearing, combining a small sizeimage with large geometrical
dimensions. This would cause the hardware to DMA past the allocated buffer
bounds. The video device nodes appear to be registered without vdev.lock.

[ ... ]

> +static int ope_disp_set_fmt(struct v4l2_subdev *sd,
> +			    struct v4l2_subdev_state *state,
> +			    struct v4l2_subdev_format *fmt)
> +{

[ ... ]

> +	v4l_bound_align_image(&fmt->format.width,  OPE_MIN_W, OPE_MAX_W, 0,
> +			      &fmt->format.height, OPE_MIN_H, OPE_MAX_H, 0, 0);
> +	fmt->format.field      = V4L2_FIELD_NONE;
> +	fmt->format.colorspace = V4L2_COLORSPACE_SRGB;
> +
> +	pad_fmt  = v4l2_subdev_state_get_format(state, fmt->pad);
> +	*pad_fmt = fmt->format;
> +	return 0;
> +}

[Severity: Medium]
Should this propagate sink format changes to the source pad?
If fmt->pad is a sink pad, it updates the sink pad format but does not
automatically propagate those changes (like mbus_code or resolution) to the
corresponding source pad, which breaks the V4L2 subdev API contract.

[ ... ]

> +static int ope_release(struct file *file)
> +{

[ ... ]

> +	kfree(fh);
> +
> +	scoped_guard(mutex, &ope->mutex) {
> +		if (--ope->open_count != 0)
> +			return 0;
> +		ope->shared_ctx = NULL;
> +	}
> +
> +	ope_ctx_destroy(ctx);
> +
> +	return 0;
> +}

[Severity: High]
Does dropping the mutex here lead to list corruption?
ope_ctx_destroy() is called completely unlocked and executes
list_del(&ctx->list). A concurrent thread calling ope_open() will acquire
ope->mutex and execute list_add(), causing list corruption.

Also, if the device has been unbound, ope_dev might already be freed via
devres, making the access to ope->mutex a Use-After-Free.

[ ... ]

> +static const struct camss_isp_entity_desc ope_entity_descs[] = {
> +	[OPE_ENTITY_FRAME_IN] = {
> +		.name      = "ope_input",
> +		.obj_type  = MEDIA_ENTITY_TYPE_VIDEO_DEVICE,
> +		.function  = MEDIA_ENT_F_IO_V4L,
> +		.vdev.caps = V4L2_CAP_VIDEO_OUTPUT_MPLANE | V4L2_CAP_STREAMING,
> +		.vdev.drvdata = (void *)(uintptr_t)OPE_QUEUE_FRAME_IN,
> +		.vdev.fops = &ope_fops,
> +		.vdev.ioctl_ops = &ope_video_ioctl_ops,

[Severity: Critical]
Is .vdev.lock missing here?
Registering the video device without initializing vdev.lock causes video_ioctl2
to execute without serialization. This leads to the format state tearing issue
in S_FMT mentioned earlier.

[ ... ]

> +static void ope_v4l2_cleanup(struct ope_dev *ope)
> +{
> +	if (ope->pipeline) {
> +		camss_isp_pipeline_unregister(ope->pipeline);
> +		camss_isp_pipeline_free(ope->pipeline);
> +		ope->pipeline = NULL;
> +	}
> +	media_device_unregister(&ope->mdev);
> +	v4l2_device_unregister(&ope->v4l2_dev);
> +	media_device_cleanup(&ope->mdev);
> +	cancel_work_sync(&ope->run_work);
> +}

[Severity: High]
Could this cause a Use-After-Free during unbind?
The hardware and interrupts are not stopped before canceling the workqueue.
Since the IRQ is managed by devres, it is still active here. An interrupt
firing immediately after cancel_work_sync() can reschedule ope->run_work.
Once the unbind finishes and devres frees ope_dev, the newly scheduled work
will access freed memory.

> +
> +static int ope_soft_reset(struct ope_dev *ope)
> +{

[ ... ]

> +	reinit_completion(&ope->reset_complete);
> +	ope_write(ope, OPE_TOP_RESET_CMD, OPE_TOP_RESET_CMD_SW);
> +
> +	if (!wait_for_completion_timeout(&ope->reset_complete,
> +					 msecs_to_jiffies(OPE_RESET_TIMEOUT_MS))) {
> +		dev_err(ope->dev, "Reset timeout\n");
> +		pm_runtime_put(ope->dev);
> +		return -ETIMEDOUT;
> +	}

[Severity: High]
Will this unconditionally time out during probe?
The hardware interrupts (OPE_TOP_IRQ_MASK) are not enabled until
__ope_irq_init() is called during streamon. Because the reset interrupt is
masked at probe time, this wait_for_completion_timeout() can never succeed.

[Severity: Medium]
Does using the asynchronous pm_runtime_put() here cause a power leak?
If probe fails, devres automatically calls pm_runtime_disable(), which cancels
the pending asynchronous suspend work leaving the device powered on permanently.
This should use pm_runtime_put_sync().

[ ... ]

> +static int ope_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ope_dev *ope;
> +	int ret, irq;
> +
> +	ope = devm_kzalloc(dev, sizeof(*ope), GFP_KERNEL);

[Severity: High]
Does allocating ope_dev via devm_kzalloc() cause a Use-After-Free?
If the device is unbound via sysfs while userspace holds an open file descriptor,
devres will free ope_dev. Subsequent ioctls or the final close() will access
the freed memory (e.g., locking ope->mutex in ope_release). The core state
structure's lifetime should be tied to the V4L2 device's internal refcount.

[ ... ]

> +static struct platform_driver ope_driver = {
> +	.probe  = ope_probe,
> +	.remove = ope_remove,
> +	.driver = {
> +		.name           = OPE_NAME,
> +		.of_match_table = ope_dt_ids,
> +		.pm             = &ope_pm_ops,
> +	},
> +};

[Severity: Medium]
Is this missing the pm_ptr() wrapper macro?
Assigning &ope_pm_ops directly causes compiler warnings and creates dead code
linked into the kernel when CONFIG_PM is disabled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-camss-isp-ope-v4-0-51207a0319d8@oss.qualcomm.com?part=6