[PATCH 0/4] drm/msm: fix GPU recovery deadlocks, add a8xx crash dumps

[PATCH 0/4] drm/msm: fix GPU recovery deadlocks, add a8xx crash dumps

[Nico Angelo Dela Cruz]

On an Adreno 830 (SM8750, OnePlus Pad 3), GMU wedges sometimes left the
screen frozen until a reboot. The wedge itself is a separate, still open
issue; this series fixes the recovery side, which should clear a wedge
but could deadlock instead, and adds the crash dumps a8xx was missing.

Patch 1: if a CP page fault arrives while recover_worker() holds
gpu->lock, the fault handler reinitializes fault_coredump_done and then
waits for gpu->lock, while recovery's GMU waits (HFI and OOB) wait on
that completion forever. Recovery now skips the wait, and the fault
handler skips the capture, while recovery is running. On the A830,
3 of 3 recoveries that took such a fault hung before this; with it,
3 natural ones all completed.

Patch 2: when devcoredump is disabled or can't allocate,
dev_coredumpm() frees the dump synchronously, and msm's free callback
takes gpu->lock, which the capture path already holds. The put now runs
from a work item.

Patch 3: a8xx had no gpu_state_get, so it never produced a
devcoredump. It now takes a memory-only one (rings, GMU log/HFI/debug,
HFI history), without reading any register, since the GMU may be wedged
and GX off. The GMU part is a pure move shared with a6xx/a7xx.

Patch 4: adreno_show_object() ascii85-encodes buffers in place on first
show, without a lock, so two readers of one devcoredump can double-free
a buffer (free_large_kmalloc "Not a kmalloc allocation" warnings).
Rendering is now serialized.

Patches 1, 2 and 4 carry Fixes: tags and Cc: stable.

Testing, all on the A830 with the series on a 7.3-rc5 based tree. It
is based on msm-next, where drivers/gpu/drm/msm builds W=1-clean:
 - IGT msm_recovery gpu-fault: 6/6 recover, each with a complete
   devcoredump; with /sys/class/devcoredump/disabled set, it recovers
   instead of hanging.
 - Natural GMU wedges during Bluetooth audio playback: 8 in about
   47 minutes, 3 with a CP page fault during recovery, all recovered in
   about 0.15 s, no hung tasks, dumps saved.
 - Two concurrent devcoredump readers: 16 kvfree warnings in one boot
   before patch 4, none after.

a6xx/a7xx: build and pure-move review only for patch 3, I have no such
hardware. Tested-by welcome.

Not addressed here: msm never calls dev_coredump_put() on unbind, so a
devcoredump still registered then can call the free callback after the
GPU is gone. That predates this series.

The fault_coredump_done wait was introduced in the review at:
Link: https://lore.kernel.org/all/CACu1E7EApr6Ne3XxFv4cUa2JuwNW2kfo1c5wSsAUN78iWyQ2sg@mail.gmail.com/

Nico Angelo Dela Cruz (4):
  drm/msm: don't wait for a fault devcoredump during GPU recovery
  drm/msm: free the crash state asynchronously
  drm/msm/a8xx: add a memory-only GPU crash state
  drm/msm: serialize devcoredump reads of the crash state

 drivers/gpu/drm/msm/adreno/a6xx_gmu.c       |  7 ++-
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c       |  7 +++
 drivers/gpu/drm/msm/adreno/a6xx_gpu.h       |  1 +
 drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 47 ++++++++++++++++-----
 drivers/gpu/drm/msm/adreno/a6xx_hfi.c       |  7 ++-
 drivers/gpu/drm/msm/adreno/adreno_gpu.c     |  3 +-
 drivers/gpu/drm/msm/msm_gpu.c               | 33 ++++++++++++++-
 drivers/gpu/drm/msm/msm_gpu.h               | 23 ++++++++++
 8 files changed, 112 insertions(+), 16 deletions(-)


base-commit: d024361c631b3f020a26c07d683e5103404b0fbf
-- 
2.53.0

[PATCH 1/4] drm/msm: don't wait for a fault devcoredump during GPU recovery

[Nico Angelo Dela Cruz]

adreno_fault_handler() calls reinit_completion() on
fault_coredump_done and then blocks in
msm_gpu_fault_crashstate_capture() on mutex_lock(&gpu->lock) to take
the devcoredump.

If recover_worker() is already running, it holds gpu->lock and, from
the runtime suspend in a6xx_recover() or a8xx_recover(), may time out
in a6xx_hfi_wait_for_msg_interrupt() or a6xx_gmu_set_oob(). Both loops
then fall into an unbounded wait_for_completion() on
fault_coredump_done, which has just been reinitialized and which only
the fault handler can complete. The fault handler is waiting for
gpu->lock, which recovery will not release until the wait returns, so
neither side makes progress.

A fault dump that needs gpu->lock can never complete while recovery
holds it, so waiting for it is wrong. Add a recovery_in_progress flag
to struct msm_gpu, set by recover_worker() under gpu->lock, and check
it in two places:

 - the fault handler doesn't start a devcoredump capture while
   recovery is running, so a fault that arrives during recovery no
   longer reinitializes the completion;

 - both GMU wait loops stop waiting for the completion while recovery
   is running and fall through to their normal timeout handling. This
   covers a fault handler that had already reinitialized the
   completion when recovery took gpu->lock.

GMU waiters that don't hold gpu->lock, such as the devfreq frequency
change path, also skip the completion wait while recovery is running
and report their timeout right away; recovery is resetting the GMU
at that point anyway.

On a OnePlus Pad 3 (Adreno 830), recoveries that saw a CP page fault
within 2 s of "hangcheck recover!" were stuck 3 of 3 times, while
12 of 12 recoveries without such a fault completed. With this patch,
three natural recoveries that took a CP page fault during recovery
all completed.

Fixes: 50a0b122cfc8 ("drm/msm: Wait for MMU devcoredump when waiting for GMU")
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/all/CACu1E7EApr6Ne3XxFv4cUa2JuwNW2kfo1c5wSsAUN78iWyQ2sg@mail.gmail.com/
Signed-off-by: Nico Angelo Dela Cruz <nicoangelodelacruz@gmail.com>
---
 drivers/gpu/drm/msm/adreno/a6xx_gmu.c   | 7 +++++--
 drivers/gpu/drm/msm/adreno/a6xx_hfi.c   | 7 +++++--
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 3 ++-
 drivers/gpu/drm/msm/msm_gpu.c           | 2 ++
 drivers/gpu/drm/msm/msm_gpu.h           | 9 +++++++++
 5 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 662f09bec..32ed49abe 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -445,12 +445,15 @@ int a6xx_gmu_set_oob(struct a6xx_gmu *gmu, enum a6xx_gmu_oob_state state)
 		if (!ret)
 			break;
 
