Re: [PATCH v3 05/13] drm/panfrost: Move all device power up and down into RPM callbacks

[sashiko-bot@kernel.org]

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 <adrian.larumbe@collabora.com>

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/20260724-claude-fixes-v3-0-542af61abd0f@collabora.com?part=5