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 <[email protected]> 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/[email protected]?part=9
