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

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

[Guangshuo Li]

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.

However, ipu_prg_remove() only removes the PRG from the global list and
does not disable runtime PM or shut down the clocks when the device is
still runtime active. As a result, removing the driver while the device
is active leaves both clocks enabled.

Disable runtime PM during removal so that pending runtime PM operations
are completed and no new runtime PM callbacks can change the device
state. If the device is still runtime active afterwards, disable the
AXI and IPG clocks explicitly.

Check the runtime PM state before disabling the clocks because the
runtime suspend callback may already have disabled them. This avoids
unbalancing the clock enable counts when removing an already suspended
device.

This issue was found by manual code inspection.

Fixes: ea9c260514c1 ("gpu: ipu-v3: add driver for Prefetch Resolve Gasket")
Cc: stable@vger.kernel.org
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 drivers/gpu/ipu-v3/ipu-prg.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/ipu-v3/ipu-prg.c b/drivers/gpu/ipu-v3/ipu-prg.c
index d38d3ba54d72..93033e847d0e 100644
--- a/drivers/gpu/ipu-v3/ipu-prg.c
+++ b/drivers/gpu/ipu-v3/ipu-prg.c
@@ -426,6 +426,13 @@ static void ipu_prg_remove(struct platform_device *pdev)
 	mutex_lock(&ipu_prg_list_mutex);
 	list_del(&prg->list);
 	mutex_unlock(&ipu_prg_list_mutex);
+
+	pm_runtime_disable(&pdev->dev);
+
+	if (!pm_runtime_status_suspended(&pdev->dev)) {
+		clk_disable_unprepare(prg->clk_axi);
+		clk_disable_unprepare(prg->clk_ipg);
+	}
 }
 
 #ifdef CONFIG_PM
-- 
2.43.0

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

[sashiko-bot]

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