[Linaro-mm-sig] Re: [PATCH v5 2/2] drm/amdgpu: fix recursive ww_mutex acquire in amdgpu_devcoredump_format

On 5/21/26 17:08, Mikhail Gavrilov wrote:

When dumping IB contents from a hung job, amdgpu_devcoredump_format() acquired the VM root PD's reservation via amdgpu_vm_lock_by_pasid() and then, for each IB, called amdgpu_bo_reserve() on the BO backing the IB. Both reservations are reservation_ww_class_mutex objects and neither used a ww_acquire_ctx, which trips lockdep:

WARNING: possible recursive locking detected

kworker/u128:0 is trying to acquire lock: ffff88838b16e1f0 (reservation_ww_class_mutex){+.+.}-{4:4}, at: amdgpu_devcoredump_format+0x1594/0x23f0 [amdgpu]

but task is already holding lock: ffff8882f82681f0 (reservation_ww_class_mutex){+.+.}-{4:4}, at: amdgpu_devcoredump_format+0x1594/0x23f0 [amdgpu]

Possible unsafe locking scenario: CPU0 ---- lock(reservation_ww_class_mutex); lock(reservation_ww_class_mutex);

*** DEADLOCK *** May be due to missing lock nesting notation

Workqueue: events_unbound amdgpu_devcoredump_deferred_work [amdgpu] Call Trace: __ww_mutex_lock.constprop.0 ww_mutex_lock amdgpu_bo_reserve amdgpu_devcoredump_format+0x1594 [amdgpu] amdgpu_devcoredump_deferred_work+0xea [amdgpu]

The two reservations are on different BOs in the captured trace, so the splat is a lockdep-correctness warning, not an observed deadlock. It becomes a real self-deadlock whenever the IB BO shares its dma_resv with the root PD (the always-valid case, see amdgpu_vm_is_bo_always_valid()): amdgpu_bo_reserve(abo) re-acquires the same ww_mutex without a ticket and blocks forever.

With amdgpu.gpu_recovery=0 the timeout handler refires every ~2 s and each invocation produces this splat, drowning the kernel ring buffer.

Now that amdgpu_vm_lock_by_pasid() takes a drm_exec context, lock the root PD and every IB BO together in a single drm_exec ticket. DRM_EXEC_IGNORE_DUPLICATES handles IB BOs that share a dma_resv (e.g. always-valid BOs, or two IBs backed by the same BO). Every lock is now a top-level acquire under one ww_acquire_ctx, so the recursive ww_mutex condition is gone, and the per-IB amdgpu_bo_reserve()/amdgpu_bo_unref() dance -- including a BO refcount leak on the amdgpu_bo_reserve() failure path -- is removed.

Reproducer (~150 LoC libdrm_amdgpu): submit a single GFX IB containing PACKET3_INDIRECT_BUFFER chained at GPU VA 0 and wait for the fence. The TDR fires within ~10 s and the deferred coredump worker produces the splat above on every invocation; with this change applied the splat is gone.

That commit message is a bit to long. It should describe the problem and the solution and not necessary how to reproduce it.

Fixes: 7b15fc2d1f1a ("drm/amdgpu: dump job ibs in the devcoredump") Suggested-by: Christian König christian.koenig@amd.com Signed-off-by: Mikhail Gavrilov mikhail.v.gavrilov@gmail.com

---

.../gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c | 105 ++++++++++++------ 1 file changed, 71 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c index d386bc775d03..456ea9911d48 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c @@ -24,6 +24,7 @@ #include <generated/utsrelease.h> #include <linux/devcoredump.h> +#include <drm/drm_exec.h> #include "amdgpu_dev_coredump.h" #include "atom.h" @@ -214,13 +215,9 @@ amdgpu_devcoredump_format(char *buffer, size_t count, struct amdgpu_coredump_inf struct drm_printer p; struct drm_print_iterator iter; struct amdgpu_vm_fault_info *fault_info;

• struct amdgpu_bo_va_mapping *mapping; struct amdgpu_ip_block *ip_block; struct amdgpu_res_cursor cursor;
• struct amdgpu_bo *abo, *root;
• uint64_t va_start, offset; struct amdgpu_ring *ring;
• struct amdgpu_vm *vm; u32 *ib_content; uint8_t *kptr; int ver, i, j, r;

