Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] IRQ handler reads hardware registers without checking if the device is active, and the suspend path improperly uses `disable_irq()` on a shared interrupt line. - [High] Missing restart of watchdog delayed work on error paths in device suspend leaves the GPU unmonitored. - [High] Deadlock in `pvr_power_reset()` via `drm_dev_unplug()` waiting for `drm_dev_enter()` SRCU read lock. -- commit dd73407528e8930d154962b48e871046b334e51f Author: Ryan Brue <[email protected]> drm/imagination: suspend the GPU for system sleep, not just runtime PM This commit adds SYSTEM_SLEEP_PM_OPS to pvr_pm_ops to ensure the GPU gets callbacks for system suspend and resume. Previously, only RUNTIME_PM_OPS were defined, which left the driver in an inconsistent state on system resume if the GPU was active during suspend, leading to timeouts and deadlocks. By forcing runtime suspend/resume during system sleep, this patch avoids that inconsistent state. > diff --git a/drivers/gpu/drm/imagination/pvr_drv.c > b/drivers/gpu/drm/imagination/pvr_drv.c > index 20b27a468327b..b7931f75832fc 100644 > --- a/drivers/gpu/drm/imagination/pvr_drv.c > +++ b/drivers/gpu/drm/imagination/pvr_drv.c > @@ -1527,6 +1527,7 @@ MODULE_DEVICE_TABLE(of, dt_match); > > static const struct dev_pm_ops pvr_pm_ops = { > RUNTIME_PM_OPS(pvr_power_device_suspend, pvr_power_device_resume, > pvr_power_device_idle) > + SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume) [Severity: High] This isn't a bug introduced by this patch, but does invoking these suspend and resume paths reveal an issue with shared interrupts? When pvr_device_irq_init() registers the interrupt, it uses the IRQF_SHARED flag. However, during the suspend sequence, pvr_power_fw_disable() executes: disable_irq(pvr_dev->irq); Will calling disable_irq() on a shared interrupt line improperly mask it for all other devices sharing that line, rendering them unusable? Additionally, if the shared interrupt fires from another device while the GPU is suspended, pvr_device_irq_handler() will execute and unconditionally access hardware registers: if (pvr_dev->has_safety_events) safety_irq_pending = pvr_device_safety_irq_pending(pvr_dev); Since there is no pm_runtime_get_if_active() check before this read, could this result in a bus error crash when attempting to access a powered-down device? [Severity: High] This is a pre-existing issue, but does pvr_power_fw_disable() fail to restart the watchdog on error paths? The suspend path synchronously cancels the watchdog delayed work: cancel_delayed_work_sync(&pvr_dev->watchdog.work); If an early error occurs, such as a timeout in pvr_power_request_idle(), the function bails out: err = pvr_power_request_idle(pvr_dev); if (err) return err; Because the function returns early without requeuing the watchdog work, the suspend operation aborts but leaves the watchdog permanently stopped. Might this leave the GPU unmonitored and unable to recover from future hardware hangs? [Severity: High] This is a pre-existing issue noted in the commit message as a separate bug, but could a failure in the reset path lead to an SRCU deadlock? When pvr_mmu_flush_exec() attempts to flush the MMU, it first enters the SRCU read-side critical section: if (!drm_dev_enter(from_pvr_device(pvr_dev), &idx)) If the operation times out and the subsequent hardware reset attempt in pvr_power_reset() fails, the driver initiates the device lost flow: pvr_device_lost(pvr_dev); This function eventually calls drm_dev_unplug(). Since drm_dev_unplug() blocks in synchronize_srcu() waiting for all drm_dev_enter() sections to complete, won't this deadlock against the current thread which is still holding the SRCU read lock? > }; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260913-rbrue-suez-upstreaming-pvr-suspend-gpu-system-sleep-v3-0-674b907d0...@gmail.com?part=4
