Re: [PATCH v13 9/9] arm64: defconfig: Enable Qualcomm CAMSS OPE driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 14 potential issue(s) to consider:
- [High] media: qcom: camss: ope: use-after-free on ope_v4l2_register() failure
- [Medium] arm64: dts: qcom: agatti: duplicate IOMMU stream IDs on camss and isp_ope
- [Medium] media: qcom: camss: premature consume and reset of params in ope_apply_params()
- [Medium] media: qcom: camss: vb2 ownership and poll bugs in ope_dqbuf() and ope_poll()
- [Medium] media: qcom: camss: invalid G_PARM and S_PARM on capture in ope_g_parm()
- [Medium] media: qcom: camss: wrong compose bounds origin in ope_proc_get_selection()
- [Medium] media: qcom: camss: missing enum_mbus_code callback in ope_disp_pad_ops
- [Medium] media: qcom: camss: max-size check and late validation in ope_buf_prepare()
- [Medium] media: qcom: camss: ope: 0 Hz OPP rounded up in ope_adjust_power()
- [Medium] media: qcom: camss: ope: split ioctl locks between vdev_lock and vbq_lock
- [Medium] media: qcom: camss: ope: unbalanced pipeline stop on unregistration
- [Medium] media: qcom: camss: premature ope_sched_resume() in ope_runtime_resume()
- [Low] media: qcom: camss: signed shift overflow in DS_RESOLUTION()
- [Low] media: qcom: camss: ope: incomplete kernel-doc in pipeline.h
--

commit e9b18dde076886d4503de413170d4ece303a05fe
Author: Nihal Kumar Gupta <nihal.gupta@oss.qualcomm.com>

arm64: defconfig: Enable Qualcomm CAMSS OPE driver

This series adds the Qualcomm Camera Subsystem Offline Processing Engine
(OPE) V4L2 memory-to-memory ISP driver, its media-controller pipeline and
parameter helpers, Devicetree nodes for Agatti and Shikra, and enables the
driver in arm64 defconfig.

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

