[PATCH v3 0/4] drm/imagination: fix system suspend, and the callback design underneath it

[PATCH v3 0/4] drm/imagination: fix system suspend, and the callback design underneath it

[Ryan Brue]

The GPU has no system-sleep callbacks, only runtime PM ones. If it is
runtime-active when the system suspends, genpd drops the power domain
behind the driver's back and the first firmware operation after resume
wedges its caller in an unkillable D state. Patch 4 is that fix, and is
what went out as v1 and v2.

The v2 review showed that wiring the force helpers in raw was not safe
as the driver stood. The cause is one design fault: the runtime PM
callbacks use drm_dev_enter() as a "device lost" test and hold the SRCU
section while waiting for the watchdog worker. That makes a lost device
fail every later suspend and makes pm_runtime_force_suspend() deadlock
against a worker that loses the device. Patch 1 fixes the callbacks,
which lets patch 2 unplug first in pvr_remove() and lets patch 4 go
back to v1's one-liner. Patch 3 fixes a watchdog-vs-teardown race found
on the way.

On the v2 review's remaining finding, that pvr_power_fw_disable() can
return without re-arming the watchdog: after a failed runtime suspend
the device is in runtime_error with a zero usage count, and the
worker's pm_runtime_get_if_in_use() returns 0 in that state, so a
re-armed watchdog could not act. No change made for it.

All four were measured on mt8173 (Rogue GX6250), both arms from one
build switched at runtime; the details are in the individual messages.
Lockdep cannot report the patch 1 deadlock: cancel_delayed_work_sync()
only touches the work's lockdep map when a worker is executing, so the
srcu -> work edge is never recorded unless the cancel races a running
worker.

Signed-off-by: Ryan Brue <ryanbrue.dev@gmail.com>
---
Changes in v3:
- Restructured as a four-patch series. Instead of wrapping the force
  helpers to work around the runtime PM callbacks, 1/4 fixes the
  callbacks: they test pvr_dev->lost rather than drm_dev_enter(), and
  hold no SRCU section while waiting for the watchdog worker. That
  removes the [Critical] AB-BA deadlock and the [High] TOCTOU at their
  source, and 4/4 returns to v1's raw pm_runtime_force_suspend()/resume().
- 2/4 fixes the pre-existing remove() ordering the review reported, and
  can now put drm_dev_unplug() first.
- 3/4 fixes a pre-existing watchdog-vs-teardown race found while
  reviewing the above.
- The review's "watchdog not re-armed" finding is answered in the cover
  letter rather than patched; see above for why.
- Added hardware evidence for both arms of every patch; v2 asserted the
  failure without showing it.
- Link to v2: https://patch.msgid.link/20260909-rbrue-suez-upstreaming-pvr-suspend-gpu-system-sleep-v2-1-66a938e92b87@gmail.com

Changes in v2:
- Don't wire pm_runtime_force_suspend/resume in directly. Once the GPU
  has been lost, drm_dev_enter() fails, pvr_power_device_suspend() returns
  -EIO, and the force helper hands that to the PM core, which aborts the
  system suspend -- permanently, since a lost device never becomes
  suspendable again. v1 would have traded a GPU that dies on suspend for a
  machine that cannot suspend at all. v2 wraps the helpers and skips the
  transition when the device is already unplugged. Caught by the Sashiko
  AI review bot.
- Commit message: say why the wrappers exist, and note that
  pvr_power_fw_disable() leaves the watchdog cancelled on its error path
  -- pre-existing, but system sleep is a new way to reach it.
- Cc'd Chen-Yu Tsai, Icenowy Zheng and YoungJoon Lee, who are testing this
  same GPU in the MT8173 powervr thread [1] and are the people best placed
  to say whether this reproduces on a Chromebook. That thread doesn't
  mention suspend at all.
- Dropped sarah.walker@imgtec.com and donald.robson@imgtec.com, both of
  which bounced on v1.
[1] https://lore.kernel.org/all/20260728091804.382753-1-wenst@chromium.org/

