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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] media: mtk-jpeg: infinite loop in worker on MTK_JPEG_SOURCE_CHANGE
- [High] media: mtk-jpeg: unprotected queue data mutation in async worker
--

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

media: mediatek: jpeg: fix resolution change event handling in decoder

This commit refines the handling of resolution change events in the JPEG
decoder worker. It updates the queue data before queuing the source
change event and setting the state to MTK_JPEG_SOURCE_CHANGE. It also
adds an early exit when the context is already in the source change state.

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

[ ... ]

> @@ -1738,11 +1738,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 calling mtk_jpeg_set_queue_data() here mutate ctx->out_q and ctx->cap_q
without holding jpeg->lock? Since this runs in an asynchronous worker thread,
can concurrent V4L2 ioctls (like VIDIOC_G_FMT) issued from user space race
with these updates?

Updating the resolution before user space acknowledges the change with
STREAMOFF might also violate the driver's documented architectural contract.

>  		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]
Could this introduce an infinite loop? If we jump to getbuf_fail here, the job
finishes without consuming the pending buffers.

Because mtk_jpeg_multicore_dec_m2m_ops lacks a .job_ready callback:

static const struct v4l2_m2m_ops mtk_jpeg_multicore_dec_m2m_ops = {
	.device_run = mtk_jpeg_multicore_dec_device_run,
};

the V4L2 M2M core assumes the device is always ready. Will the M2M core
immediately requeue the context, causing the worker to run again, hit this
same check, and loop infinitely consuming 100% CPU?

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

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