Re: [PATCH v17 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 2 potential issue(s) to consider:
- [High] media: mtk-jpeg: Suspend disables active clocks causing hardware hang
- [Low] media: mtk-jpeg: Missing pm_ptr() for PM callback structures
--

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

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

This commit adds suspend and resume callback functions for each multi-core
hardware. It defines dev_pm_ops and sets them up in the platform_driver for
both decoders and encoders.

> 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
[ ... ]
> +static int mtk_jpegdec_suspend(struct device *dev)
> +{
> +	struct mtk_jpegdec_comp_dev *jpeg = dev_get_drvdata(dev);
> +
> +	v4l2_m2m_suspend(jpeg->master_dev->m2m_dev);
> +
> +	return pm_runtime_force_suspend(dev);
> +}

[Severity: High]
Does this suspend path risk causing a system crash by forcefully disabling
clocks while the JPEG hardware is actively performing DMA operations?

Because the multi-core driver design calls v4l2_m2m_job_finish() immediately
after starting the hardware, the TRANS_RUNNING flag is cleared early. This
causes v4l2_m2m_suspend() to not block here. The callback then proceeds
directly to pm_runtime_force_suspend(), which invokes
mtk_jpegdec_pm_suspend() to disable the clocks, disregarding that the hardware
might still be in the MTK_JPEG_HW_BUSY state.

[ ... ]
>  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 assignment be wrapped with pm_ptr() to avoid dead code?

When CONFIG_PM is disabled, mtk_jpegdec_pm_ops and the PM callbacks will be
compiled into the kernel but remain unused. Using pm_ptr(&mtk_jpegdec_pm_ops)
allows the compiler to eliminate the dead code.

>  	},
>  };
>  
> 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
[ ... ]
> +static int mtk_jpegenc_suspend(struct device *dev)
> +{
> +	struct mtk_jpegenc_comp_dev *jpeg = dev_get_drvdata(dev);
> +
> +	v4l2_m2m_suspend(jpeg->master_dev->m2m_dev);
> +
> +	return pm_runtime_force_suspend(dev);
> +}

[Severity: High]
Does this suspend path have the same risk of forcefully disabling active
clocks during DMA operations as in the decoder hardware?

Similar to the decoder, v4l2_m2m_suspend() might not block here because
v4l2_m2m_job_finish() is called early in the encoder path, causing the
clocks to be cut while the encoder might still be active.

[ ... ]
>  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 assignment also be wrapped with pm_ptr() to avoid dead code when
CONFIG_PM is disabled, similar to mtk_jpegdec_pm_ops?

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