Re: [PATCH v16 05/12] media: mediatek: jpeg: Fix multi-core clk suspend and resume setting

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unsafe global M2M device suspend/resume from component PM callbacks.
- [Low] Unconditional assignment of dev_pm_ops structures without pm_ptr(), leading to dead code.

Pre-existing issues:
- [Critical] Race condition between hardware IRQ handler and timeout worker.
- [High] Timeout handlers advertise hardware as IDLE before fully consuming hardware parameters, leading to queue corruption.
- [High] Use-After-Free due to dangling pointers on component probe failure.
--

--- Patch [5]: [PATCH v16 05/12] media: mediatek: jpeg: Fix multi-core clk suspend and resume setting ---
commit 11ea0f3a8e261149b85cc30379d74f6a788b6148
Author: Kyrie Wu <kyrie.wu@mediatek.com>
Date:   Mon Jul 20 13:34:25 2026 +0000

    media: mediatek: jpeg: Fix multi-core clk suspend and resume setting

The suspend/resume callback function is defined in the dev_pm_ops
structure, which is defined in platform_driver. For multiple-core
architecture, each hardware driver will register a platform_driver
structure, so it is necessary to add a suspend/resume callback
function for each hardware to support this operation.

> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c
> index 9ca68cde45a48..970c0a30e3659 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c
> @@ -533,13 +533,12 @@ static void mtk_jpegdec_timeout_work(struct work_struct *work)
>  	v4l2_m2m_buf_copy_metadata(src_buf, dst_buf);
>  
>  	mtk_jpeg_dec_reset(cjpeg->reg_base);
> -	clk_disable_unprepare(cjpeg->jdec_clk.clks->clk);
> -	pm_runtime_put(cjpeg->dev);
>  	cjpeg->hw_state = MTK_JPEG_HW_IDLE;
>  	atomic_inc(&master_jpeg->hw_rdy);
>  	wake_up(&master_jpeg->hw_wq);
>  	v4l2_m2m_buf_done(src_buf, buf_state);
>  	mtk_jpegdec_put_buf(cjpeg);

[Severity: High]
This is a pre-existing issue, but does waking up waiters before
mtk_jpegdec_put_buf() consumes the hardware parameters risk queue corruption?

If a waiting worker thread wakes up and overwrites cjpeg->hw_param before
mtk_jpegdec_put_buf() executes, the newly overwritten parameters might be
used to complete the wrong buffer, leaving the actual timed-out buffer stuck.

> +	pm_runtime_put(cjpeg->dev);
>  }
>  
>  static irqreturn_t mtk_jpegdec_hw_irq_handler(int irq, void *priv)
> @@ -547,7 +546,6 @@ static irqreturn_t mtk_jpegdec_hw_irq_handler(int irq, void *priv)
>  	struct vb2_v4l2_buffer *src_buf, *dst_buf;
>  	struct mtk_jpeg_src_buf *jpeg_src_buf;
>  	enum vb2_buffer_state buf_state;
> -	struct mtk_jpeg_ctx *ctx;
>  	u32 dec_irq_ret;
>  	u32 irq_status;
>  	int i;
> @@ -557,7 +555,6 @@ static irqreturn_t mtk_jpegdec_hw_irq_handler(int irq, void *priv)
>  
>  	cancel_delayed_work(&jpeg->job_timeout_work);

[Severity: Critical]
This is a pre-existing issue, but is there a race condition between this
hardware IRQ handler and the timeout worker?

If the hardware interrupt fires at the exact moment the job timeout worker
begins execution, cancel_delayed_work() will return false. Without further
synchronization, both threads might concurrently execute v4l2_m2m_buf_done(),
pm_runtime_put(), and atomic_inc() on the same job, which could lead to a
use-after-free or memory corruption.

