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: Wed, 27 May 2026 14:42:32 +1000
Message-ID: <review-patch13-20260526145137.160554-14-robin.clark@oss.qualcomm.com>
In-Reply-To: <20260526145137.160554-14-robin.clark@oss.qualcomm.com>
References: <20260526145137.160554-1-robin.clark@oss.qualcomm.com> <20260526145137.160554-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 main patch. Several observations:

**Include style**: The new includes use quotes (`"linux/anon_inodes.h"`) rather than angle brackets (`<linux/anon_inodes.h>`). While this works (the compiler searches different paths), the MSM driver overwhelmingly uses angle brackets for kernel headers. This should probably be made consistent with the rest of the driver.

```c
+#include "drm/drm_file.h"
+#include "drm/msm_drm.h"
+#include "linux/anon_inodes.h"
+#include "linux/gfp_types.h"
+#include "linux/poll.h"
+#include "linux/slab.h"
```

**UAPI definition**: The ioctl is `DRM_IOW` (write-only from userspace perspective), and the fd is returned as the ioctl return value. The comment in the header explains why - if `DRM_IOWR` were used, a faulting `copy_to_user()` in `drm_ioctl()` would leak the fd. This is a correct and well-documented design choice.

**Stream read path**: `msm_perfcntrs_stream_read()` correctly handles both blocking and non-blocking modes. The `fifo_count_to_end()` macro uses `smp_load_acquire()` on the head, and `smp_store_release()` on the tail update, which is correct for the single-producer/single-consumer circ_buf pattern.

**Ioctl validation**: Thorough. Checks for invalid flags, group_stride, nr_groups, permission (perfmon_capable for stream), buffer size limits (128M max), and per-group validation including duplicate group detection. The `E2BIG` handling with `MSM_PERFCNTR_UPDATE` to return available counts is a nice touch.

**Potential issue - `nr_counters` zero-group detection**: The code uses `nr_counters[idx] = g.nr_countables + 1` with the comment `/* +1 to catch duplicate zero sized groups */`. This means a group with 0 countables sets `nr_counters[idx] = 1`, and the duplicate check `if (nr_counters[idx])` will catch it. At commit time, it subtracts the +1 back: `perfcntrs->groups[i]->allocated_counters = nr_counters[i] - 1`. This is correct but a bit subtle. Groups that are not mentioned at all have `nr_counters[i] = 0`, so subtracting 1 would underflow to `UINT8_MAX` (since `nr_counters` is `uint8_t*`). However, the loop only iterates over `gpu->num_perfcntr_groups`, and the subtract only happens for indices where `nr_counters[i]` was set. Wait - actually the commit loop iterates over ALL groups:

```c
for (unsigned i = 0; i < gpu->num_perfcntr_groups; i++)
    perfcntrs->groups[i]->allocated_counters = nr_counters[i] - 1;
```

This means for groups NOT mentioned in the ioctl, `nr_counters[i]` is 0, and `0 - 1` wraps to 255 for a `uint8_t`. But `allocated_counters` is likely a `uint32_t` or `unsigned`, so it would wrap to `UINT32_MAX`/`UINT_MAX`. This is a **bug** - groups not specified by the user would get `allocated_counters` set to a very large value.

**Wait** - looking more carefully, this only runs in the `MSM_PERFCNTR_STREAM` path, and there `nr_groups` is validated to be non-zero, but it's possible that fewer groups are specified than exist. The fix should probably be:

```c
for (unsigned i = 0; i < gpu->num_perfcntr_groups; i++)
    perfcntrs->groups[i]->allocated_counters = nr_counters[i] ? nr_counters[i] - 1 : 0;
```

**sel_worker / locking**: The comment about why `cancel_work_sync()` can't be used in the suspend path (lock ordering with `gpu->lock` and `perfcntr_lock`) is well-explained. Using async `cancel_work()` in suspend but `cancel_work_sync()` in release (after dropping `perfcntr_lock`) is correct.

**sample_worker runtime PM**: The sample worker expects the GPU to be powered (comment at line ~8097). The timer and kthread work are canceled in suspend. However, there's a subtle race: what if the timer fires and queues sample_work just before suspend cancels it? The `kthread_cancel_work_sync()` in suspend should handle this, since it waits for any in-flight execution to complete.

---
Generated by Claude Code Patch Reviewer