Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] drm/panfrost: PM domain race in remove path due to early pm_runtime_set_suspended() --
commit a90e5a94bd1de191941b3e25b2a2eb1b45c873d9 Author: Adrián Larumbe <[email protected]> drm/panfrost: Fix PM refcnt and autosuspend issues at device probe/remove This commit adjusts the PM reference count at device probe and removal time to ensure the device goes to sleep when idle and doesn't leak references on unload. It also reorders pm_runtime_set_suspended() to occur before panfrost_device_fini() in the teardown paths so resource unwinding happens in the opposite order of initialization. > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c > b/drivers/gpu/drm/panfrost/panfrost_drv.c > index 55fc22e8d4d4e..02ddd391231c0 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c [ ... ] > @@ -866,13 +871,16 @@ static int panfrost_probe(struct platform_device *pdev) > if (err < 0) > goto err_out1; > > + pm_runtime_put_autosuspend(pfdev->base.dev); > > return 0; > > err_out1: > + pm_runtime_dont_use_autosuspend(pfdev->base.dev); > pm_runtime_disable(pfdev->base.dev); > - panfrost_device_fini(pfdev); > + pm_runtime_put_noidle(pfdev->base.dev); > pm_runtime_set_suspended(pfdev->base.dev); > + panfrost_device_fini(pfdev); [Severity: High] Does moving pm_runtime_set_suspended() before panfrost_device_fini() in panfrost_probe() create a race condition with the PM workqueue? When pm_runtime_set_suspended() is called, it triggers rpm_put_suppliers() which drops the active reference for the attached secondary PM domains (shader/tiler) and queues an asynchronous suspend via pm_request_idle(). Concurrently, panfrost_device_fini() proceeds to execute panfrost_gpu_fini(), which writes to GPU MMIO (e.g., SHADER_PWROFF_LO) and polls for power transitions. Could the PM workqueue power off the domains asynchronously during this teardown, leading to hardware lockups or fatal bus errors if MMIO accesses are made to unpowered domains? > err_out0: > return err; > } > @@ -884,9 +892,11 @@ static void panfrost_remove(struct platform_device *pdev) > drm_dev_unregister(&pfdev->base); > > pm_runtime_get_sync(pfdev->base.dev); > + pm_runtime_dont_use_autosuspend(pfdev->base.dev); > + pm_runtime_put_noidle(pfdev->base.dev); > pm_runtime_disable(pfdev->base.dev); > - panfrost_device_fini(pfdev); > pm_runtime_set_suspended(pfdev->base.dev); > + panfrost_device_fini(pfdev); [Severity: High] Can this race condition also happen in the panfrost_remove() path? Similar to the probe error path, setting the status to RPM_SUSPENDED before panfrost_device_fini() might prematurely power off the secondary PM domains while hardware teardown is still accessing them. > } > > static ssize_t profiling_show(struct device *dev, -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
