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: Mon, 25 May 2026 21:26:30 +1000
Message-ID: <review-patch13-20260520162454.18391-14-robin.clark@oss.qualcomm.com>
In-Reply-To: <20260520162454.18391-14-robin.clark@oss.qualcomm.com>
References: <20260520162454.18391-1-robin.clark@oss.qualcomm.com> <20260520162454.18391-14-robin.clark@oss.qualcomm.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

This is the main ioctl patch. Several observations:

**Include style**: The includes use `"linux/anon_inodes.h"` (quotes) rather=
 than `<linux/anon_inodes.h>` (angle brackets). Kernel convention is angle =
brackets for kernel headers:
```c
+#include "linux/anon_inodes.h"
+#include "linux/gfp_types.h"
+#include "linux/poll.h"
+#include "linux/slab.h"
```
Should be `<linux/...>`.

**`copy_from_user` with `group_stride`**: The ioctl copies `args->group_str=
ide` bytes from each user group entry:
```c
if (copy_from_user(&g, userptr, args->group_stride))
```
There's no validation that `args->group_stride <=3D sizeof(struct drm_msm_p=
erfcntr_group)`. If userspace passes a `group_stride` larger than the kerne=
l's struct size, this overflows the stack-allocated `g`. The stride should =
be clamped to `min(args->group_stride, sizeof(g))`.

**`bufsz_shift` validation**: `stream->fifo_size =3D 1 << args->bufsz_shift=
`. If `bufsz_shift` is very large (e.g., 63), this is undefined behavior du=
e to left-shifting past the type width. There's a later check `stream->fifo=
_size > SZ_128M`, but the UB happens first. Should validate `bufsz_shift` i=
s reasonable (e.g., `<=3D 27` for 128M) before the shift.

**Non-blocking read returns 0**: In `msm_perfcntrs_stream_read()`, when `O_=
NONBLOCK` is set and the FIFO is empty, `fifo_count_to_end()` returns 0, so=
 `count` becomes 0, `copy_to_user` copies nothing, and the function returns=
 0. A non-blocking read on an empty fd should return `-EAGAIN`, not 0. Retu=
rning 0 signals EOF to userspace.

**`fifo_space` in `sample_worker`**: The `fifo_space()` macro reads `stream=
->fifo.tail` without `smp_load_acquire`. This is the producer reading the c=
onsumer's tail =E2=80=94 per the kernel circ_buf documentation, this should=
 use `READ_ONCE()` at minimum (the macro uses `CIRC_SPACE` which doesn't in=
clude barriers). In practice, this likely works because the kthread worker =
is the only producer and the mutex-protected read path is the only consumer=
, but it would be more correct to use `READ_ONCE` on the tail read.

**Counter allocation not reverted on error**: In the ioctl, when processing=
 groups with `MSM_PERFCNTR_STREAM`, the code sets `perfcntrs->groups[idx]->=
allocated_counters` and copies countables. If a later group fails validatio=
n (e.g., `-E2BIG` or `-EFAULT` from a subsequent `copy_from_user`), these a=
lready-modified group states are not rolled back. This could leave stale `a=
llocated_counters` values since the `perfcntrs->groups[]` state is persiste=
nt (pre-allocated).

**UAPI comment typo**: "The data read from the has the following format" =
=E2=80=94 missing noun after "from the".

---
Generated by Claude Code Patch Reviewer