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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] media: mediatek: jpeg: system suspend disables clock while HW is running
- [High] media: mediatek: jpeg: Unsafe PM suspend forced while hardware is active
- [High] media: mediatek: jpeg: M2M queue unpaused before components resume
- [Low] media: mediatek: jpeg: dev_pm_ops assigned unconditionally causing dead code
--

commit 4893ce2818141ce09ed0c83da36108870b5d7724
Author: Kyrie Wu <kyrie.wu@mediatek.com>
Subject: media: mediatek: jpeg: Fix multi-core clk suspend and resume setting

This patch updates clock management and power management suspend/resume
callbacks for multi-hardware component devices, leaving single-core
callbacks on the master device. It aims to synchronize job suspension
using v4l2_m2m_suspend() before disabling component clocks. Runtime PM
references are used to keep clocks enabled until a job completes or times out.

> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c
[ ... ]
> +static int mtk_jpegdec_suspend(struct device *dev)
> +{
> +	struct mtk_jpegdec_comp_dev *jpeg = dev_get_drvdata(dev);
> +
> +	/* Stop new jobs and wait for the current job before clock-off. */
> +	v4l2_m2m_suspend(jpeg->master_dev->m2m_dev);

[Severity: Critical]
Does this wait mechanism function correctly if v4l2_m2m_job_finish() is
called immediately after starting the hardware?

In mtk_jpegdec_worker(), v4l2_m2m_job_finish() is invoked right after
mtk_jpeg_dec_start(), which clears the active M2M context early to allow
scheduling on other cores. Consequently, v4l2_m2m_suspend() will see no
active job and return immediately without waiting.

This can lead to pm_runtime_force_suspend() abruptly shutting off hardware
clocks while the hardware is still actively processing a frame, which can
lock up the bus.

Additionally, if the hardware is left running without clocks, the pending
job_timeout_work scheduled on the system workqueue will eventually fire. When
it calls mtk_jpeg_dec_reset(), it will attempt to access unclocked device
registers, which can trigger a synchronous external abort and crash the
system.

> +
> +	return pm_runtime_force_suspend(dev);
> +}
> +
> +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]
Can this unpause the global M2M queue before all hardware components are
fully resumed?

During system resume, the PM core sequentially resumes component devices.
If the first resumed component calls v4l2_m2m_resume(), it unpauses the
global M2M queue and schedules the worker thread on a non-freezable
workqueue. 

The worker thread could execute immediately, select a sibling component
that is still suspended, and call pm_runtime_resume_and_get(). If this
occurs concurrently with the PM core executing pm_runtime_force_resume() on
that same component, the runtime PM call fails with -EACCES, resulting in
dropped V4L2 frames.

> +
> +	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]
Should this be wrapped with pm_ptr() to avoid compiler warnings and dead code
retention when CONFIG_PM is disabled?

>  	},
>  };
>  
> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c
[ ... ]
> +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]
Should this also be wrapped with pm_ptr()?

>  	},
>  };

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