Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] panthor: panthor_device_resume() incorrectly propagates -EAGAIN on AW race - [High] panthor: post-yield worker accesses hardware with clocks disabled during system suspend - [High] panthor: hardware fails to re-initialize on system resume if suspended while GPU_STOPPED - [Medium] panthor: hardware components skip graceful teardown on runtime suspend race - [Medium] panthor: pm_runtime_put_noidle() in post_yield_work stalls autosuspend indefinitely --
commit 8aa28a6a344b704e733c2685619e190f808b85d0 Author: Karunika Choo <[email protected]> drm/panthor: Route HW component PM through access windows This commit makes access-window resume and suspend own GPU access when AW support is present, while keeping the direct HW component PM path for older GPUs. It also handles WINDOW_CLOSED and arbiter restart events and defers post-yield cleanup to a workqueue context. > diff --git a/drivers/gpu/drm/panthor/panthor_aw.c > b/drivers/gpu/drm/panthor/panthor_aw.c > index 40122bb4b83af..4fcfb3ad05073 100644 > --- a/drivers/gpu/drm/panthor/panthor_aw.c > +++ b/drivers/gpu/drm/panthor/panthor_aw.c [ ... ] > @@ -118,6 +121,52 @@ static int panthor_aw_state_wait_transition(struct > panthor_aw *aw, u32 timeout_m > timeout_ms); > } > > +static void panthor_aw_post_yield_work(struct work_struct *work) > +{ > + struct panthor_aw *aw = > + container_of(work, struct panthor_aw, post_yield_work); > + struct panthor_device *ptdev = aw->ptdev; > + struct device *dev = ptdev->base.dev; > + int ret; > + > + /* Something else has progressed the state to READY. */ > + if (atomic_read(&aw->state) != PANTHOR_AW_STATE_GPU_STOPPED) > + return; > + > + /* > + * Ensure in-progress resume is completed and prevent future RPM suspend > + * to keep the clocks turned on when accessing HW registers. > + */ > + pm_runtime_get_noresume(dev); > + pm_runtime_barrier(dev); > + > + ret = panthor_device_suspend_hw_components(ptdev); [Severity: Medium] If a WINDOW_CLOSED event queues panthor_aw_post_yield_work() and races with a runtime suspend, could this skip the graceful hardware teardown? Since panthor_aw_suspend() skips panthor_device_suspend_hw_components() when the state is PANTHOR_AW_STATE_GPU_STOPPED, the clocks would be disabled and the device becomes RPM_SUSPENDED. When this worker executes, pm_runtime_get_noresume() does not wake the device. Will panthor_device_suspend_hw_components() then see pm_runtime_suspended() as true and skip the teardown entirely, leaving caches and firmware improperly halted? [Severity: High] Could this worker also access hardware registers with clocks disabled during a system suspend? If a system suspend races with this worker, panthor_device_suspend() could call panthor_aw_suspend(), which skips the teardown due to the GPU_STOPPED state. Clocks are then disabled, but since system PM leaves the runtime status as RPM_ACTIVE, panthor_device_suspend_hw_components() might proceed to access hardware registers without clocks, potentially causing a bus fault. > + if (ret) > + dev_warn(dev, "Failed to suspend hardware components."); > + > + panthor_aw_state_set(aw, PANTHOR_AW_STATE_READY); > + > + /* > + * schedule an immediate tick to re-evaluate if there is any additional > + * work. This will request access to the GPU again and if the driver has > + * gone idle, it will yield GPU access via the runtime suspend path. > + */ > + panthor_sched_resume(ptdev); > + > + pm_runtime_put_noidle(dev); [Severity: Medium] Does pm_runtime_put_noidle() drop the PM reference without re-arming the autosuspend timer? If the autosuspend timer fires while the temporary reference is held in this function, the suspend aborts due to a non-zero usage count. Using pm_runtime_put_noidle() drops the usage count but does not queue an idle notification. Could this leave the device actively powered on indefinitely until another explicit PM transition occurs? > +} [ ... ] > @@ -396,20 +531,45 @@ int panthor_aw_suspend(struct panthor_device *ptdev) > > /* suspend hw components directly if AW is not supported */ > if (!aw) > - return 0; > + return panthor_device_suspend_hw_components(ptdev); > > if (atomic_read(&aw->state) == PANTHOR_AW_STATE_READY) > goto out_irq_suspend; > > + if (atomic_read(&aw->state) == PANTHOR_AW_STATE_GPU_GRANTED) { > + ret = panthor_device_suspend_hw_components(ptdev); > + if (ret) > + goto out_irq_suspend; > + } > + > if (panthor_aw_state_try_set(aw, PANTHOR_AW_STATE_GPU_GRANTED, > - PANTHOR_AW_STATE_GPU_STOPPED)) > + PANTHOR_AW_STATE_STOPPED_IDLE)) { [Severity: High] If a system suspend occurs while aw->state is PANTHOR_AW_STATE_GPU_STOPPED, could this fail to re-initialize the hardware on resume? Since this skips panthor_device_suspend_hw_components(), the ptdev->pm.hw_component_state remains PANTHOR_DEVICE_PM_STATE_ACTIVE. During system resume, panthor_device_resume_hw_components() will check if the state is ACTIVE and immediately return 0. Does this leave the firmware, MMU, and GPU uninitialized after losing power in S3? > ret = panthor_aw_yield(aw); [ ... ] > diff --git a/drivers/gpu/drm/panthor/panthor_device.c > b/drivers/gpu/drm/panthor/panthor_device.c > index 2ccd8ad6318d1..ea35c5e1d87b4 100644 > --- a/drivers/gpu/drm/panthor/panthor_device.c > +++ b/drivers/gpu/drm/panthor/panthor_device.c [ ... ] > @@ -577,11 +577,11 @@ int panthor_device_resume(struct device *dev) > atomic_set(&ptdev->reset.pending, 0); > } > > - ret = panthor_device_resume_hw_components(ptdev); > + ret = panthor_aw_resume(ptdev); > if (ret && ptdev->reset.fast) { > drm_err(&ptdev->base, "Fast reset failed, trying a slow > reset"); > ptdev->reset.fast = false; > - ret = panthor_device_resume_hw_components(ptdev); > + ret = panthor_aw_resume(ptdev); > } [Severity: High] Does panthor_device_resume() incorrectly propagate -EAGAIN on an AW race? If a WINDOW_CLOSED event races with a runtime PM resume, panthor_aw_resume() observes PANTHOR_AW_STATE_GPU_STOPPED and returns -EAGAIN. Propagating this directly to the PM core here causes the runtime resume to fail entirely. [ ... ] > diff --git a/drivers/gpu/drm/panthor/panthor_device.h > b/drivers/gpu/drm/panthor/panthor_device.h > index 42c5052ac83b9..59bc48d05eb1e 100644 > --- a/drivers/gpu/drm/panthor/panthor_device.h > +++ b/drivers/gpu/drm/panthor/panthor_device.h [ ... ] > @@ -466,8 +467,18 @@ static inline int panthor_device_resume_and_get(struct > panthor_device *ptdev) > * succeeded. Given resume errors are not expected, this is probably > * something we can live with. > */ > - if (ret && atomic_cmpxchg(&ptdev->pm.recovery_needed, 1, 0) == 1) > - pm_runtime_set_suspended(ptdev->base.dev); > + if (ret) { > + if (atomic_cmpxchg(&ptdev->pm.recovery_needed, 1, 0) == 1) > + pm_runtime_set_suspended(ptdev->base.dev); > + > + return ret; > + } > + > + ret = panthor_aw_ensure_gpu_access(ptdev); [Severity: High] Does this early bailout bypass the intended retry logic? Because panthor_device_resume() propagates the -EAGAIN failure directly, the initial pm_runtime_resume_and_get() call fails and populates ret. Returning ret here completely bypasses the panthor_aw_ensure_gpu_access() retry loop that was specifically designed to handle the -EAGAIN state. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=24
