Claude review: accel/amdxdna: Support sensors for column utilization

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

Patch Review

**1. Removed buffer size validation (medium severity)**

The original code had:
```c
if (args->buffer_size < sizeof(sensor))
    return -EINVAL;
```

This is removed by the patch. The new code proceeds to `copy_to_user()` of the first (power) sensor without checking whether the user buffer is large enough for even one sensor. If userspace passes `buffer_size = 0`, the copy proceeds unconditionally and the `<=` check at line 170 (`args->buffer_size <= sensors_count * sizeof(sensor)`) only fires *after* the copy. The minimum buffer size check should be retained before the first `copy_to_user`.

**2. Off-by-one / boundary check logic**

```c
sensors_count++;
if (args->buffer_size <= sensors_count * sizeof(sensor))
    goto out;
```

This check is after copying, so it guards the *next* sensor, not the current one. For the first sensor (power), there's no pre-check at all (see point 1). The logic works for subsequent column sensors since the check prevents writing beyond the buffer, but it's fragile and inconsistent — the first sensor has no guard while subsequent ones do.

**3. Hard-coded `8` instead of using `ARRAY_SIZE`**

```c
for (i = 0; i < min_t(u32, ndev->total_col, 8); i++) {
```

The `8` corresponds to the size of `npu_metrics.npu_busy[8]` in `struct amd_pmf_npu_metrics`. This should use `ARRAY_SIZE(npu_metrics.npu_busy)` to avoid a silent bug if the struct is ever expanded.

**4. `!CONFIG_AMD_PMF` stub uses wrong error return**

```c
#define AIE2_GET_PMF_NPU_METRICS(metrics)				\
({									\
	typeof(metrics) _m = metrics;					\
	memset(_m, 0xff, sizeof(*_m));					\
	(-EOPNOTSUPP);							\
})
```

The `memset` to `0xff` is presumably for debug purposes, but since the caller checks `if (ret) return ret;` this data is never used. More importantly, the existing `AIE2_GET_PMF_NPU_DATA` stub uses `-EOPNOTSUPP` too but also fills in a `U32_MAX` default value and returns `0` (success) — the two stubs have inconsistent error semantics. The new `AIE2_GET_PMF_NPU_METRICS` returns an error, meaning sensors will be completely unavailable when `CONFIG_AMD_PMF` is disabled, whereas the old `AIE2_GET_PMF_NPU_DATA` returned success with a sentinel value. If the original behavior was to still report sensors (with a sentinel) when PMF is unavailable, this changes that contract.

**5. `sensor.input` type mismatch**

Looking at the structs:
- `struct amd_pmf_npu_metrics` has `u16 npu_busy[8]`
- `struct amdxdna_drm_query_sensor` has `__u32 input`

```c
sensor.input = npu_metrics.npu_busy[i];
```

This is fine (implicit widening from `u16` to `u32`), but worth noting that the `npu_power` field is also `u16` in the metrics struct. Both assignments are safe.

**6. `ndev->total_col` could exceed 8**

```c
for (i = 0; i < min_t(u32, ndev->total_col, 8); i++) {
```

The `min_t` clamp is correct since `npu_busy` has only 8 entries. But if hardware with >8 columns exists, userspace will silently get incomplete data with no indication. A comment would be helpful here.

**7. The old `AIE2_GET_PMF_NPU_DATA` macro is now dead code**

The patch adds `AIE2_GET_PMF_NPU_METRICS` and converts the sole caller, but doesn't remove the now-unused `AIE2_GET_PMF_NPU_DATA` macro (both the `CONFIG_AMD_PMF` and `!CONFIG_AMD_PMF` variants) or the related `SENSOR_DEFAULT_npu_power` define. These should be cleaned up.

**8. Minor: goto label placement**

```c
out:
	args->buffer_size = sensors_count * sizeof(sensor);

	return 0;
```

The `out:` label with its blank line above the assignment is fine stylistically, but placing it directly above the assignment makes the flow clearer that `buffer_size` is always updated regardless of how many sensors were written.

---
Generated by Claude Code Patch Reviewer