Re: [PATCH v3 1/4] drm/imagination: don't gate the runtime PM callbacks on drm_dev_enter()

[sashiko-bot@kernel.org]

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