Claude review: accel/ivpu: Limit number of maximum contexts and doorbells per user

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

Patch Review

**Off-by-one in context limit check**

> +	hash_for_each_possible(vdev->user_limits, limits, hash_node, uid) {
> +		if (limits->uid == uid) {
> +			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_get(&limits->ref);
> +			return limits;
> +		}
> +	}
> +
> +	return ivpu_user_limits_alloc(vdev, uid);

The first context for a user goes through `ivpu_user_limits_alloc()`, which calls `kref_init()` (setting the refcount to 1) and returns without hitting the limit check. Every subsequent open hits the `hash_for_each_possible` path and checks `kref_read() > max_ctx_count` *before* calling `kref_get()`. So when `max_ctx_count` contexts have already been allocated, `kref_read()` returns exactly `max_ctx_count`, the condition `> max_ctx_count` is false, and one more context is permitted. This allows `max_ctx_count + 1` contexts in total. The check should use `>=` instead of `>`.

**SSID range produces 129 contexts, not 128**

> -#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` being 2, this gives `MAX_SSID = 130`. The `ivpu_get_context_count()` function returns `max - min + 1 = 130 - 2 + 1 = 129`. The commit message says root gets 128 contexts and non-root gets 64, but root actually gets 129 and non-root gets `129 / 2 = 64` (integer truncation). Combined with the off-by-one above, root can actually get 130 contexts. Was the intent to use `+ 127` here to get exactly 128 contexts?

**Raw uid comparison instead of proper credential check**

> +	if (uid == 0) {
> +		limits->max_ctx_count = ivpu_get_context_count(vdev);
> +		limits->max_db_count = ivpu_get_doorbell_count(vdev);

Comparing `uid == 0` directly ignores user namespaces. In a container where a process has uid 0 within its namespace but maps to a non-zero uid in the init namespace, `current_uid().val` will return the namespaced uid. Depending on intent, this could be the wrong check. The more standard approach for privilege checks in kernel code is `capable(CAP_SYS_ADMIN)` or `uid_eq(current_uid(), GLOBAL_ROOT_UID)`. If the intent is specifically "init namespace root", `uid_eq` with `GLOBAL_ROOT_UID` is the proper idiom.

**Atomic increment-then-check pattern in doorbell limit**

> +	if (atomic_inc_return(&limits->db_count) > limits->max_db_count) {
> +		ivpu_dbg(vdev, IOCTL, "Maximum number of %u doorbells for uid %u reached\n",
> +			 limits->max_db_count, limits->uid);
> +		ret = -EBUSY;
> +		goto err_dec_db_count;
> +	}

This has the same off-by-one flavor as the context check. If `max_db_count` is 127, then `atomic_inc_return` returning 128 causes the check `128 > 127` to fire, so only 127 doorbells are permitted. But for `max_db_count = 255` (root), doorbell IDs only range from 1 to 255 (255 entries), so `xa_alloc_cyclic` would fail at 256 anyway. The limit is consistent here since the increment happens first, so 255 increments means the 255th returns 255, which is not > 255, so it's allowed. This is fine.

**Removal of IVPU_NUM_PRIORITIES and IVPU_NUM_CMDQS_PER_CTX**

> -#define IVPU_NUM_PRIORITIES    4
> -#define IVPU_NUM_CMDQS_PER_CTX (IVPU_NUM_PRIORITIES)

These macros are removed. The grep confirms they have no remaining users in the tree, so this is fine. Though the removal is unrelated to the stated purpose of this patch and isn't mentioned in the commit message.

**UAPI change for DRM_IVPU_PARAM_NUM_CONTEXTS**

> -		args->value = ivpu_get_context_count(vdev);
> +		args->value = file_priv->user_limits->max_ctx_count;

This changes the semantics of the `DRM_IVPU_PARAM_NUM_CONTEXTS` ioctl from returning the total system context count to returning the per-user maximum. This is a UAPI behavioral change -- existing userspace that queries this value may get a different (smaller) number than before. This may or may not be the right thing to do, but it's worth calling out that this is a change in the user-visible API contract. Userspace that previously saw 64 will now see 64 (for non-root, coincidentally the same here), but for root it will see 129 instead of the old 64. If the SSID range is corrected to give 128 total, non-root would see 64 and root would see 128.

**Unrelated whitespace changes**

> -		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);

> -	.owner		= THIS_MODULE,
> +	.owner = THIS_MODULE,

> -	{ }
> +	{}

These are cosmetic whitespace/style changes unrelated to the feature. They add noise to the patch and make it harder to review and bisect.

**lockdep_assert_held addition in ivpu_cmdq_destroy**

> +	lockdep_assert_held(&file_priv->lock);

This is a good addition. All callers (`ivpu_cmdq_release_all_locked` and the `cmdq_manage_ioctl` destroy path) do hold `file_priv->lock`, confirmed by the existing `lockdep_assert_held` in `ivpu_cmdq_release_all_locked` and the `guard(mutex)` in the ioctl path.

**ivpu_cmdq_reset correctly guards against double-decrement**

> +		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;
> +		}

This is a good change. The original code would call `xa_erase` with `db_id = 0` which is harmless for xarray but sloppy. The new code properly skips already-unregistered queues and avoids decrementing `db_count` for queues that never registered a doorbell.

**No cleanup of user_limits hashtable on device teardown**

There is no code to walk and free remaining `ivpu_user_limits` entries from the hashtable during device removal. If all file_priv instances are properly closed before device removal, the kref would drop to zero and entries would be freed. But if any path leaks a file_priv reference, the hash entries would leak. The `drmm_mutex_init` for the lock will destroy the mutex on device finalization, but the hash entries themselves are not freed as part of any managed cleanup. This is likely acceptable if the driver guarantees all file_priv references are dropped, but worth considering whether a `hash_for_each_safe` cleanup pass is warranted.

---
Generated by Claude Code Patch Reviewer