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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 17 potential issue(s) to consider:
- [High] media: qcom: camss: UAF on qos_req and late enable_irq() in ope_probe()
- [High] media: qcom: camss: lost edge IRQ race in ope_runtime_resume() and ope_irq()
- [High] media: qcom: camss: premature runtime PM enable in ope_init_power()
- [High] media: qcom: camss: split locks and missing pipeline check in ope_s_fmt*()
- [High] media: qcom: camss: NULL sd->v4l2_dev dereference in ope_state_is_busy()
- [High] media: qcom: camss: ope: race between reset and __ope_irq() restarts DMA
- [High] media: qcom: camss: race with in-flight IRQ in ope_flush_hw()
- [High] media: qcom: camss: unbalanced pipeline stop and mdev teardown in ope_unregister()
- [Medium] media: qcom: camss: unreleased 0 Hz OPP and premature BW drop on stream stop
- [Medium] media: qcom: camss: bogus HW reset on params stop and curr_ctx clear in cancel
- [Medium] media: qcom: camss: broken custom vb2 wrappers in ope_poll() and ope_dqbuf()
- [Medium] media: qcom: camss: V4L2 UAPI compliance bugs in format and parm callbacks
- [Medium] media: qcom: camss: signed shift overflow in DS_RESOLUTION() macro
- [Medium] media: qcom: camss: unaligned C-plane DMA and stride overflow for NV24/NV42
- [Medium] media: qcom: camss: missing ope_hw_available() check in ope_open()
- [Medium] media: qcom: camss: IRQ masks cleared by ope_hw_reset_poll() in probe
- [Medium] media: qcom: camss: ope: race between ope_start_streaming() and subdev ioctls
--

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

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

This commit adds a V4L2 memory-to-memory ISP driver for the Qualcomm
Offline Processing Engine (OPE), supporting raw Bayer to YUV conversion,
white balance, demosaic, color correction, gamma LUT, chroma enhancement,
and downscaling via stripe-based processing. It also introduces a
declarative media-controller topology builder and V4L2 ISP parameter buffer
validation helpers.

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
> new file mode 100644
> index 0000000000000..e111fd1b2b1a2
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/ope/core.c

[ ... ]

> +/* Downscaler fixed-point helpers */
> +#define Q21(v)			(((uint64_t)(v)) << 21)
> +#define DS_Q21(n, d)		((uint32_t)div_u64((uint64_t)(n) << 21, (d)))
> +#define DS_RESOLUTION(in, out) \
> +	(((out) * 128 <= (in)) ? 0x0 : \
> +	 ((out) * 16  <= (in)) ? 0x1 : \
> +	 ((out) * 8   <= (in)) ? 0x2 : 0x3)

[ ... ]

