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

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

Patch Review

**Rigid buffer size requirement breaks forward/backward compatibility** (`aie2_pci.c`):

```c
util_count = min_t(u32, ndev->total_col, 8);
sensors_count = 1 + util_count;

if (args->buffer_size < sensors_count * sizeof(sensor))
    return -EINVAL;
```

This is the most significant issue in the series. Patch 1 requires space for 1 sensor. Patch 2 changes this to require space for `1 + min(total_col, 8)` sensors. Problems:

1. **No discovery mechanism**: Userspace has no way to query how many sensors exist before allocating the buffer. It must know `total_col` a priori, which is a driver implementation detail.
2. **All-or-nothing**: If userspace only wants the power sensor, it cannot query just that one — it must allocate for all sensors or get `-EINVAL`.
3. **Hardware-dependent size**: Different hardware with different `total_col` values require different buffer sizes for the same IOCTL parameter.

A better design would be: fill as many sensors as fit in the provided buffer and set `args->buffer_size` to the number of bytes actually written. Userspace can then allocate a larger buffer and retry if it wants more. This is a common pattern in kernel ioctls.

**`AIE2_GET_PMF_NPU_DATA` becomes dead code** (`aie2_pci.h`):

Patch 2 adds:
```c
#define AIE2_GET_PMF_NPU_METRICS(metrics) amd_pmf_get_npu_data(metrics)
```

and changes `aie2_get_sensors()` to use `AIE2_GET_PMF_NPU_METRICS()` instead of `AIE2_GET_PMF_NPU_DATA()`. But the old macro (and its `SENSOR_DEFAULT_npu_power` helper) are left in place with no remaining callers. These should be removed.

Also, `AIE2_GET_PMF_NPU_METRICS` is a trivial one-line wrapper that adds no value — it just calls `amd_pmf_get_npu_data()`. Consider calling the function directly (guarded by `#if IS_ENABLED(CONFIG_AMD_PMF)`) instead of adding a macro that just aliases the function.

**Partial copy_to_user on failure** (`aie2_pci.c`):

```c
if (copy_to_user(u64_to_user_ptr(args->buffer), &sensor, sizeof(sensor)))
    return -EFAULT;

for (i = 0; i < util_count; i++) {
    ...
    if (copy_to_user(u64_to_user_ptr(args->buffer) + (i + 1) * sizeof(sensor),
                     &sensor, sizeof(sensor)))
        return -EFAULT;
}

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

If the power sensor is successfully copied but a subsequent column utilization copy fails, the function returns `-EFAULT` without setting `args->buffer_size`. Userspace has received partial data in its buffer but has no indication of how much was written. This is a minor concern in practice (a partial `copy_to_user` failure typically indicates a corrupted user pointer), but building into a local buffer and doing a single `copy_to_user` would be cleaner and avoid partial writes entirely.

**`npu_busy` is `u16` but `sensor.input` is `__u32`** — this is fine, the implicit widening is correct. Just noting for completeness that the PMF metrics use `u16` values while the UAPI struct uses `__u32`.

**Missing sensor index/identifier**: The column utilization sensors are only distinguished by their `label` string ("Column 0 Utilization", etc.). There is no numeric index field in the sensor struct. Userspace must parse the label string to determine which column a sensor corresponds to. Adding a field or using a sub-type would be more robust, though this would require a UAPI change.

**Version bump scope**: The version bump to 0.7 is in patch 1 with the comment `"Support getting power and utilization data"`, but utilization support is only added in patch 2. The version bump should either be in patch 2 (when the full feature is present) or the comment in patch 1 should only mention power data.

---
Generated by Claude Code Patch Reviewer