Claude review: accel: ethosu: Add performance counter support

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

Patch Review

**UAPI Issues:**

1. **Breaking ABI change by repurposing `pad` field in `drm_ethosu_submit`**: The patch renames the `pad` field to `perfmon_id` and removes the validation that `pad == 0`. This is an ABI break - existing userspace that happens to pass non-zero padding would now silently trigger perfmon behavior. The existing kernel rejects `pad != 0`, so technically any value in that field from old userspace would be 0, but the removal of the check (lines 480-484 of the diff) is concerning. It would be cleaner to keep checking `perfmon_id` for validity (i.e., return error if non-zero and no perfmon found) rather than silently ignoring invalid IDs.

2. **`drm_ethosu_npu_info` struct extension without versioning**: Adding `pmu_counters` to `drm_ethosu_npu_info` changes its size. The existing `drm_ethosu_dev_query` mechanism with its `size` field should handle this gracefully, but it's worth confirming that the query path uses `min(size, actual_size)` correctly so old userspace doesn't read garbage.

3. **`drm_ethosu_perfmon_create` has no padding**: The struct has `__u32 id`, `__u32 ncounters`, `__u16 counters[8]` = 4+4+16 = 24 bytes. This is fine for alignment, but there's no reserved/flags field for future extensibility.

4. **`drm_ethosu_perfmon_destroy` is only 4 bytes**: A `__u32 id` with no padding. This is quite small and has no room for future flags. Consider adding a `__u32 pad` that must be zero.

**Concurrency and Locking Issues:**

5. **`ethosu_switch_perfmon` has redundant checks**: Lines 337-341 in the diff:
```c
if (perfmon == ethosu->active_perfmon)
    return;

if (perfmon != ethosu->active_perfmon)
    ethosu_perfmon_stop(ethosu, ethosu->active_perfmon, true);
```
The second `if` is always true after the first check passes - it's redundant. The function also has a third check `if (perfmon && ethosu->active_perfmon != perfmon)` which is also always true at that point. This should be simplified.

6. **`active_perfmon` and `global_perfmon` lack proper locking**: These fields on `ethosu_device` are accessed from multiple paths (ioctl handlers, job run, close_file, etc.) without consistent locking. `cmpxchg` and `xchg` are used in some places for `global_perfmon` but not for `active_perfmon`. The `ethosu_switch_perfmon` is called from `ethosu_job_run` under no device-level lock. There's a `perfmon->lock` mutex but `active_perfmon` itself is set and read without synchronization.

7. **`ethosu_perfmon_set_global` leaks refcount**: `ethosu_perfmon_find` takes a reference, but both the set and clear paths in `ethosu_ioctl_perfmon_set_global` return without calling `ethosu_perfmon_put`:
```c
perfmon = ethosu_perfmon_find(ethosu_priv, req->id);  // takes ref
if (!perfmon)
    return -EINVAL;

if (req->flags & DRM_ETHOSU_PERFMON_CLEAR_GLOBAL) {
    ...
    return 0;  // leaked ref
}

if (cmpxchg(&ethosu->global_perfmon, NULL, perfmon))
    return -EBUSY;  // leaked ref

return 0;  // leaked ref (though the stored pointer "owns" it conceptually?)
```
The intent may be that the `global_perfmon` pointer holds the reference, but then the clear path also needs to put the old reference, and the `-EBUSY` path is a definite leak.

8. **`ethosu_ioctl_perfmon_get_values` stops the perfmon but doesn't restart it**: Calling `ethosu_perfmon_stop` captures the counters and sets `active_perfmon = NULL`. But it never restarts counting, so a `get_values` call will stop the perfmon permanently. If the intent is to read intermediate results, this is broken. If the intent is that get_values is a one-shot terminal operation, it's not documented as such.

**Logic Bugs:**

9. **PMU register base adjustment is order-dependent and fragile**: In `ethosu_init`:
```c
ethosudev->npu_info.pmu_counters = FIELD_GET(PMCR_NUM_EVENT_CNT_MASK,
    readl_relaxed(ethosudev->pmu_regs + NPU_REG_PMCR));
...
if (!ethosu_is_u65(ethosudev))
    ethosudev->pmu_regs += 0x1000;
```
The PMCR is read *before* the U85 offset adjustment. This means for U85, the PMCR read uses the wrong register address. On U85 the PMU registers are offset by 0x1000, so the initial read at the U65 offset would read garbage on U85 hardware.

10. **`ncounters` semantics are confusing**: The `ncounters` in the UAPI struct is the number of event counters, but after `perfmon_create` it gets incremented to include the cycle counter. Then in `perfmon_start`/`perfmon_stop`, the code does `ncounters = perfmon->ncounters - 1` to get back to event counters. This is error-prone. The `__counted_by(ncounters)` on `values[]` means the flex array is sized for the incremented count which is correct, but the back-and-forth is fragile and should be documented clearly.

11. **`perfmon_create` modifies the input struct**: `req->ncounters++` modifies the ioctl input/output struct, which means the incremented value gets copied back to userspace. Userspace sees ncounters = N+1 rather than the N it passed in. This is surprising behavior and may confuse userspace.

12. **Missing `ethosu_perfmon_put` in `ethosu_job_cleanup`**: The `ethosu_job_err_cleanup` path calls `ethosu_perfmon_put(job->perfmon)`, but the normal `ethosu_job_cleanup` (called via kref_put) does not. This means the perfmon reference taken in `ethosu_perfmon_find` during submit is leaked for successfully completed jobs.

13. **Magic constant `0x000011`**: In `ethosu_perfmon_start`:
```c
writel_relaxed(0x000011, ethosu->pmu_regs + NPU_REG_PMCCNTR_CFG);
```
This magic value should have a define or at least a comment explaining what bits are being set.

14. **Magic constant `0x80000000` for cycle counter enable**: Used in both start and stop:
```c
mask = 0x80000000;
```
Should use `BIT(31)` for clarity.

**Minor Issues:**

15. The comment "The perform lifetime is controlled by userspace" should be "The perfmon lifetime...".

16. The SPDX identifier on `ethosu_perfmon.c` says `GPL-2.0-only or MIT` (lowercase "or") while other files in the driver use `GPL-2.0-only OR MIT`. Should be consistent with the SPDX standard (uppercase `OR`).

---
Generated by Claude Code Patch Reviewer