Re: [PATCH] gpu: ipu-v3: prg: fix clock cleanup on remove

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but a sleeping function `readl_poll_timeout` is used in a display driver function designed to be called from the atomic commit path.
- [High] This problem wasn't introduced by this patch, but the return value of `pm_runtime_get_sync()` is ignored, leading to unclocked hardware register access on failure.
--

commit e147aed08964b1f246d1a5b035f162b665564efa
Author: Guangshuo Li <lgs201920130244@gmail.com>

gpu: ipu-v3: prg: fix clock cleanup on remove

ipu_prg_probe() enables the IPG and AXI clocks before enabling runtime
PM. The clocks are subsequently controlled by the runtime suspend and
resume callbacks.

> diff --git a/drivers/gpu/ipu-v3/ipu-prg.c b/drivers/gpu/ipu-v3/ipu-prg.c
> index d38d3ba54d722..93033e847d0e5 100644
> --- a/drivers/gpu/ipu-v3/ipu-prg.c
> +++ b/drivers/gpu/ipu-v3/ipu-prg.c

[ ... ]

While reviewing this file, I noticed a few pre-existing issues regarding
runtime PM and atomic context that might be worth addressing.

drivers/gpu/ipu-v3/ipu-prg.c:ipu_prg_channel_disable() {
    ...
	pm_runtime_get_sync(prg->dev);

	val = readl(prg->regs + IPU_PRG_CTL);
	val |= IPU_PRG_CTL_BYPASS(prg_chan);
	writel(val, prg->regs + IPU_PRG_CTL);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but the return value of
pm_runtime_get_sync() is ignored, leading to potential unclocked hardware
register access on failure.

If clk_prepare_enable() fails during resume, the clocks remain disabled.

Is it possible for the hardware to trigger a Synchronous External Abort (AXI
bus error) and a kernel panic when readl() and writel() unconditionally access
the unclocked registers immediately after?

The same pattern also exists in ipu_prg_channel_configure():

drivers/gpu/ipu-v3/ipu-prg.c:ipu_prg_channel_configure() {
    ...
	pm_runtime_get_sync(prg->dev);

	val = (stride - 1) & IPU_PRG_STRIDE_STRIDE_MASK;
	writel(val, prg->regs + IPU_PRG_STRIDE(prg_chan));
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but the return value of
pm_runtime_get_sync() is also ignored here.

Should we check the return value and handle the error to prevent unclocked
access?

drivers/gpu/ipu-v3/ipu-prg.c:ipu_prg_channel_configure() {
    ...
	/* wait for both double buffers to be filled */
	readl_poll_timeout(prg->regs + IPU_PRG_STATUS, val,
			   (val & IPU_PRG_STATUS_BUFFER0_READY(prg_chan)) &&
			   (val & IPU_PRG_STATUS_BUFFER1_READY(prg_chan)),
			   5, 1000);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but a sleeping function
readl_poll_timeout() is used in a display driver function designed to be
called from the atomic commit path.

ipu_prg_channel_configure() is called from ipu_plane_atomic_update(), which is
a plane atomic update callback.

Since readl_poll_timeout() internally calls usleep_range() and will sleep,
could this trigger might_sleep() warnings or deadlocks in the atomic commit
path? 

Should readl_poll_timeout_atomic() be used here instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260913131117.1561280-1-lgs201920130244@gmail.com?part=1