Re: [PATCH v17 08/12] media: mediatek: jpeg: fix remove buffer removal timing for multi-core

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] media: mtk-jpeg: list corruption due to racy buffer removal
--

commit d081c3bb256d4600d543f05fb28f936f6f62ecbc
Author: Kyrie Wu <kyrie.wu@mediatek.com>

media: mediatek: jpeg: fix remove buffer removal timing for multi-core

This commit moves the calls to v4l2_m2m_src_buf_remove and
v4l2_m2m_dst_buf_remove inside the spinlock-protected scope in the
JPEG encoding and decoding workers. The intent is to ensure proper
synchronization before buffers are removed from their queues.

> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> index edd9e2d0ab05f..0907960db1436 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c

[ ... ]

> @@ -1674,6 +1671,8 @@ static void mtk_jpegenc_worker(struct work_struct *work)
>  			     &src_buf->vb2_buf);
>  	mtk_jpeg_set_enc_params(ctx, comp_jpeg[hw_id]->reg_base);
>  	mtk_jpeg_enc_start(comp_jpeg[hw_id]->reg_base);
> +	v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> +	v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);

[Severity: High]
Can this cause list corruption if the hardware completes very quickly?

By calling mtk_jpeg_enc_start before removing the buffers from the ready
queues, is there a window where the interrupt handler completes the operation,
marks the buffers as done, and wakes up userspace?

If the IRQ handler runs concurrently without acquiring the hardware lock and
userspace rapidly requeues the same buffer, could it invoke list operations on
a buffer that is still in the ready queue? When the worker thread subsequently
calls v4l2_m2m_src_buf_remove, would this removal operation completely corrupt
the list pointers?

>  	v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
>  	spin_unlock_irqrestore(&comp_jpeg[hw_id]->hw_lock, flags);
>  

[ ... ]

> @@ -1782,6 +1778,8 @@ static void mtk_jpegdec_worker(struct work_struct *work)
>  				jpeg_src_buf->bs_size,
>  				&bs,
>  				&fb);
> +	v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> +	v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
>  	mtk_jpeg_dec_start(comp_jpeg[hw_id]->reg_base);
>  	v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
>  	spin_unlock_irqrestore(&comp_jpeg[hw_id]->hw_lock, flags);

For the decoding worker, it looks like mtk_jpeg_dec_start is called after
the buffers are removed, which avoids the race condition described above.
Would it be safer to reorder the encoder side to match this sequence and
remove the buffers before starting the hardware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922091530.241762-1-kyrie.wu@mediatek.com?part=8