Claude review: drm/amdgpu: replace PASID IDR with XArray

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Critical bug — hardirq deadlock not fixed:**

The core issue is that `amdgpu_pasid_free()` is called from hardirq context (via `amdgpu_pasid_free_cb` → fence signal path), and `amdgpu_pasid_alloc()` is called from process context. Both must use IRQ-disabling locking 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);        /* = spin_lock() -- IRQs NOT disabled */
	entry = __xa_erase(xa, index);
	xa_unlock(xa);      /* = spin_unlock() */
	return entry;
}
```

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

The `XA_FLAGS_LOCK_IRQ` flag only affects `xas_lock_type()`/`xas_unlock_type()` used internally by `__xas_nomem()` when the XArray needs to drop the lock 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 situation:
- `xa_erase_irq()` — uses `xa_lock_irq()`/`xa_unlock_irq()`
- `xa_alloc_cyclic_irq()` — uses `xa_lock_irq()`/`xa_unlock_irq()`

**However**, `xa_erase_irq()` uses `spin_unlock_irq()` which unconditionally 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) should 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)` — using `XA_FLAGS_ALLOC1` to prevent PASID 0 allocation is appropriate, and makes the explicit `XA_LIMIT(1, ...)` minimum redundant but harmless.

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

- Storing `xa_mk_value(0)` as the entry is fine — XArray needs a non-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_alloc_cyclic(..., NULL, ...)`.

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

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

**Recommendation: NAK in current form.** The patch must use IRQ-safe locking wrappers. The simplest correct approach would be to use `xa_alloc_cyclic_irq()` for allocation and `xa_lock_irqsave`/`__xa_erase`/`xa_unlock_irqrestore` for free, as the v5 approach partially identified.

---
Generated by Claude Code Patch Reviewer