> diff --git a/arch/arm64/boot/dts/qcom/agatti.dtsi b/arch/arm64/boot/dts/qcom/agatti.dtsi
> index d9f4cbfd2861c..8f2b451b8f6e8 100644
> --- a/arch/arm64/boot/dts/qcom/agatti.dtsi
> +++ b/arch/arm64/boot/dts/qcom/agatti.dtsi
> @@ -1990,6 +1990,83 @@ port@1 {
>  			};
>  		};
>  
> +		isp_ope: isp@5c42000 {
> +			compatible = "qcom,qcm2290-camss-ope";

[ ... ]

> +			iommus = <&apps_smmu 0x800 0x0>,
> +				 <&apps_smmu 0x820 0x0>,
> +				 <&apps_smmu 0x840 0x0>;

[Severity: Medium]
Does isp_ope conflict with the stream IDs already claimed by camss in
arch/arm64/boot/dts/qcom/agatti.dtsi?

Right above isp_ope, the camss@5c11000 node still lists the OPE CDM, read,
and write stream IDs alongside the VFE stream ID:

	camss: camss@5c11000 {
		...
		iommus = <&apps_smmu 0x400 0x0>,
			 <&apps_smmu 0x800 0x0>,
			 <&apps_smmu 0x820 0x0>,
			 <&apps_smmu 0x840 0x0>;

Unlike shikra.dtsi (where camss@5c11000 only lists 0x400), agatti.dtsi keeps
0x800, 0x820, and 0x840 on camss. On boards or overlays that enable camss
alongside the default-enabled isp_ope node, both devices claim those three
stream IDs on apps_smmu. When isp_ope probes first, arm_smmu_device_group()
assigns a group to SMEs 0x800, 0x820, and 0x840, and when camss later calls
arm_smmu_device_group(), SME 0x400 has group == NULL while SMEs 0x800, 0x820,
and 0x840 match isp_ope's group, returning early without recording the group
for SME 0x400 and forcing both devices to share a single IOMMU group. Should
0x800, 0x820, and 0x840 be removed from camss@5c11000 in agatti.dtsi?

> 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)

[ ... ]

> +	/* 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: Low]
Can shifting the result of DS_RESOLUTION() left by 30 bits in
ope_prog_stripe() trigger a signed 32-bit shift overflow?

DS_RESOLUTION() evaluates to signed int literals 0x0, 0x1, 0x2, or 0x3. When
the downscaling ratio is less than 16x (including every NV12 or NV16 frame
where OPE_DS_C_PRE scales 2x horizontally), DS_RESOLUTION() returns 0x2 or
0x3, and shifting that signed int by 30 overflows into the sign bit under
CONFIG_UBSAN_SHIFT. Could DS_RESOLUTION() use unsigned literals (0x0U..0x3U)
or FIELD_PREP(GENMASK(31, 30), ...)?

[ ... ]

> +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);
> +}

[Severity: Medium]
Can ope_apply_params() consume a parameter buffer before OPE_QUEUE_PARAMS has
finished starting streaming, and does stopping OPE_QUEUE_PARAMS abort active
video frames?

During VIDIOC_STREAMON on ope_params, vb2_start_streaming() passes pre-queued
buffers to ope_buf_queue() before calling ope_start_streaming():

	vb2_start_streaming()
	  ope_buf_queue()
	    list_add_tail(&buf->list, &ctx->queues[idx].rdy_queue);
	    ope_try_schedule(ctx);
	  ope_start_streaming()

While OPE_QUEUE_FRAME_IN and OPE_QUEUE_DISP_OUT are gated by ope_job_ready()
checking ctx->queues[...].streaming, ope_apply_params() calls
ope_queue_remove(ctx, OPE_QUEUE_PARAMS) without checking
ctx->queues[OPE_QUEUE_PARAMS].streaming. If ope_input and ope_disp_output are
already streaming, a job running before ope_start_streaming() sets
streaming = true will dequeue and complete the parameter buffer via
vb2_buffer_done(), triggering WARN_ON(!list_empty(&q->done_list)) in
vb2_start_streaming() if ope_start_streaming() fails.

In addition, in ope_stop_streaming() and ope_queues_release():

	ope_sched_cancel(ctx);
	ope_queue_drain(ctx, idx, VB2_BUF_STATE_ERROR);
	ope_sched_resume(ctx->ope);

stopping or closing ope_params while a video frame is in flight calls
ope_sched_cancel(ctx), which resets the OPE hardware via ope_hw_reset_sync()
and completes the active video frame buffers with VB2_BUF_STATE_ERROR, even
though parameter buffers are CPU-copied before hardware execution starts.

[ ... ]

> +static int ope_buf_prepare(struct vb2_buffer *vb)
> +{
> +	struct ope_ctx *ctx = ope_ctx_from_vq(vb->vb2_queue);
> +	unsigned int idx = ope_idx_from_vq(vb->vb2_queue);
> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> +	unsigned int sizeimage;
> +
> +	if (idx == OPE_QUEUE_FRAME_IN)
> +		sizeimage = ctx->fmt_in.sizeimage;
> +	else if (idx == OPE_QUEUE_DISP_OUT)
> +		sizeimage = ctx->fmt_out.sizeimage;
> +	else
> +		sizeimage = v4l2_isp_buffer_size(CAMSS_PARAMS_OPE_MAX_PAYLOAD);
> +
> +	if (V4L2_TYPE_IS_OUTPUT(vb->vb2_queue->type)) {
> +		if (vb->vb2_queue->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> +			if (vbuf->field == V4L2_FIELD_ANY)
> +				vbuf->field = V4L2_FIELD_NONE;
> +			if (vbuf->field != V4L2_FIELD_NONE)
> +				return -EINVAL;
> +		}
> +	}
> +
> +	if (vb2_plane_size(vb, 0) < sizeimage)
> +		return -EINVAL;

[Severity: Medium]
Does ope_buf_prepare() reject valid variable-length VB2_USERPTR parameter
buffers while deferring v4l2-isp buffer validation until the worker runs?

OPE_QUEUE_PARAMS enables VB2_USERPTR in ope_init_vq() and uses the extensible
variable-length v4l2-isp format. However, ope_buf_prepare() requires
vb2_plane_size(vb, 0) to be at least
v4l2_isp_buffer_size(CAMSS_PARAMS_OPE_MAX_PAYLOAD), which rejects USERPTR
buffers sized for only a subset of parameter blocks.

At the same time, ope_buf_prepare() does not call
v4l2_isp_params_validate_buffer_size() or v4l2_isp_params_validate_buffer()
at VIDIOC_PREPARE_BUF / VIDIOC_QBUF time; validation and copying only happen
asynchronously in ope_run_job() -> ope_apply_params(), allowing malformed
parameter buffers to succeed at VIDIOC_QBUF.

[ ... ]

> +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);

[Severity: Medium]
As reported by Sashiko on v12 [1], when the last streaming context stops in
ope_stop_streaming(), pixclk is 0, but ope_adjust_power() unconditionally
calls dev_pm_opp_find_freq_ceil(ope->dev, &pixclk).

Because dev_pm_opp_find_freq_ceil() rounds pixclk = 0 up to the lowest OPP
frequency (19.2 MHz), dev_pm_opp_set_rate(ope->dev, pixclk) is never called
with 0, so the OPP table is not disabled and the required-opps vote on the cx
power domain remains active while the device is idle. Should the
dev_pm_opp_find_freq_ceil() lookup be skipped when pixclk is 0?

[ ... ]

> +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;
> +	int ret;
> +
> +	qpriv = kzalloc_obj(*qpriv);
> +	if (!qpriv)
> +		return -ENOMEM;
> +
> +	qpriv->ctx = ctx;
> +	qpriv->queue_idx = idx;
> +	qpriv->vdev = camss_isp_pipeline_get_vdev(ope->pipeline, ope_queue_to_entity[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 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;

[ ... ]

> +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;

[Severity: Medium]
As reported by Sashiko on v12 [2], does assigning vdev->lock =
&pipeline->vdev_lock in isp_register_vdev() while setting q->lock =
&ctx->vbq_lock and qpriv->vdev->queue = q in ope_init_vq() split
serialization between queue and format ioctls?

In v4l2_ioctl_get_lock(), ioctls with INFO_FL_QUEUE (VIDIOC_REQBUFS,
VIDIOC_CREATE_BUFS, VIDIOC_PREPARE_BUF, VIDIOC_QBUF, VIDIOC_STREAMON) lock
vdev->queue->lock (&ctx->vbq_lock), whereas non-queue ioctls (VIDIOC_S_FMT,
VIDIOC_G_FMT, VIDIOC_S_PARM, VIDIOC_G_PARM) lock vdev->lock
(&pipeline->vdev_lock). As a result, 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 under pipeline->vdev_lock while
concurrent vb2 callbacks (ope_queue_setup(), ope_buf_prepare(),
ope_start_streaming(), ope_adjust_power()) read those fields under
ctx->vbq_lock.

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 pipeline is
already streaming. Because video_device_pipeline_alloc_start() only validates
media links when the first video node starts streaming (when
pipe->start_count == 0), starting streaming on one node (such as ope_input or
ope_params) and then calling VIDIOC_S_FMT on another node whose queue has no
buffers yet allows ctx->fmt_out or ctx->fmt_in to be changed after link
validation has already completed.

[ ... ]

> +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;
> +}

[Severity: Medium]
Should VIDIOC_G_PARM and VIDIOC_S_PARM be disabled on the ope_disp_output
capture node?

Both ope_input and ope_disp_output share ope_video_ioctl_ops, which wires up
ope_g_parm() and ope_s_parm(). On ope_disp_output (VFL_DIR_RX), calling
VIDIOC_G_PARM or VIDIOC_S_PARM with V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE passes
check_fmt() in v4l2-ioctl.c and then fails with -EINVAL in ope_g_parm() /
ope_s_parm() because !V4L2_TYPE_IS_OUTPUT(sp->type) is true, whereas V4L2
expects -ENOTTY when an ioctl is not supported on a capture node. Could
v4l2_disable_ioctl() be used on ope_disp_output for VIDIOC_G_PARM and
VIDIOC_S_PARM (or separate ioctl_ops tables be used)?

[ ... ]

> +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_prepare_buf(struct file *file, void *priv, struct v4l2_buffer *buf)
> +{
> +	struct ope_ctx *ctx = ope_ctx_from_file(file);
> +	struct video_device *vdev = video_devdata(file);
> +	struct vb2_queue *vq = ope_vq_from_file(file, ctx);
> +
> +	if (!vq)
> +		return -EINVAL;
> +	if (vb2_queue_is_busy(vq, file))
> +		return -EBUSY;
> +
> +	return vb2_prepare_buf(vq, vdev->v4l2_dev->mdev, buf);
> +}
> +
> +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;
> +}

[ ... ]

> +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 [3], why not use the standard vb2_ioctl_* and
vb2_fop_* helpers instead of custom wrappers in ope_dqbuf(),
ope_create_bufs(), and ope_poll()?

There are three issues in these custom wrappers:

1. ope_dqbuf() does not check vb2_queue_is_busy(vq, file), allowing a second
open file descriptor to dequeue buffers from an active stream owned by
another file handle.

2. ope_create_bufs() checks vb2_queue_is_busy(vq, file) even when
create->count == 0, causing read-only capability queries to fail with -EBUSY
instead of populating capabilities and max_num_buffers as
vb2_ioctl_create_bufs() does.

3. ope_poll() directly accesses private vb2_queue fields (done_wq, done_lock,
done_list, error) without holding vq->lock. Because it does not check
poll_requested_events(wait), polling only for EPOLLPRI events while the queue
is not streaming immediately returns EPOLLERR, and because it does not check
vq->waiting_for_buffers on capture queues, polling after VIDIOC_STREAMON with
no queued capture buffers blocks instead of returning EPOLLERR.

[ ... ]

> +static int ope_proc_get_selection(struct v4l2_subdev *sd,
> +				  const struct v4l2_subdev_client_info *info,
> +				  struct v4l2_subdev_state *state,
> +				  struct v4l2_subdev_selection *sel)
> +{

[ ... ]

> +	if (sel->pad == OPE_PROC_PAD_SOURCE) {
> +		const struct v4l2_rect *crop =
> +			v4l2_subdev_state_get_crop(state, OPE_PROC_PAD_SINK_IN);
> +
> +		switch (sel->target) {
> +		case V4L2_SEL_TGT_COMPOSE:
> +			sel->r = *v4l2_subdev_state_get_compose(state, OPE_PROC_PAD_SOURCE);
> +			return 0;
> +		case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> +		case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> +			sel->r = *crop;
> +			return 0;

[Severity: Medium]
In ope_proc_get_selection(), should V4L2_SEL_TGT_COMPOSE_DEFAULT and
V4L2_SEL_TGT_COMPOSE_BOUNDS on OPE_PROC_PAD_SOURCE return origin (0, 0)
instead of copying crop->left and crop->top?

In ope_proc_adjust_src_compose(), the compose rectangle is clamped against
bounds starting at (0, 0):

	bounds = (struct v4l2_rect){ 0, 0, sink_crop->width, sink_crop->height };
	ope_sd_adjust_crop_rect(compose, &bounds, halign, valign);

However, ope_proc_get_selection() sets sel->r = *crop, which includes any
non-zero crop->left and crop->top from OPE_PROC_PAD_SINK_IN. When a non-zero
sink crop offset is configured, a valid compose rectangle at (0, 0) falls
outside the reported V4L2_SEL_TGT_COMPOSE_BOUNDS.

[ ... ]

> +static const struct v4l2_subdev_pad_ops ope_disp_pad_ops = {
> +	.get_fmt       = v4l2_subdev_get_fmt,
> +	.set_fmt       = ope_disp_set_fmt,
> +	.link_validate = v4l2_subdev_link_validate_default,
> +};

[Severity: Medium]
Does ope_disp_pad_ops need an .enum_mbus_code callback?

isp_register_subdev() sets V4L2_SUBDEV_FL_HAS_DEVNODE on every subdev in the
pipeline, exposing a /dev/v4l-subdevN device node for ope_disp, but
ope_disp_pad_ops omits .enum_mbus_code, so VIDIOC_SUBDEV_ENUM_MBUS_CODE on
ope_disp always fails with -ENOTTY.

[ ... ]

> +	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 [4], if ope_v4l2_register(ope) fails in
ope_probe(), doesn't cpu_latency_qos_remove_request(&ope->qos_req) access
freed memory?

Every failure path in ope_v4l2_register() invokes ope_v4l2_dev_release(),
which frees ope via kfree(ope):

	ope_probe()
	  cpu_latency_qos_add_request(&ope->qos_req, PM_QOS_DEFAULT_VALUE)
	  ope_v4l2_register(ope)
	    ...
	    ope_v4l2_dev_release(&ope->v4l2_dev)
	      kfree(ope)
	  cpu_latency_qos_remove_request(&ope->qos_req)  <-- use-after-free

In addition, on the success path of ope_probe(), enable_irq(ope->irq) is
called after ope_v4l2_register(ope) has already published the V4L2 and media
device nodes to userspace, leaving a window where userspace can open the
device and start streaming before the IRQ is enabled.

[ ... ]

> +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: Medium]
Can scheduling ope->run_work from inside ope_runtime_resume() race with
pm_runtime_force_resume() and drop an edge-triggered hardware interrupt?

During system resume, pm_runtime_force_resume() invokes ope_runtime_resume()
while dev->power.disable_depth > 0, before calling pm_runtime_enable(dev).
Inside ope_runtime_resume(), ope_sched_resume(ope) queues ope->run_work on
system_wq, which can execute on another CPU and call ope_run_job() ->
ope_start(ope).

If the task running pm_runtime_force_resume() is preempted before reaching
pm_runtime_enable(dev), the hardware can raise its IRQ_TYPE_EDGE_RISING
interrupt while dev->power.disable_depth > 0. In that state,
pm_runtime_get_if_in_use(ope->dev) in ope_irq() returns -EINVAL, causing
ope_irq() to return IRQ_NONE without reading or clearing OPE_TOP_IRQ_STATUS
and stalling the active job.

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

[ ... ]

> +void camss_isp_pipeline_unregister(struct camss_isp_pipeline *pipeline)
> +{
> +	int i;
> +
> +	/* Unregister entities in reverse order. */
> +	for (i = (int)pipeline->num_entities - 1; i >= 0; i--) {
> +		struct camss_isp_pipeline_entity *slot = &pipeline->entities[i];
> +
> +		if (slot->registered) {
> +			switch (slot->obj_type) {
> +			case MEDIA_ENTITY_TYPE_VIDEO_DEVICE:
> +				if (video_device_pipeline(&slot->vdev))
> +					video_device_pipeline_stop(&slot->vdev);
> +				video_unregister_device(&slot->vdev);
> +				break;

[Severity: Medium]
As reported by Sashiko on v12 [5], can camss_isp_pipeline_unregister() call
video_device_pipeline_stop() an unbalanced number of times or unregister
subdevs while their pads are still part of an active media_pipeline?

When any video node in the connected graph calls
video_device_pipeline_alloc_start() in ope_start_streaming(),
__media_pipeline_start() sets pad->pipe on every connected pad in the graph.
As a result, video_device_pipeline(&slot->vdev) becomes non-NULL on all three
video nodes (OPE_ENTITY_FRAME_IN at index 0, OPE_ENTITY_PARAMS at index 1,
and OPE_ENTITY_DISP_OUT at index 4) even if only one or two of them started
streaming.

Additionally, because OPE_ENTITY_DISP (index 3) and OPE_ENTITY_PROC (index 2)
are unregistered before OPE_ENTITY_PARAMS (index 1) and OPE_ENTITY_FRAME_IN
(index 0), if two or more video nodes are streaming during unbind, stopping
index 4 only decrements pipe->start_count without terminating the pipeline,
and the subdevs at indices 3 and 2 are unregistered while their pads are
still attached to the active media_pipeline.

> diff --git a/drivers/media/platform/qcom/camss/ope/pipeline.h b/drivers/media/platform/qcom/camss/ope/pipeline.h
> new file mode 100644
> index 0000000000000..62e4116012ad0
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/ope/pipeline.h

[ ... ]

> +struct camss_isp_entity_desc {
> +	const char				*name;
> +	u32					obj_type;
> +	u32					function;
> +	const struct camss_isp_pad_desc		*pads;
> +
> +	union {
> +		/* MEDIA_ENTITY_TYPE_VIDEO_DEVICE */
> +		struct {
> +			u32					caps;
> +			void					*drvdata;
> +			const struct v4l2_file_operations	*fops;
> +			const struct v4l2_ioctl_ops		*ioctl_ops;
> +			const struct media_entity_operations	*entity_ops;
> +		} vdev;
> +		/* MEDIA_ENTITY_TYPE_V4L2_SUBDEV */
> +		struct {
> +			const struct v4l2_subdev_ops		*ops;
> +			const struct v4l2_subdev_internal_ops	*internal_ops;
> +			const struct media_entity_operations	*entity_ops;
> +		} subdev;
> +	};
> +};

[ ... ]

> +/**
> + * camss_isp_pipeline_register() - validate descriptors and register the graph
> + * @pipeline:    pipeline (allocated with camss_isp_pipeline_alloc())
> + * @v4l2_dev:    caller-owned and already-registered V4L2 device; its
> + *               associated media_device (v4l2_dev->mdev) must also be
> + *               initialised and registered before this call.

[Severity: Low]
As reported by Sashiko on v12 [6], this isn't a bug, but the kernel-doc
comment for struct camss_isp_entity_desc documents the nested @vdev.* and
@subdev.* members without @vdev: and @subdev: entries, which causes
scripts/kernel-doc to warn that struct members vdev and subdev are not
described.

Also, the kernel-doc comment for camss_isp_pipeline_register() states that
v4l2_dev->mdev must already be registered before the call, whereas
ope_v4l2_register() only calls media_device_init() beforehand and calls
media_device_register() after camss_isp_pipeline_register() succeeds.

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

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