- Link to v1: https://patch.msgid.link/20260909-rbrue-suez-upstreaming-pvr-suspend-gpu-system-sleep-v1-1-f45847bc951b@gmail.com

---
Ryan Brue (4):
      drm/imagination: don't gate the runtime PM callbacks on drm_dev_enter()
      drm/imagination: unplug the device before tearing it down in pvr_remove()
      drm/imagination: stop the watchdog before tearing the device down
      drm/imagination: suspend the GPU for system sleep, not just runtime PM

 drivers/gpu/drm/imagination/pvr_drv.c   | 17 ++++++++++---
 drivers/gpu/drm/imagination/pvr_power.c | 44 +++++++++++++--------------------
 2 files changed, 31 insertions(+), 30 deletions(-)
---
base-commit: df2908090cda368b01ff43709f51890076c56157
change-id: 20260909-rbrue-suez-upstreaming-pvr-suspend-gpu-system-sleep-bfb9dfd7007b

Best regards,
--  
Ryan Brue <ryanbrue.dev@gmail.com>

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

[Ryan Brue]

pvr_power_device_suspend() and pvr_power_device_resume() take
drm_dev_enter() for their whole body and return -EIO once the device is
unplugged. The section protects nothing: the callbacks run on the
platform device, which stays bound until pvr_remove() returns. It is
being used as a "device lost" test, and pvr_dev->lost already is one.

Used that way it causes two problems. The suspend callback waits for
the watchdog worker in pvr_power_fw_disable() while holding the section,
and a worker losing the device calls drm_dev_unplug(), which waits in
synchronize_srcu() for that section: neither finishes. Runtime PM never
lets the two overlap because the worker holds a usage-count reference,
but pm_runtime_force_suspend() ignores the count, so a system-sleep
implementation on top of these callbacks deadlocks. Reproduced on
mt8173 with the worker delayed between taking its reference and losing
the device: system suspend froze until the hardware watchdog fired. And
once the device is lost the callbacks fail forever, which leaves runtime
PM in runtime_error and would abort every system suspend.

Test pvr_dev->lost instead and hold no section; a lost device is a
no-op for both callbacks. Re-test the flag after the cancel in
pvr_power_fw_disable(), since the worker just waited for may be the one
that lost the device.

Fixes: 727538a4bbff ("drm/imagination: Implement power management")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Ryan Brue <ryanbrue.dev@gmail.com>
---
 drivers/gpu/drm/imagination/pvr_power.c | 44 +++++++++++++--------------------
 1 file changed, 17 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/imagination/pvr_power.c b/drivers/gpu/drm/imagination/pvr_power.c
index eb4b6ecdf4f4..8d82b9a79daf 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);
 
+		/* 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;
@@ -372,24 +376,19 @@ pvr_power_device_suspend(struct device *dev)
 	struct platform_device *plat_dev = to_platform_device(dev);
 	struct drm_device *drm_dev = platform_get_drvdata(plat_dev);
 	struct pvr_device *pvr_dev = to_pvr_device(drm_dev);
-	int err = 0;
-	int idx;
+	int err;
 
-	if (!drm_dev_enter(drm_dev, &idx))
-		return -EIO;
+	/* A lost device is left as the failed reset left it. */
+	if (pvr_dev->lost)
+		return 0;
 
 	if (READ_ONCE(pvr_dev->fw_dev.initialised)) {
 		err = pvr_power_fw_disable(pvr_dev, false, true);
 		if (err)
-			goto err_drm_dev_exit;
+			return pvr_dev->lost ? 0 : err;
 	}
 
-	err = pvr_dev->device_data->pwr_ops->power_off(pvr_dev);
-
-err_drm_dev_exit:
-	drm_dev_exit(idx);
-
-	return err;
+	return pvr_dev->device_data->pwr_ops->power_off(pvr_dev);
 }
 
 int
@@ -398,33 +397,24 @@ pvr_power_device_resume(struct device *dev)
 	struct platform_device *plat_dev = to_platform_device(dev);
 	struct drm_device *drm_dev = platform_get_drvdata(plat_dev);
 	struct pvr_device *pvr_dev = to_pvr_device(drm_dev);
