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 18:21:41 +1000
Message-ID: <review-patch13-20260522173349.55491-14-robin.clark@oss.qualcomm.com>
In-Reply-To: <20260522173349.55491-14-robin.clark@oss.qualcomm.com>
References: <20260522173349.55491-1-robin.clark@oss.qualcomm.com> <20260522173349.55491-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:

**Bug -- Missing `-EAGAIN` for `O_NONBLOCK` reads:**

```c
static ssize_t
msm_perfcntrs_stream_read(struct file *file, char __user *buf,
			  size_t count, loff_t *ppos)
{
	...
	if (!(file->f_flags & O_NONBLOCK)) {
		ret = wait_event_interruptible(stream->poll_wq,
					       fifo_count(stream) > 0);
		if (ret)
			return ret;
	}

	guard(mutex)(&stream->read_lock);

	struct circ_buf *fifo = &stream->fifo;
	const char *fptr = &fifo->buf[fifo->tail];

	count = min_t(size_t, count, fifo_count_to_end(stream));
	if (copy_to_user(buf, fptr, count))
		return -EFAULT;
	...
	return count;
```

When `O_NONBLOCK` is set and the FIFO is empty, `fifo_count_to_end` returns 0, `count` becomes 0, and the function returns 0. A return of 0 from `read()` signals EOF to userspace. For a streaming fd, this should return `-EAGAIN`. Standard pattern would be:

```c
count = min_t(size_t, count, fifo_count_to_end(stream));
if (!count)
    return -EAGAIN;
```

This affects any userspace using `poll()` + `read()` with `O_NONBLOCK`, which is the expected usage pattern for a perfcntr stream fd.

**Locking and work cancellation design** is well thought-out. The comment about why `cancel_work()` (non-blocking) is used for `sel_work` inside `perfcntr_lock`, followed by `cancel_work_sync()` after dropping the lock in the release path, is excellent documentation of a subtle design decision.

**`fifo_count_to_end` uses `smp_load_acquire`** on the head:
```c
#define fifo_count_to_end(stream) \
	(CIRC_CNT_TO_END(smp_load_acquire(&(stream)->fifo.head), ...))
```

This pairs with `smp_store_release(&stream->fifo.head, head)` in `sample_worker`. Correct for the single-producer/single-consumer pattern.

**UABI design**: Using `DRM_IOW` to return an fd via the ioctl return value (rather than a struct field) avoids the fd-leak-on-copy_to_user-fault problem. The comment documenting this is helpful.

**Duplicate group detection**:
```c
if (nr_counters[idx])
    return UERR(EINVAL, dev, "groups[%d]: duplicate group", i);
```

This correctly catches duplicate groups, but note that if a group is requested with `nr_countables == 0`, it sets `nr_counters[idx] = 0`. A second occurrence of the same group with 0 countables would not be caught. This is arguably harmless (0 counters allocated either way) but slightly inconsistent.

---
Generated by Claude Code Patch Reviewer