From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/amdgpu: replace PASID IDR with XArray
Date: Wed, 01 Apr 2026 07:48:28 +1000
Message-ID: <review-patch1-20260331111808.16578-1-mikhail.v.gavrilov@gmail.com>
In-Reply-To: <20260331111808.16578-1-mikhail.v.gavrilov@gmail.com>
References: <20260331111808.16578-1-mikhail.v.gavrilov@gmail.com> <20260331111808.16578-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

**Critical bug =E2=80=94 hardirq deadlock not fixed:**

The core issue is that `amdgpu_pasid_free()` is called from hardirq context=
 (via `amdgpu_pasid_free_cb` =E2=86=92 fence signal path), and `amdgpu_pasi=
d_alloc()` is called from process context. Both must use IRQ-disabling lock=
ing to prevent deadlock.

```c
xa_erase(&amdgpu_pasid_xa, pasid);
```

`xa_erase()` is defined in `lib/xarray.c` as:
```c
void *xa_erase(struct xarray *xa, unsigned long index)
{
	xa_lock(xa);        /* =3D spin_lock() -- IRQs NOT disabled */
	entry =3D __xa_erase(xa, index);
	xa_unlock(xa);      /* =3D spin_unlock() */
	return entry;
}
```

Similarly, `xa_alloc_cyclic()` in `include/linux/xarray.h`:
```c
static inline int xa_alloc_cyclic(...)
{
	xa_lock(xa);        /* =3D spin_lock() -- IRQs NOT disabled */
	err =3D __xa_alloc_cyclic(...);
	xa_unlock(xa);
	return err < 0 ? err : 0;
}
```

The `XA_FLAGS_LOCK_IRQ` flag only affects `xas_lock_type()`/`xas_unlock_typ=
e()` used internally by `__xas_nomem()` when the XArray needs to drop the l=
ock for memory allocation. It has **no effect** on the outer locking in `xa=
_erase()` or `xa_alloc_cyclic()`.

The XArray API provides dedicated IRQ-safe wrappers for exactly this situat=
ion:
- `xa_erase_irq()` =E2=80=94 uses `xa_lock_irq()`/`xa_unlock_irq()`
- `xa_alloc_cyclic_irq()` =E2=80=94 uses `xa_lock_irq()`/`xa_unlock_irq()`

**However**, `xa_erase_irq()` uses `spin_unlock_irq()` which unconditionall=
y re-enables interrupts. Calling this from hardirq context would re-enable =
interrupts inside the hardirq handler, which is also wrong.

**The correct fix for `amdgpu_pasid_free()`** (callable from any context) s=
hould use `xa_lock_irqsave`/`xa_unlock_irqrestore` as v5 proposed:

```c
void amdgpu_pasid_free(u32 pasid)
{
	unsigned long flags;

	trace_amdgpu_pasid_freed(pasid);

	xa_lock_irqsave(&amdgpu_pasid_xa, flags);
	__xa_erase(&amdgpu_pasid_xa, pasid);
	xa_unlock_irqrestore(&amdgpu_pasid_xa, flags);
}
```

For `amdgpu_pasid_alloc()` (process context only), `xa_alloc_cyclic_irq()` =
would work correctly, or alternatively the same `irqsave` pattern.

**Other observations (minor, all correct):**

- `DEFINE_XARRAY_FLAGS(amdgpu_pasid_xa, XA_FLAGS_LOCK_IRQ | XA_FLAGS_ALLOC1=
)` =E2=80=94 using `XA_FLAGS_ALLOC1` to prevent PASID 0 allocation is appro=
priate, and makes the explicit `XA_LIMIT(1, ...)` minimum redundant but har=
mless.

- The return value handling is correct: `xa_alloc_cyclic()` returns 0 on su=
ccess (squashing the wrap indicator), stores the ID in `&pasid`, and the co=
de correctly checks `r >=3D 0` then returns `pasid`.

- Storing `xa_mk_value(0)` as the entry is fine =E2=80=94 XArray needs a no=
n-NULL entry for the slot to be considered allocated, and `xa_mk_value(0)` =
serves this purpose since the original IDR code stored `NULL` via `idr_allo=
c_cyclic(..., NULL, ...)`.

- The `xa_destroy()` call in `amdgpu_pasid_mgr_cleanup()` is correct and do=
es not need external locking.

- Dropping `Cc: stable` is correct since the regression (commit 8f1de51f49b=
e) hasn't reached a stable kernel.

**Recommendation: NAK in current form.** The patch must use IRQ-safe lockin=
g wrappers. The simplest correct approach would be to use `xa_alloc_cyclic_=
irq()` for allocation and `xa_lock_irqsave`/`__xa_erase`/`xa_unlock_irqrest=
ore` for free, as the v5 approach partially identified.

---
Generated by Claude Code Patch Reviewer