Claude review: drm/qxl: Convert qxl release idr to xarray

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

Patch Review

**Bug: seqno assigned after xa_alloc but before error check**

In `qxl_release_alloc()`, the seqno is assigned *between* `xa_alloc` and the error check:

```c
r = xa_alloc(&qdev->release_xa, &handle, release, xa_limit_31b, GFP_KERNEL);
release->base.seqno = (u32)atomic_inc_return(&qdev->release_seqno);
if (r < 0) {
    kfree(release);
    *ret = NULL;
    return r;
}
```

If `xa_alloc` fails (`r < 0`), the code still increments `release_seqno` before taking the error path and freeing `release`. This wastes a sequence number on every allocation failure. The original IDR code had the same logical issue (seqno incremented inside the spinlock before the error check), so this is a pre-existing problem being preserved, but the conversion to `atomic_inc_return` makes it slightly more visible. Moving the seqno assignment after the error check would be cleaner:

```c
r = xa_alloc(&qdev->release_xa, &handle, release, xa_limit_31b, GFP_KERNEL);
if (r < 0) {
    kfree(release);
    *ret = NULL;
    return r;
}
release->base.seqno = (u32)atomic_inc_return(&qdev->release_seqno);
```

This is not a regression since the original code had the same behavior, but since you're already refactoring, it would be a nice cleanup.

**Unnecessary explicit locking in `qxl_release_from_id_locked`**

```c
xa_lock(&qdev->release_xa);
release = xa_load(&qdev->release_xa, id);
xa_unlock(&qdev->release_xa);
```

`xa_load()` is already RCU-safe and internally handles its own read-side locking via `rcu_read_lock()`. The explicit `xa_lock`/`xa_unlock` around `xa_load` is unnecessary — `xa_load` does not require the xa_lock to be held. This could simply be:

```c
release = xa_load(&qdev->release_xa, id);
```

The returned pointer remains valid because the caller holds the release via reference or the garbage collector is serialized. Using `xa_load` alone is both correct and more performant (RCU read-side vs. spinlock).

**Missing `xa_destroy` in teardown path**

Looking at `qxl_device_fini()`, there is no `idr_destroy()` call for `release_idr` in the existing code, and likewise the patch does not add an `xa_destroy()` call for `release_xa`. While xarray (like IDR) will not leak the entries themselves (those are freed via `qxl_release_free`), calling `xa_destroy(&qdev->release_xa)` in `qxl_device_fini()` would free any internal xarray nodes and is good practice. This is a pre-existing issue but worth fixing while converting.

**Minor: variable rename churn**

The `idr_ret` → `xa_ret` renames in `qxl_alloc_surface_release_reserved` and `qxl_alloc_release_reserved` are fine but purely cosmetic. The variable could equally be just `ret` or `r` to be more generic, but this is a style nit.

**Good aspects:**
- `xa_limit_31b` correctly keeps IDs within `INT_MAX`, matching the old `idr_alloc(..., 1, 0, ...)` range (v2 fix).
- `XA_FLAGS_ALLOC1` correctly mirrors `idr_init_base(&..., 1)`, starting allocation from 1.
- `GFP_KERNEL` is appropriate since the callers are in process context (v2 fix).
- The `(u32)` cast on `atomic_inc_return` prevents sign-extension issues (v2 fix).
- `xa_erase` in `qxl_release_free` is a clean 1:1 replacement for the locked `idr_remove`.

---
Generated by Claude Code Patch Reviewer