Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Critical] GPU cores remain unpowered after probe due to uninitialized feature masks during the initial RPM resume. - [High] Driver unbind leaves hardware active and violates PM guidelines, risking Use-After-Free and clock API violations. - [High] Error paths in probe trigger RPM suspend callbacks that call `synchronize_irq()` on uninitialized or negative IRQ numbers. - [Low] Duplicate hardware teardown in `panfrost_device_init` error path. - [Low] Unbalanced reset control assertion in probe error path. --
--- Patch [5]: [PATCH v4 05/13] drm/panfrost: Move all device power up and down into RPM callbacks --- commit 0a032cad0321fa83e882812bf23789897712f899 Author: Adrián Larumbe <[email protected]> Subject: drm/panfrost: Move all device power up and down into RPM callbacks This patch moves all GPU enabling and disabling into runtime PM resume and suspend callbacks. It aims to fix issues where the device remains powered on forever due to usage count mismatches during probe failures or module unloads. > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c > b/drivers/gpu/drm/panfrost/panfrost_device.c > index 03905495aee2d..a9d29d9ee22bb 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_device.c > +++ b/drivers/gpu/drm/panfrost/panfrost_device.c [ ... ] > @@ -48,7 +82,7 @@ static int panfrost_clk_init(struct panfrost_device *pfdev) [ ... ] > static void panfrost_clk_fini(struct panfrost_device *pfdev) > { > - clk_disable_unprepare(pfdev->bus_ace_clock); > - clk_disable_unprepare(pfdev->bus_clock); > - clk_disable_unprepare(pfdev->clock); > + if (!(pfdev->comp->pm_features & BIT(GPU_PM_RT))) > + panfrost_clk_disable(pfdev); [Severity: Low] Can this lead to an unbalanced reset control assertion in the probe error path? When panfrost_clk_fini() is called during the out_clk error path in panfrost_device_init(), it calls panfrost_clk_disable(), which asserts pfdev->rstc. The error path then falls through to out_reset, which calls panfrost_reset_fini(). This asserts the exact same reset control a second time, which can trigger a kernel warning from the reset controller subsystem due to unbalanced assertion counts. [ ... ] > @@ -265,10 +437,25 @@ int panfrost_device_init(struct panfrost_device *pfdev) [ ... ] > + err = pm_runtime_resume_and_get(pfdev->base.dev); > + if (err) > + goto out_regulator; > + > + /* If PM is disabled, we need to call panfrost_device_runtime_resume() > manually. */ > + if (!IS_ENABLED(CONFIG_PM)) { > + err = panfrost_device_runtime_resume(pfdev->base.dev); > + if (err) > + goto out_regulator; > + } > + > - err = panfrost_gpu_init(pfdev); > + err = panfrost_gpu_init(pfdev); [Severity: Critical] Does this sequence leave the GPU cores unpowered? When pm_runtime_resume_and_get() triggers the runtime resume callback (panfrost_device_runtime_resume()), it calls panfrost_gpu_power_on(). However, this happens before panfrost_gpu_init() has initialized pfdev->features. Because pfdev->features is uninitialized at this point, it is entirely zero. Consequently, panfrost_gpu_power_on() will write 0 to the power-on registers (L2_PWRON_LO, etc.), which fails to power on the cores. [ ... ] > @@ -295,6 +482,8 @@ int panfrost_device_init(struct panfrost_device *pfdev) > panfrost_mmu_fini(pfdev); > out_gpu: > panfrost_gpu_fini(pfdev); > +out_rpm_put: > + panfrost_try_suspend_device(pfdev); [Severity: Low] Does this error path duplicate hardware teardown? If initialization fails after MMU init, the code jumps to out_mmu, manually calling panfrost_mmu_fini() and panfrost_gpu_fini(). It then falls through to out_rpm_put and calls panfrost_try_suspend_device(). This triggers the RPM suspend callback, which unconditionally calls panfrost_mmu_suspend_irq(), panfrost_gpu_suspend_irq(), and panfrost_gpu_power_off() a second time. [Severity: High] Can this error path trigger an out-of-bounds access in the IRQ descriptor tree? If panfrost_device_init() fails early (e.g., inside panfrost_gpu_init()), the error path jumps to out_rpm_put and forces a runtime suspend. This unconditionally executes panfrost_mmu_suspend_irq() and panfrost_gpu_suspend_irq(). Because initialization failed early, pfdev->mmu_irq is 0, and if platform_get_irq_byname() failed, pfdev->gpu_irq contains a negative error code. Passing these uninitialized or negative values to synchronize_irq() can cause undefined behavior or synchronize unrelated system interrupts. > out_regulator: > panfrost_regulator_fini(pfdev); [ ... ] > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c > b/drivers/gpu/drm/panfrost/panfrost_drv.c > index 331a3bd5b98cb..27f4c4b217f5b 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c [ ... ] > @@ -1019,10 +1016,9 @@ static void panfrost_remove(struct platform_device > *pdev) > > drm_dev_unregister(&pfdev->base); > > - pm_runtime_get_sync(pfdev->base.dev); > - pm_runtime_disable(pfdev->base.dev); > + drm_WARN_ON(&pfdev->base, pm_runtime_get_sync(pfdev->base.dev) < 0); > + > panfrost_device_fini(pfdev); > - pm_runtime_set_suspended(pfdev->base.dev); > } [Severity: High] Does this driver unbind sequence risk leaving the hardware active? By removing pm_runtime_disable(), the teardown relies on panfrost_try_suspend_device() (called inside panfrost_device_fini) to suspend the device. However, if panfrost_jm_is_idle() returns false, or if a userspace process holds the PM usage count > 0 via sysfs power/control, the RPM suspend callback is never executed. In this scenario, panfrost_device_fini() proceeds to unconditionally free resources and unprepare clocks while they are still enabled and the GPU is still active. This risks clock API violations and unhandled interrupt storms since the hardware was never powered off or masked. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