[ ... ]
> @@ -673,11 +669,58 @@ static int mtk_jpegdec_hw_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
[ ... ]
> +static int mtk_jpegdec_resume(struct device *dev)
> +{
> +	struct mtk_jpegdec_comp_dev *jpeg = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = pm_runtime_force_resume(dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	v4l2_m2m_resume(jpeg->master_dev->m2m_dev);

[Severity: High]
Does calling v4l2_m2m_resume() from a component's resume callback unpause
the global master job queue prematurely?

If the system enters suspend, the first component to resume will unpause the
global queue. This might cause the worker thread to dispatch jobs to other
components that have not yet resumed, potentially leading to deadlocks or
queue corruption. Should the v4l2_m2m_suspend() and v4l2_m2m_resume() calls
be restricted to the master device instead?

> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops mtk_jpegdec_pm_ops = {
> +	SYSTEM_SLEEP_PM_OPS(mtk_jpegdec_suspend, mtk_jpegdec_resume)
> +	RUNTIME_PM_OPS(mtk_jpegdec_pm_suspend, mtk_jpegdec_pm_resume, NULL)
> +};
> +
>  static struct platform_driver mtk_jpegdec_hw_driver = {
>  	.probe = mtk_jpegdec_hw_probe,
>  	.driver = {
>  		.name = "mtk-jpegdec-hw",
>  		.of_match_table = mtk_jpegdec_hw_ids,
> +		.pm             = &mtk_jpegdec_pm_ops,

[Severity: Low]
Does unconditionally assigning the pm pointer here generate dead code when
CONFIG_PM is disabled?

According to the subsystem guidelines, the dev_pm_ops structure pointer
should be wrapped with pm_ptr() to eliminate dead code and prevent compiler
warnings when PM support is disabled.

>  	},
>  };

[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk during
probe failure in mtk_jpegdec_hw_probe()?

Looking at mtk_jpegdec_hw_probe():

	i = atomic_add_return(1, &master_dev->hw_index) - 1;
	master_dev->dec_hw_dev[i] = dev;
	master_dev->reg_decbase[i] = dev->reg_base;
	dev->master_dev = master_dev;

	ret = mtk_jpegdec_smmu_init(dev);
	if (ret)
		return ret;

If mtk_jpegdec_smmu_init() fails, the component's memory is freed, but it
remains in the master_dev->dec_hw_dev array. Could the master device's worker
later iterate through this array, find this available core, and dereference
the freed memory?

> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c
> index 44a064dc7b3d0..4a8559c359536 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c
> @@ -263,13 +263,12 @@ static void mtk_jpegenc_timeout_work(struct work_struct *work)
>  	v4l2_m2m_buf_copy_metadata(src_buf, dst_buf);
>  
>  	mtk_jpeg_enc_reset(cjpeg->reg_base);
> -	clk_disable_unprepare(cjpeg->venc_clk.clks->clk);
> -	pm_runtime_put(cjpeg->dev);
>  	cjpeg->hw_state = MTK_JPEG_HW_IDLE;
>  	atomic_inc(&master_jpeg->hw_rdy);
>  	wake_up(&master_jpeg->hw_wq);
>  	v4l2_m2m_buf_done(src_buf, buf_state);
>  	mtk_jpegenc_put_buf(cjpeg);

[Severity: High]
This is a pre-existing issue, but does waking up waiters before
mtk_jpegenc_put_buf() consumes the hardware parameters risk queue corruption?

If a waiting worker thread wakes up and overwrites cjpeg->hw_param before
mtk_jpegenc_put_buf() executes, the newly overwritten parameters might be
used to complete the wrong buffer, leaving the actual timed-out buffer stuck.

> +	pm_runtime_put(cjpeg->dev);
>  }
>  

[Severity: Critical]
This is a pre-existing issue, but does mtk_jpegenc_hw_irq_handler() share the
same race condition with the timeout worker as the decoder?

Similar to the decoder, if the hardware interrupt fires when the job timeout
worker begins execution, cancel_delayed_work() will return false, potentially
leading to use-after-free or memory corruption.

[ ... ]
> @@ -394,11 +392,58 @@ static int mtk_jpegenc_hw_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
[ ... ]
> +static int mtk_jpegenc_resume(struct device *dev)
> +{
> +	struct mtk_jpegenc_comp_dev *jpeg = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = pm_runtime_force_resume(dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	v4l2_m2m_resume(jpeg->master_dev->m2m_dev);

[Severity: High]
Does calling v4l2_m2m_resume() from a component's resume callback unpause
the global master job queue prematurely?

Similar to the decoder, if the system enters suspend, the first component to
resume will unpause the global queue, which could cause the worker thread to
dispatch jobs to other components that have not yet resumed.

> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops mtk_jpegenc_pm_ops = {
> +	SYSTEM_SLEEP_PM_OPS(mtk_jpegenc_suspend, mtk_jpegenc_resume)
> +	RUNTIME_PM_OPS(mtk_jpegenc_pm_suspend, mtk_jpegenc_pm_resume, NULL)
> +};
> +
>  static struct platform_driver mtk_jpegenc_hw_driver = {
>  	.probe = mtk_jpegenc_hw_probe,
>  	.driver = {
>  		.name = "mtk-jpegenc-hw",
>  		.of_match_table = mtk_jpegenc_drv_ids,
> +		.pm = &mtk_jpegenc_pm_ops,

[Severity: Low]
Does unconditionally assigning the pm pointer here generate dead code when
CONFIG_PM is disabled?

The pm_ptr() macro should be used to wrap the pointer.

>  	},
>  };

[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk during
probe failure in mtk_jpegenc_hw_probe()?

Looking at mtk_jpegenc_hw_probe():

	i = atomic_add_return(1, &master_dev->hw_index) - 1;
	master_dev->enc_hw_dev[i] = dev;
	master_dev->reg_encbase[i] = dev->reg_base;
	dev->master_dev = master_dev;

	ret = mtk_jpegenc_smmu_init(dev);
	if (ret)
		return ret;

If mtk_jpegenc_smmu_init() fails, the component's memory is freed, but it
remains in the master_dev->enc_hw_dev array. The master device's worker
could later iterate through this array and dereference the freed memory.

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