-	int idx;
 	int err;
 
-	if (!drm_dev_enter(drm_dev, &idx))
-		return -EIO;
+	if (pvr_dev->lost)
+		return 0;
 
 	err = pvr_dev->device_data->pwr_ops->power_on(pvr_dev);
 	if (err)
-		goto err_drm_dev_exit;
+		return err;
 
 	if (READ_ONCE(pvr_dev->fw_dev.initialised)) {
 		err = pvr_power_fw_enable(pvr_dev, true);
-		if (err)
-			goto err_power_off;
+		if (err) {
+			pvr_dev->device_data->pwr_ops->power_off(pvr_dev);
+			return err;
+		}
 	}
 
-	drm_dev_exit(idx);
-
 	return 0;
-
-err_power_off:
-	pvr_dev->device_data->pwr_ops->power_off(pvr_dev);
-
-err_drm_dev_exit:
-	drm_dev_exit(idx);
-
-	return err;
 }
 
 int

-- 
2.55.0

[PATCH v3 2/4] drm/imagination: unplug the device before tearing it down in pvr_remove()

[Ryan Brue]

pvr_remove() destroys the job and free list xarrays, powers the GPU
down and runs pvr_device_fini() before calling drm_dev_unplug(), so an
ioctl that entered its drm_dev_enter() section before the unbind can
still be walking structures that have already been freed.

Call drm_dev_unplug() first. It makes every later drm_dev_enter() fail
and waits for the sections already in flight. The runtime suspend that
follows works on an unplugged device now that the callbacks no longer
gate on drm_dev_enter().

Skip the unplug if pvr_device_lost() has already done it: drm_dev_unplug()
is not idempotent, and the second call oopses in
drm_client_sysrq_unregister() on a node the first one removed. Unbinding
a GPU that a failed reset marked lost hits that deterministically.

Holding an ioctl in its section for 4s on mt8173 while unbinding shows
the change: before, pvr_device_fini() completed 3.1s before the ioctl
left its section; after, drm_dev_unplug() blocks for those 3.1s and the
teardown follows.

Fixes: 1f88f017e649 ("drm/imagination: Get GPU resources")
Fixes: 727538a4bbff ("drm/imagination: Implement power management")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Ryan Brue <ryanbrue.dev@gmail.com>
---
 drivers/gpu/drm/imagination/pvr_drv.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
index 5c965ef0274f..fc92a82a7208 100644
--- a/drivers/gpu/drm/imagination/pvr_drv.c
+++ b/drivers/gpu/drm/imagination/pvr_drv.c
@@ -1469,15 +1469,23 @@ static void pvr_remove(struct platform_device *plat_dev)
 	struct drm_device *drm_dev = platform_get_drvdata(plat_dev);
 	struct pvr_device *pvr_dev = to_pvr_device(drm_dev);
 
+	/*
+	 * Unplug before freeing anything, so no ioctl is still inside
+	 * drm_dev_enter(). pvr_device_lost() may already have done it, and
+	 * drm_dev_unplug() is not idempotent.
+	 */
+	if (!pvr_dev->lost)
+		drm_dev_unplug(drm_dev);
+
 	WARN_ON(!xa_empty(&pvr_dev->job_ids));
 	WARN_ON(!xa_empty(&pvr_dev->free_list_ids));
 
+	pm_runtime_suspend(drm_dev->dev);
+
 	xa_destroy(&pvr_dev->job_ids);
 	xa_destroy(&pvr_dev->free_list_ids);
 
-	pm_runtime_suspend(drm_dev->dev);
 	pvr_device_fini(pvr_dev);
-	drm_dev_unplug(drm_dev);
 	pvr_watchdog_fini(pvr_dev);
 	pvr_queue_device_fini(pvr_dev);
 	pvr_context_device_fini(pvr_dev);

-- 
2.55.0

[PATCH v3 3/4] drm/imagination: stop the watchdog before tearing the device down

[Ryan Brue]

