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