Re: [PATCH v6 08/16] 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] Calling pm_runtime_put_noidle() before pm_runtime_disable() during device teardown introduces a race condition that can lead to accessing suspended hardware.

Pre-existing issues:
- [High] Unchecked pm_runtime_get_sync() failure leads to register access while hardware is suspended.
--

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

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

This commit addresses issues where the PM usage count is not correctly
adjusted during probe and unplug, which prevented the runtime suspend
callback from being invoked or caused the usage count to monotonically
increase on reloads. It manually adjusts the PM reference count in these
paths and disables autosuspend during device teardown.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 0cc277efb5850..f6066aef47666 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 calling pm_runtime_put_noidle() before pm_runtime_disable() create a
race condition here?

If a concurrent job completes in the background (since schedulers have not
been stopped yet), could its interrupt handler call
pm_runtime_put_autosuspend()?

Because the teardown thread just dropped its reference, this concurrent put
could drop the usage count to 0 and suspend the device immediately.

When pm_runtime_disable() then flushes pending work and returns, the teardown
thread will proceed to panfrost_device_disable_hw(). Could accessing the
registers of the now-suspended device result in a synchronous external abort?

Would it be safer to call pm_runtime_put_noidle() after pm_runtime_disable()
to guarantee no further runtime PM state transitions occur during teardown?

>  	panfrost_device_disable_hw(pfdev);
>  	panfrost_gem_fini(pfdev);
[ ... ]
> @@ -363,6 +368,8 @@ int panfrost_device_init(struct panfrost_device *pfdev)
>  void panfrost_device_fini(struct panfrost_device *pfdev)
>  {
>  	pm_runtime_get_sync(pfdev->base.dev);

[Severity: High]
This isn't a bug introduced by this patch, but does ignoring the return value
of pm_runtime_get_sync() risk causing a system crash?

If waking the device fails (for example, due to a clock or regulator
failure), it will return a negative error code and the device remains
suspended. Since the error is ignored, panfrost_device_fini() proceeds
unconditionally.

Could this lead to panfrost_device_disable_hw() attempting to access
registers while the hardware is powered off, resulting in a bus error?

Note that while follow-up patch 9 in this series adds a drm_WARN_ON() here,
it doesn't appear to abort the teardown or prevent subsequent hardware accesses
on failure.

> +	pm_runtime_dont_use_autosuspend(pfdev->base.dev);
> +	pm_runtime_put_noidle(pfdev->base.dev);
>  	pm_runtime_disable(pfdev->base.dev);

[Severity: High]
Does the same race condition mentioned above also apply here in
panfrost_device_fini()?

If a background job completes and triggers a suspend after the reference is
dropped but before pm_runtime_disable() finishes, could this cause
panfrost_device_disable_hw() to access a powered-off device?

>  
>  	panfrost_device_disable_hw(pfdev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826-claude-fixes-v6-0-78119af409e7@collabora.com?part=8