pvr_remove() runs pvr_watchdog_fini() after pvr_device_fini(), so the
watchdog work can still be queued while pvr_fw_fini() unmaps the
fwif_osdata it reads. The pm_runtime_suspend() earlier in remove
normally cancels the work on its way through pvr_power_fw_disable(), but
it returns -EAGAIN without calling the callback when the usage count is
raised, and -EINVAL when the device is in runtime_error, and then
nothing has cancelled the watchdog since the last pvr_power_fw_enable():

  echo on > /sys/devices/platform/soc/13000000.gpu/power/control
  echo 13000000.gpu > /sys/bus/platform/drivers/powervr/unbind

The worker fires within 500ms of the free and reads freed memory.

Cancel it first, before the unplug: the worker is one of the callers of
pvr_device_lost(), so waiting for it here also means the pvr_dev->lost
test that guards the unplug cannot race it.

Fixes: 727538a4bbff ("drm/imagination: Implement power management")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Ryan Brue <ryanbrue.dev@gmail.com>
---
 drivers/gpu/drm/imagination/pvr_drv.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
index fc92a82a7208..20b27a468327 100644
--- a/drivers/gpu/drm/imagination/pvr_drv.c
+++ b/drivers/gpu/drm/imagination/pvr_drv.c
@@ -1469,6 +1469,9 @@ static void pvr_remove(struct platform_device *plat_dev)
 	struct drm_device *drm_dev = platform_get_drvdata(plat_dev);
 	struct pvr_device *pvr_dev = to_pvr_device(drm_dev);
 
+	/* Stop the watchdog before anything it reads is freed. */
+	pvr_watchdog_fini(pvr_dev);
+
 	/*
 	 * Unplug before freeing anything, so no ioctl is still inside
 	 * drm_dev_enter(). pvr_device_lost() may already have done it, and
@@ -1486,7 +1489,6 @@ static void pvr_remove(struct platform_device *plat_dev)
 	xa_destroy(&pvr_dev->free_list_ids);
 
 	pvr_device_fini(pvr_dev);
-	pvr_watchdog_fini(pvr_dev);
 	pvr_queue_device_fini(pvr_dev);
 	pvr_context_device_fini(pvr_dev);
 	pvr_power_domains_fini(pvr_dev);

-- 
2.55.0

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

[Ryan Brue]

pvr_pm_ops has only RUNTIME_PM_OPS, so the GPU gets no callback for
system suspend. That works only while it happens to be runtime-suspended
when the system goes down. If it is runtime-active, genpd counts it as
suspended anyway and turns the power domain off. After resume the driver
still believes its firmware is running, the first firmware operation
times out, and pvr_power_reset() is entered from inside an ioctl's
drm_dev_enter() section, where its drm_dev_unplug() waits for that very
section: the caller is left in an unkillable D state and the GPU is dead
until reboot. On mt8173 it is deterministic:

  echo on > /sys/devices/platform/soc/13000000.gpu/power/control
  echo mem > /sys/power/state          # wakes on an RTC alarm
  vulkaninfo --summary                 # hangs in D state, every time

With the default 50ms autosuspend delay the same bug is intermittent in
ordinary use, depending only on whether the GPU was idle at the instant
of suspend.

pvr_power_device_suspend() already disables the firmware and powers the
GPU off and pvr_power_device_resume() is its inverse, so drive both for
system sleep with the force helpers. The reset-path deadlock is a
separate bug; this only stops the system-sleep path from reaching it.

Fixes: 727538a4bbff ("drm/imagination: Implement power management")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Ryan Brue <ryanbrue.dev@gmail.com>
---
 drivers/gpu/drm/imagination/pvr_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
index 20b27a468327..b7931f75832f 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)
 };
 
 static struct platform_driver pvr_driver = {

-- 
2.55.0

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

[sashiko-bot]

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

Re: [PATCH v3 2/4] drm/imagination: unplug the device before tearing it down in pvr_remove()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] TOCTOU race on `pvr_dev->lost` in `pvr_remove()` allows a double `drm_dev_unplug()` execution, causing a kernel oops.

Pre-existing issues:
- [Critical] Deadlock caused by calling `drm_dev_unplug()` from within a `drm_dev_enter()` SRCU read critical section via `pvr_device_lost()`.
- [Critical] Incorrect teardown ordering in `pvr_remove()` destroys firmware memory before stopping asynchronous workers (DRM schedulers), leading to Use-After-Free.
- [High] Missing `drm_dev_enter()` guards in teardown ioctls invalidate the commit's core synchronization assumption.
--

commit 3adf692930871afc287c13c7680a8b09a7c0ad9e
Author: Ryan Brue <ryanbrue.dev@gmail.com>

drm/imagination: unplug the device before tearing it down in pvr_remove()

This commit reorders the teardown sequence in pvr_remove() to call
drm_dev_unplug() before destroying the job and free list xarrays, powering
down the GPU, and running pvr_device_fini(). It skips the unplug if
pvr_device_lost() has already performed it.

Fixes: 1f88f017e649 ("drm/imagination: Get GPU resources")
Fixes: 727538a4bbff ("drm/imagination: Implement power management")
Cc: stable@vger.kernel.org

> diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
> index 5c965ef0274ff..fc92a82a72084 100644
> --- a/drivers/gpu/drm/imagination/pvr_drv.c
> +++ b/drivers/gpu/drm/imagination/pvr_drv.c
> @@ -1469,15 +1469,23 @@ static void pvr_remove(struct platform_device *plat_dev)
>  	struct drm_device *drm_dev = platform_get_drvdata(plat_dev);
>  	struct pvr_device *pvr_dev = to_pvr_device(drm_dev);
>  
> +	/*
> +	 * Unplug before freeing anything, so no ioctl is still inside
> +	 * drm_dev_enter(). pvr_device_lost() may already have done it, and

[Severity: Critical]
This is a pre-existing issue, but does calling drm_dev_unplug() from
pvr_device_lost() introduce a potential self-deadlock when triggered from
an MMU flush?

Looking at pvr_mmu_flush_exec(), it acquires the SRCU read lock via
drm_dev_enter(). If the flush times out, it attempts recovery and calls
pvr_device_lost(), which subsequently triggers drm_dev_unplug() and its
internal synchronize_srcu():

drivers/gpu/drm/imagination/pvr_mmu.c:pvr_mmu_flush_exec() {
        if (!drm_dev_enter(from_pvr_device(pvr_dev), &idx))
                return -EIO;
        ...
        /* On timeout calls pvr_power_reset() or pvr_device_lost() */
}

