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: accel/amdxdna: Support sensors for column utilization
Date: Tue, 03 Mar 2026 14:37:39 +1000
Message-ID: <review-patch2-20260228061109.361239-3-superm1@kernel.org>
In-Reply-To: <20260228061109.361239-3-superm1@kernel.org>
References: <20260228061109.361239-1-superm1@kernel.org> <20260228061109.361239-3-superm1@kernel.org>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

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

```c
util_count =3D min_t(u32, ndev->total_col, 8);
sensors_count =3D 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 fo=
r 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 senso=
rs exist before allocating the buffer. It must know `total_col` a priori, w=
hich is a driver implementation detail.
2. **All-or-nothing**: If userspace only wants the power sensor, it cannot =
query just that one =E2=80=94 it must allocate for all sensors or get `-EIN=
VAL`.
3. **Hardware-dependent size**: Different hardware with different `total_co=
l` values require different buffer sizes for the same IOCTL parameter.

A better design would be: fill as many sensors as fit in the provided buffe=
r and set `args->buffer_size` to the number of bytes actually written. User=
space 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()` instea=
d 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 shoul=
d be removed.

Also, `AIE2_GET_PMF_NPU_METRICS` is a trivial one-line wrapper that adds no=
 value =E2=80=94 it just calls `amd_pmf_get_npu_data()`. Consider calling t=
he 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 =3D 0; i < util_count; i++) {
    ...
    if (copy_to_user(u64_to_user_ptr(args->buffer) + (i + 1) * sizeof(senso=
r),
                     &sensor, sizeof(sensor)))
        return -EFAULT;
}

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

If the power sensor is successfully copied but a subsequent column utilizat=
ion copy fails, the function returns `-EFAULT` without setting `args->buffe=
r_size`. Userspace has received partial data in its buffer but has no indic=
ation of how much was written. This is a minor concern in practice (a parti=
al `copy_to_user` failure typically indicates a corrupted user pointer), bu=
t building into a local buffer and doing a single `copy_to_user` would be c=
leaner and avoid partial writes entirely.

**`npu_busy` is `u16` but `sensor.input` is `__u32`** =E2=80=94 this is fin=
e, 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 onl=
y distinguished by their `label` string ("Column 0 Utilization", etc.). The=
re 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 f=
ield 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 comm=
ent `"Support getting power and utilization data"`, but utilization support=
 is only added in patch 2. The version bump should either be in patch 2 (wh=
en the full feature is present) or the comment in patch 1 should only menti=
on power data.

---
Generated by Claude Code Patch Reviewer