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: Tue, 03 Mar 2026 12:50:56 +1000 Message-ID: In-Reply-To: <20260302202207.469442-1-maciej.falkowski@linux.intel.com> References: <20260302202207.469442-1-maciej.falkowski@linux.intel.com> <20260302202207.469442-1-maciej.falkowski@linux.intel.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review **Off-by-one in IVPU_USER_CONTEXT_MAX_SSID (medium)** The commit message states "Root users can allocate all available contexts (128)" but the new define yields 129 contexts: ```c #define IVPU_USER_CONTEXT_MAX_SSID (IVPU_USER_CONTEXT_MIN_SSID + 128) ``` With `IVPU_USER_CONTEXT_MIN_SSID = 2`, MAX_SSID becomes 130. Then `ivpu_get_context_count()` returns `130 - 2 + 1 = 129`. The original code had `+ 63` which gave exactly 64 contexts. To get exactly 128, this should be `+ 127`. The v1->v2 changelog says "Fixed off-by-one error" but it appears the fix was applied in the wrong direction, or the commit message is wrong about the intended count. **UID==0 check instead of capability check (medium)** ```c if (uid == 0) { limits->max_ctx_count = ivpu_get_context_count(vdev); limits->max_db_count = ivpu_get_doorbell_count(vdev); } ``` Direct UID comparisons are discouraged in the kernel. This bypasses the `kuid_t` type-safety system and doesn't integrate with capability-based access control. The standard kernel pattern would be to use `capable(CAP_SYS_ADMIN)` (or a more specific capability) at the point where limits are being set, or at minimum `uid_eq(current_uid(), GLOBAL_ROOT_UID)`. Using `capable()` would also correctly handle containerized workloads. Additionally, the UID is captured at allocation time and cached in the `ivpu_user_limits` struct. If `capable()` were used instead, this check would need to happen at lookup time rather than allocation time, since different processes with the same UID could have different capabilities. **Using kref_read() for control-flow decisions (minor)** ```c if (kref_read(&limits->ref) >= 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_read()` is documented as being primarily for debugging purposes, not for making control-flow decisions. While the mutex protection makes this functionally safe, a dedicated `u32 ctx_count` counter (mirroring the `atomic_t db_count` pattern) would be more idiomatic and consistent within the patch itself. It's also somewhat confusing that context counting uses kref while doorbell counting uses an atomic - the two resources are tracked with different mechanisms for no obvious reason. **UAPI behavior change (medium)** ```c case DRM_IVPU_PARAM_NUM_CONTEXTS: - args->value = ivpu_get_context_count(vdev); + args->value = file_priv->user_limits->max_ctx_count; ``` This changes what `DRM_IVPU_PARAM_NUM_CONTEXTS` reports from the total device context count to the per-user maximum. Existing userspace that queries this parameter will now see a different (potentially halved) value. This could break applications that use this value for sizing or configuration. If this is intentional, it should be explicitly noted in the commit message as a UAPI behavior change. Consider whether a separate parameter should be added instead. **Unrelated formatting changes mixed in (minor)** The patch includes several unrelated whitespace changes: - `ivpu_wait_for_ready`: line-wrapping change for `ivpu_err` - `ivpu_fops`: `.owner\t\t= THIS_MODULE` changed to `.owner = THIS_MODULE` - `ivpu_pci_ids`: `{ }` changed to `{}` These should be in a separate cleanup patch to keep the functional change reviewable. **No doorbell limit reported to userspace (minor)** While the context limit is now exposed via `DRM_IVPU_PARAM_NUM_CONTEXTS`, there is no corresponding mechanism for userspace to query its doorbell limit. If userspace needs to know how many doorbells it can allocate, there's no way to discover this. **lockdep_assert_held added to ivpu_cmdq_destroy (good)** ```c static void ivpu_cmdq_destroy(struct ivpu_file_priv *file_priv, struct ivpu_cmdq *cmdq) { + lockdep_assert_held(&file_priv->lock); ``` Good addition - all callers already hold `file_priv->lock`, and this makes the locking requirement explicit. **ivpu_cmdq_reset doorbell check (good)** ```c xa_for_each(&file_priv->cmdq_xa, cmdq_id, cmdq) { - xa_erase(&file_priv->vdev->db_xa, cmdq->db_id); - cmdq->db_id = 0; + 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 = 0; + } } ``` The added `if (cmdq->db_id)` guard is a correct improvement - the original code would have called `xa_erase` with id 0 for unregistered command queues, which is harmless but wasteful. With the new atomic decrement, the guard is now necessary to avoid decrementing below zero. **Removal of unused macros (fine)** `IVPU_NUM_PRIORITIES` and `IVPU_NUM_CMDQS_PER_CTX` are defined but never referenced anywhere in the codebase, so removing them is correct, though it would be cleaner in a separate patch. --- Generated by Claude Code Patch Reviewer