Since synchronize_srcu() waits indefinitely for all SRCU readers to exit, and
the calling thread itself holds the read lock, won't it hang forever waiting
for its own completion?

> +	 * drm_dev_unplug() is not idempotent.
> +	 */
> +	if (!pvr_dev->lost)
> +		drm_dev_unplug(drm_dev);

[Severity: High]
Is this vulnerable to a time-of-check to time-of-use race with
pvr_power_reset() and pvr_device_lost()?

If pvr_remove() runs on the unbind path and evaluates !pvr_dev->lost as true,
could it be preempted before calling drm_dev_unplug()?

Concurrently, if an asynchronous context like a DRM scheduler timeout triggers
pvr_power_reset(), it would call pvr_device_lost(). Because the flag isn't set
yet, pvr_device_lost() would also evaluate !pvr_dev->lost as true, set the
flag, and execute drm_dev_unplug():

drivers/gpu/drm/imagination/pvr_power.c:pvr_device_lost() {
        if (!pvr_dev->lost) {
                pvr_dev->lost = true;
                drm_dev_unplug(from_pvr_device(pvr_dev));
        }
}

When pvr_remove() resumes, wouldn't it execute drm_dev_unplug() a second time,
causing a kernel oops inside drm_client_sysrq_unregister() since
drm_dev_unplug() is not idempotent?

[Severity: High]
This is a pre-existing issue, but does this unplug actually synchronize with
all ioctls as intended?

While this patch relies on drm_dev_unplug() to wait for in-flight ioctls via
drm_dev_enter() and drm_dev_exit(), some teardown ioctls appear to lack these
guards entirely.