> +static void ope_gen_stripe_yuv_dst(struct ope_ctx *ctx, struct ope_stripe *stripe,
> +				   dma_addr_t dst, const struct v4l2_rect *compose)
> +{

[ ... ]

> +	/* UV plane (appended to Y plane)*/
> +	c_stride = bpl * 2 / fo->fmt->c_hsub;
> +	base = dst + bpl * img_h + (compose->top / fo->fmt->c_vsub) * c_stride;

[ ... ]

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

[ ... ]

> +	/* Downscalers */
> +	for (int i = 0; i < OPE_DS_MAX; i++) {
> +		struct ope_dsc_config *dsc = &stripe->dsc[i];
> +		u32 base = ope_ds_base[i];
> +		u32 cfg = 0;
> +
> +		if (dsc->input_width != dsc->output_width) {
> +			dsc->phase_step_h |=
> +				DS_RESOLUTION(dsc->input_width,
> +					      dsc->output_width) << 30;
> +			cfg |= OPE_PP_DOWNSCALE_MN_DS_CFG_H_SCALE_EN;
> +		}
> +		if (dsc->input_height != dsc->output_height) {
> +			dsc->phase_step_v |=
> +				DS_RESOLUTION(dsc->input_height,
> +					      dsc->output_height) << 30;
> +			cfg |= OPE_PP_DOWNSCALE_MN_DS_CFG_V_SCALE_EN;
> +		}

[Severity: Medium]
In drivers/media/platform/qcom/camss/ope/core.c, DS_RESOLUTION() evaluates
to signed int literals (0x0, 0x1, 0x2, or 0x3).

When ope_prog_stripe() shifts the result of DS_RESOLUTION() left by 30 bits
for downscale factors greater than 1/16 (such as standard 2:1 chroma
subsampling where DS_RESOLUTION() evaluates to 0x2 or 0x3), does this shift
into the sign bit of a 32-bit signed int and trigger signed integer
overflow undefined behavior?

Would using unsigned constants (0x0U..0x3U) or FIELD_PREP() with GENMASK(31,
30) avoid this?

[ ... ]

> +static void ope_apply_params(struct ope_ctx *ctx)
> +{
> +	struct vb2_v4l2_buffer *vbuf;
> +	int ret;
> +
> +	vbuf = ope_queue_remove(ctx, OPE_QUEUE_PARAMS);
> +	if (!vbuf)
> +		return;
> +
> +	ret = camss_isp_params_apply(ctx->ope->dev, &vbuf->vb2_buf,
> +				     ctx->params_buf,
> +				     ope_params_type_info,
> +				     ope_params_handlers,
> +				     ARRAY_SIZE(ope_params_handlers),
> +				     ctx);
> +	if (ret)
> +		dev_warn_ratelimited(ctx->ope->dev, "invalid parameters\n");
> +
> +	vb2_buffer_done(&vbuf->vb2_buf, ret ? VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);
> +}

[ ... ]

> +static int ope_hw_reset_poll(struct ope_dev *ope)
> +{
> +	u32 status;
> +	int ret;
> +
> +	guard(mutex)(&ope->reset_mutex);
> +
> +	ope_write(ope, OPE_TOP_IRQ_MASK, OPE_TOP_IRQ_STATUS_RST_DONE);
> +	ope_write(ope, OPE_TOP_RESET_CMD, OPE_TOP_RESET_CMD_SW);
> +
> +	ret = read_poll_timeout(ope_read, status,
> +				status & OPE_TOP_IRQ_STATUS_RST_DONE,
> +				OPE_RESET_POLL_US,
> +				OPE_RESET_TIMEOUT_MS * USEC_PER_MSEC, false,
> +				ope, OPE_TOP_IRQ_STATUS);
> +
> +	ope_write(ope, OPE_TOP_IRQ_CLEAR, OPE_TOP_IRQ_STATUS_RST_DONE);
> +	ope_write(ope, OPE_TOP_IRQ_CMD, OPE_TOP_IRQ_CMD_CLEAR);
> +	ope_write(ope, OPE_TOP_IRQ_MASK, 0);
> +
> +	if (ret)
> +		return ret;
> +
> +	ope_glut_cache_invalidate(ope);
> +
> +	return 0;
> +}

[ ... ]

> +static void ope_flush_hw(struct ope_dev *ope)
> +{
> +	struct ope_ctx *ctx;
> +
> +	guard(mutex)(&ope->mutex);
> +
> +	scoped_guard(spinlock_irqsave, &ope->sched_lock)
> +		ctx = ope->curr_ctx;
> +
> +	if (!ctx)
> +		return;
> +
> +	if (ope_hw_reset_poll(ope))
> +		dev_err(ope->dev, "reset timeout while flushing\n");
> +
> +	scoped_guard(spinlock_irqsave, &ope->sched_lock)
> +		ope->hw_ctx = NULL;
> +
> +	ope_glut_cache_invalidate(ope);
> +
> +	ope_job_finish(ctx, VB2_BUF_STATE_ERROR);
> +
> +	/* Drain a handler that may still be reading registers */
> +	synchronize_irq(ope->irq);
> +}

[Severity: High]
As reported by Sashiko on v12 [1], during system suspend,
pm_runtime_force_suspend() disables runtime PM and calls
ope_runtime_suspend() -> ope_flush_hw() without waiting for an in-flight
ope_irq() that already passed pm_runtime_get_if_in_use(ope->dev).

In ope_flush_hw() in drivers/media/platform/qcom/camss/ope/core.c,
synchronize_irq(ope->irq) is only called at the end of the function and is
skipped entirely when ope->curr_ctx is NULL:

  1. If ope->curr_ctx is non-NULL, an in-flight __ope_irq() can run
     concurrently with ope_hw_reset_poll(), either programming stripe
     registers via ope_prog_stripe() during the reset or clearing
     OPE_TOP_IRQ_STATUS_RST_DONE before read_poll_timeout() sees it.

  2. If the in-flight __ope_irq() just completed the last stripe and cleared
     ope->curr_ctx in ope_job_finish(), ope_flush_hw() sees !ctx and
     returns early without calling synchronize_irq(ope->irq), allowing
     ope_runtime_suspend() to call clk_bulk_disable_unprepare() while
     __ope_irq() is still accessing MMIO registers.

Should synchronize_irq(ope->irq) be called at the start of ope_flush_hw()
before checking ope->curr_ctx?

> +
> +static void ope_sched_cancel(struct ope_ctx *ctx)
> +{
> +	struct ope_dev *ope = ctx->ope;
> +	bool running;
> +
> +	ope_sched_pause(ope);
> +
> +	scoped_guard(spinlock_irqsave, &ope->sched_lock) {
> +		running = (ope->hw_running && ope->hw_ctx == ctx);
> +		ctx->queued = false;
> +	}
> +
> +	/* If the hardware is running the context, reset it */
> +	if (running) {
> +		if (ope_hw_reset_sync(ope)) {
> +			dev_err(ope->dev, "reset timeout during cancel\n\");
> +			ope_job_finish(ctx, VB2_BUF_STATE_ERROR);
> +		}
> +	}
> +
> +	/* Drain any handler that loaded ctx before hw_running was cleared */
> +	synchronize_irq(ope->irq);
> +
> +	/* Mark the context as no longer queued/running */
> +	scoped_guard(spinlock_irqsave, &ope->sched_lock) {
> +		if (ope->curr_ctx) {
> +			ope->curr_ctx->queued = false;
> +			ope->curr_ctx = NULL;
> +		}
> +
> +		if (ope->hw_ctx == ctx) {
> +			ope->hw_ctx = NULL;
> +			ope->hw_running = false;
> +		}
> +	}
> +}

[Severity: High]
In drivers/media/platform/qcom/camss/ope/core.c, __ope_irq() snapshots ctx
under ope->sched_lock:

	scoped_guard(spinlock, &ope->sched_lock)
		ctx = ope->hw_running ? ope->hw_ctx : NULL;

and then releases ope->sched_lock before handling OPE_TOP_IRQ_STATUS_WE
(ope_we_irq() -> ope_prog_stripe()) and OPE_TOP_IRQ_STATUS_IDLE
(ope_start()).

In ope_sched_cancel() (and similarly in ope_flush_hw()), ope->hw_running is
not cleared before issuing the software reset, and synchronize_irq(ope->irq)
is called before the locked block that sets ope->hw_running = false.

If __ope_irq() on another CPU has already loaded a non-NULL ctx for a
stripe RUP_DONE or IDLE interrupt just as ope_hw_reset_sync() writes
OPE_TOP_RESET_CMD_SW, can that in-flight __ope_irq() execute
ope_prog_stripe() and ope_start() during or right after the reset,
restarting DMA on buffers that are simultaneously returned to vb2 and
unmapped?

[ ... ]

> +static irqreturn_t ope_irq(int irq, void *dev_id)
> +{
> +	struct ope_dev *ope = dev_id;
> +	irqreturn_t ret;
> +
> +	/*
> +	 * Increment the usage counter only to guarantee registers
> +	 * accessibility + ensure that IRQ is expected (job running).
> +	 * A negative return means runtime PM is disabled, i.e. the device is
> +	 * suspending or being unbound, and the registers are not clocked.
> +	 */
> +	if (pm_runtime_get_if_in_use(ope->dev) <= 0)
> +		return IRQ_NONE;
> +
> +	ret = __ope_irq(ope);
> +
> +	pm_runtime_put(ope->dev);
> +
> +	return ret;
> +}

[ ... ]

> +/* -------- Power scaling -------- */
> +
> +static inline unsigned long ope_pixclk(const struct ope_fmt_state *fs, unsigned int fps)
> +{
> +	return (unsigned long)fs->width * fs->height * fps;
> +}

[ ... ]

> +static void ope_adjust_power(struct ope_dev *ope)
> +{

[ ... ]

> +	opp = dev_pm_opp_find_freq_ceil(ope->dev, &pixclk);
> +	if (IS_ERR(opp)) {
> +		unsigned long max_freq = ULONG_MAX;
> +
> +		dev_warn(ope->dev, "Requested pixel clock %luHz exceeds hardware limit\n", pixclk);
> +
> +		/* Clamp to the highest available OPP */
> +		opp = dev_pm_opp_find_freq_floor(ope->dev, &max_freq);
> +		if (!IS_ERR(opp)) {
> +			pixclk = max_freq;
> +			dev_pm_opp_put(opp);
> +		}
> +	} else {
> +		dev_pm_opp_put(opp);
> +	}
> +
> +	ret = dev_pm_opp_set_rate(ope->dev, pixclk);
> +	if (ret)
> +		dev_warn(ope->dev, "Failed to set OPP rate: %d\n", ret);
> +
> +	ret = icc_set_bw(ope->icc_data, loadavg, loadpeak);
> +	if (ret)
> +		dev_warn(ope->dev, "Failed to set data path BW: %d\n", ret);
> +
> +	ret = icc_set_bw(ope->icc_config, loadconfig, loadconfig * 5);
> +	if (ret)
> +		dev_warn(ope->dev, "Failed to set config path BW: %d\n", ret);
> +
> +	cpu_latency_qos_update_request(&ope->qos_req, latency);
> +}
> +
> +static int ope_start_streaming(struct vb2_queue *q, unsigned int count)
> +{
> +	struct ope_ctx *ctx = ope_ctx_from_vq(q);
> +	struct video_device *vdev = ope_vdev_from_vq(q);
> +	unsigned int idx = ope_idx_from_vq(q);
> +	int ret;
> +
> +	dev_dbg(ctx->ope->dev, "streaming start ctx=%p queue=%u\n", ctx, idx);
> +
> +	if (!ope_hw_available(ctx->ope)) {
> +		ret = -ENODEV;
> +		goto err_drain;
> +	}
> +
> +	ret = video_device_pipeline_alloc_start(vdev);
> +	if (ret)
> +		goto err_drain;
> +
> +	ret = pm_runtime_resume_and_get(ctx->ope->dev);
> +	if (ret) {
> +		dev_err(ctx->ope->dev, "resume failed (%d)\n", ret);
> +		goto err_pipeline_stop;
> +	}
> +
> +	/* From here on a job is allowed to consume this queue's buffers */
> +	scoped_guard(spinlock_irqsave, &ctx->queues_lock)
> +		ctx->queues[idx].streaming = true;

[ ... ]

> +static void ope_stop_streaming(struct ope_ctx *ctx_unused_in_sig)
> +{
> +	struct ope_ctx *ctx = ope_ctx_from_vq(q);
> +	struct video_device *vdev = ope_vdev_from_vq(q);
> +	unsigned int idx = ope_idx_from_vq(q);
> +
> +	dev_dbg(ctx->ope->dev, "streaming stop ctx=%p queue=%u\n", ctx, idx);
> +
> +	scoped_guard(spinlock_irqsave, &ctx->queues_lock)
> +		ctx->queues[idx].streaming = false;
> +
> +	if (idx == OPE_QUEUE_FRAME_IN)
> +		ope_adjust_power(ctx->ope);
> +
> +	/* Drain with the scheduler held off, so no job can pick up a buffer */
> +	ope_sched_cancel(ctx);
> +	ope_queue_drain(ctx, idx, VB2_BUF_STATE_ERROR);
> +	ope_sched_resume(ctx->ope);
> +
> +	pm_runtime_put(ctx->ope->dev);
> +
> +	if (video_device_pipeline(vdev))
> +		video_device_pipeline_stop(vdev);
> +}

[Severity: Medium]
As reported by Sashiko on v12 [2], there are three power scaling issues in
ope_adjust_power(), ope_stop_streaming(), and ope_pixclk() in
drivers/media/platform/qcom/camss/ope/core.c:

  1. When all contexts stop streaming on OPE_QUEUE_FRAME_IN, pixclk is 0,
     but ope_adjust_power() unconditionally calls
     dev_pm_opp_find_freq_ceil(ope->dev, &pixclk). That matches the lowest
     OPP in the table and overwrites pixclk with a non-zero frequency
     before dev_pm_opp_set_rate(ope->dev, pixclk) is called, so
     dev_pm_opp_set_rate(ope->dev, 0) is never called to release the
     required-OPP vote on the cx power domain while idle.

  2. In ope_stop_streaming(), when idx == OPE_QUEUE_FRAME_IN,
     ope_adjust_power(ctx->ope) is called before ope_sched_cancel(ctx),
     dropping icc_data and icc_config bandwidth votes to 0 while an
     in-flight job may still be performing DMA and before
     ope_hw_reset_sync() accesses registers to reset the hardware.

  3. In ope_pixclk(), (unsigned long)fs->width * fs->height * fps can reach
     5376 * 8192 * 1000 = 44,040,192,000, which overflows a 32-bit unsigned
     long on 32-bit COMPILE_TEST builds.

Could ope_stop_streaming() call ope_adjust_power() after
ope_sched_cancel(), and could ope_adjust_power() skip
dev_pm_opp_find_freq_ceil() when pixclk is 0?

[Severity: Medium]
As reported by Sashiko on v12 [3], there are also three queue and context
state issues across ope_stop_streaming(), ope_apply_params(), and
ope_sched_cancel() in drivers/media/platform/qcom/camss/ope/core.c:

  1. In ope_stop_streaming() (and ope_queues_release()), stopping or
     releasing OPE_QUEUE_PARAMS unconditionally calls ope_sched_cancel(ctx).
     Because parameter buffers are copied by the CPU in ope_apply_params()
     before ope->hw_running is set and are never used by hardware DMA,
     does stopping ope_params unnecessarily reset the hardware and abort
     in-flight video frames with VB2_BUF_STATE_ERROR?

  2. In ope_apply_params(), ope_queue_remove(ctx, OPE_QUEUE_PARAMS)
     dequeues and completes parameter buffers without checking
     ctx->queues[OPE_QUEUE_PARAMS].streaming. If VIDIOC_STREAMON is called
     on ope_params while video streaming is already active,
     vb2_start_streaming() queues pre-queued buffers via ope_buf_queue()
     before calling ope_start_streaming(), allowing a concurrent
     ope_run_job() to complete a parameter buffer before OPE_QUEUE_PARAMS
     is marked streaming.

  3. At the end of ope_sched_cancel(), the cleanup block checks
     if (ope->curr_ctx) instead of if (ope->curr_ctx == ctx), clearing
     ope->curr_ctx->queued and ope->curr_ctx even if another context owns
     ope->curr_ctx.

Should ope_apply_params() check ctx->queues[OPE_QUEUE_PARAMS].streaming,
ope_stop_streaming() avoid cancelling in-flight hardware jobs when idx ==
OPE_QUEUE_PARAMS, and ope_sched_cancel() check ope->curr_ctx == ctx?

[ ... ]

> +static int ope_init_vq(struct ope_ctx *ctx, unsigned int idx)
> +{

[ ... ]

> +	q->drv_priv = qpriv;
> +	q->ops = &ope_vb2_ops;
> +	q->lock = &ctx->vbq_lock;

[ ... ]

> +	/* Only valid because there is a single device-wide ope_ctx for now */
> +	qpriv->vdev->queue = q;
> +
> +	return 0;
> +}

[ ... ]

> +static const struct ope_fmt *ope_fmt_try(struct ope_dev *ope, bool is_output,
> +					 struct v4l2_pix_format_mplane *pix)
> +{

[ ... ]

> +	/* Chroma-subsampled output formats need an even height */
> +	halign = (is_output && fmt->c_vsub == 2) ? 1 : 0;
> +
> +	v4l_bound_align_image(&pix->width,  OPE_MIN_W, max_w, fmt->align,
> +			      &pix->height, OPE_MIN_H, max_h, halign, 0);

[ ... ]

> +	/* The WE requires the C plane base address to be 16-byte aligned.
> +	 * c_base = bpl * img_h + (compose_top / c_vsub) * c_stride where
> +	 * c_stride = bpl * 2 / c_hsub.  To guarantee alignment for any
> +	 * compose_top, bpl must be a multiple of 16 / c_hsub:
> +	 *   NV12/NV16 (c_hsub=2): bpl must be multiple of 8... but c_stride=bpl
> +	 *   so we need bpl % 16 == 0.
> +	 *   NV24/NV42 (c_hsub=1): c_stride=bpl*2, need bpl*2 % 16 == 0,
> +	 *   so bpl must be a multiple of 8.
> +	 *   GREY: no C plane, no constraint.
> +	 */
> +	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: Medium]
As reported by Sashiko on v12 [4], in ope_fmt_try() and
ope_gen_stripe_yuv_dst() in drivers/media/platform/qcom/camss/ope/core.c,
for V4L2_PIX_FMT_NV24 and V4L2_PIX_FMT_NV42 (where c_hsub == 1 and
c_vsub == 1):

  1. ope_fmt_try() sets align = 8 * fmt->c_hsub = 8 and halign = 0,
     allowing bytesperline to be an odd multiple of 8 (such as 24) and
     pix->height to be odd (such as 17). In ope_gen_stripe_yuv_dst(), the
     UV plane base address dst + bpl * img_h + (compose->top / c_vsub) *
     c_stride will then only be 8-byte aligned, violating the Write Engine
     16-byte alignment requirement.

  2. ope_fmt_try() clamps bytesperline to ALIGN_DOWN(OPE_MAX_STRIDE, align)
     (65528), but ope_gen_stripe_yuv_dst() computes c_stride = bpl * 2 /
     fo->fmt->c_hsub = bpl * 2 for NV24/NV42, overflowing the 16-bit
     OPE_MAX_STRIDE limit whenever bytesperline > 32767.

Should ope_fmt_try() align bytesperline to 16 bytes for all semi-planar YUV
output formats and cap bytesperline so that c_stride = bpl * 2 /
fmt->c_hsub does not exceed OPE_MAX_STRIDE?

[ ... ]

> +static int ope_s_fmt_vid_out(struct file *file, void *priv, struct v4l2_format *f)
> +{
> +	struct ope_ctx *ctx = ope_ctx_from_file(file);
> +	const struct ope_fmt *fmt;
> +
> +	if (vb2_is_busy(&ctx->queues[OPE_QUEUE_FRAME_IN].vq))
> +		return -EBUSY;
> +
> +	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;

[ ... ]

> +static int ope_s_fmt_vid_cap(struct file *file, void *priv, struct v4l2_format *f)
> +{
> +	struct ope_ctx *ctx = ope_ctx_from_file(file);
> +	const struct ope_fmt *fmt;
> +
> +	if (vb2_is_busy(&ctx->queues[OPE_QUEUE_DISP_OUT].vq))
> +		return -EBUSY;
> +
> +	fmt = ope_fmt_try(ctx->ope, true, &f->fmt.pix_mp);
> +
> +	ctx->fmt_out.fmt	    = fmt;
> +	ctx->fmt_out.width	    = f->fmt.pix_mp.width;
> +	ctx->fmt_out.height	    = f->fmt.pix_mp.height;
> +	ctx->fmt_out.bytesperline   = f->fmt.pix_mp.plane_fmt[0].bytesperline;
> +	ctx->fmt_out.sizeimage	    = f->fmt.pix_mp.plane_fmt[0].sizeimage;
> +	ctx->fmt_out.colorspace	    = f->fmt.pix_mp.colorspace;
> +	ctx->fmt_out.xfer_func	    = f->fmt.pix_mp.xfer_func;
> +	ctx->fmt_out.ycbcr_enc	    = f->fmt.pix_mp.ycbcr_enc;
> +	ctx->fmt_out.quantization   = f->fmt.pix_mp.quantization;
> +
> +	return 0;
> +}
> +
> +static int ope_g_fmt_meta(struct file *file, void *priv, struct v4l2_format *f)
> +{
> +	f->fmt.meta.dataformat = V4L2_META_FMT_QCOM_ISP_PARAMS;
> +	f->fmt.meta.buffersize = v4l2_isp_buffer_size(CAMSS_PARAMS_OPE_MAX_PAYLOAD);
> +
> +	return 0;
> +}
> +
> +static int ope_enum_fmt_meta_out(struct file *file, void *priv, struct v4l2_fmtdesc *f)
> +{
> +	if (f->index > 0)
> +		return -EINVAL;
> +
> +	f->pixelformat = V4L2_META_FMT_QCOM_ISP_PARAMS;
> +
> +	return 0;
> +}
> +
> +static int ope_g_parm(struct file *file, void *priv, struct v4l2_streamparm *sp)
> +{
> +	struct ope_ctx *ctx = ope_ctx_from_file(file);
> +
> +	if (!V4L2_TYPE_IS_OUTPUT(sp->type))
> +		return -EINVAL;
> +
> +	sp->parm.output.capability = V4L2_CAP_TIMEPERFRAME;
> +	sp->parm.output.timeperframe = ctx->fmt_in.timeperframe;
> +
> +	return 0;
> +}
> +
> +static int ope_s_parm(struct file *file, void *priv, struct v4l2_streamparm *sp)
> +{
> +	struct ope_ctx *ctx = ope_ctx_from_file(file);
> +	struct v4l2_fract *tpf = &sp->parm.output.timeperframe;
> +
> +	if (!V4L2_TYPE_IS_OUTPUT(sp->type))
> +		return -EINVAL;
> +
> +	if (vb2_is_busy(&ctx->queues[OPE_QUEUE_FRAME_IN].vq))
> +		return -EBUSY;

[Severity: High]
As reported by Sashiko on v12 [5], in isp_register_vdev() in
drivers/media/platform/qcom/camss/ope/pipeline.c, vdev->lock is set to
&pipeline->vdev_lock, whereas ope_init_vq() in
drivers/media/platform/qcom/camss/ope/core.c sets q->lock = &ctx->vbq_lock
and assigns qpriv->vdev->queue = q.

Because v4l2_ioctl_get_lock() uses vdev->queue->lock (&ctx->vbq_lock) for
INFO_FL_QUEUE ioctls (REQBUFS, CREATE_BUFS, PREPARE_BUF, QBUF, STREAMON)
and vdev->lock (&pipeline->vdev_lock) for non-queue ioctls (S_FMT, G_FMT,
S_PARM, G_PARM), ope_s_fmt_vid_out(), ope_s_fmt_vid_cap(), and ope_s_parm()
check vb2_is_busy() and mutate ctx->fmt_in, ctx->fmt_out, and
ctx->framerate without holding ctx->vbq_lock. Can a concurrent S_FMT race
with REQBUFS/QBUF/STREAMON and update ctx->fmt_in or ctx->fmt_out to a
larger frame size after smaller buffers have been allocated and queued,
causing ope_gen_stripes() to program out-of-bounds DMA transfers?

Furthermore, ope_s_fmt_vid_out() and ope_s_fmt_vid_cap() only check
vb2_is_busy() on their own queue rather than checking whether the media
pipeline is already active (e.g. via video_device_pipeline(vdev)). Since
video_device_pipeline_alloc_start() only validates media links when
pipe->start_count == 0, if userspace starts streaming on ope_params or
ope_input first and then calls VIDIOC_S_FMT on ope_disp_output before
starting streaming on ope_disp_output, won't that bypass media link
validation?

> +
> +	if (!tpf->denominator)
> +		tpf->denominator = 1;
> +	if (!tpf->numerator)
> +		tpf->numerator = 1;
> +
> +	ctx->framerate = clamp(tpf->denominator / tpf->numerator, 1U, (u32)OPE_MAX_FRAMERATE);
> +	tpf->numerator = 1;
> +	tpf->denominator = ctx->framerate;
> +
> +	ctx->fmt_in.timeperframe = *tpf;
> +	sp->parm.output.capability = V4L2_CAP_TIMEPERFRAME;
> +
> +	return 0;
> +}
> +
> +static int ope_enum_framesizes(struct file *file, void *priv, struct v4l2_frmsizeenum *fsize)
> +{
> +	unsigned int idx = ope_queue_idx_from_file(file);
> +	const struct ope_fmt *fmts;
> +	unsigned int n;
> +
> +	if (idx == OPE_QUEUE_FRAME_IN) {
> +		fmts = ope_input_fmts;
> +		n = ARRAY_SIZE(ope_input_fmts);
> +	} else {
> +		fmts = ope_output_fmts;
> +		n = ARRAY_SIZE(ope_output_fmts);
> +	}
> +
> +	if (fsize->index > 0 || !ope_find_fmt(fmts, n, fsize->pixel_format))
> +		return -EINVAL;
> +
> +	fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
> +	fsize->stepwise.min_width = OPE_MIN_W;
> +	fsize->stepwise.max_width = OPE_MAX_W;
> +	fsize->stepwise.step_width = 1;
> +	fsize->stepwise.min_height = OPE_MIN_H;
> +	fsize->stepwise.max_height = OPE_MAX_H;
> +	fsize->stepwise.step_height = 1;
> +
> +	return 0;
> +}

[Severity: Medium]
There are a few V4L2 UAPI compliance issues in these format and streamparm
callbacks in drivers/media/platform/qcom/camss/ope/core.c:

  1. ope_enum_framesizes() reports step_width = 1 and step_height = 1 for
     all formats, whereas ope_fmt_try() aligns width to 1 << fmt->align (4
     for 10-bit packed Bayer input formats, 2 for NV12/NV21/NV16/NV61
     output formats) and aligns height to 2 when c_vsub == 2.

  2. ope_video_ioctl_ops is shared between ope_input and ope_disp_output.
     When VIDIOC_G_PARM or VIDIOC_S_PARM is called on the capture node
     ope_disp_output, ope_g_parm() and ope_s_parm() return -EINVAL instead
     of -ENOTTY (and ope_s_parm() returns -EBUSY when buffers are allocated).

  3. isp_register_vdev() sets V4L2_CAP_IO_MC on ope_params, but
     ope_enum_fmt_meta_out() does not reject unsupported non-zero
     f->mbus_code values (other than MEDIA_BUS_FMT_FIXED).

  4. isp_register_subdev() sets V4L2_SUBDEV_FL_HAS_DEVNODE on ope_disp,
     which implements .get_fmt and .set_fmt in ope_disp_pad_ops, but omits
     .enum_mbus_code.

Should these callbacks be updated to match the V4L2 and MC specifications?

[ ... ]

> +static int ope_dqbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
> +{
> +	struct ope_ctx *ctx = ope_ctx_from_file(file);
> +	struct vb2_queue *vq = ope_vq_from_file(file, ctx);
> +
> +	return vq ? vb2_dqbuf(vq, buf, file->f_flags & O_NONBLOCK) : -EINVAL;
> +}

[ ... ]

> +static int ope_create_bufs(struct file *file, void *priv, struct v4l2_create_buffers *create)
> +{
> +	struct ope_ctx *ctx = ope_ctx_from_file(file);
> +	struct vb2_queue *vq = ope_vq_from_file(file, ctx);
> +	int ret;
> +
> +	if (!vq)
> +		return -EINVAL;
> +	if (vb2_queue_is_busy(vq, file))
> +		return -EBUSY;
> +	ret = vb2_create_bufs(vq, create);
> +	if (!ret && create->count)
> +		vq->owner = file->private_data;
> +
> +	return ret;
> +}

[ ... ]

> +/* True as soon as any of the context's video nodes is streaming */
> +static bool ope_ctx_is_streaming(struct ope_ctx *ctx)
> +{
> +	guard(spinlock_irqsave)(&ctx->queues_lock);
> +
> +	for (unsigned int i = 0; i < OPE_QUEUE_COUNT; i++)
> +		if (ctx->queues[i].streaming)
> +			return true;
> +
> +	return false;
> +}
> +
> +static bool ope_state_is_busy(struct v4l2_subdev *sd, struct v4l2_subdev_state *state)
> +{
> +	struct ope_dev *ope = container_of(sd->v4l2_dev, struct ope_dev, v4l2_dev);
> +	struct ope_ctx *ctx;
> +	bool busy = false;
> +
> +	if (state != sd->active_state)
> +		return false;
> +
> +	scoped_guard(spinlock, &ope->ctx_lock)
> +		list_for_each_entry(ctx, &ope->ctx_list, list)
> +			busy |= ope_ctx_is_streaming(ctx);
> +
> +	return busy;
> +}

[Severity: Medium]
In ope_start_streaming() in drivers/media/platform/qcom/camss/ope/core.c,
video_device_pipeline_alloc_start(vdev) validates the media pipeline links
and sets pipe->start_count = 1 before pm_runtime_resume_and_get() runs and
before ctx->queues[idx].streaming is set to true.

Because VIDIOC_SUBDEV_S_FMT and VIDIOC_SUBDEV_S_SELECTION on ope_proc and
ope_disp only lock the subdev active_state and check ope_state_is_busy()
(which only inspects ctx->queues[i].streaming), if a subdev format or
selection ioctl runs during the window between
video_device_pipeline_alloc_start() and setting
ctx->queues[idx].streaming = true, won't ope_state_is_busy() return false
and allow modifying the active subdev state after link validation has
already succeeded?

[ ... ]

> +static int ope_input_link_validate(struct media_link *link)
> +{
> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(link->sink->entity);
> +	struct ope_dev *ope = container_of(sd->v4l2_dev, struct ope_dev, v4l2_dev);
> +	struct ope_ctx *ctx = ope->shared_ctx;

[Severity: High]
In drivers/media/platform/qcom/camss/ope/core.c, both ope_state_is_busy()
and ope_input_link_validate() derive ope via:

	struct ope_dev *ope = container_of(sd->v4l2_dev, struct ope_dev, v4l2_dev);

During driver unbind, ope_unregister() -> camss_isp_pipeline_unregister()
calls v4l2_device_unregister_subdev(&slot->subdev), which sets
sd->v4l2_dev = NULL before unregistering the media entity and subdev
devnode (and without holding the subdev active_state lock or waiting for
in-flight subdev_do_ioctl_lock() calls). Moreover,
camss_isp_pipeline_unregister() unregisters OPE_ENTITY_PROC before
OPE_ENTITY_FRAME_IN.

If a concurrent VIDIOC_SUBDEV_S_FMT or VIDIOC_SUBDEV_S_SELECTION on
ope_proc/ope_disp reaches ope_state_is_busy(), or a concurrent
VIDIOC_STREAMON on ope_input reaches ope_input_link_validate() after
sd->v4l2_dev is cleared to NULL, will container_of(sd->v4l2_dev, ...)
compute an invalid pointer and cause a NULL pointer dereference when
accessing ope->ctx_lock or ope->shared_ctx?

[ ... ]

> +static int ope_open(struct file *file)
> +{
> +	struct video_device *vdev = video_devdata(file);
> +	struct ope_dev *ope = container_of(vdev->v4l2_dev, struct ope_dev, v4l2_dev);
> +	struct v4l2_fh *fh;
> +	int ret = 0;
> +
> +	fh = kzalloc_obj(*fh);
> +	if (!fh)
> +		return -ENOMEM;
> +
> +	if (mutex_lock_interruptible(&ope->mutex)) {
> +		kfree(fh);
> +		return -ERESTARTSYS;
> +	}
> +
> +	v4l2_fh_init(fh, vdev);
> +	v4l2_fh_add(fh, file);
> +
> +	/*
> +	 * For now, only a single shared context is supported,
> +	 * until media multi-context support is available.
> +	 */
> +	if (!ope->shared_ctx) {
> +		struct ope_ctx *ctx = ope_ctx_create(ope);

[Severity: Medium]
In ope_open() in drivers/media/platform/qcom/camss/ope/core.c, if a
process passes the video_is_registered(vdev) check in v4l2_open() and
blocks on mutex_lock_interruptible(&ope->mutex) while ope_unregister() runs
on another CPU, ope_unregister() will unregister the video devices and call
v4l2_device_unregister(&ope->v4l2_dev) under ope->mutex.

When ope_open() then acquires ope->mutex, it does not check
ope_hw_available(ope) before calling ope_ctx_create(ope). Can this allocate
a new ope_ctx and attach vb2 queues to already-unregistered video devices?

[ ... ]

> +static __poll_t ope_poll(struct file *file, poll_table *wait)
> +{
> +	unsigned int idx = ope_queue_idx_from_file(file);
> +	struct ope_ctx *ctx = ope_ctx_from_file(file);
> +	struct v4l2_fh *fh = file_to_v4l2_fh(file);
> +	struct vb2_queue *vq;
> +	unsigned long flags;
> +	__poll_t rc = 0;
> +
> +	if (idx >= OPE_QUEUE_COUNT)
> +		return EPOLLERR;
> +
> +	vq = &ctx->queues[idx].vq;
> +
> +	poll_wait(file, &vq->done_wq, wait);
> +	poll_wait(file, &fh->wait, wait);
> +
> +	spin_lock_irqsave(&vq->done_lock, flags);
> +	if (!vb2_is_streaming(vq) || vq->error) {
> +		rc |= EPOLLERR;
> +	} else if (!list_empty(&vq->done_list)) {
> +		if (V4L2_TYPE_IS_OUTPUT(vq->type))
> +			rc |= EPOLLOUT | EPOLLWRNORM;
> +		else
> +			rc |= EPOLLIN | EPOLLRDNORM;
> +	}
> +	spin_unlock_irqrestore(&vq->done_lock, flags);
> +
> +	if (v4l2_event_pending(fh))
> +		rc |= EPOLLPRI;
> +
> +	return rc;
> +}

[Severity: Medium]
As reported by Sashiko on v12 [6], since ope_init_vq() sets
qpriv->vdev->queue = q, the custom vb2 wrappers in
drivers/media/platform/qcom/camss/ope/core.c diverge from the standard
vb2_fop_* and vb2_ioctl_* helpers in three places:

  1. ope_poll() directly accesses private vb2_queue fields (vq->done_wq,
     vq->done_lock, vq->done_list, vq->error) without holding the queue
     lock and returns EPOLLERR whenever !vb2_is_streaming(vq), even when
     userspace is only polling for V4L2 events (EPOLLPRI).

  2. ope_dqbuf() omits the vb2_queue_is_busy(vq, file) check, allowing any
     open file descriptor to dequeue buffers from a queue owned by another
     file handle.

  3. ope_create_bufs() checks vb2_queue_is_busy(vq, file) before calling
     vb2_create_bufs(), rejecting create->count == 0 queries with -EBUSY.

Could the driver use the standard vb2_fop_poll, vb2_fop_mmap, and
vb2_ioctl_* helpers (or vb2_poll() and consistent ownership checks)?

[ ... ]

> +	[OPE_ENTITY_PROC] = {
> +		.name       = "ope_proc",
> +		.obj_type   = MEDIA_ENTITY_TYPE_V4L2_SUBDEV,
> +		.function   = MEDIA_ENT_F_PROC_VIDEO_ISP,
> +		.subdev.ops = &ope_proc_ops,
> +		.subdev.internal_ops = &ope_proc_internal_ops,
> +		.subdev.entity_ops   = &ope_proc_entity_ops,
> +		.pads = (const struct camss_isp_pad_desc[]) {
> +			{ MEDIA_PAD_FL_SINK,   OPE_ENTITY_FRAME_IN, 0, 0 },
> +			{ MEDIA_PAD_FL_SINK,   OPE_ENTITY_PARAMS, 0, 0 },
> +			{ MEDIA_PAD_FL_SOURCE, OPE_ENTITY_DISP, OPE_DISP_PAD_SINK,
> +			  MEDIA_LNK_FL_ENABLED },
> +			{ }
> +		},
> +	},

[ ... ]

> +static void ope_v4l2_dev_release(struct v4l2_device *v4l2_dev)
> +{
> +	struct ope_dev *ope = container_of(v4l2_dev, struct ope_dev, v4l2_dev);
> +	struct device *dev = ope->dev;
> +
> +	/* Reached once the V4L2 core has released every video and subdev node */
> +	camss_isp_pipeline_free(ope->pipeline);
> +	media_device_cleanup(&ope->mdev);
> +	mutex_destroy(&ope->mutex);
> +	mutex_destroy(&ope->reset_mutex);
> +	kfree(ope);
> +
> +	put_device(dev);
> +}

[ ... ]

> +static void ope_unregister(struct ope_dev *ope)
> +{
> +	struct ope_ctx *ctx;
> +	int ret;
> +
> +	ope_sched_pause(ope);
> +	ope_flush_hw(ope);
> +
> +	ret = pm_runtime_force_suspend(ope->dev);
> +	if (ret)
> +		pm_runtime_disable(ope->dev);
> +
> +	if (ope->pipeline)
> +		camss_isp_pipeline_unregister(ope->pipeline);
> +
> +	media_device_unregister(&ope->mdev);
> +
> +	scoped_guard(mutex, &ope->mutex) {
> +		cpu_latency_qos_remove_request(&ope->qos_req);
> +		v4l2_device_unregister(&ope->v4l2_dev);
> +	}

[Severity: High]
As reported by Sashiko on v12 [7], there are media pipeline lifecycle and
link flag issues in camss_isp_pipeline_unregister() in
drivers/media/platform/qcom/camss/ope/pipeline.c and ope_unregister(),
ope_start_streaming(), ope_stop_streaming(), and ope_entity_descs[] in
drivers/media/platform/qcom/camss/ope/core.c:

  1. When any video device calls video_device_pipeline_alloc_start(),
     pad->pipe is set on all pads in the connected graph, so
     video_device_pipeline(&slot->vdev) is non-NULL for all three video
     devices even if only one queue started streaming. In
     camss_isp_pipeline_unregister(), the reverse loop calls
     video_device_pipeline_stop(&slot->vdev) on every video device where
     video_device_pipeline(&slot->vdev) is non-NULL, which underflows
     pipe->start_count when non-streaming video devices are visited and
     also unregisters the subdevs at indices 3 and 2 before stopping the
     video devices at indices 1 and 0.

  2. In ope_unregister(), camss_isp_pipeline_unregister(ope->pipeline) and
     media_device_unregister(&ope->mdev) (which zeroes pad->graph_obj.mdev
     on all pads) run before v4l2_device_unregister(&ope->v4l2_dev) clears
     ope->v4l2_dev.dev and without serializing against ctx->vbq_lock. Can a
     concurrent ope_start_streaming() or ope_stop_streaming() dereference a
     NULL pad->graph_obj.mdev inside media_pipeline_alloc_start() or
     media_pipeline_stop()?

  3. In ope_entity_descs[], the link from OPE_ENTITY_PROC pad 2 to
     OPE_ENTITY_DISP pad 0 sets link_flags = MEDIA_LNK_FL_ENABLED without
     MEDIA_LNK_FL_IMMUTABLE, and no pads set MEDIA_PAD_FL_MUST_CONNECT. If
     userspace disables that link via MEDIA_IOC_SETUP_LINK before
     VIDIOC_STREAMON, won't media_pipeline_start() skip validating the
     ope_proc -> ope_disp -> ope_disp_output path while ope_run_job() still
     unconditionally uses the display write engines?

[ ... ]

> +static int ope_soft_reset(struct ope_dev *ope)
> +{
> +	u32 version;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(ope->dev);
> +	if (ret)
> +		return dev_err_probe(ope->dev, ret, "resume failed\n");
> +
> +	version = ope_read(ope, OPE_TOP_HW_VERSION);
> +	ope->hw_version = version;
> +	dev_dbg(ope->dev, "HW version %u.%u.%u\n",
> +		OPE_HW_VERSION_GEN(version),
> +		OPE_HW_VERSION_REV(version),
> +		OPE_HW_VERSION_STEP(version));
> +
> +	ope_dump_pipeline(ope);
> +
> +	ret = ope_hw_reset_poll(ope);
> +	if (ret)
> +		dev_err(ope->dev, "Reset timeout\n");
> +
> +	pm_runtime_put_sync(ope->dev);
> +
> +	return ret;
> +}

[Severity: Medium]
In ope_soft_reset() in drivers/media/platform/qcom/camss/ope/core.c,
pm_runtime_resume_and_get() invokes ope_runtime_resume(), which initializes
the interrupt masks via __ope_irq_init(ope). Immediately afterward,
ope_soft_reset() calls ope_hw_reset_poll(ope), which resets the hardware and
writes 0 to OPE_TOP_IRQ_MASK without calling __ope_irq_init(ope) again.

If the device does not actually runtime-suspend on pm_runtime_put_sync()
(for instance if runtime PM is kept active via sysfs power/control),
subsequent pm_runtime_resume_and_get() calls in ope_start_streaming() will
not invoke ope_runtime_resume(). Won't all OPE interrupts remain masked in
that case, causing jobs to hang?

[ ... ]

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

[ ... ]

> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

[ ... ]

> +static int ope_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	ret = ope_init_power(ope);
> +	if (ret) {
> +		ret = dev_err_probe(dev, ret, "Power init failed\n");
> +		goto err_free;
> +	}
> +
> +	ret = ope_init_mmio(ope);
> +	if (ret) {
> +		ret = dev_err_probe(dev, ret, "MMIO init failed\n");
> +		goto err_free;
> +	}
> +
> +	ret = ope_soft_reset(ope);
> +	if (ret)
> +		goto err_free;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		ret = dev_err_probe(dev, irq, "Unable to get IRQ\n");
> +		goto err_free;
> +	}
> +
> +	ope->irq = irq;
> +
> +	ret = devm_request_irq(dev, ope->irq, ope_irq, IRQF_NO_AUTOEN,
> +			       "camss-ope", ope);
> +	if (ret < 0) {
> +		ret = dev_err_probe(dev, ret, "Requesting IRQ failed\n");
> +		goto err_free;
> +	}

[Severity: High]
As reported by Sashiko on v12 [8], in ope_probe() in
drivers/media/platform/qcom/camss/ope/core.c, ope_init_power() calls
devm_pm_runtime_enable(dev) before ope_init_mmio() maps ope->base,
ope->base_rd, ope->base_wr, and ope->base_pp.

If a runtime resume is triggered after devm_pm_runtime_enable(dev) before
ope_init_mmio() finishes, ope_runtime_resume() -> __ope_irq_init(ope) will
dereference NULL MMIO pointers. In addition, if ope_probe() fails at any
step after ope_init_power(), err_free clears drvdata to NULL and frees ope
while runtime PM remains enabled until devres teardown runs after
ope_probe() returns.

Should runtime PM be enabled only after ope_init_mmio() succeeds, and
disabled before freeing ope on probe failure paths?

> +
> +	cpu_latency_qos_add_request(&ope->qos_req, PM_QOS_DEFAULT_VALUE);
> +
> +	/* ope ownership is transferred to ope_v4l2_register(), even on failure */
> +	ret = ope_v4l2_register(ope);
> +	if (ret) {
> +		cpu_latency_qos_remove_request(&ope->qos_req);
> +		platform_set_drvdata(pdev, NULL);
> +		return dev_err_probe(dev, ret, "V4L2 registration failed\n");
> +	}
> +
> +	/* Everything @ope_irq() touches is in place, let interrupts in */
> +	enable_irq(ope->irq);

[Severity: High]
As reported by Sashiko on v12 [9], in ope_probe() in
drivers/media/platform/qcom/camss/ope/core.c,
cpu_latency_qos_add_request(&ope->qos_req, PM_QOS_DEFAULT_VALUE) registers
&ope->qos_req before calling ope_v4l2_register(ope).

Every failure path inside ope_v4l2_register() invokes
ope_v4l2_dev_release(&ope->v4l2_dev), which calls kfree(ope). When
ope_v4l2_register(ope) returns an error to ope_probe(), doesn't
cpu_latency_qos_remove_request(&ope->qos_req) access the already-freed ope
structure?

Additionally, enable_irq(ope->irq) is called after ope_v4l2_register(ope)
has already published the video, subdev, and media device nodes to
userspace. Should enable_irq(ope->irq) be called before
ope_v4l2_register(ope), and cpu_latency_qos_remove_request(&ope->qos_req)
be handled before kfree(ope)?

[ ... ]

> +static int ope_runtime_resume(struct device *dev)
> +{
> +	struct ope_dev *ope = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = clk_bulk_prepare_enable(ope->nclks, ope->clks);
> +	if (ret)
> +		return ret;
> +
> +	__ope_irq_init(ope);
> +
> +	/*
> +	 * Let the contexts that are still streaming pick up a new job. The job
> +	 * is started from a work item, which may run before runtime PM reports
> +	 * the device active again, i.e. while ope_irq() still drops interrupts.
> +	 * Programming a job takes long enough for that window to be closed by
> +	 * the time the hardware can raise one.
> +	 */
> +	ope_sched_resume(ope);
> +
> +	return 0;
> +}

[Severity: High]
In ope_runtime_resume() in drivers/media/platform/qcom/camss/ope/core.c,
ope_sched_resume(ope) is called inside the .runtime_resume callback while
dev->power.runtime_status is still RPM_RESUMING (and while
dev->power.disable_depth > 0 during system resume in
pm_runtime_force_resume(), which only re-enables runtime PM after the
callback returns).

If the thread running ope_runtime_resume() is preempted after
ope_sched_resume(ope) schedules ope->run_work, ope_run_work() on another CPU
can program the first stripe and write GO_CMD via ope_start(ope). The
hardware then immediately latches the double-buffered registers and raises
its edge-triggered interrupt (such as RUP_DONE).

If ope_irq() runs before rpm_resume() sets runtime_status to RPM_ACTIVE (or
before pm_runtime_force_resume() calls pm_runtime_enable()),
pm_runtime_get_if_in_use(ope->dev) in ope_irq() returns 0 or -EINVAL, so
ope_irq() returns IRQ_NONE without reading or clearing OPE_TOP_IRQ_STATUS.
Won't leaving OPE_TOP_IRQ_STATUS asserted on an edge-triggered interrupt
line prevent any further rising edges and permanently wedge the pipeline?

[1] https://lore.kernel.org/r/20261102-camss-isp-ope-v12-6-702254689feb@oss.qualcomm.com
[2] https://lore.kernel.org/r/20261102-camss-isp-ope-v12-6-702254689feb@oss.qualcomm.com
[3] https://lore.kernel.org/r/20261102-camss-isp-ope-v12-6-702254689feb@oss.qualcomm.com
[4] https://lore.kernel.org/r/20261102-camss-isp-ope-v12-6-702254689feb@oss.qualcomm.com
[5] https://lore.kernel.org/r/20261102-camss-isp-ope-v12-6-702254689feb@oss.qualcomm.com
[6] https://lore.kernel.org/r/20261102-camss-isp-ope-v12-6-702254689feb@oss.qualcomm.com
[7] https://lore.kernel.org/r/20261102-camss-isp-ope-v12-6-702254689feb@oss.qualcomm.com
[8] https://lore.kernel.org/r/20261102-camss-isp-ope-v12-6-702254689feb@oss.qualcomm.com
[9] https://lore.kernel.org/r/20261102-camss-isp-ope-v12-6-702254689feb@oss.qualcomm.com

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