Re: [PATCH v15 07/12] media: mediatek: jpeg: fix resolution change event handling in decoder

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Calling `mtk_jpeg_set_queue_data()` asynchronously in the worker thread causes a lockless data race on V4L2 queue parameters and violates the source change event contract.

Pre-existing issues:
- [Critical] Calling `VIDIOC_STREAMOFF` on the decoder after a resolution change results in an unrecoverable kernel deadlock due to stranded buffers.
- [High] Missing `.job_ready` callback in multi-core ops combined with early worker exit causes an infinite workqueue scheduling loop that consumes 100% CPU.
--

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

media: mediatek: jpeg: fix resolution change event handling in decoder
    
This patch refines the handling of resolution change events within
JPEG decoder worker. The mtk_jpeg_set_queue_data function is now
called to set up queue data before signaling a source change through
mtk_jpeg_queue_src_chg_event. By reorganizing these calls, the
patch ensures that necessary queue information is updated prior to
transitioning the context state to MTK_JPEG_SOURCE_CHANGE.
A condition is added to exit early if the context is already in the
MTK_JPEG_SOURCE_CHANGE state, preventing redundant operations and
improving processing efficiency.

Fixes: dedc21500334 ("media: mtk-jpegdec: add jpeg decode worker interface")
Signed-off-by: Kyrie Wu <kyrie.wu@mediatek.com>

> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> index 4dc574e03bd5a..02efaed198b85 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> @@ -1737,11 +1737,15 @@ static void mtk_jpegdec_worker(struct work_struct *work)
>  
>  	if (mtk_jpeg_check_resolution_change(ctx,
>  					     &jpeg_src_buf->dec_param)) {
> -		mtk_jpeg_queue_src_chg_event(ctx);
> +		mtk_jpeg_set_queue_data(ctx, &jpeg_src_buf->dec_param);

[Severity: High]
Does this code introduce a lockless data race on V4L2 queue parameters?

Modifying format queues via mtk_jpeg_set_queue_data() directly from the
worker thread in process context runs without holding the V4L2 device
lock or vb2 queue locks.

Could this lead to torn reads or state corruption if userspace concurrently
queries or sets the format via VIDIOC_G_FMT or VIDIOC_S_FMT? It also appears
to violate the V4L2 contract, which specifies that the format should remain
unchanged until userspace acknowledges the event via VIDIOC_STREAMOFF.

>  		ctx->state = MTK_JPEG_SOURCE_CHANGE;
> +		mtk_jpeg_queue_src_chg_event(ctx);
>  		goto getbuf_fail;
>  	}
>  
> +	if (ctx->state == MTK_JPEG_SOURCE_CHANGE)
> +		goto getbuf_fail;

[Severity: High]
This is a pre-existing issue, but does this early exit without consuming
buffers lead to an infinite workqueue scheduling loop?

Because mtk_jpeg_multicore_dec_m2m_ops lacks a .job_ready callback, the
V4L2 M2M framework defaults to assuming the context is always ready if
buffers are present. Since this early exit path drops to getbuf_fail
without removing the buffers, v4l2_m2m_job_finish() will immediately
reschedule the context.

Could this loop continuously without delay, causing a 100% CPU lockup on
a single core?

[Severity: Critical]
This isn't a bug introduced by this patch, but does taking this getbuf_fail
path after a resolution change cause an unrecoverable deadlock later?

When the worker drops out here, the buffers remain in the v4l2_m2m queues
because v4l2_m2m_buf_done() is never called.

If userspace then handles the resolution change by calling VIDIOC_STREAMOFF
on the capture queue, mtk_jpeg_dec_stop_streaming() executes:

drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c:mtk_jpeg_dec_stop_streaming() {
    ...
    if (jpeg->variant->multi_core)
        vb2_wait_for_all_buffers(q);
    ...
}

Since the driver still owns the stranded buffers, will vb2_wait_for_all_buffers
block forever? The code to flush the m2m queues is placed after the wait
call, which seems to guarantee a deadlock.

> +
>  	mtk_jpegdec_set_hw_param(ctx, hw_id, src_buf, dst_buf);
>  	ret = pm_runtime_resume_and_get(comp_jpeg[hw_id]->dev);

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