On 5/19/26 18:15, Mikhail Gavrilov wrote:
> When dumping IB contents from a hung job, amdgpu_devcoredump_format()
> acquires the VM root PD's reservation lock via amdgpu_vm_lock_by_pasid()
> and then, for each IB referenced by the job, calls amdgpu_bo_reserve()
> on the BO that backs the IB. Both reservations are taken on
> reservation_ww_class_mutex objects but neither uses 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.
>
> Fix it in two steps:
>
> 1. Collect per-IB BO references under the root PD's reservation, then
> release the root before locking the IB BOs. The walk over the VM
> mapping tree must remain under the root lock (mappings can be torn
> down without it), but the actual content copies do not.
>
> 2. Lock all the IB BOs together using drm_exec(9) with a single
> ww_acquire_ctx. DRM_EXEC_IGNORE_DUPLICATES handles the case where
> IB BOs share a dma_resv (e.g. always-valid BOs). Each lock attempt
> is now a top-level acquire under one ticket, with retry-on-
> contention handled by drm_exec; the recursive ww_mutex condition
> is gone.
>
> The collect/lock/release logic is factored out into three small helpers
> (amdgpu_devcoredump_{collect,lock,release}_ib_refs) to keep the main
> function readable and within the kernel coding style indentation
> guideline.
>
> This also fixes a BO refcount leak in the original code: when
> amdgpu_bo_reserve() failed, control jumped to free_ib_content without
> running amdgpu_bo_unref(). In the new structure the per-IB BO refs
> are released unconditionally in the cleanup helper.
>
> 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.
>
> v2: switch from per-IB amdgpu_bo_reserve() to drm_exec for the IB BO
> locking as suggested by Christian König; the snapshot approach
> for collecting BO references under the root PD's reservation is
> retained.
>
> Fixes: 7b15fc2d1f1a ("drm/amdgpu: dump job ibs in the devcoredump")
> Suggested-by: Christian König <[email protected]>
> Signed-off-by: Mikhail Gavrilov <[email protected]>
> ---
> .../gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c | 187 ++++++++++++++----
> 1 file changed, 148 insertions(+), 39 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..9ac958cf09fd 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"
>
> @@ -207,6 +208,108 @@ static void amdgpu_devcoredump_fw_info(struct
> amdgpu_device *adev,
> }
> }
>
> +struct amdgpu_devcoredump_ib_ref {
> + struct amdgpu_bo *bo;
> + u64 offset;
> +};
> +
> +/*
> + * Walk the VM's mapping tree under the root PD's reservation to obtain the
> BO
> + * that backs each IB and pin it with a refcount. The root PD reservation is
> + * dropped before this function returns; the caller can then lock each IB BO
> + * via drm_exec without nesting reservations on reservation_ww_class_mutex.
> + *
> + * Returns an array of num_ibs entries (each ib_refs[i].bo may be NULL if its
> + * mapping was not found), or NULL on allocation failure / VM lookup failure.
> + * The caller must release the BO refs and free the array via
> + * amdgpu_devcoredump_release_ib_refs().
> + */
> +static struct amdgpu_devcoredump_ib_ref *
> +amdgpu_devcoredump_collect_ib_refs(struct amdgpu_device *adev,
> + struct amdgpu_coredump_info *coredump)
> +{
> + struct amdgpu_devcoredump_ib_ref *ib_refs;
> + struct amdgpu_bo_va_mapping *mapping;
> + struct amdgpu_bo *root;
> + struct amdgpu_vm *vm;
> + u64 va_start;
> +
> + ib_refs = kcalloc(coredump->num_ibs, sizeof(*ib_refs), GFP_KERNEL);
> + if (!ib_refs)
> + return NULL;
> +
> + vm = amdgpu_vm_lock_by_pasid(adev, &root, coredump->pasid);
> + if (!vm) {
> + kfree(ib_refs);
> + return NULL;
> + }
> +
> + for (int i = 0; i < coredump->num_ibs; i++) {
> + 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)
> + continue;
> +
> + ib_refs[i].bo = amdgpu_bo_ref(mapping->bo_va->base.bo);
> + ib_refs[i].offset = va_start -
> + mapping->start * AMDGPU_GPU_PAGE_SIZE;
> + }
> +
> + amdgpu_bo_unreserve(root);
> + amdgpu_bo_unref(&root);
> +
> + return ib_refs;
> +}
That whole infrastructure is superflous. You just need to modify
amdgpu_vm_lock_by_pasid() to take a drm_exec object to lock the root BO.
Regards,
Christian.
> +
> +static void
> +amdgpu_devcoredump_release_ib_refs(struct amdgpu_devcoredump_ib_ref *ib_refs,
> + int num_ibs)
> +{
> + if (!ib_refs)
> + return;
> +
> + for (int i = 0; i < num_ibs; i++)
> + if (ib_refs[i].bo)
> + amdgpu_bo_unref(&ib_refs[i].bo);
> + kfree(ib_refs);
> +}
> +
> +/*
> + * Lock all collected IB BOs together using a single drm_exec ticket. This
> + * eliminates the nested ww_mutex acquire that lockdep flags as recursive
> + * locking (and that becomes a real self-deadlock for IB BOs sharing their
> + * dma_resv with the root PD).
> + *
> + * Returns 0 if drm_exec was initialised and the BOs are locked; the caller
> + * must call drm_exec_fini() on success. Returns non-zero on failure, in
> which
> + * case drm_exec is already torn down.
> + */
> +static int
> +amdgpu_devcoredump_lock_ib_refs(struct drm_exec *exec,
> + struct amdgpu_devcoredump_ib_ref *ib_refs,
> + int num_ibs)
> +{
> + int r = 0;
> +
> + drm_exec_init(exec, DRM_EXEC_IGNORE_DUPLICATES, num_ibs);
> + drm_exec_until_all_locked(exec) {
> + r = 0;
> + for (int i = 0; i < num_ibs; i++) {
> + if (!ib_refs[i].bo)
> + continue;
> + r = drm_exec_lock_obj(exec, &ib_refs[i].bo->tbo.base);
> + drm_exec_retry_on_contention(exec);
> + if (r)
> + break;
> + }
> + if (r)
> + break;
> + }
> + if (r)
> + drm_exec_fini(exec);
> + return r;
> +}
> +
> static ssize_t
> amdgpu_devcoredump_format(char *buffer, size_t count, struct
> amdgpu_coredump_info *coredump)
> {
> @@ -214,13 +317,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 +442,52 @@ 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_devcoredump_ib_ref *ib_refs = NULL;
> + struct drm_exec exec;
> + bool ibs_locked = false;
> +
> + /*
> + * Collect the BO that backs each IB under the root PD's
> + * reservation, drop the root reservation, then lock all the
> + * IB BOs together in one drm_exec ticket. This avoids nesting
> + * amdgpu_bo_reserve() inside the root PD's reservation, which
> + * would be a recursive reservation_ww_class_mutex acquire
> + * without a ww_acquire_ctx (lockdep splat, and a real
> + * self-deadlock for always-valid BOs that share their dma_resv
> + * with the root PD).
> + *
> + * Skip lookup/locking entirely on the sizing pass: it does not
> + * write IB content, and 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) {
> + ib_refs = amdgpu_devcoredump_collect_ib_refs(adev,
> coredump);
> + if (ib_refs) {
> + r = amdgpu_devcoredump_lock_ib_refs(&exec,
> ib_refs,
> +
> coredump->num_ibs);
> + if (!r)
> + ibs_locked = true;
> + }
> + }
> +
> + for (int i = 0; i < coredump->num_ibs; i++) {
> + struct amdgpu_bo *abo = ibs_locked ? ib_refs[i].bo :
> NULL;
> + u64 offset = ibs_locked ? ib_refs[i].offset : 0;
> + 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 (!abo)
> 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;
> -
> - 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;
> -
> 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 +499,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 +513,23 @@ 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 (ibs_locked)
> + drm_exec_fini(&exec);
> +
> + amdgpu_devcoredump_release_ib_refs(ib_refs, coredump->num_ibs);
> }
>
> return count - iter.remain;
