Re: [PATCH v3 4/4] drm/imagination: suspend the GPU for system sleep, not just runtime PM

[sashiko-bot@kernel.org]

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 <ryanbrue.dev@gmail.com>

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-674b907d0652@gmail.com?part=4