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/qxl: Convert qxl release idr to xarray Date: Thu, 23 Apr 2026 08:54:44 +1000 Message-ID: In-Reply-To: <20260421060023.96618-1-liuqiangneo@163.com> References: <20260421060023.96618-1-liuqiangneo@163.com> <20260421060023.96618-1-liuqiangneo@163.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 **Bug: seqno assigned after xa_alloc but before error check** In `qxl_release_alloc()`, the seqno is assigned *between* `xa_alloc` and th= e error check: ```c r =3D xa_alloc(&qdev->release_xa, &handle, release, xa_limit_31b, GFP_KERNE= L); release->base.seqno =3D (u32)atomic_inc_return(&qdev->release_seqno); if (r < 0) { kfree(release); *ret =3D NULL; return r; } ``` If `xa_alloc` fails (`r < 0`), the code still increments `release_seqno` be= fore taking the error path and freeing `release`. This wastes a sequence nu= mber on every allocation failure. The original IDR code had the same logica= l issue (seqno incremented inside the spinlock before the error check), so = this is a pre-existing problem being preserved, but the conversion to `atom= ic_inc_return` makes it slightly more visible. Moving the seqno assignment = after the error check would be cleaner: ```c r =3D xa_alloc(&qdev->release_xa, &handle, release, xa_limit_31b, GFP_KERNE= L); if (r < 0) { kfree(release); *ret =3D NULL; return r; } release->base.seqno =3D (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 =3D xa_load(&qdev->release_xa, id); xa_unlock(&qdev->release_xa); ``` `xa_load()` is already RCU-safe and internally handles its own read-side lo= cking via `rcu_read_lock()`. The explicit `xa_lock`/`xa_unlock` around `xa_= load` is unnecessary =E2=80=94 `xa_load` does not require the xa_lock to be= held. This could simply be: ```c release =3D 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 `relea= se_idr` in the existing code, and likewise the patch does not add an `xa_de= stroy()` call for `release_xa`. While xarray (like IDR) will not leak the e= ntries themselves (those are freed via `qxl_release_free`), calling `xa_des= troy(&qdev->release_xa)` in `qxl_device_fini()` would free any internal xar= ray nodes and is good practice. This is a pre-existing issue but worth fixi= ng while converting. **Minor: variable rename churn** The `idr_ret` =E2=86=92 `xa_ret` renames in `qxl_alloc_surface_release_rese= rved` and `qxl_alloc_release_reserved` are fine but purely cosmetic. The va= riable 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 `id= r_alloc(..., 1, 0, ...)` range (v2 fix). - `XA_FLAGS_ALLOC1` correctly mirrors `idr_init_base(&..., 1)`, starting al= location 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 (v= 2 fix). - `xa_erase` in `qxl_release_free` is a clean 1:1 replacement for the locke= d `idr_remove`. --- Generated by Claude Code Patch Reviewer