Re: [PATCH v3 4/4] media: qcom: jpeg: Add Qualcomm JPEG V4L2 encoder

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [Critical] The top-half IRQ handler performs an unprotected MMIO read on a power-managed device without checking pm_runtime_get_if_active().
- [Critical] The device remove callback unbinds the driver without synchronizing with active M2M jobs, ioctls, or file releases, causing hardware accesses to unmapped memory.
- [High] The driver fails to call v4l2_device_unregister() in the remove path, permanently leaking the device and m2m context.
- [High] Bidirectional DMA synchronization on capture buffers mapped as DMA_FROM_DEVICE violates the DMA API.
- [High] The driver enables vb2_dma_sg_memops but only programs the first SG segment's address into the hardware, causing linear DMA overruns on fragmented buffers.
- [High] The driver returns active DMA buffers to userspace on STREAMOFF or job abort without halting the hardware, risking memory corruption.
- [High] Missing synchronization between the threaded IRQ handler, workqueue cancellation, and file release causes Use-After-Free during context teardown.
- [High] The threaded IRQ handler signals job completion before stopping the hardware, creating a race where a newly started job is abruptly stopped mid-flight.
- [Medium] The driver fails to reinitialize the hardware when resuming an active encoding session from system suspend.
- [Medium] The hardware write limit is set to the full buffer size, but the start address is advanced by the header size, allowing the hardware to write past the end of the allocated DMA buffer.
- [Low] The driver calls non-existent macros and APIs (kzalloc_obj, v4l2_m2m_suspend, v4l2_m2m_resume), which will cause compilation to fail.
--

commit 86e68a88243e817a1f879ac5131b1a179ec73c9b
Author: Atanas Filipov <atanas.filipov@oss.qualcomm.com>
Subject: media: qcom: jpeg: Add Qualcomm JPEG V4L2 encoder

This commit introduces a Qualcomm JPEG encoder driver based on the V4L2
mem2mem framework. It handles format negotiation, queue management, hardware
initialization, interrupt-driven encoding, and power management integration
for the SM8250 platform.

> diff --git a/drivers/media/platform/qcom/jpeg/qcom_jenc_dev.c b/drivers/media/platform/qcom/jpeg/qcom_jenc_dev.c
> index c3573853f24ad..0000000000000
> --- a/drivers/media/platform/qcom/jpeg/qcom_jenc_dev.c
> +++ b/drivers/media/platform/qcom/jpeg/qcom_jenc_dev.c
> @@ -152,6 +152,15 @@ static int qcom_jpeg_pm_resume(struct device *dev)
>  {
>  	struct qcom_jenc_dev *jenc = dev_get_drvdata(dev);
>  	int rc;
>  
>  	rc = qcom_jpeg_icc_on(jenc);
>  	if (rc)
>  		return rc;
>  
>  	rc = qcom_jpeg_clk_on(jenc);

[Severity: Medium]
Does the hardware need to be reinitialized after power loss? During suspend,
the hardware is forcefully powered down and loses internal register state.
This resume path restores clocks but skips the jpeg_init hardware reset
sequence, which might leave the hardware in an uninitialized state for
subsequent encode jobs.

>  	if (rc) {
>  		qcom_jpeg_icc_off(jenc);
>  		return rc;
>  	}
>  
>  	return 0;
>  }
>  
>  static int qcom_jpeg_suspend(struct device *dev)
>  {
>  	struct qcom_jenc_dev *jenc = dev_get_drvdata(dev);
>  	int rc;
>  
>  	v4l2_m2m_suspend(jenc->m2m_dev);

[Severity: Low]
Is this calling an out-of-tree macro? The v4l2_m2m_suspend and
v4l2_m2m_resume APIs do not appear to exist in the standard Linux kernel and
will cause compilation to fail.

>  
>  	rc = pm_runtime_force_suspend(dev);
>  	if (rc)
>  		v4l2_m2m_resume(jenc->m2m_dev);
>  
>  	return rc;
>  }

[ ... ]

