Re: [PATCH v7 09/17] drm/panfrost: Fix PM refcnt and autosuspend issues at device probe/remove

[sashiko-bot@kernel.org]

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

New issues:
- [High] Dropping the PM usage counter before disabling runtime PM in the init error path creates a window for unintended device suspend, leading to a potential crash.

Pre-existing issues:
- [High] panfrost_device_fini() unconditionally accesses hardware registers even if pm_runtime_get_sync() fails, leading to a potential crash.
--

commit 2b42dddf42e663beb9d859051cf7ae5891d997e4
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

drm/panfrost: Fix PM refcnt and autosuspend issues at device probe/remove

This commit addresses power management reference count and autosuspend
issues during device probe and removal. It manually adjusts the PM refcount
and calls the autosuspend or noidle functions at the appropriate times to
prevent the device from remaining powered on forever or increasing its
usage count monotonically on module reloads.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 99f7da2180f92..70c8109ea6987 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
[ ... ]
> @@ -334,9 +335,13 @@ int panfrost_device_init(struct panfrost_device *pfdev)
>  	if (err < 0)
>  		goto out_devreg;
>  
> +	pm_runtime_put_autosuspend(pfdev->base.dev);
> +
>  	return 0;
>  
>  out_devreg:
> +	pm_runtime_dont_use_autosuspend(pfdev->base.dev);
> +	pm_runtime_put_noidle(pfdev->base.dev);
>  	pm_runtime_disable(pfdev->base.dev);

[Severity: High]
Does dropping the PM usage counter before disabling runtime PM create a race
window here? 

If `pm_runtime_put_noidle()` drops the PM usage count to 0 while PM runtime
is still active, a concurrent thread (e.g., userspace modifying sysfs power
controls) could trigger an asynchronous device suspend. This would turn off
clocks and power domains right before the error path proceeds to call
`panfrost_device_disable_hw()`, attempting to write to memory-mapped hardware
registers that are now powered off, which can cause a Synchronous External
Abort.

Should `pm_runtime_disable()` be called before `pm_runtime_put_noidle()` to
guarantee the device remains active during teardown?

>  	panfrost_device_disable_hw(pfdev);
>  	panfrost_gem_fini(pfdev);
[ ... ]
> @@ -363,7 +368,9 @@ int panfrost_device_init(struct panfrost_device *pfdev)
>  void panfrost_device_fini(struct panfrost_device *pfdev)
>  {
>  	pm_runtime_get_sync(pfdev->base.dev);
> +	pm_runtime_dont_use_autosuspend(pfdev->base.dev);
>  	pm_runtime_disable(pfdev->base.dev);
> +	pm_runtime_put_noidle(pfdev->base.dev);
>  
>  	panfrost_jm_stop_sched_jobs(pfdev);
>  	panfrost_device_disable_hw(pfdev);

[Severity: High]
This is a pre-existing issue, but panfrost_device_fini() unconditionally
accesses hardware registers even if pm_runtime_get_sync() fails.

If `pm_runtime_get_sync()` returns `< 0` (e.g., due to a hardware fault,
regulator issue, or clock failure), the function continues unconditionally
to `panfrost_device_disable_hw()`. This attempts to write to GPU/MMU
registers without power/clocks, which will result in a system crash
(Synchronous External Abort).

Can we add a check to handle the `pm_runtime_get_sync()` failure and avoid
accessing the powered-down hardware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828-claude-fixes-v7-0-72a13b2c125d@collabora.com?part=9