@@ -343,43 +340,84 @@ amdgpu_devcoredump_format(char *buffer, size_t count, struct amdgpu_coredump_inf drm_printf(&p, "VRAM is lost due to GPU reset!\n"); if (coredump->num_ibs) {

• /* Don't try to lookup the VM or map the BOs when calculating the
  

•  * size required to store the devcoredump.
  

• struct amdgpu_bo_va_mapping *mapping;
  

• struct amdgpu_bo *abo;
  

• struct drm_exec exec;
  

• struct amdgpu_vm *vm;
  

• u64 va_start, offset;
  

It's probably a good idea to put the IB dumping into a separate function.

• bool locked = false;
  

Drop that variable and handling, it is superflous.

• /*
  

•  * Lock the VM root PD and every IB BO together in a single
  

•  * drm_exec ticket. Reserving the IB BOs one by one while the
  

•  * root PD is held would be a recursive reservation_ww_class_mutex
  

•  * acquire without a ww_acquire_ctx, which trips lockdep and
  

•  * self-deadlocks for IB BOs that share their dma_resv with the
  

•  * root PD (always-valid BOs).
  

•  *
  

•  * Skip locking entirely on the sizing pass: it does not write
  

•  * IB content, so the size estimate doesn't depend on whether
  

•  * the BOs are reachable.
  

  */

• if (sizing_pass)
  

• 	vm = NULL;
  

• else
  

• 	vm = amdgpu_vm_lock_by_pasid(adev, &root, coredump->pasid);
  

• if (!sizing_pass) {
  

• 	drm_exec_init(&exec, DRM_EXEC_IGNORE_DUPLICATES,
  

• 		      1 + coredump->num_ibs);
  

• 	drm_exec_until_all_locked(&exec) {
  

• 		vm = amdgpu_vm_lock_by_pasid(adev, coredump->pasid,
  

• 					     &exec);
  

• 		drm_exec_retry_on_contention(&exec);
  

• 		if (!vm)
  

• 			break;
  

This should use goto error handling, when we can't find the VM we should abort here.

• 		for (int i = 0; i < coredump->num_ibs; i++) {
  

• 			u64 pfn;
  

• 			va_start = coredump->ibs[i].gpu_addr &
  

• 				   AMDGPU_GMC_HOLE_MASK;
  

• 			pfn = va_start / AMDGPU_GPU_PAGE_SIZE;
  

• 			mapping = amdgpu_vm_bo_lookup_mapping(vm, pfn);
  

• 			if (!mapping)
  

• 				continue;
  

That's also an error, it could be that we just want to print the IB start address in that case.

• 			abo = mapping->bo_va->base.bo;
  

• 			r = drm_exec_lock_obj(&exec, &abo->tbo.base);
  

• 			drm_exec_retry_on_contention(&exec);
  

• 			if (r)
  

• 				break;
  

Dito

• 		}
  

• 		if (r)
  

• 			break;
  

And here as well.

• 	}
  

• 	if (vm && !r)
  

• 		locked = true;
  

• 	else
  

• 		drm_exec_fini(&exec);
  

Don't call drm_exec_fini() here.

Regards, Christian.

• }
  

• for (int i = 0; i < coredump->num_ibs; i++) {
  

• 	bool emit_content = sizing_pass;
  

• for (int i = 0; i < coredump->num_ibs && (sizing_pass || vm); i++) {
  ib_content = kvmalloc_array(coredump->ibs[i].ib_size_dw, 4,
  			    GFP_KERNEL);
  if (!ib_content)
  	continue;
  

• 	/* vm=NULL can only happen when 'sizing_pass' is true. Skip to the
  

• 	 * drm_printf() calls (ib_content doesn't need to be initialized
  

• 	 * as its content won't be written anywhere).
  

• 	 */
  

• 	if (!vm)
  

• 	if (!locked)
  	goto output_ib_content;
  

va_start = coredump->ibs[i].gpu_addr & AMDGPU_GMC_HOLE_MASK; mapping = amdgpu_vm_bo_lookup_mapping(vm, va_start / AMDGPU_GPU_PAGE_SIZE); if (!mapping)

• 		goto free_ib_content;
  

• 		goto output_ib_content;
  

• 	offset = va_start - (mapping->start * AMDGPU_GPU_PAGE_SIZE);
  

• 	abo = amdgpu_bo_ref(mapping->bo_va->base.bo);
  

• 	r = amdgpu_bo_reserve(abo, false);
  

• 	if (r)
  

• 		goto free_ib_content;
  

• 	abo = mapping->bo_va->base.bo;
  

• 	offset = va_start - mapping->start * AMDGPU_GPU_PAGE_SIZE;
  

if (abo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS) { off = 0; if (abo->tbo.resource->mem_type != TTM_PL_VRAM)

• 			goto unreserve_abo;
  

• 			goto output_ib_content;
  

amdgpu_res_first(abo->tbo.resource, offset, coredump->ibs[i].ib_size_dw * 4, @@ -391,12 +429,13 @@ amdgpu_devcoredump_format(char *buffer, size_t count, struct amdgpu_coredump_inf off += cursor.size; amdgpu_res_next(&cursor, cursor.size); }

• 		emit_content = true;
  } else {
  	r = ttm_bo_kmap(&abo->tbo, 0,
  			PFN_UP(abo->tbo.base.size),
  			&abo->kmap);
  	if (r)
  

• 			goto unreserve_abo;
  

• 			goto output_ib_content;
  

kptr = amdgpu_bo_kptr(abo); kptr += offset; @@ -404,23 +443,21 @@ amdgpu_devcoredump_format(char *buffer, size_t count, struct amdgpu_coredump_inf coredump->ibs[i].ib_size_dw * 4); amdgpu_bo_kunmap(abo);

• 		emit_content = true;
  }
  

output_ib_content: drm_printf(&p, "\nIB #%d 0x%llx %d dw\n", i, coredump->ibs[i].gpu_addr, coredump->ibs[i].ib_size_dw);

• 	for (int j = 0; j < coredump->ibs[i].ib_size_dw; j++)
  

• 		drm_printf(&p, "0x%08x\n", ib_content[j]);
  

-unreserve_abo:

• 	if (vm)
  

• 		amdgpu_bo_unreserve(abo);
  

-free_ib_content:

• 	if (emit_content) {
  

• 		for (int j = 0; j < coredump->ibs[i].ib_size_dw; j++)
  

• 			drm_printf(&p, "0x%08x\n", ib_content[j]);
  

• 	}
  kvfree(ib_content);
  

  }

• if (vm) {
  

• 	amdgpu_bo_unreserve(root);
  

• 	amdgpu_bo_unref(&root);
  

• }
  

• if (locked)
  

• 	drm_exec_fini(&exec);
  

  }

return count - iter.remain;