From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/amdgpu: replace PASID IDR with XArray Date: Tue, 31 Mar 2026 17:08:59 +1000 Message-ID: In-Reply-To: <20260330145049.21936-1-mikhail.v.gavrilov@gmail.com> References: <20260330145049.21936-1-mikhail.v.gavrilov@gmail.com> <20260330145049.21936-1-mikhail.v.gavrilov@gmail.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Commit message:** Excellent. Clearly describes both bugs, includes the de= adlock call chain, identifies the real-world trigger (RX 7900 XTX + Vulkan/= Proton), and explains why XArray fixes both issues. The version history is = thorough. Has appropriate Fixes/Cc-stable tags. **Code review:** The conversion is straightforward and correct: ```c static DEFINE_XARRAY_ALLOC(amdgpu_pasid_xa); static u32 amdgpu_pasid_xa_next; ``` `DEFINE_XARRAY_ALLOC` is the right macro for cyclic allocation use. The `u3= 2` next cursor is correct for `xa_alloc_cyclic()`. ```c r =3D xa_alloc_cyclic(&amdgpu_pasid_xa, &pasid, xa_mk_value(0), XA_LIMIT(1, (1U << bits) - 1), &amdgpu_pasid_xa_next, GFP_KERNEL); ``` This is correct. The limit upper bound is `(1U << bits) - 1`, which is righ= t =E2=80=94 the old IDR code used `1U << bits` as the exclusive end, while = `XA_LIMIT` takes an inclusive max. `xa_mk_value(0)` stores a tagged NULL-is= h value, which is fine since the entry just needs to be non-NULL to mark th= e PASID as allocated (XArray skips NULL entries in allocation). **Minor nit:** `xa_alloc_cyclic()` returns 1 when wrapping occurs and 0 on = normal success. The check `if (r >=3D 0)` handles both correctly. The retur= n-value restructuring (early return on success, fall through to return erro= r) is fine and arguably cleaner than the original. ```c xa_erase(&amdgpu_pasid_xa, pasid); ``` Correct =E2=80=94 `xa_erase()` uses `xa_lock_irq()` internally, so it's saf= e from hardirq context. This directly fixes the deadlock bug. ```c xa_destroy(&amdgpu_pasid_xa); ``` Correct =E2=80=94 no external locking needed, `xa_destroy()` handles it. **One minor observation:** The `bits` parameter could theoretically be 32, = in which case `1U << 32` is undefined behavior in C. However, this is not a= regression introduced by this patch =E2=80=94 the original IDR code had th= e same issue, and in practice `bits` is always 16 (from `amdgpu_vm_pasid_bi= ts`). Not something to block this patch on. **Reviewed-by worthy:** Yes. The patch is correct, well-documented, and a c= lear improvement over the buggy IDR+spinlock approach. --- Generated by Claude Code Patch Reviewer