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

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

New issues:
- [High] Removing the explicit `pm_runtime_disable()` call in `panfrost_remove()` leaves PM runtime active while the driver tears down software structures and hardware abstractions. Because `devm_pm_runtime_enable()` postpones disabling until after `remove()` returns, a concurrent sysfs write (e.g., `echo on > power/control`) can trigger `panfrost_device_runtime_resume()` on a partially destroyed device, causing use-after-free or null pointer dereferences.
- [High] The patch moves hardware teardown into `panfrost_device_runtime_suspend()` and relies on `pm_runtime_put_sync_suspend()` in `panfrost_try_suspend_device()` to execute it during device removal. However, if the PM usage count is > 0 (e.g., userspace held a sysfs power reference via `power/control = on`), the device will not actually suspend. The driver then proceeds to unconditionally unprepare clocks (`clk_unprepare()`) and regulators in `panfrost_device_fini()`, even though they are still enabled and powering active hardware.

Pre-existing issues:
- [High] In `panfrost_remove()`, the driver calls `pm_runtime_get_sync()` to wake the hardware for teardown. If this fails, the hardware remains suspended (clocks and power domains off). The driver warns but unconditionally continues to `panfrost_device_fini()`, which invokes `panfrost_jm_fini()` and performs an MMIO write (`job_write(pfdev, JOB_INT_MASK, 0)`). Writing to unpowered/unclocked MMIO registers triggers a synchronous external abort on ARM.
--

--- Patch [5]: [PATCH v5 05/11] drm/panfrost: Move all device power up and down into RPM callbacks ---
commit 01a89b3f3fd9da2c5ff706aa75efcef9f9333b91
Author: Adrián Larumbe <[email protected]>

drm/panfrost: Move all device power up and down into RPM callbacks

This commit moves all GPU enabling and disabling into runtime PM resume and
suspend callbacks to ensure the device is resumed before touching registers.
It also replaces explicit pm_runtime_disable() calls during module unload
with devres management.

> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -212,6 +258,127 @@ static int panfrost_pm_domain_init(struct panfrost_device *pfdev)
[ ... ]
> +void panfrost_try_suspend_device(struct panfrost_device *pfdev)
> +{
> +	pm_runtime_put_sync_suspend(pfdev->base.dev);

[Severity: High]
If userspace holds a power reference (for example, by setting power/control
to "on" via sysfs), the PM usage count will be greater than zero and the
device will not actually suspend here.

If the suspend silently aborts, the driver will proceed to
panfrost_device_fini() where panfrost_clk_fini() unconditionally calls
clk_unprepare():

panfrost_device_fini() ->
  panfrost_clk_fini()
    clk_unprepare(pfdev->bus_ace_clock);
    clk_unprepare(pfdev->bus_clock);
    clk_unprepare(pfdev->clock);

Does this leave the hardware abruptly deprived of prepared clocks and
regulators while it is still enabled?

> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -1015,10 +1015,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);
> +

[Severity: High]
This is a pre-existing issue, but if pm_runtime_get_sync() fails, the
hardware will remain suspended with its clocks and power domains off.

Because execution unconditionally continues to panfrost_device_fini(),
panfrost_jm_fini() will perform an MMIO write to JOB_INT_MASK:

panfrost_remove() ->
  panfrost_device_fini() ->
    panfrost_jm_fini()
       job_write(pfdev, JOB_INT_MASK, 0);

Writing to unpowered or unclocked MMIO registers usually triggers a
synchronous external abort on ARM. Should the teardown path be skipped or
handled differently if the resume fails?

>  	panfrost_device_fini(pfdev);
> -	pm_runtime_set_suspended(pfdev->base.dev);
>  }

[Severity: High]
Can removing the explicit pm_runtime_disable() call here cause a
use-after-free during teardown?

Since devm_pm_runtime_enable() postpones disabling PM runtime until after
the remove() function returns, PM runtime remains active while
panfrost_device_fini() destroys software structures and hardware abstractions.

Could a concurrent sysfs write (like echoing "on" to power/control) trigger
panfrost_device_runtime_resume() on a partially destroyed device?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5