For example, in pvr_ioctl_destroy_context():

drivers/gpu/drm/imagination/pvr_drv.c:pvr_ioctl_destroy_context() {
        ...
        return pvr_context_destroy(pvr_file, args->handle);
}

And in pvr_ioctl_vm_unmap():

drivers/gpu/drm/imagination/pvr_drv.c:pvr_ioctl_vm_unmap() {
        ...
        err = pvr_vm_unmap(vm_ctx, args->device_addr, args->size);
        ...
}

Since these ioctls bypass the drm_dev_enter() check, couldn't userspace
execute them concurrently during or after the unplug, interacting with
hardware queues via pvr_queue_cleanup_fw_context() after pvr_remove() has
already freed them?

[ ... ]

>  	xa_destroy(&pvr_dev->job_ids);
>  	xa_destroy(&pvr_dev->free_list_ids);
>  
> -	pm_runtime_suspend(drm_dev->dev);
>  	pvr_device_fini(pvr_dev);
> -	drm_dev_unplug(drm_dev);
>  	pvr_watchdog_fini(pvr_dev);
>  	pvr_queue_device_fini(pvr_dev);

[Severity: Critical]
This is a pre-existing issue, but does the ordering here tear down firmware
memory before stopping asynchronous workers?

When pvr_device_fini() is called, it tears down hardware components and frees
firmware memory such as pvr_dev->fw_dev. However, the DRM scheduler threads
executing on pvr_dev->sched_wq are not stopped until pvr_queue_device_fini()
is called later.

Couldn't drm_sched concurrently execute pvr_queue_run_job() while or after the
firmware memory is freed, leading to a use-after-free when it accesses that
memory to submit KCCB commands?

Note that while a subsequent patch in the series moves pvr_watchdog_fini(),
the DRM scheduler teardown issue appears to remain unresolved here.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260913-rbrue-suez-upstreaming-pvr-suspend-gpu-system-sleep-v3-0-674b907d0652@gmail.com?part=2

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

[sashiko-bot]

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

Re: [PATCH v3 3/4] drm/imagination: stop the watchdog before tearing the device down

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Moving pvr_watchdog_fini() before drm_dev_unplug() creates a race where concurrent IOCTLs can trigger a PM resume and re-queue the watchdog work after it has been cancelled.

Pre-existing issues:
- [Critical] pvr_remove() ignores the return value of pm_runtime_suspend() and frees firmware memory while the GPU hardware may still be running.
- [Critical] Inverted teardown order in pvr_remove() destroys the scheduler workqueue after firmware structures are freed, causing pending jobs to execute against freed memory.
- [High] Use-after-free of pvr_dev and sched_wq when a dma_fence outlives the DRM device.
- [High] Concurrent calls to drm_dev_unplug() due to TOCTOU race on the non-atomic pvr_dev->lost flag.
--

commit 34ac43451671ac25507b846dc7e38412b5318f2e
Author: Ryan Brue <ryanbrue.dev@gmail.com>
drm/imagination: stop the watchdog before tearing the device down

This commit moves the watchdog finalization to the beginning of device
removal, aiming to prevent the worker from reading freed memory if it
fires during teardown.

Cc: stable@vger.kernel.org

> diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
> index fc92a82a72084..20b27a468327b 100644
> --- a/drivers/gpu/drm/imagination/pvr_drv.c
> +++ b/drivers/gpu/drm/imagination/pvr_drv.c
> @@ -1469,6 +1469,9 @@ static void pvr_remove(struct platform_device *plat_dev)
>  	struct drm_device *drm_dev = platform_get_drvdata(plat_dev);
>  	struct pvr_device *pvr_dev = to_pvr_device(drm_dev);
>  
> +	/* Stop the watchdog before anything it reads is freed. */
> +	pvr_watchdog_fini(pvr_dev);
> +

[Severity: High]
Can a concurrent ioctl re-arm the watchdog right after it is cancelled?

Since drm_dev_unplug() has not blocked ioctls yet, an incoming job
submission could call pm_runtime_resume_and_get(), triggering
pvr_power_fw_enable().

