From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: accel/ivpu: Limit number of maximum contexts and doorbells per user Date: Fri, 27 Feb 2026 13:00:26 +1000 Message-ID: In-Reply-To: <20260225180638.316126-1-maciej.falkowski@linux.intel.com> References: <20260225180638.316126-1-maciej.falkowski@linux.intel.com> <20260225180638.316126-1-maciej.falkowski@linux.intel.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 **Off-by-one in SSID range (likely bug)** The SSID range is changed from `+ 63` to `+ 128`: ```c -#define IVPU_USER_CONTEXT_MAX_SSID (IVPU_USER_CONTEXT_MIN_SSID + 63) +#define IVPU_USER_CONTEXT_MAX_SSID (IVPU_USER_CONTEXT_MIN_SSID + 128) ``` With `IVPU_USER_CONTEXT_MIN_SSID =3D 2`, the new MAX is 130, and `ivpu_get_= context_count()` returns `130 - 2 + 1 =3D 129`. But the commit message stat= es root users get 128 contexts and non-root users get 64. Non-root gets `12= 9 / 2 =3D 64` (integer division), which happens to match, but root gets 129= , not 128. If the intent is 128 total contexts, this should be `+ 127`. If = 129 is intentional, the commit message should be updated. Given the v1=E2= =86=92v2 changelog says "Fixed off-by-one error", this warrants clarificati= on =E2=80=94 it's not clear the off-by-one was actually fixed rather than i= ntroduced. **Misuse of `kref_read()` for context counting** ```c + if (kref_read(&limits->ref) >=3D limits->max_ctx_count) { + ivpu_dbg(vdev, IOCTL, "User %u exceeded max ctx count %u\n", uid, + limits->max_ctx_count); + return ERR_PTR(-EMFILE); + } + + kref_get(&limits->ref); ``` Using `kref_read()` for application-level flow control is a kernel anti-pat= tern. `kref` is meant for object lifetime management, not as a general-purp= ose counter. The doorbell tracking already uses `atomic_t db_count` for thi= s exact purpose =E2=80=94 a parallel `atomic_t ctx_count` would be more con= sistent and idiomatic. `kref_read()` documentation explicitly warns it shou= ld only be used for debugging/logging, not for flow control decisions. **UID stored as `u32` instead of `kuid_t`** ```c +struct ivpu_user_limits { + ... + u32 uid; + ... +}; ``` The UID is obtained from `current_uid().val` and stored as a bare `u32`. In= the kernel, UIDs should be stored as `kuid_t` to maintain proper user name= space awareness. While `current_uid().val` gives the initial-namespace valu= e (so the root check `uid =3D=3D 0` is technically safe against namespace e= scapes), using `kuid_t` with helpers like `uid_eq(limits->uid, GLOBAL_ROOT_= UID)` would be the proper kernel idiom and more future-proof. **No hash table cleanup on device teardown** The `user_limits` hash table entries are allocated with `kzalloc` and freed= when their kref drops to zero. However, there is no explicit cleanup durin= g device removal (`ivpu_dev_fini` or equivalent). If any user_limits entrie= s are leaked (e.g., due to a bug in the teardown path), they become permane= nt memory leaks. Consider adding a hash table walk in the device fini path = that warns on and frees remaining entries. **Unrelated whitespace/style changes** Several cosmetic changes are mixed in with the functional change: ```c - .owner =3D THIS_MODULE, + .owner =3D THIS_MODULE, ``` ```c - { } + {} ``` ```c - ivpu_err(vdev, "Invalid NPU ready message: 0x%x\n", - ipc_hdr.data_addr); + ivpu_err(vdev, "Invalid NPU ready message: 0x%x\n", ipc_hdr.data_addr); ``` These should either be in a separate preparatory patch or dropped entirely.= Mixing cosmetic changes with functional changes makes the patch harder to = review and bisect. **`ivpu_cmdq_reset` doorbell tracking fix is good** ```c + if (cmdq->db_id) { xa_erase(&file_priv->vdev->db_xa, cmdq->db_id); + atomic_dec(&file_priv->user_limits->db_count); cmdq->db_id =3D 0; + } ``` This correctly guards against decrementing `db_count` for doorbells that we= ren't actually registered (db_id =3D=3D 0). Without this guard, the atomic = counter would underflow. Good catch. **Doorbell increment-then-check pattern** ```c + if (atomic_inc_return(&limits->db_count) > limits->max_db_count) { + ... + ret =3D -EBUSY; + goto err_dec_db_count; + } ``` This is correct but worth noting: between `atomic_inc_return` and the `atom= ic_dec` in the error path, the counter temporarily exceeds the limit. This = is a well-known pattern and acceptable here since other threads checking th= is value will also be doing their own increment-then-check. **`lockdep_assert_held` addition in `ivpu_cmdq_destroy` is good** ```c + lockdep_assert_held(&file_priv->lock); ``` This documents and enforces an existing locking requirement. Good improveme= nt. **Error message demotion to debug is appropriate** Changing `ivpu_err` to `ivpu_dbg` for doorbell and command queue allocation= failures is correct =E2=80=94 these are user-triggered conditions, and usi= ng `ivpu_err` could allow unprivileged users to flood `dmesg`. **`IVPU_NUM_PRIORITIES` / `IVPU_NUM_CMDQS_PER_CTX` removal** These defines are removed but appear to be unused (only defined in the head= er, never referenced). This is fine dead-code cleanup, though it's unrelate= d to the per-user limits feature and could be a separate patch. **Summary**: The approach is reasonable and addresses a real resource exhau= stion concern. The main issues to address are: (1) the off-by-one discrepan= cy between code and commit message in the SSID range, (2) replacing `kref_r= ead()` flow control with a proper atomic counter, (3) using `kuid_t` instea= d of bare `u32` for the UID, and (4) splitting out unrelated whitespace cha= nges. --- Generated by Claude Code Patch Reviewer