Re: [PATCH] drm/lima: fix autosuspend cleanup during teardown

[sashiko-bot@kernel.org]

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

New issues:
- [High] Calling `pm_runtime_dont_use_autosuspend()` before `pm_runtime_disable()` forces a synchronous runtime suspend, causing a fatal bus fault when teardown code accesses hardware registers with clocks disabled, and leads to double-disable warnings.

Pre-existing issues:
- [High] Interrupt handlers read MMIO registers directly without verifying that the device is actively powered on.
- [High] Missing `synchronize_irq()` in the runtime suspend callback for a driver using shared IRQs.
--

--- Patch [1]: [PATCH] drm/lima: fix autosuspend cleanup during teardown ---
commit 4763e4050f129243cd08b2beca088f25dfc1da22
Author: Guangshuo Li <lgs201920130244@gmail.com>

drm/lima: fix autosuspend cleanup during teardown

lima_pdev_probe() calls pm_runtime_use_autosuspend(), but its failure
path does not call the matching pm_runtime_dont_use_autosuspend()
before disabling runtime PM. The remove path has the same issue.

> diff --git a/drivers/gpu/drm/lima/lima_drv.c b/drivers/gpu/drm/lima/lima_drv.c
> index 9a0eb1092ad3d..9bd44c191e6c3 100644
> --- a/drivers/gpu/drm/lima/lima_drv.c
> +++ b/drivers/gpu/drm/lima/lima_drv.c
[ ... ]
> @@ -434,6 +434,7 @@ static int lima_pdev_probe(struct platform_device *pdev)
>  	return 0;
>  
>  err_out3:
> +	pm_runtime_dont_use_autosuspend(ldev->dev);
>  	pm_runtime_disable(ldev->dev);

[Severity: High]
Does calling pm_runtime_dont_use_autosuspend() before pm_runtime_disable()
force a synchronous runtime suspend here?

Because lima_device lacks a runtime_idle callback, the idle check from
dropping the usage count falls through to rpm_suspend(), which executes
lima_device_suspend() and disables device clocks. 

When the teardown path proceeds to lima_device_fini() -> lima_pmu_hw_fini(),
it attempts to read LIMA_PMU_STATUS, which will cause a fatal bus fault
because the device clocks are already off. It also leads to double-disabling
the clocks.

Had pm_runtime_disable() been called first, its disable_depth increment
would have safely prevented rpm_idle() from suspending the device.

>  	lima_devfreq_fini(ldev);
>  err_out2:
[ ... ]
> @@ -456,6 +457,7 @@ static void lima_pdev_remove(struct platform_device *pdev)
>  
>  	/* stop autosuspend to make sure device is in active state */
>  	pm_runtime_set_autosuspend_delay(ldev->dev, -1);
> +	pm_runtime_dont_use_autosuspend(ldev->dev);
>  	pm_runtime_disable(ldev->dev);

[Severity: High]
Does this have the same issue as the probe error path?

Calling pm_runtime_dont_use_autosuspend() before pm_runtime_disable()
triggers rpm_idle() and suspends the device before lima_devfreq_fini() and
lima_device_fini() execute, potentially causing a crash when they access
powered down hardware.

>  
>  	lima_devfreq_fini(ldev);

As an additional observation, there are some pre-existing issues in the
driver:

[Severity: High]
This isn't a bug introduced by this patch, but do the interrupt handlers
read MMIO registers directly without verifying that the device is actively
powered on?

drivers/gpu/drm/lima/lima_mmu.c:lima_mmu_irq_handler() {
	u32 status = mmu_read(LIMA_MMU_INT_STATUS);

	/* for shared irq case */
	if (!status)
		return IRQ_NONE;
...
}

Similar patterns exist in lima_gp_irq_handler() and lima_pp_irq_handler().

Since the driver uses the IRQF_SHARED flag and supports runtime PM, if
another device on the same IRQ line triggers an interrupt while the lima
device is runtime suspended, these handlers will execute and unconditionally
read from powered-down hardware registers. This can cause an external abort
and crash the system. Should these handlers use pm_runtime_get_if_active()
before accessing hardware?

[Severity: High]
This is a pre-existing issue, but does the runtime suspend callback disable
clocks without waiting for in-flight interrupt handlers to finish?

drivers/gpu/drm/lima/lima_device.c:lima_device_suspend() {
...
	lima_regulator_disable(ldev);

	lima_clk_disable(ldev);

	return 0;
}

Because IRQF_SHARED is used, an interrupt handler could be actively
executing on another CPU when the suspend callback runs. Pulling the power
and clocks mid-execution causes the handler's MMIO accesses to fault. Does
this need a synchronize_irq() call before powering down the hardware?

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