>  static int qcom_jpeg_probe(struct platform_device *pdev)
>  {
>  	const struct qcom_dev_resources *res;
>  	struct qcom_jenc_dev *jenc;
>  	int rc;
>  
>  	jenc = kzalloc_obj(*jenc, GFP_KERNEL);

[Severity: Low]
Does kzalloc_obj exist in the upstream kernel? It appears to be an undefined
macro which will cause compilation to fail.

>  	if (!jenc)
>  		return -ENOMEM;

[ ... ]

>  static void qcom_jpeg_remove(struct platform_device *pdev)
>  {
>  	struct qcom_jenc_dev *jenc = platform_get_drvdata(pdev);
>  
>  	/* v4l2_device_unregister() is called from the release callback. */
>  	qcom_jpeg_v4l2_unregister(jenc);
>  
>  	dev_dbg(jenc->dev, "Qualcomm JPEG encoder deregistered\n");
>  }

[Severity: Critical]
Does this remove function properly synchronize with active hardware? If an
encode job is running or a file descriptor remains open when the module is
unloaded, devres will automatically unmap the hardware registers. When the
file is later closed, fop_jpeg_file_release will call jpeg_deinit and attempt
to write to the unmapped registers, leading to a kernel panic. Should this
explicitly stop M2M jobs and wait for open file descriptors to close?

[Severity: High]
Is it safe to omit v4l2_device_unregister from the remove path? The comment
notes it is called from the release callback jenc_v4l2_dev_release, but the
v4l2_device reference count starts at 1 and is only decremented by
v4l2_device_unregister. If it is never called here, the refcount will never
reach zero, permanently leaking the device and m2m context.

> diff --git a/drivers/media/platform/qcom/jpeg/qcom_jenc_ops.c b/drivers/media/platform/qcom/jpeg/qcom_jenc_ops.c
> index a34c87fc59ecd..0000000000000
> --- a/drivers/media/platform/qcom/jpeg/qcom_jenc_ops.c
> +++ b/drivers/media/platform/qcom/jpeg/qcom_jenc_ops.c
> @@ -578,21 +578,22 @@ static irqreturn_t op_jpeg_irq_bot(int irq, void *data)
>  	irq_status = READ_ONCE(jenc->pending_irq_status);
>  
>  	irq_mask = jenc->res->hw_mask[JMSK_IRQ_STATUS_SESSION_DONE];
>  	if (jpeg_bits_get(irq_mask, irq_status)) {
>  		struct jenc_context *ctx;
>  
>  		spin_lock_irqsave(&jenc->hw_lock, flags);
>  		ctx = jenc->actx;
>  		jenc->actx = NULL;
>  		spin_unlock_irqrestore(&jenc->hw_lock, flags);

[Severity: High]
Can this race with fop_jpeg_file_release and cause a use-after-free?
The handler reads jenc->actx and drops the lock. If fop_jpeg_file_release
runs concurrently immediately after the lock is dropped, it will see
jenc->actx is NULL, bypass cancellation, and free the context memory. The IRQ
handler would then access the freed ctx in enc_hw_irq_cb.

>  
>  		if (ctx && ctx->bufq[JENC_DST_QUEUE].buff_id >= 0) {
>  			struct qcom_jenc_queue *dq = &ctx->bufq[JENC_DST_QUEUE];
>  			struct qcom_jpeg_buff *frame = &dq->buff[dq->buff_id];
>  			size_t out_size = jpeg_io_read(jenc, offs->enc_out_size);
>  
>  			dev_dbg(jenc->dev, "complete idx:%d addr=%pad size=%zu\n",
>  				dq->buff_id, &frame->plns[0].dma, out_size);
>  
>  			jenc->enc_hw_irq_cb(ctx, VB2_BUF_STATE_DONE,
>  					    out_size + ctx->hdr_cache.size);
>  		}
>  
>  		jpeg_stop(jenc);
>  	}

[Severity: High]
Does signalling completion before stopping the hardware introduce a race
condition? The enc_hw_irq_cb callback signals v4l2_m2m_job_finish, which
can instantly trigger the V4L2 framework to run the next queued job on
another CPU. If the new job starts the hardware, this subsequent jpeg_stop
call could erroneously halt the newly started job mid-flight.

[ ... ]

> @@ -641,12 +641,13 @@ static int jpeg_apply_we_addr(struct jenc_context *ectx, struct qcom_jenc_queue *q,
>  	mutex_unlock(&ectx->quality_mutex);
>  
>  	/*
>  	 * Invalidate stale CPU cache lines before writing the JPEG header
>  	 * with the CPU into the destination buffer.
>  	 */
>  	jpeg_sync_sg(jenc->dev, frame, DMA_BIDIRECTIONAL, false);

[Severity: High]
Does using DMA_BIDIRECTIONAL here violate the DMA API? The destination
capture queue is typically mapped as DMA_FROM_DEVICE. Supplying a direction
to sync helpers that does not match the original mapping direction leads to
incorrect cache maintenance operations and potential silent memory corruption
on non-coherent architectures.

>  
>  	header_size = min_t(size_t, vb->planes[0].length, ectx->hdr_cache.size);
>  	header_size = qcom_jenc_header_emit(&ectx->hdr_cache, mptr,
>  					    (u32)header_size, (u16)q->vf.width,
>  					    (u16)q->vf.height);
>  
>  	dma += header_size;
>  	qcom_jenc_dqts_emit(&ectx->hdr_cache, mptr);
>  
>  	/*
>  	 * Flush CPU writes to the header before handing the buffer to the
>  	 * hardware DMA engine.
>  	 */
>  	jpeg_sync_sg(jenc->dev, frame, DMA_BIDIRECTIONAL, true);
>  
>  	jpeg_io_write(jenc, offs->we.pntr[pln], dma);

[Severity: Medium]
Could advancing the DMA base address by header_size allow the hardware to
write past the end of the allocated DMA buffer? The hardware write limit
is set to the full buffer size (see jpeg_setup_we_size below). By shifting
the start address forward without reducing the maximum hardware output size
by the same amount, the allowed write window extends past the end of the
mapping.

[ ... ]

> @@ -693,12 +693,13 @@ static int jpeg_store_we_next(struct jenc_context *ectx, struct vb2_buffer *vb2)
>  {
>  	struct qcom_jenc_queue *q = &ectx->bufq[TYPE2QID(vb2->type)];
>  	struct qcom_jpeg_buff *frame = &q->buff[vb2->index];
>  	struct sg_table *sgt;
>  	dma_addr_t dma;
>  
>  	sgt = vb2_dma_sg_plane_desc(vb2, 0);
>  	if (!sgt || !sgt->sgl)
>  		return -EINVAL;
>  
>  	dma = sg_dma_address(sgt->sgl);

[Severity: High]
Will the hardware correctly handle fragmented memory here? The queue is
configured with vb2_dma_sg_memops, allowing buffers to be physically
fragmented across multiple scatterlist entries. However, the driver only
extracts sg_dma_address for the first element. If the buffer is fragmented,
the hardware will linearly overrun the end of the first segment and corrupt
adjacent physical memory.

>  	if (!dma)
>  		return -EINVAL;

[ ... ]

> @@ -711,9 +711,10 @@ static int jpeg_setup_we_size(struct jenc_context *ectx, struct qcom_jenc_queue
>  {
>  	struct qcom_jenc_dev *jenc = ectx->jenc;
>  	const struct qcom_jpeg_reg_offs *offs = jenc->res->hw_offs;
>  	struct v4l2_pix_format_mplane *dfmt = &q->vf;
>  	u8 pln;
>  
>  	for (pln = 0; pln < QCOM_JPEG_MAX_PLANES; pln++)
>  		jpeg_rw_bits(jenc, offs->we.stride[pln], JMSK_PLNS_WR_STRIDE, 0);
>  
>  	jpeg_io_write(jenc, offs->we.bsize[0], dfmt->plane_fmt[0].sizeimage);

(Related to the DMA bounds observation above: the hardware maximum bound is
set to the full sizeimage without taking the header offset into account).

[ ... ]

> @@ -1016,13 +1016,14 @@ static irqreturn_t op_jpeg_irq_top(int irq, void *data)
>  {
>  	struct qcom_jenc_dev *jenc = data;
>  	const struct qcom_jpeg_reg_offs *offs = jenc->res->hw_offs;
>  	unsigned long flags;
>  	u32 irq_status;
>  	u32 irq_mask;
>  
>  	spin_lock_irqsave(&jenc->hw_lock, flags);
>  
>  	irq_status = jpeg_io_read(jenc, offs->int_status);

[Severity: Critical]
Is it possible for this interrupt handler to run while the device is
suspended? This accesses the int_status hardware register without using
pm_runtime_get_if_active to verify that the power domain and clocks are
enabled. If a spurious or delayed interrupt fires while suspended, this
access will cause a fatal bus hang.

> diff --git a/drivers/media/platform/qcom/jpeg/qcom_jenc_v4l2.c b/drivers/media/platform/qcom/jpeg/qcom_jenc_v4l2.c
> index 20f698f77acfe..0000000000000
> --- a/drivers/media/platform/qcom/jpeg/qcom_jenc_v4l2.c
> +++ b/drivers/media/platform/qcom/jpeg/qcom_jenc_v4l2.c
> @@ -835,13 +835,14 @@ static const struct vb2_ops qcom_jpeg_v4l2_vb2_ops = {
>  	.stop_streaming		= bop_jpeg_vb2_stop_streaming,
>  };
>  
>  static void mop_jpeg_m2m_job_abort(void *priv)
>  {
>  	struct jenc_context *ectx = priv;
>  	struct qcom_jenc_dev *jenc = ectx->jenc;
>  
>  	mutex_lock(&jenc->dev_mutex);
>  
>  	jpeg_v4l2_work_stop(ectx, VB2_BUF_STATE_ERROR);
>  
>  	mutex_unlock(&jenc->dev_mutex);
>  }

[Severity: High]
Does this abort sequence correctly stop the executing hardware? The
jpeg_v4l2_work_stop function systematically unqueues all buffers and returns
them to userspace, but the driver never sends a stop command to the hardware.
The executing hardware context could continue running asynchronously and DMA
encoded data into buffers that have already been returned to userspace.

[ ... ]

> @@ -996,10 +996,11 @@ static int jpeg_v4l2_init_queue(void *priv, struct vb2_queue *sq, struct vb2_queue
>  
>  	sq->drv_priv		= ectx;
>  	sq->dev			= ectx->dev;
>  	sq->type		= V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
>  	sq->io_modes		= VB2_MMAP | VB2_DMABUF | VB2_USERPTR;
>  	sq->buf_struct_size	= sizeof(struct v4l2_m2m_buffer);
>  	sq->ops			= &qcom_jpeg_v4l2_vb2_ops;
>  	sq->mem_ops		= &vb2_dma_sg_memops;

(Related to the DMA fragmentation observation earlier: the queue is explicitly
configured here to allow scattered memory allocations).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629121750.3469292-1-atanas.filipov@oss.qualcomm.com?part=4