Claude review: drm/msm: Add PERFCNTR_CONFIG ioctl

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

Patch Review

This is the core patch. Several observations:

**Issue: No upper bound on `bufsz_shift`.**

```c
stream->fifo_size = 1 << args->bufsz_shift;
...
void *buf = kmalloc(1 << args->bufsz_shift, GFP_KERNEL);
```

A malicious/buggy user could pass `bufsz_shift = 30` and try to allocate 1 GB. The ioctl requires `perfmon_capable()` for STREAM mode so this is somewhat mitigated, but a bounds check would be prudent. Also, if `bufsz_shift` is 0 and STREAM is not set, `fifo_size` would be 1 which is odd but harmless since the FIFO is never used.

**Issue: `guard(pm_runtime_active_auto)` does not check return value.**

```c
guard(pm_runtime_active_auto)(&gpu->pdev->dev);
```

This expands to `pm_runtime_get_sync()` which can fail. The guard pattern doesn't allow checking the return value. If the device fails to resume, the ioctl will proceed with a powered-off GPU. Consider using an explicit `pm_runtime_get_sync()` with error checking, or at minimum documenting why this is acceptable.

**Good: UAPI design.** The `DRM_IOW` direction is correctly chosen — the comment explains that returning the fd as a return value (rather than via an output parameter) avoids the `copy_to_user` fault-after-fd-creation problem. The `MSM_PERFCNTR_UPDATE` flag for returning available counter counts on `E2BIG` is a nice touch for discoverability.

**Good: Locking discipline.** The `perfcntr_lock` protects counter allocation state, `read_lock` protects FIFO consumer side, and the single-producer pattern (kthread worker) avoids needing locks on the producer side. The `sel_work` is correctly dispatched on the scheduler's submit_wq to serialize with GEM_SUBMITs.

**Minor: `UERR(ETOOSMALL, ...)`.** Is `ETOOSMALL` a standard errno? I believe the correct name is `ETOOMANYREFS` or similar — this may be a custom MSM driver macro. If `UERR` handles non-standard names, fine; otherwise this should be `EINVAL` or a more standard error.

**Minor: `group_stride` validation.** The stride is validated to be non-zero when `nr_groups > 0`, but there's no check that `group_stride >= sizeof(struct drm_msm_perfcntr_group)`. A too-small stride would cause `copy_from_user` to read partial data. However, since `copy_from_user` uses `args->group_stride` as the size, a smaller stride would just read fewer fields — the zero-initialization of `g` mitigates this.

**Minor: `scoped_guard` with `break`.** In `sel_worker`:

```c
scoped_guard (mutex, &gpu->lock) {
	guard(mutex)(&gpu->perfcntr_lock);
	if (stream != gpu->perfcntrs->stream)
		break;
	...
}
```

Using `break` to exit a `scoped_guard` block is valid but unusual — it works because `scoped_guard` expands to a loop-like construct. This is fine but may surprise readers.

---
Generated by Claude Code Patch Reviewer