[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 1/4] drm/imagination: don't gate the runtime PM callbacks on drm_dev_enter()

[Gyeyoung Baek]

Hi Ryan,

>  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;
>

'lost' looks protected by reset_sem, but it's read here without the
lock. Shouldn't this at least be READ_ONCE()?

>         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
>

-- 
Thanks,
Gyeyoung

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

[Ryan Brue]

Hi Gyeyoung,

On 10/12/26 10:43 PM, Gyeyoung Baek wrote:
> 'lost' looks protected by reset_sem, but it's read here without the
> lock. Shouldn't this at least be READ_ONCE()?
Thanks for looking, you're right. Lost isn't fully covered by reset_sem 
anyway, as pvr_mmu_flush_exec() marks the device lost without it, and 
the watchdog worker already reads it unlocked. I'll make it a 
READ_ONCE(), with a matching WRITE_ONCE() in pvr_device_lost() in the 
next revision.

Thanks again,
Ryan Brue