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 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 = 2`, the new MAX is 130, and `ivpu_get_context_count()` returns `130 - 2 + 1 = 129`. But the commit message states root users get 128 contexts and non-root users get 64. Non-root gets `129 / 2 = 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→v2 changelog says "Fixed off-by-one error", this warrants clarification — it's not clear the off-by-one was actually fixed rather than introduced.

**Misuse of `kref_read()` for context counting**

```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_get(&limits->ref);
```

Using `kref_read()` for application-level flow control is a kernel anti-pattern. `kref` is meant for object lifetime management, not as a general-purpose counter. The doorbell tracking already uses `atomic_t db_count` for this exact purpose — a parallel `atomic_t ctx_count` would be more consistent and idiomatic. `kref_read()` documentation explicitly warns it should 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 namespace awareness. While `current_uid().val` gives the initial-namespace value (so the root check `uid == 0` is technically safe against namespace escapes), 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 during device removal (`ivpu_dev_fini` or equivalent). If any user_limits entries are leaked (e.g., due to a bug in the teardown path), they become permanent 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		= THIS_MODULE,
+	.owner = 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 = 0;
+		}
```

This correctly guards against decrementing `db_count` for doorbells that weren't actually registered (db_id == 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 = -EBUSY;
+		goto err_dec_db_count;
+	}
```

This is correct but worth noting: between `atomic_inc_return` and the `atomic_dec` in the error path, the counter temporarily exceeds the limit. This is a well-known pattern and acceptable here since other threads checking this 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 improvement.

**Error message demotion to debug is appropriate**

Changing `ivpu_err` to `ivpu_dbg` for doorbell and command queue allocation failures is correct — these are user-triggered conditions, and using `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 header, never referenced). This is fine dead-code cleanup, though it's unrelated to the per-user limits feature and could be a separate patch.

**Summary**: The approach is reasonable and addresses a real resource exhaustion concern. The main issues to address are: (1) the off-by-one discrepancy between code and commit message in the SSID range, (2) replacing `kref_read()` flow control with a proper atomic counter, (3) using `kuid_t` instead of bare `u32` for the UID, and (4) splitting out unrelated whitespace changes.

---
Generated by Claude Code Patch Reviewer