Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] AB-BA Deadlock between soft reset in IRQ handler and watchdog worker - [High] Watchdog worker is left permanently cancelled if firmware disable fails during suspend -- commit 492c71c98e6c2dced455b7063ec9414f76434f9f Author: Ryan Brue <[email protected]> drm/imagination: don't gate the runtime PM callbacks on drm_dev_enter() This commit removes drm_dev_enter() and drm_dev_exit() gating from the system sleep and runtime PM suspend/resume callbacks. Instead, it relies on checking pvr_dev->lost to prevent a deadlock between the watchdog worker and the system sleep paths. > diff --git a/drivers/gpu/drm/imagination/pvr_power.c > b/drivers/gpu/drm/imagination/pvr_power.c > index eb4b6ecdf4f4d..8d82b9a79daf8 100644 > --- a/drivers/gpu/drm/imagination/pvr_power.c > +++ b/drivers/gpu/drm/imagination/pvr_power.c > @@ -97,6 +97,10 @@ pvr_power_fw_disable(struct pvr_device *pvr_dev, bool > hard_reset, bool rpm_suspe > if (!hard_reset) { > cancel_delayed_work_sync(&pvr_dev->watchdog.work); [Severity: High] This isn't a bug introduced by this patch, but is there an AB-BA deadlock between the soft reset in the IRQ handler and the watchdog worker here? If the firmware requests a soft reset in process_fwccb_command (ROGUE_FWIF_FWCCB_CMD_REQUEST_GPU_RESTART), the IRQ handler calls pvr_power_reset() and acquires the reset semaphore: pvr_power_reset() down_write(&pvr_dev->reset_sem); ... pvr_power_fw_disable(); Then pvr_power_fw_disable() blocks here waiting for the watchdog worker to finish via cancel_delayed_work_sync(). If the watchdog worker (pvr_watchdog_worker) detects a stall concurrently, it calls pvr_power_reset(), which blocks trying to acquire the same reset_sem, causing both the kernel workqueue thread and the GPU IRQ thread to hang permanently. > > + /* The worker just cancelled may have lost the device. */ > + if (pvr_dev->lost) > + return -EIO; > + > err = pvr_power_request_idle(pvr_dev); > if (err) > return err; [Severity: High] This is also a pre-existing issue, but does this leave the watchdog worker permanently cancelled if the firmware disable fails during suspend? When pvr_power_device_suspend() calls pvr_power_fw_disable(), the watchdog worker is cancelled synchronously above. We then send a forced idle request here in pvr_power_request_idle(). If the firmware is stalled, this KCCB command times out and returns an error. We then return that error immediately, which aborts the suspend. The PM core leaves the device in the RPM_ACTIVE state, but the watchdog worker is never restarted on this error path. Because the firmware is stalled and the watchdog is disabled, it seems the GPU will never recover. [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260913-rbrue-suez-upstreaming-pvr-suspend-gpu-system-sleep-v3-0-674b907d0...@gmail.com?part=1
