From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/msm: Add PERFCNTR_CONFIG ioctl
Date: Sat, 16 May 2026 10:50:10 +1000
Message-ID: <review-patch13-20260514134052.361771-14-robin.clark@oss.qualcomm.com>
In-Reply-To: <20260514134052.361771-14-robin.clark@oss.qualcomm.com>
References: <20260514134052.361771-1-robin.clark@oss.qualcomm.com> <20260514134052.361771-14-robin.clark@oss.qualcomm.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

This is the core ioctl implementation. Several observations:

**Issue: Global counter state modified before validation complete.** In the ioctl, when `MSM_PERFCNTR_STREAM` is set, the code modifies `perfcntrs->groups[idx]->allocated_counters` inside the group loop:
```c
perfcntrs->groups[idx]->allocated_counters = g.nr_countables;
```
But if a later iteration returns an error (e.g., `copy_from_user` fails for countables, or a later group is invalid), the already-modified `allocated_counters` values are not rolled back. This leaves the global perfcntr state corrupted on error. The `__free(kfree)` on `stream` prevents the stream from being installed, but the group state has been partially modified.

**Issue: `get_group_idx` string comparison.** The comparison uses:
```c
if (!strncmp(group->name, name, len))
```
where `len` is `sizeof(g.group_name)` (16). But `group->name` could be shorter than the user-supplied name, in which case `strncmp` would match a prefix. For example, if `group->name` is `"CP"` and the user passes `"CP\0\0\0\0..."`, it works. But if the user passes `"CPFOO\0..."`, `strncmp` would compare the first 16 chars and find a mismatch at index 2. So this is actually fine since both are null-terminated within the 16-byte window. No real bug, but `strcmp` on the kernel-side name against the fixed-size user buffer would be clearer.

**Issue: `msm_perfcntrs_stream_read` does not handle wrap-around.** The read function only reads `fifo_count_to_end()` bytes in a single call:
```c
count = min_t(size_t, count, fifo_count_to_end(stream));
```
If data wraps around the circular buffer, the caller would need multiple `read()` calls to get all available data. This is technically correct (partial reads are allowed), but could surprise userspace if the sample period straddles the wrap point. Most kernel circ_buf readers handle this with a two-phase copy. Not a bug per se, but worth noting.

**Issue: `bufsz_shift` validation.** There's a check that `fifo_size > SZ_128M`, but no lower bound on `bufsz_shift`. A `bufsz_shift` of 0 would give `fifo_size = 1`, which is smaller than the period_size and would be rejected. But `bufsz_shift` of 1 gives `fifo_size = 2`, and a valid period_size check would also catch this. So the validation works transitively through the `fifo_size <= bufsz` check. Fine.

**Observation: UABI fd return.** The ioctl is `DRM_IOW` (write-only from userspace perspective), and the stream fd is returned as the ioctl return value. The comment in the code explains why this is intentional (avoids `copy_to_user` faulting and leaking the fd). This is a reasonable design choice.

**Minor: `#include` style.** The includes use `"drm/drm_file.h"` and `"linux/anon_inodes.h"` with quotes instead of angle brackets. While this works, kernel convention is `<linux/...>` for system headers.

---
Generated by Claude Code Patch Reviewer