Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Critical] Unconditional access to uninitialized subsystem states causes a NULL pointer dereference on early init failures. - [Critical] Hardware MMIO registers are accessed after PM domains and regulators are detached, causing a system fault. - [High] Double free/cleanup underflow of clocks and resets on early initialization failure. - [High] Clock prepare counts are leaked and never cleaned up during module unload. - [Medium] Typo in panfrost_clk_init error path causes double unprepare of bus_clock and leaks bus_ace_clock. --
--- Patch [5]: [PATCH v3 05/13] drm/panfrost: Move all device power up and down into RPM callbacks --- commit 79a6eca5bba776aeb8e6557364e0049b9f9eebb5 Author: Adrián Larumbe <[email protected]> drm/panfrost: Move all device power up and down into RPM callbacks This commit moves the GPU enablement and disablement into runtime PM resume and suspend callbacks. It is designed to prevent the device from remaining powered on forever if probe fails and to prevent devfreq OPP transition notifications from filling the kernel ring buffer. > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c > b/drivers/gpu/drm/panfrost/panfrost_device.c > --- a/drivers/gpu/drm/panfrost/panfrost_device.c > +++ b/drivers/gpu/drm/panfrost/panfrost_device.c > @@ -88,14 +109,21 @@ static int panfrost_clk_init(struct panfrost_device > *pfdev) [ ... ] > - err = clk_prepare_enable(pfdev->bus_ace_clock); > + err = clk_prepare(pfdev->bus_ace_clock); > if (err) > - goto disable_bus_clock; > + goto unprepare_bus_clock; > + > + if (!(pfdev->comp->pm_features & BIT(GPU_PM_RT))) { > + err = panfrost_clk_enable(pfdev); > + if (err) > + goto unprepare_bus_ace_clock; > + } > > return 0; > > -disable_bus_clock: > - clk_disable_unprepare(pfdev->bus_clock); > -disable_clock: > - clk_disable_unprepare(pfdev->clock); > +unprepare_bus_ace_clock: > + clk_unprepare(pfdev->bus_clock); > +unprepare_bus_clock: > + clk_unprepare(pfdev->bus_clock); [Severity: Medium] Is there a typo in this error path? If clock enablement fails, the code jumps here, but it looks like clk_unprepare(pfdev->bus_clock) is called twice under both labels, and bus_ace_clock is left prepared. [ ... ] > @@ -212,6 +251,143 @@ static int panfrost_pm_domain_init(struct > panfrost_device *pfdev) > return err; > } > > +static int panfrost_device_runtime_resume(struct device *dev) [ ... ] > +static int panfrost_device_runtime_suspend(struct device *dev) > +{ > + struct panfrost_device *pfdev = dev_get_drvdata(dev); > + > + if (!panfrost_jm_is_idle(pfdev)) > + return -EBUSY; [Severity: Critical] Does this cause a NULL pointer dereference on early initialization failures? If panfrost_gpu_init() or panfrost_mmu_init() fails during probe, the error unwinding path calls panfrost_try_suspend_device(), which synchronously invokes this callback. At this point, panfrost_jm_init() hasn't run yet, so pfdev->js is NULL. The unconditional call to panfrost_jm_is_idle(pfdev) dereferences pfdev->js without checking for NULL. > + > + panfrost_devfreq_suspend(pfdev); > + panfrost_jm_suspend_irq(pfdev); > + panfrost_mmu_suspend_irq(pfdev); > + panfrost_gpu_suspend_irq(pfdev); > + panfrost_gpu_power_off(pfdev); > + > + if (panfrost_device_is_initialized(pfdev) && > + pfdev->comp->pm_features & BIT(GPU_PM_RT)) { > + clk_disable(pfdev->bus_ace_clock); > + clk_disable(pfdev->bus_clock); > + clk_disable(pfdev->clock); > + reset_control_assert(pfdev->rstc); > + } > + > + if (!panfrost_device_is_initialized(pfdev)) { > + panfrost_clk_fini(pfdev); > + reset_control_assert(pfdev->rstc); > + } > + > + return 0; > +} [Severity: High] Can this lead to a clock prepare count leak on module unload? During a clean module unload, panfrost_device_is_initialized() is true since pfdev->js is still allocated. This means panfrost_clk_fini() will be skipped here. Since panfrost_clk_fini() was removed from panfrost_device_fini() in this patch, it looks like the clocks are never disabled or unprepared during driver removal. [ ... ] > @@ -295,6 +486,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); > out_regulator: > panfrost_regulator_fini(pfdev); [Severity: High] Does this cause a double free and clock prepare underflow? If panfrost_device_init() fails early, the out_rpm_put label calls panfrost_try_suspend_device(pfdev). The suspend callback explicitly calls panfrost_clk_fini(pfdev) and reset_control_assert(pfdev->rstc). Execution then falls through the error ladder to out_clk and out_reset, calling panfrost_clk_fini(pfdev) and panfrost_reset_fini(pfdev) again. [ ... ] > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c > b/drivers/gpu/drm/panfrost/panfrost_drv.c > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c > @@ -1002,13 +1000,13 @@ static int panfrost_probe(struct platform_device > *pdev) > if (err < 0) > goto err_out1; > > + pm_runtime_put_autosuspend(pfdev->base.dev); > > return 0; > > err_out1: > - pm_runtime_disable(pfdev->base.dev); > panfrost_device_fini(pfdev); > - pm_runtime_set_suspended(pfdev->base.dev); > + panfrost_try_suspend_device(pfdev); > err_out0: > return err; > } [Severity: Critical] Does this write to hardware MMIO registers after the device is powered off? Here, panfrost_device_fini(pfdev) runs first and detaches PM domains and regulators, physically powering off the GPU. Then panfrost_try_suspend_device(pfdev) runs. It invokes panfrost_device_runtime_suspend(), which calls panfrost_gpu_power_off() and writes to MMIO registers like SHADER_PWROFF_LO. Writing to unpowered hardware registers can trigger a synchronous external abort or system lockup. > @@ -1019,10 +1017,11 @@ 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); > + > + panfrost_try_suspend_device(pfdev); > } [Severity: Critical] This appears to have the same teardown ordering issue mentioned above. panfrost_device_fini() powers down the PM domains before panfrost_try_suspend_device() writes to the unpowered MMIO registers. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