-		if (completion_done(&a6xx_gpu->base.fault_coredump_done))
+		if (completion_done(&a6xx_gpu->base.fault_coredump_done) ||
+		    READ_ONCE(a6xx_gpu->base.base.recovery_in_progress))
 			break;
 
 		/* We may timeout because the GMU is temporarily wedged from
 		 * pending faults from the GPU and we are taking a devcoredump.
-		 * Wait until the MMU is resumed and try again.
+		 * Wait until the MMU is resumed and try again. A fault dump
+		 * cannot complete while recovery holds gpu->lock, so don't
+		 * wait for one then.
 		 */
 		wait_for_completion(&a6xx_gpu->base.fault_coredump_done);
 	} while (true);
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_hfi.c b/drivers/gpu/drm/msm/adreno/a6xx_hfi.c
index 6b34e704a..fe4e35728 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_hfi.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_hfi.c
@@ -122,12 +122,15 @@ static int a6xx_hfi_wait_for_msg_interrupt(struct a6xx_gmu *gmu, u32 id, u32 seq
 		if (!ret)
 			break;
 
-		if (completion_done(&a6xx_gpu->base.fault_coredump_done))
+		if (completion_done(&a6xx_gpu->base.fault_coredump_done) ||
+		    READ_ONCE(a6xx_gpu->base.base.recovery_in_progress))
 			break;
 
 		/* We may timeout because the GMU is temporarily wedged from
 		 * pending faults from the GPU and we are taking a devcoredump.
-		 * Wait until the MMU is resumed and try again.
+		 * Wait until the MMU is resumed and try again. A fault dump
+		 * cannot complete while recovery holds gpu->lock, so don't
+		 * wait for one then.
 		 */
 		wait_for_completion(&a6xx_gpu->base.fault_coredump_done);
 	} while (true);
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index dadfe4c69..138bebf5e 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -289,7 +289,8 @@ int adreno_fault_handler(struct msm_gpu *gpu, unsigned long iova, int flags,
 	struct msm_mmu *mmu = to_msm_vm(gpu->vm)->mmu;
 	const char *type = "UNKNOWN";
 	bool do_devcoredump = info && (info->fsr & ARM_SMMU_FSR_SS) &&
-		!READ_ONCE(gpu->crashstate);
+		!READ_ONCE(gpu->crashstate) &&
+		!READ_ONCE(gpu->recovery_in_progress);
 	unsigned long irq_flags;
 
 	/*
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 0c2c35636..caa86279f 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -474,6 +474,7 @@ static void recover_worker(struct kthread_work *work)
 	int i;
 
 	mutex_lock(&gpu->lock);
+	WRITE_ONCE(gpu->recovery_in_progress, true);
 
 	DRM_DEV_ERROR(dev->dev, "%s: hangcheck recover!\n", gpu->name);
 
@@ -585,6 +586,7 @@ static void recover_worker(struct kthread_work *work)
 	pm_runtime_put(&gpu->pdev->dev);
 
 out_unlock:
+	WRITE_ONCE(gpu->recovery_in_progress, false);
 	mutex_unlock(&gpu->lock);
 
 	msm_gpu_retire(gpu);
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index d27d54bdb..5cf220f75 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -258,6 +258,15 @@ struct msm_gpu {
 
 	struct msm_gpu_state *crashstate;
 
+	/**
+	 * @recovery_in_progress: true while recover_worker() holds @lock, so
+	 * that fault capture and GMU waits don't wait on a fault devcoredump
+	 * that can't complete while recovery holds @lock.  Written with
+	 * WRITE_ONCE() under @lock; readers (the fault handler, GMU wait
+	 * loops) don't hold @lock and use READ_ONCE().
+	 */
+	bool recovery_in_progress;
+
 	/* True if the hardware supports expanded apriv (a650 and newer) */
 	bool hw_apriv;
 
-- 
2.53.0

[PATCH 2/4] drm/msm: free the crash state asynchronously

[Nico Angelo Dela Cruz]

msm_gpu_crashstate_capture() sets gpu->crashstate and calls
dev_coredumpm() while the caller (recover_worker() or
msm_gpu_fault_crashstate_capture()) holds gpu->lock. When devcoredump
is disabled (/sys/class/devcoredump/disabled, a write-once knob) or
its allocation fails, dev_coredumpm_timeout() calls the free callback
synchronously.

msm_gpu_devcoredump_free() then calls msm_gpu_crashstate_put(), which
does mutex_lock(&gpu->lock) and so recursively takes a mutex the
caller already holds. On systems with devcoredump disabled, every GPU
recovery that captures a crash state deadlocks.

Defer the put to a work item on system_dfl_wq instead. This is safe
because msm_gpu_devcoredump_read() takes and drops its own crash
state reference, and the work takes gpu->lock normally once the
holder has released it. gpu->crashstate stays set until the work
runs, which only means a capture in that window is skipped.
msm_gpu_cleanup() flushes an already queued free before teardown.

Tested on an Adreno 830 with /sys/class/devcoredump/disabled set to 1:
IGT msm_recovery gpu-fault recovers and the GPU resumes.

Fixes: c0fec7f562ec ("drm/msm/gpu: Capture the GPU state on a GPU hang")
Cc: stable@vger.kernel.org
Signed-off-by: Nico Angelo Dela Cruz <nicoangelodelacruz@gmail.com>
---
 drivers/gpu/drm/msm/msm_gpu.c | 21 ++++++++++++++++++++-
 drivers/gpu/drm/msm/msm_gpu.h |  6 ++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index caa86279f..fded78fdb 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -175,6 +175,14 @@ int msm_gpu_hw_init(struct msm_gpu *gpu)
 	return ret;
 }
 
+static void msm_gpu_crashstate_free_worker(struct work_struct *work)
+{
+	struct msm_gpu *gpu = container_of(work, struct msm_gpu,
+					   crashstate_free_work);
+
+	msm_gpu_crashstate_put(gpu);
+}
+
 #ifdef CONFIG_DEV_COREDUMP
 static ssize_t msm_gpu_devcoredump_read(char *buffer, loff_t offset,
 		size_t count, void *data, size_t datalen)