Looking at drivers/gpu/drm/imagination/pvr_power.c:pvr_power_fw_enable():

    queue_delayed_work(pvr_dev->sched_wq, &pvr_dev->watchdog.work,
                       msecs_to_jiffies(WATCHDOG_TIME_MS));

If this occurs, the work is re-queued. Later in pvr_remove(), if
pm_runtime_suspend() fails (returning -EAGAIN) due to the elevated usage
count, the watchdog is never cancelled again and could fire after
pvr_device_fini() frees the firmware memory.

>  	/*
>  	 * Unplug before freeing anything, so no ioctl is still inside
>  	 * drm_dev_enter(). pvr_device_lost() may already have done it, and
>  	 * drm_dev_unplug() is not idempotent.
>  	 */
>  	if (!pvr_dev->lost)
>  		drm_dev_unplug(drm_dev);

[Severity: High]
This is a pre-existing issue, but does checking this non-atomic flag
without locking create a race condition with pvr_device_lost()?

If a hardware timeout (e.g., from an MMU cache flush via
pvr_mmu_flush_exec) concurrently triggers pvr_device_lost() in pvr_power.c,
both threads might check !pvr_dev->lost at the exact same time, evaluate it
as true, and both call drm_dev_unplug().

Since drm_dev_unplug() is not idempotent, couldn't this result in a double
unregister kernel panic?

>  
>  	WARN_ON(!xa_empty(&pvr_dev->job_ids));
>  	WARN_ON(!xa_empty(&pvr_dev->free_list_ids));
>  
>  	pm_runtime_suspend(drm_dev->dev);

[Severity: Critical]
This is a pre-existing issue, but what happens if the runtime PM usage
count is elevated (e.g., due to an active job or a prior sysfs write to
power/control)?

If the usage count is elevated, pm_runtime_suspend() will return -EAGAIN
and skip the suspend callback. The hardware will remain physically powered
on and the firmware will continue to run.

Because pvr_remove() ignores this return value, it will proceed to unmap
MMIO and free firmware OS data unconditionally in pvr_device_fini(), which
might lead to IOMMU faults or memory corruption when the actively running
firmware accesses those freed structures.

>  
>  	xa_destroy(&pvr_dev->job_ids);
>  	xa_destroy(&pvr_dev->free_list_ids);
>  
>  	pvr_device_fini(pvr_dev);
> -	pvr_watchdog_fini(pvr_dev);
>  	pvr_queue_device_fini(pvr_dev);

[Severity: Critical]
This is a pre-existing issue, but does this teardown order cause pending
jobs to execute against freed memory?

pvr_device_fini() unmaps and frees firmware memory. Right after it,
pvr_queue_device_fini() destroys the scheduler workqueue:

drivers/gpu/drm/imagination/pvr_queue.c:pvr_queue_device_fini() {
    destroy_workqueue(pvr_dev->sched_wq);
}

Calling destroy_workqueue() flushes and drains the workqueue, which would
force pending DRM scheduler jobs to execute synchronously via
pvr_queue_run_job().

Since the firmware memory was already freed by pvr_device_fini(), won't
these executed jobs access freed memory?

[Severity: High]
This is a pre-existing issue, but is it possible for a dma_fence to outlive
the DRM device if it is exported to userspace as a sync_file FD?

If a sync_file FD outlives device unbind, closing it later triggers the
fence release callback. In drivers/gpu/drm/imagination/pvr_queue.c:

pvr_queue_fence_release() {
    queue_work(pvr_dev->sched_wq, &fence->release_work);
}

Since pvr_remove() destroys pvr_dev->sched_wq, calling queue_work() on the
destroyed workqueue (and accessing potentially freed pvr_dev) would cause a
crash.

Does pvr_queue_fence_init() need to take a reference to the drm_device
instead of just the pvr_context?

>  	pvr_context_device_fini(pvr_dev);
>  	pvr_power_domains_fini(pvr_dev);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260913-rbrue-suez-upstreaming-pvr-suspend-gpu-system-sleep-v3-0-674b907d0652@gmail.com?part=3