@@ -215,7 +223,13 @@ static void msm_gpu_devcoredump_free(void *data)
 {
 	struct msm_gpu *gpu = data;
 
-	msm_gpu_crashstate_put(gpu);
+	/*
+	 * dev_coredumpm() calls this synchronously when it can't keep the
+	 * dump (devcoredump disabled, or out of memory), while
+	 * msm_gpu_crashstate_capture() holds gpu->lock, which
+	 * msm_gpu_crashstate_put() also takes.  Defer the put to a worker.
+	 */
+	queue_work(system_dfl_wq, &gpu->crashstate_free_work);
 }
 
 static void msm_gpu_crashstate_get_bo(struct msm_gpu_state *state,
@@ -899,6 +913,7 @@ int msm_gpu_init(struct drm_device *drm, struct platform_device *pdev,
 	gpu->dev = drm;
 	gpu->funcs = funcs;
 	gpu->name = name;
+	INIT_WORK(&gpu->crashstate_free_work, msm_gpu_crashstate_free_worker);
 
 	gpu->worker = kthread_run_worker(0, "gpu-worker");
 	if (IS_ERR(gpu->worker)) {
@@ -1047,6 +1062,10 @@ void msm_gpu_cleanup(struct msm_gpu *gpu)
 
 	DBG("%s", gpu->name);
 
+	/* May be unset if probe failed before msm_gpu_init() */
+	if (gpu->crashstate_free_work.func)
+		flush_work(&gpu->crashstate_free_work);
+
 	for (i = 0; i < ARRAY_SIZE(gpu->rb); i++) {
 		msm_ringbuffer_destroy(gpu->rb[i]);
 		gpu->rb[i] = NULL;
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index 5cf220f75..db9e11e7f 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -258,6 +258,12 @@ struct msm_gpu {
 
 	struct msm_gpu_state *crashstate;
 
+	/**
+	 * @crashstate_free_work: drops the devcoredump's crashstate reference
+	 * outside of @lock, see msm_gpu_devcoredump_free().
+	 */
+	struct work_struct crashstate_free_work;
+
 	/**
 	 * @recovery_in_progress: true while recover_worker() holds @lock, so
 	 * that fault capture and GMU waits don't wait on a fault devcoredump
-- 
2.53.0

[PATCH 3/4] drm/msm/a8xx: add a memory-only GPU crash state

[Nico Angelo Dela Cruz]

a8xx had no gpu_state_get callback, so no devcoredump was ever
produced when the GPU hung or faulted.

Add a8xx_gpu_state_get(), a memory-only snapshot: the generic Adreno
state from adreno_gpu_state_get() (ring buffer contents and fences,
with rptr read from the hw_apriv shadow rather than a register), the
GMU log/HFI/debug buffers and the HFI queue history. No register is
read, because the GMU may be wedged and GX powered off when this runs
from the recover worker or the SMMU fault path. As a result a8xx
dumps always report "gpu-initialized: 0", as a6xx/a7xx dumps do when
GX is off.

The shared part moves out of a6xx_gpu_state_get() into
a6xx_gpu_state_get_gmu(), which also runs adreno_gpu_state_get().
It is a pure move except for a new gmu_regs parameter that gates
a6xx_get_gmu_registers(); a6xx/a7xx pass true, so their output is
unchanged. Reviewed with git diff --color-moved=dimmed-zebra: apart
from the moved lines, the new code is the helper's signature, the
"if (gmu_regs)" check, the locals the helper needs, and the call.

Tested on an Adreno 830: IGT msm_recovery gpu-fault (6 of 6 runs) and
natural GPU hangs produce dumps with the ringbuffer, gmu-log, gmu-hfi
and queue-history sections; capture takes 1.1 to 4.0 ms.

a6xx/a7xx: build and pure-move review only, Tested-by welcome.

Signed-off-by: Nico Angelo Dela Cruz <nicoangelodelacruz@gmail.com>
---
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c       |  7 +++
 drivers/gpu/drm/msm/adreno/a6xx_gpu.h       |  1 +
 drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 47 ++++++++++++++++-----
 3 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 92e8c8abb..0f4b0af0a 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -2918,9 +2918,16 @@ const struct adreno_gpu_funcs a8xx_gpu_funcs = {
 		.active_ring = a6xx_active_ring,
 		.irq = a8xx_irq,
 		.destroy = a6xx_destroy,
+#if defined(CONFIG_DRM_MSM_GPU_STATE)
+		.show = a6xx_show,
+#endif
 		.gpu_busy = a8xx_gpu_busy,
 		.gpu_get_freq = a6xx_gmu_get_freq,
 		.gpu_set_freq = a6xx_gpu_set_freq,
+#if defined(CONFIG_DRM_MSM_GPU_STATE)
+		.gpu_state_get = a8xx_gpu_state_get,
+		.gpu_state_put = a6xx_gpu_state_put,
+#endif
 		.create_vm = a6xx_create_vm,
 		.create_private_vm = a6xx_create_private_vm,
 		.get_rptr = a6xx_get_rptr,
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
index 79912a33e..03432abf9 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
@@ -338,6 +338,7 @@ void a6xx_show(struct msm_gpu *gpu, struct msm_gpu_state *state,
 		struct drm_printer *p);
 
 struct msm_gpu_state *a6xx_gpu_state_get(struct msm_gpu *gpu);
+struct msm_gpu_state *a8xx_gpu_state_get(struct msm_gpu *gpu);
 int a6xx_gpu_state_put(struct msm_gpu_state *state);
 
 void a6xx_bus_clear_pending_transactions(struct adreno_gpu *adreno_gpu, bool gx_off);
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
index 26c17861c..a3751358a 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
@@ -1629,25 +1629,19 @@ static void a7xx_get_indexed_registers(struct msm_gpu *gpu,
 	return;
 }
 
-struct msm_gpu_state *a6xx_gpu_state_get(struct msm_gpu *gpu)
+static void a6xx_gpu_state_get_gmu(struct msm_gpu *gpu,
+				   struct a6xx_gpu_state *a6xx_state, bool gmu_regs)
 {
-	struct a6xx_crashdumper _dumper = { 0 }, *dumper = NULL;
 	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
 	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
-	struct a6xx_gpu_state *a6xx_state = kzalloc_obj(*a6xx_state);
-	bool stalled;
-
-	if (!a6xx_state)
-		return ERR_PTR(-ENOMEM);
-
-	INIT_LIST_HEAD(&a6xx_state->objs);
 
 	/* Get the generic state from the adreno core */
 	adreno_gpu_state_get(gpu, &a6xx_state->base);
 
 	if (!adreno_has_gmu_wrapper(adreno_gpu) &&
 	    !adreno_has_rgmu(adreno_gpu)) {
-		a6xx_get_gmu_registers(gpu, a6xx_state);
+		if (gmu_regs)
+			a6xx_get_gmu_registers(gpu, a6xx_state);
 
 		a6xx_state->gmu_log = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.log);
 		a6xx_state->gmu_hfi = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.hfi);
@@ -1655,6 +1649,21 @@ struct msm_gpu_state *a6xx_gpu_state_get(struct msm_gpu *gpu)
 
 		a6xx_snapshot_gmu_hfi_history(gpu, a6xx_state);
 	}
+}
+
+struct msm_gpu_state *a6xx_gpu_state_get(struct msm_gpu *gpu)
+{
+	struct a6xx_crashdumper _dumper = { 0 }, *dumper = NULL;
+	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
+	struct a6xx_gpu_state *a6xx_state = kzalloc_obj(*a6xx_state);
+	bool stalled;
+
+	if (!a6xx_state)
+		return ERR_PTR(-ENOMEM);
+
+	INIT_LIST_HEAD(&a6xx_state->objs);
+
+	a6xx_gpu_state_get_gmu(gpu, a6xx_state, true);
 
 	/* If GX isn't on the rest of the data isn't going to be accessible */
 	if (!adreno_gpu->funcs->gx_is_on(adreno_gpu))
@@ -1714,6 +1723,24 @@ struct msm_gpu_state *a6xx_gpu_state_get(struct msm_gpu *gpu)
 	return  &a6xx_state->base;
 }
 
+/*
+ * Memory-only snapshot for a8xx: no register is read, as the GMU may be
+ * wedged and GX powered off by the time this runs.
+ */
+struct msm_gpu_state *a8xx_gpu_state_get(struct msm_gpu *gpu)
+{
+	struct a6xx_gpu_state *a6xx_state = kzalloc_obj(*a6xx_state);
+
+	if (!a6xx_state)
+		return ERR_PTR(-ENOMEM);
+
+	INIT_LIST_HEAD(&a6xx_state->objs);
+
+	a6xx_gpu_state_get_gmu(gpu, a6xx_state, false);
+
+	return &a6xx_state->base;
+}
+
 static void a6xx_gpu_state_destroy(struct kref *kref)
 {
 	struct a6xx_state_memobj *obj, *tmp;
-- 
2.53.0

[PATCH 4/4] drm/msm: serialize devcoredump reads of the crash state

[Nico Angelo Dela Cruz]

adreno_show_object() converts each captured buffer to ascii85 the
first time the crash state is shown, in place: it checks *encoded,
replaces *ptr with the encoded copy and kvfree()s the raw buffer.
msm_gpu_devcoredump_read() only takes gpu->lock around the reference
get, and every read() of the devcoredump renders the whole state
again, so two processes reading the same devcoredump at once can both
see *encoded == false and both free the same buffer, or print a
buffer the other one just freed:

  WARNING: mm/slub.c:6733 at free_large_kmalloc+0xb0/0xf8
   kvfree+0x44/0x60
   adreno_show_object+0x190/0x240
   adreno_show+0x1d0/0x438
   a6xx_show+0x68/0xbd0
   msm_gpu_devcoredump_read+0x144/0x1f8
   devcd_data_read+0x34/0x48
  page dumped because: Not a kmalloc allocation

The dump also changes length between reads, so a reader can get a
short copy that ends at a page boundary.

Seen on an Adreno 830 with IGT's msm_recovery reading the devcoredump
while a udev helper copied it out: 16 such warnings in one boot, none
with this patch.

Add a mutex to struct msm_gpu and hold it while the state is
rendered. Only one crash state exists at a time, so a per-GPU lock is
enough. gpu->lock isn't used because a devcoredump reader would then
hold up recovery and fault capture for as long as it takes to render
the dump. The new lock is taken after msm_gpu_crashstate_get() and
dropped before msm_gpu_crashstate_put(), so it never nests with
gpu->lock, and it is taken interruptibly so a reader waiting behind
a slow render can be killed.

Fixes: 1df4289d5b23 ("drm/msm: Optimize adreno_show_object()")
Cc: stable@vger.kernel.org
Signed-off-by: Nico Angelo Dela Cruz <nicoangelodelacruz@gmail.com>
---
 drivers/gpu/drm/msm/msm_gpu.c | 10 ++++++++++
 drivers/gpu/drm/msm/msm_gpu.h |  8 ++++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index fded78fdb..428dc9666 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -191,11 +191,18 @@ static ssize_t msm_gpu_devcoredump_read(char *buffer, loff_t offset,
 	struct drm_print_iterator iter;
 	struct drm_printer p;
 	struct msm_gpu_state *state;
+	int ret;
 
 	state = msm_gpu_crashstate_get(gpu);
 	if (!state)
 		return 0;
 
+	ret = mutex_lock_interruptible(&gpu->crashstate_show_lock);
+	if (ret) {
+		msm_gpu_crashstate_put(gpu);
+		return ret;
+	}
+
 	iter.data = buffer;
 	iter.offset = 0;
 	iter.start = offset;
@@ -214,6 +221,8 @@ static ssize_t msm_gpu_devcoredump_read(char *buffer, loff_t offset,
 
 	gpu->funcs->show(gpu, state, &p);
 
+	mutex_unlock(&gpu->crashstate_show_lock);
+
 	msm_gpu_crashstate_put(gpu);
 
 	return count - iter.remain;
@@ -914,6 +923,7 @@ int msm_gpu_init(struct drm_device *drm, struct platform_device *pdev,
 	gpu->funcs = funcs;
 	gpu->name = name;
 	INIT_WORK(&gpu->crashstate_free_work, msm_gpu_crashstate_free_worker);
+	mutex_init(&gpu->crashstate_show_lock);
 
 	gpu->worker = kthread_run_worker(0, "gpu-worker");
 	if (IS_ERR(gpu->worker)) {
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index db9e11e7f..9b683973b 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -264,6 +264,14 @@ struct msm_gpu {
 	 */
 	struct work_struct crashstate_free_work;
 
+	/**
+	 * @crashstate_show_lock: serializes rendering of @crashstate for the
+	 * devcoredump.  The show functions convert captured buffers to ascii85
+	 * in place on first use (see adreno_show_object()), so concurrent
+	 * readers of the same dump must not render it at the same time.
+	 */
+	struct mutex crashstate_show_lock;
+
 	/**
 	 * @recovery_in_progress: true while recover_worker() holds @lock, so
 	 * that fault capture and GMU waits don't wait on a fault devcoredump
-- 
2.53.0

Re: [PATCH 2/4] drm/msm: free the crash state asynchronously

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm: premature flush_work() of crashstate_free_work in msm_gpu_cleanup()
--

commit 9e216db338c387162280bcdd65d14c7f062f6e18
Author: Nico Angelo Dela Cruz <nicoangelodelacruz@gmail.com>

drm/msm: free the crash state asynchronously

This commit defers msm_gpu_crashstate_put() in msm_gpu_devcoredump_free() to
a work item on system_dfl_wq so that synchronous free callbacks from
dev_coredumpm() do not recursively acquire gpu->lock.

> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index caa86279f561d..fded78fdbf8db 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c

[ ... ]

> @@ -1047,6 +1062,10 @@ void msm_gpu_cleanup(struct msm_gpu *gpu)
>  
>  	DBG("%s", gpu->name);
>  
> +	/* May be unset if probe failed before msm_gpu_init() */
> +	if (gpu->crashstate_free_work.func)
> +		flush_work(&gpu->crashstate_free_work);

[Severity: High]
Could flushing gpu->crashstate_free_work at the beginning of
msm_gpu_cleanup() lead to a use-after-free of struct msm_gpu if
msm_gpu_devcoredump_free() is invoked later during teardown?

In msm_gpu_cleanup(), the MMU fault handler and gpu->worker are only stopped
further down:

drivers/gpu/drm/msm/msm_gpu.c:msm_gpu_cleanup() {
	if (gpu->crashstate_free_work.func)
		flush_work(&gpu->crashstate_free_work);
	...
	if (!IS_ERR_OR_NULL(gpu->vm)) {
		struct msm_mmu *mmu = to_msm_vm(gpu->vm)->mmu;
		mmu->funcs->detach(mmu);
		drm_gpuvm_put(gpu->vm);
	}

	if (gpu->worker) {
		kthread_destroy_worker(gpu->worker);
	}
}

Both recover_worker() (running on gpu->worker) and adreno_fault_handler()
call msm_gpu_crashstate_capture() -> dev_coredumpm(...,
msm_gpu_devcoredump_free). During unbind in adreno_unbind(), if
adreno_system_suspend() times out waiting for a hung GPU or returns once
retire_submits() drops active_submits to zero before recover_worker()
finishes, adreno_unbind() proceeds to gpu->funcs->destroy(gpu) ->
adreno_gpu_cleanup() -> msm_gpu_cleanup().

If kthread_destroy_worker(gpu->worker) drains a pending recover_work (or a
fault occurs before mmu->funcs->detach(mmu)) and dev_coredumpm() fails or
devcoredump is disabled, msm_gpu_devcoredump_free() will queue
gpu->crashstate_free_work on system_dfl_wq after the flush_work() at the top
of msm_gpu_cleanup() has already completed.

Similarly, if an earlier dev_coredumpm() succeeded and left an active
devcoredump device, msm_gpu_cleanup() does not call
dev_coredump_put(&gpu->pdev->dev) before flushing crashstate_free_work. When
that devcoredump later expires or is cleared via sysfs, devcd_dev_release()
calls msm_gpu_devcoredump_free(gpu) and queues gpu->crashstate_free_work.

Once msm_gpu_cleanup() and adreno_gpu_cleanup() return, the destroy callback
(such as a6xx_destroy()) frees the enclosing GPU structure with kfree(), so
msm_gpu_crashstate_free_worker() can subsequently run on system_dfl_wq and
dereference freed memory in msm_gpu_crashstate_put(gpu).

Should msm_gpu_cleanup() call dev_coredump_put(&gpu->pdev->dev) and
flush_work(&gpu->crashstate_free_work) after mmu->funcs->detach(mmu) and
kthread_destroy_worker(gpu->worker) have completed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101175553.53746-1-nicoangelodelacruz@gmail.com?part=2

Re: [PATCH 1/4] drm/msm: don't wait for a fault devcoredump during GPU recovery

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/msm: 3-way deadlock on fault_coredump_done with non-recovery GMU waiters
- [High] drm/msm: adreno_fault_handler() blocks on gpu->lock across recover_worker()
- [High] drm/msm: deadlock on fault_coredump_done with gpu->lock and devfreq
--

commit 10d3d28539be32b5770e7379d7853931850318cb
Author: Nico Angelo Dela Cruz <nicoangelodelacruz@gmail.com>

drm/msm: don't wait for a fault devcoredump during GPU recovery

This commit adds a recovery_in_progress flag to struct msm_gpu, set by
recover_worker() while holding gpu->lock, so that adreno_fault_handler()
does not start a devcoredump and GMU wait loops in a6xx_gmu_set_oob() and
a6xx_hfi_wait_for_msg_interrupt() do not wait on fault_coredump_done during
GPU recovery.

Link: https://lore.kernel.org/all/CACu1E7EApr6Ne3XxFv4cUa2JuwNW2kfo1c5wSsAUN78iWyQ2sg@mail.gmail.com/

> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> index 662f09bec9d52..32ed49abedc28 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> @@ -445,12 +445,15 @@ int a6xx_gmu_set_oob(struct a6xx_gmu *gmu, enum a6xx_gmu_oob_state state)
>  		if (!ret)
>  			break;
>  
> -		if (completion_done(&a6xx_gpu->base.fault_coredump_done))
> +		if (completion_done(&a6xx_gpu->base.fault_coredump_done) ||
> +		    READ_ONCE(a6xx_gpu->base.base.recovery_in_progress))
>  			break;
>  
>  		/* We may timeout because the GMU is temporarily wedged from
>  		 * pending faults from the GPU and we are taking a devcoredump.
> -		 * Wait until the MMU is resumed and try again.
> +		 * Wait until the MMU is resumed and try again. A fault dump
> +		 * cannot complete while recovery holds gpu->lock, so don't
> +		 * wait for one then.
>  		 */
>  		wait_for_completion(&a6xx_gpu->base.fault_coredump_done);
>  	} while (true);

[Severity: High]
Can a non-recovery GMU waiter pass the recovery_in_progress check before
recover_worker() acquires gpu->lock, leading to a three-way deadlock?

In adreno_fault_handler(), fault_coredump_done is reinitialized before
msm_gpu_fault_crashstate_capture() acquires gpu->lock:

  adreno_fault_handler()
    reinit_completion(&adreno_gpu->fault_coredump_done);
    msm_gpu_fault_crashstate_capture(gpu, &fault_info)
      mutex_lock(&gpu->lock);
    complete_all(&adreno_gpu->fault_coredump_done);

If a fault occurs just before recover_worker() acquires gpu->lock,
adreno_fault_handler() sees recovery_in_progress as false, resets
fault_coredump_done, and blocks on gpu->lock when recover_worker() wins
the lock.

Meanwhile, a GMU caller that does not hold gpu->lock, such as
msm_devfreq_target() -> a6xx_gpu_set_freq() -> a6xx_gmu_set_freq()
(holding df->devfreq->lock, df->lock, and a6xx_gpu->gmu.lock) or
a6xx_gmu_sysprof_setup() (holding a runtime PM reference and
a6xx_gpu->gmu.lock), can time out in a6xx_gmu_set_oob() or
a6xx_hfi_wait_for_msg_interrupt(). If it checks recovery_in_progress
before recover_worker() sets it, it enters wait_for_completion() on
fault_coredump_done.

When recover_worker() sets recovery_in_progress to true, it does not wake
existing waiters on fault_coredump_done. It then calls a6xx_recover() or
a8xx_recover() -> pm_runtime_put_sync() -> a6xx_gmu_pm_suspend():

  a6xx_gmu_pm_suspend()
    msm_devfreq_suspend(gpu);
    mutex_lock(&a6xx_gpu->gmu.lock);
    ret = a6xx_gmu_stop(a6xx_gpu);

At that point, recover_worker() holds gpu->lock and blocks on df->lock or
a6xx_gpu->gmu.lock, the devfreq or sysprof thread holds those locks and
waits on fault_coredump_done, and adreno_fault_handler() waits on
gpu->lock before it can call complete_all(&adreno_gpu->fault_coredump_done).

> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_hfi.c b/drivers/gpu/drm/msm/adreno/a6xx_hfi.c
> index 6b34e704a70d0..fe4e3572898ab 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_hfi.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_hfi.c
> @@ -122,12 +122,15 @@ static int a6xx_hfi_wait_for_msg_interrupt(struct a6xx_gmu *gmu, u32 id, u32 seq
>  		if (!ret)
>  			break;
>  
> -		if (completion_done(&a6xx_gpu->base.fault_coredump_done))
> +		if (completion_done(&a6xx_gpu->base.fault_coredump_done) ||
> +		    READ_ONCE(a6xx_gpu->base.base.recovery_in_progress))
>  			break;
>  
>  		/* We may timeout because the GMU is temporarily wedged from
>  		 * pending faults from the GPU and we are taking a devcoredump.
> -		 * Wait until the MMU is resumed and try again.
> +		 * Wait until the MMU is resumed and try again. A fault dump
> +		 * cannot complete while recovery holds gpu->lock, so don't
> +		 * wait for one then.
>  		 */
>  		wait_for_completion(&a6xx_gpu->base.fault_coredump_done);
>  	} while (true);

[Severity: High]
Could this also deadlock when callers other than recover_worker() hold
gpu->lock while recovery_in_progress is false?

First, msm_job_run() -> msm_gpu_submit() holds gpu->lock with
recovery_in_progress set to false:

  msm_gpu_submit()
    pm_runtime_get_sync(&gpu->pdev->dev);
    msm_gpu_hw_init(gpu);
    ...
    msm_devfreq_active(gpu);

If adreno_fault_handler() calls reinit_completion() on fault_coredump_done
and blocks on gpu->lock in msm_gpu_fault_crashstate_capture() while
msm_gpu_submit() holds gpu->lock, any GMU OOB or HFI timeout during
pm_runtime_get_sync(), msm_gpu_hw_init(), or msm_devfreq_active() (or in a
devfreq worker holding df->devfreq->lock while msm_devfreq_active() waits
for it) will see recovery_in_progress as false and sleep in
wait_for_completion(&a6xx_gpu->base.fault_coredump_done) while gpu->lock
is held.

Second, msm_gpu_fault_crashstate_capture() itself runs under gpu->lock
with recovery_in_progress set to false while fault_coredump_done is still
incomplete:

  msm_gpu_fault_crashstate_capture()
    mutex_lock(&gpu->lock);
    ...
    pm_runtime_get_sync(&gpu->pdev->dev);
    msm_gpu_crashstate_capture(gpu, submit, fault_info, comm, cmd);
    pm_runtime_put_sync(&gpu->pdev->dev);
    ...
    mutex_unlock(&gpu->lock);

If the last submit retired in retire_worker and the autosuspend delay
elapsed during msm_gpu_crashstate_capture(), pm_runtime_put_sync() will
synchronously run adreno_runtime_suspend() -> a6xx_gmu_pm_suspend(). That
path can either block in msm_devfreq_suspend() / mutex_lock(&gmu->lock) on
a devfreq waiter sleeping on fault_coredump_done, or time out in
a6xx_gmu_notify_slumber() and call wait_for_completion() on
fault_coredump_done on the fault handler thread itself.

> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index dadfe4c694384..138bebf5ef70d 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -289,7 +289,8 @@ int adreno_fault_handler(struct msm_gpu *gpu, unsigned long iova, int flags,
>  	struct msm_mmu *mmu = to_msm_vm(gpu->vm)->mmu;
>  	const char *type = "UNKNOWN";
>  	bool do_devcoredump = info && (info->fsr & ARM_SMMU_FSR_SS) &&
> -		!READ_ONCE(gpu->crashstate);
> +		!READ_ONCE(gpu->crashstate) &&
> +		!READ_ONCE(gpu->recovery_in_progress);
>  	unsigned long irq_flags;
>  
>  	/*

[Severity: High]
What happens if adreno_fault_handler() evaluates do_devcoredump as true
just before recover_worker() acquires gpu->lock and sets
recovery_in_progress?

In that case, msm_gpu_fault_crashstate_capture() blocks on
mutex_lock(&gpu->lock) while recover_worker() runs to completion. Because
adreno_fault_handler() is invoked synchronously from
arm_smmu_context_fault() before ARM_SMMU_CB_FSR and ARM_SMMU_CB_RESUME are
written, the SMMU context bank remains stalled throughout recovery while
recover_worker() resets the GPU and replays submits.

Once recover_worker() finishes, advances cur_ring->memptrs->fence past the
faulting submit, replays the remaining submits, and releases gpu->lock,
msm_gpu_fault_crashstate_capture() acquires gpu->lock without rechecking
whether recovery occurred:

  msm_gpu_fault_crashstate_capture()
    mutex_lock(&gpu->lock);
    submit = find_submit(cur_ring, cur_ring->memptrs->fence + 1);
    if (submit && submit->fault_dumped)
            goto resume_smmu;
    if (submit) {
            submit->fault_dumped = true;
            ...
    }
    pm_runtime_get_sync(&gpu->pdev->dev);
    msm_gpu_crashstate_capture(gpu, submit, fault_info, comm, cmd);

Wouldn't find_submit(cur_ring, cur_ring->memptrs->fence + 1) now look up
the next replayed submit on cur_ring, mark that innocent submit with
fault_dumped = true, and (if gpu->crashstate is NULL) run
a6xx_gpu_state_get(), which writes REG_A6XX_CP_SQE_CNTL = 3 and halts SQE
on the freshly recovered GPU?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101175553.53746-1-nicoangelodelacruz@gmail.com?part=1

Re: [PATCH 2/4] drm/msm: free the crash state asynchronously

[Nico Angelo Dela Cruz]

[-- Attachment #1: Type: text/plain, Size: 4263 bytes --]

Yes, this is right. Draining gpu->worker can still run a recover_work
that captures a crash state, and a devcoredump that is still registered
queues the free when it is released, so the flush has to come last. I'll
move it after kthread_destroy_worker(), add dev_coredump_put() before it
(which also closes the pre-existing lifetime gap mentioned in the cover
letter), and send that in v2 once the series has had human review.

Thanks,
Nico

On Mon, Nov 2, 2026 at 2:20 AM <sashiko-bot@kernel.org> wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> - [High] drm/msm: premature flush_work() of crashstate_free_work in
> msm_gpu_cleanup()
> --
>
> commit 9e216db338c387162280bcdd65d14c7f062f6e18
> Author: Nico Angelo Dela Cruz <nicoangelodelacruz@gmail.com>
>
> drm/msm: free the crash state asynchronously
>
> This commit defers msm_gpu_crashstate_put() in msm_gpu_devcoredump_free()
> to
> a work item on system_dfl_wq so that synchronous free callbacks from
> dev_coredumpm() do not recursively acquire gpu->lock.
>
> > diff --git a/drivers/gpu/drm/msm/msm_gpu.c
> b/drivers/gpu/drm/msm/msm_gpu.c
> > index caa86279f561d..fded78fdbf8db 100644
> > --- a/drivers/gpu/drm/msm/msm_gpu.c
> > +++ b/drivers/gpu/drm/msm/msm_gpu.c
>
> [ ... ]
>
> > @@ -1047,6 +1062,10 @@ void msm_gpu_cleanup(struct msm_gpu *gpu)
> >
> >       DBG("%s", gpu->name);
> >
> > +     /* May be unset if probe failed before msm_gpu_init() */
> > +     if (gpu->crashstate_free_work.func)
> > +             flush_work(&gpu->crashstate_free_work);
>
> [Severity: High]
> Could flushing gpu->crashstate_free_work at the beginning of
> msm_gpu_cleanup() lead to a use-after-free of struct msm_gpu if
> msm_gpu_devcoredump_free() is invoked later during teardown?
>
> In msm_gpu_cleanup(), the MMU fault handler and gpu->worker are only
> stopped
> further down:
>
> drivers/gpu/drm/msm/msm_gpu.c:msm_gpu_cleanup() {
>         if (gpu->crashstate_free_work.func)
>                 flush_work(&gpu->crashstate_free_work);
>         ...
>         if (!IS_ERR_OR_NULL(gpu->vm)) {
>                 struct msm_mmu *mmu = to_msm_vm(gpu->vm)->mmu;
>                 mmu->funcs->detach(mmu);
>                 drm_gpuvm_put(gpu->vm);
>         }
>
>         if (gpu->worker) {
>                 kthread_destroy_worker(gpu->worker);
>         }
> }
>
> Both recover_worker() (running on gpu->worker) and adreno_fault_handler()
> call msm_gpu_crashstate_capture() -> dev_coredumpm(...,
> msm_gpu_devcoredump_free). During unbind in adreno_unbind(), if
> adreno_system_suspend() times out waiting for a hung GPU or returns once
> retire_submits() drops active_submits to zero before recover_worker()
> finishes, adreno_unbind() proceeds to gpu->funcs->destroy(gpu) ->
> adreno_gpu_cleanup() -> msm_gpu_cleanup().
>
> If kthread_destroy_worker(gpu->worker) drains a pending recover_work (or a
> fault occurs before mmu->funcs->detach(mmu)) and dev_coredumpm() fails or
> devcoredump is disabled, msm_gpu_devcoredump_free() will queue
> gpu->crashstate_free_work on system_dfl_wq after the flush_work() at the
> top
> of msm_gpu_cleanup() has already completed.
>
> Similarly, if an earlier dev_coredumpm() succeeded and left an active
> devcoredump device, msm_gpu_cleanup() does not call
> dev_coredump_put(&gpu->pdev->dev) before flushing crashstate_free_work.
> When
> that devcoredump later expires or is cleared via sysfs, devcd_dev_release()
> calls msm_gpu_devcoredump_free(gpu) and queues gpu->crashstate_free_work.
>
> Once msm_gpu_cleanup() and adreno_gpu_cleanup() return, the destroy
> callback
> (such as a6xx_destroy()) frees the enclosing GPU structure with kfree(), so
> msm_gpu_crashstate_free_worker() can subsequently run on system_dfl_wq and
> dereference freed memory in msm_gpu_crashstate_put(gpu).
>
> Should msm_gpu_cleanup() call dev_coredump_put(&gpu->pdev->dev) and
> flush_work(&gpu->crashstate_free_work) after mmu->funcs->detach(mmu) and
> kthread_destroy_worker(gpu->worker) have completed?
>
> --
> Sashiko AI review ·
> https://sashiko.dev/#/patchset/20261101175553.53746-1-nicoangelodelacruz@gmail.com?part=2
>

[-- Attachment #2: Type: text/html, Size: 5201 bytes --]

Re: [PATCH 3/4] drm/msm/a8xx: add a memory-only GPU crash state

[Rob Clark]

On Sun, Nov 1, 2026 at 10:05 AM Nico Angelo Dela Cruz
<nicoangelodelacruz@gmail.com> wrote:
>
> a8xx had no gpu_state_get callback, so no devcoredump was ever
> produced when the GPU hung or faulted.
>
> Add a8xx_gpu_state_get(), a memory-only snapshot: the generic Adreno
> state from adreno_gpu_state_get() (ring buffer contents and fences,
> with rptr read from the hw_apriv shadow rather than a register), the
> GMU log/HFI/debug buffers and the HFI queue history. No register is
> read, because the GMU may be wedged and GX powered off when this runs
> from the recover worker or the SMMU fault path. As a result a8xx
> dumps always report "gpu-initialized: 0", as a6xx/a7xx dumps do when
> GX is off.
>
> The shared part moves out of a6xx_gpu_state_get() into
> a6xx_gpu_state_get_gmu(), which also runs adreno_gpu_state_get().
> It is a pure move except for a new gmu_regs parameter that gates
> a6xx_get_gmu_registers(); a6xx/a7xx pass true, so their output is
> unchanged. Reviewed with git diff --color-moved=dimmed-zebra: apart
> from the moved lines, the new code is the helper's signature, the
> "if (gmu_regs)" check, the locals the helper needs, and the call.
>
> Tested on an Adreno 830: IGT msm_recovery gpu-fault (6 of 6 runs) and
> natural GPU hangs produce dumps with the ringbuffer, gmu-log, gmu-hfi
> and queue-history sections; capture takes 1.1 to 4.0 ms.
>
> a6xx/a7xx: build and pure-move review only, Tested-by welcome.

If you are interested in a8xx gpu devcoredump support, may I suggest:

   https://patchwork.freedesktop.org/series/176505/

I'm still looking for someone who has a830 who can test it

BR,
-R

> Signed-off-by: Nico Angelo Dela Cruz <nicoangelodelacruz@gmail.com>
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c       |  7 +++
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.h       |  1 +
>  drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 47 ++++++++++++++++-----
>  3 files changed, 45 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 92e8c8abb..0f4b0af0a 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -2918,9 +2918,16 @@ const struct adreno_gpu_funcs a8xx_gpu_funcs = {
>                 .active_ring = a6xx_active_ring,
>                 .irq = a8xx_irq,
>                 .destroy = a6xx_destroy,
> +#if defined(CONFIG_DRM_MSM_GPU_STATE)
> +               .show = a6xx_show,
> +#endif
>                 .gpu_busy = a8xx_gpu_busy,
>                 .gpu_get_freq = a6xx_gmu_get_freq,
>                 .gpu_set_freq = a6xx_gpu_set_freq,
> +#if defined(CONFIG_DRM_MSM_GPU_STATE)
> +               .gpu_state_get = a8xx_gpu_state_get,
> +               .gpu_state_put = a6xx_gpu_state_put,
> +#endif
>                 .create_vm = a6xx_create_vm,
>                 .create_private_vm = a6xx_create_private_vm,
>                 .get_rptr = a6xx_get_rptr,
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> index 79912a33e..03432abf9 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> @@ -338,6 +338,7 @@ void a6xx_show(struct msm_gpu *gpu, struct msm_gpu_state *state,
>                 struct drm_printer *p);
>
>  struct msm_gpu_state *a6xx_gpu_state_get(struct msm_gpu *gpu);
> +struct msm_gpu_state *a8xx_gpu_state_get(struct msm_gpu *gpu);
>  int a6xx_gpu_state_put(struct msm_gpu_state *state);
>
>  void a6xx_bus_clear_pending_transactions(struct adreno_gpu *adreno_gpu, bool gx_off);
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> index 26c17861c..a3751358a 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> @@ -1629,25 +1629,19 @@ static void a7xx_get_indexed_registers(struct msm_gpu *gpu,
>         return;
>  }
>
> -struct msm_gpu_state *a6xx_gpu_state_get(struct msm_gpu *gpu)
> +static void a6xx_gpu_state_get_gmu(struct msm_gpu *gpu,
> +                                  struct a6xx_gpu_state *a6xx_state, bool gmu_regs)
>  {
> -       struct a6xx_crashdumper _dumper = { 0 }, *dumper = NULL;
>         struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>         struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
> -       struct a6xx_gpu_state *a6xx_state = kzalloc_obj(*a6xx_state);
> -       bool stalled;
> -
> -       if (!a6xx_state)
> -               return ERR_PTR(-ENOMEM);
> -
> -       INIT_LIST_HEAD(&a6xx_state->objs);
>
>         /* Get the generic state from the adreno core */
>         adreno_gpu_state_get(gpu, &a6xx_state->base);
>
>         if (!adreno_has_gmu_wrapper(adreno_gpu) &&
>             !adreno_has_rgmu(adreno_gpu)) {
> -               a6xx_get_gmu_registers(gpu, a6xx_state);
> +               if (gmu_regs)
> +                       a6xx_get_gmu_registers(gpu, a6xx_state);
>
>                 a6xx_state->gmu_log = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.log);
>                 a6xx_state->gmu_hfi = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.hfi);
> @@ -1655,6 +1649,21 @@ struct msm_gpu_state *a6xx_gpu_state_get(struct msm_gpu *gpu)
>
>                 a6xx_snapshot_gmu_hfi_history(gpu, a6xx_state);
>         }
> +}
> +
> +struct msm_gpu_state *a6xx_gpu_state_get(struct msm_gpu *gpu)
> +{
> +       struct a6xx_crashdumper _dumper = { 0 }, *dumper = NULL;
> +       struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> +       struct a6xx_gpu_state *a6xx_state = kzalloc_obj(*a6xx_state);
> +       bool stalled;
> +
> +       if (!a6xx_state)
> +               return ERR_PTR(-ENOMEM);
> +
> +       INIT_LIST_HEAD(&a6xx_state->objs);
> +
> +       a6xx_gpu_state_get_gmu(gpu, a6xx_state, true);
>
>         /* If GX isn't on the rest of the data isn't going to be accessible */
>         if (!adreno_gpu->funcs->gx_is_on(adreno_gpu))
> @@ -1714,6 +1723,24 @@ struct msm_gpu_state *a6xx_gpu_state_get(struct msm_gpu *gpu)
>         return  &a6xx_state->base;
>  }
>
> +/*
> + * Memory-only snapshot for a8xx: no register is read, as the GMU may be
> + * wedged and GX powered off by the time this runs.
> + */
> +struct msm_gpu_state *a8xx_gpu_state_get(struct msm_gpu *gpu)
> +{
> +       struct a6xx_gpu_state *a6xx_state = kzalloc_obj(*a6xx_state);
> +
> +       if (!a6xx_state)
> +               return ERR_PTR(-ENOMEM);
> +
> +       INIT_LIST_HEAD(&a6xx_state->objs);
> +
> +       a6xx_gpu_state_get_gmu(gpu, a6xx_state, false);
> +
> +       return &a6xx_state->base;
> +}
> +
>  static void a6xx_gpu_state_destroy(struct kref *kref)
>  {
>         struct a6xx_state_memobj *obj, *tmp;
> --
> 2.53.0
>

Re: [PATCH 2/4] drm/msm: free the crash state asynchronously

[Rob Clark]

On Sun, Nov 1, 2026 at 10:00 AM Nico Angelo Dela Cruz
<nicoangelodelacruz@gmail.com> wrote:
>
> msm_gpu_crashstate_capture() sets gpu->crashstate and calls
> dev_coredumpm() while the caller (recover_worker() or
> msm_gpu_fault_crashstate_capture()) holds gpu->lock. When devcoredump
> is disabled (/sys/class/devcoredump/disabled, a write-once knob) or
> its allocation fails, dev_coredumpm_timeout() calls the free callback
> synchronously.
>
> msm_gpu_devcoredump_free() then calls msm_gpu_crashstate_put(), which
> does mutex_lock(&gpu->lock) and so recursively takes a mutex the
> caller already holds. On systems with devcoredump disabled, every GPU
> recovery that captures a crash state deadlocks.
>
> Defer the put to a work item on system_dfl_wq instead. This is safe
> because msm_gpu_devcoredump_read() takes and drops its own crash
> state reference, and the work takes gpu->lock normally once the
> holder has released it. gpu->crashstate stays set until the work
> runs, which only means a capture in that window is skipped.
> msm_gpu_cleanup() flushes an already queued free before teardown.
>
> Tested on an Adreno 830 with /sys/class/devcoredump/disabled set to 1:
> IGT msm_recovery gpu-fault recovers and the GPU resumes.

It seems like this is missing test coverage in msm_recovery test.. it
wasn't a case that had occurred to me.

Care to also send an igt patch to add this case?

BR,
-R

> Fixes: c0fec7f562ec ("drm/msm/gpu: Capture the GPU state on a GPU hang")
> Cc: stable@vger.kernel.org
> Signed-off-by: Nico Angelo Dela Cruz <nicoangelodelacruz@gmail.com>
> ---
>  drivers/gpu/drm/msm/msm_gpu.c | 21 ++++++++++++++++++++-
>  drivers/gpu/drm/msm/msm_gpu.h |  6 ++++++
>  2 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index caa86279f..fded78fdb 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -175,6 +175,14 @@ int msm_gpu_hw_init(struct msm_gpu *gpu)
>         return ret;
>  }
>
> +static void msm_gpu_crashstate_free_worker(struct work_struct *work)
> +{
> +       struct msm_gpu *gpu = container_of(work, struct msm_gpu,
> +                                          crashstate_free_work);
> +
> +       msm_gpu_crashstate_put(gpu);
> +}
> +
>  #ifdef CONFIG_DEV_COREDUMP
>  static ssize_t msm_gpu_devcoredump_read(char *buffer, loff_t offset,
>                 size_t count, void *data, size_t datalen)
> @@ -215,7 +223,13 @@ static void msm_gpu_devcoredump_free(void *data)
>  {
>         struct msm_gpu *gpu = data;
>
> -       msm_gpu_crashstate_put(gpu);
> +       /*
> +        * dev_coredumpm() calls this synchronously when it can't keep the
> +        * dump (devcoredump disabled, or out of memory), while
> +        * msm_gpu_crashstate_capture() holds gpu->lock, which
> +        * msm_gpu_crashstate_put() also takes.  Defer the put to a worker.
> +        */
> +       queue_work(system_dfl_wq, &gpu->crashstate_free_work);
>  }
>
>  static void msm_gpu_crashstate_get_bo(struct msm_gpu_state *state,
> @@ -899,6 +913,7 @@ int msm_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>         gpu->dev = drm;
>         gpu->funcs = funcs;
>         gpu->name = name;
> +       INIT_WORK(&gpu->crashstate_free_work, msm_gpu_crashstate_free_worker);
>
>         gpu->worker = kthread_run_worker(0, "gpu-worker");
>         if (IS_ERR(gpu->worker)) {
> @@ -1047,6 +1062,10 @@ void msm_gpu_cleanup(struct msm_gpu *gpu)
>
>         DBG("%s", gpu->name);
>
> +       /* May be unset if probe failed before msm_gpu_init() */
> +       if (gpu->crashstate_free_work.func)
> +               flush_work(&gpu->crashstate_free_work);
> +
>         for (i = 0; i < ARRAY_SIZE(gpu->rb); i++) {
>                 msm_ringbuffer_destroy(gpu->rb[i]);
>                 gpu->rb[i] = NULL;
> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
> index 5cf220f75..db9e11e7f 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.h
> +++ b/drivers/gpu/drm/msm/msm_gpu.h
> @@ -258,6 +258,12 @@ struct msm_gpu {
>
>         struct msm_gpu_state *crashstate;
>
> +       /**
> +        * @crashstate_free_work: drops the devcoredump's crashstate reference
> +        * outside of @lock, see msm_gpu_devcoredump_free().
> +        */
> +       struct work_struct crashstate_free_work;
> +
>         /**
>          * @recovery_in_progress: true while recover_worker() holds @lock, so
>          * that fault capture and GMU waits don't wait on a fault devcoredump
> --
> 2.53.0
>

Re: [PATCH 4/4] drm/msm: serialize devcoredump reads of the crash state

[Rob Clark]

On Sun, Nov 1, 2026 at 10:05 AM Nico Angelo Dela Cruz
<nicoangelodelacruz@gmail.com> wrote:
>
> adreno_show_object() converts each captured buffer to ascii85 the
> first time the crash state is shown, in place: it checks *encoded,
> replaces *ptr with the encoded copy and kvfree()s the raw buffer.
> msm_gpu_devcoredump_read() only takes gpu->lock around the reference
> get, and every read() of the devcoredump renders the whole state
> again, so two processes reading the same devcoredump at once can both
> see *encoded == false and both free the same buffer, or print a
> buffer the other one just freed:
>
>   WARNING: mm/slub.c:6733 at free_large_kmalloc+0xb0/0xf8
>    kvfree+0x44/0x60
>    adreno_show_object+0x190/0x240
>    adreno_show+0x1d0/0x438
>    a6xx_show+0x68/0xbd0
>    msm_gpu_devcoredump_read+0x144/0x1f8
>    devcd_data_read+0x34/0x48
>   page dumped because: Not a kmalloc allocation
>
> The dump also changes length between reads, so a reader can get a
> short copy that ends at a page boundary.
>
> Seen on an Adreno 830 with IGT's msm_recovery reading the devcoredump
> while a udev helper copied it out: 16 such warnings in one boot, none
> with this patch.

Probably also something that igt should cover..

> Add a mutex to struct msm_gpu and hold it while the state is
> rendered. Only one crash state exists at a time, so a per-GPU lock is
> enough. gpu->lock isn't used because a devcoredump reader would then
> hold up recovery and fault capture for as long as it takes to render
> the dump. The new lock is taken after msm_gpu_crashstate_get() and
> dropped before msm_gpu_crashstate_put(), so it never nests with
> gpu->lock, and it is taken interruptibly so a reader waiting behind
> a slow render can be killed.

You could have a 2nd state snapshot live with the debugfs path, fwiw.
Note that I'm working on some IFPC fixes in that area[1], originally
part of the a8xx state snapshot support.

BR,
-R

[1] https://patchwork.freedesktop.org/series/177370/

> Fixes: 1df4289d5b23 ("drm/msm: Optimize adreno_show_object()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Nico Angelo Dela Cruz <nicoangelodelacruz@gmail.com>
> ---
>  drivers/gpu/drm/msm/msm_gpu.c | 10 ++++++++++
>  drivers/gpu/drm/msm/msm_gpu.h |  8 ++++++++
>  2 files changed, 18 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index fded78fdb..428dc9666 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -191,11 +191,18 @@ static ssize_t msm_gpu_devcoredump_read(char *buffer, loff_t offset,
>         struct drm_print_iterator iter;
>         struct drm_printer p;
>         struct msm_gpu_state *state;
> +       int ret;
>
>         state = msm_gpu_crashstate_get(gpu);
>         if (!state)
>                 return 0;
>
> +       ret = mutex_lock_interruptible(&gpu->crashstate_show_lock);
> +       if (ret) {
> +               msm_gpu_crashstate_put(gpu);
> +               return ret;
> +       }
> +
>         iter.data = buffer;
>         iter.offset = 0;
>         iter.start = offset;
> @@ -214,6 +221,8 @@ static ssize_t msm_gpu_devcoredump_read(char *buffer, loff_t offset,
>
>         gpu->funcs->show(gpu, state, &p);
>
> +       mutex_unlock(&gpu->crashstate_show_lock);
> +
>         msm_gpu_crashstate_put(gpu);
>
>         return count - iter.remain;
> @@ -914,6 +923,7 @@ int msm_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>         gpu->funcs = funcs;
>         gpu->name = name;
>         INIT_WORK(&gpu->crashstate_free_work, msm_gpu_crashstate_free_worker);
> +       mutex_init(&gpu->crashstate_show_lock);
>
>         gpu->worker = kthread_run_worker(0, "gpu-worker");
>         if (IS_ERR(gpu->worker)) {
> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
> index db9e11e7f..9b683973b 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.h
> +++ b/drivers/gpu/drm/msm/msm_gpu.h
> @@ -264,6 +264,14 @@ struct msm_gpu {
>          */
>         struct work_struct crashstate_free_work;
>
> +       /**
> +        * @crashstate_show_lock: serializes rendering of @crashstate for the
> +        * devcoredump.  The show functions convert captured buffers to ascii85
> +        * in place on first use (see adreno_show_object()), so concurrent
> +        * readers of the same dump must not render it at the same time.
> +        */
> +       struct mutex crashstate_show_lock;
> +
>         /**
>          * @recovery_in_progress: true while recover_worker() holds @lock, so
>          * that fault capture and GMU waits don't wait on a fault devcoredump
> --
> 2.53.0
>