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: Thu, 12 Mar 2026 06:48:17 +1000
Message-ID: <review-patch1-20260311171842.473453-1-lizhi.hou@amd.com>
In-Reply-To: <20260311171842.473453-1-lizhi.hou@amd.com>
References: <20260311171842.473453-1-lizhi.hou@amd.com> <20260311171842.473453-1-lizhi.hou@amd.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

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 =3D 0`, the co=
py proceeds unconditionally and the `<=3D` check at line 170 (`args->buffer=
_size <=3D sensors_count * sizeof(sensor)`) only fires *after* the copy. Th=
e minimum buffer size check should be retained before the first `copy_to_us=
er`.

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

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

This check is after copying, so it guards the *next* sensor, not the curren=
t 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 =E2=80=94 the=
 first sensor has no guard while subsequent ones do.

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

```c
for (i =3D 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 av=
oid 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 =3D metrics;					\
	memset(_m, 0xff, sizeof(*_m));					\
	(-EOPNOTSUPP);							\
})
```

The `memset` to `0xff` is presumably for debug purposes, but since the call=
er 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) =E2=80=94 the =
two stubs have inconsistent error semantics. The new `AIE2_GET_PMF_NPU_METR=
ICS` returns an error, meaning sensors will be completely unavailable when =
`CONFIG_AMD_PMF` is disabled, whereas the old `AIE2_GET_PMF_NPU_DATA` retur=
ned success with a sentinel value. If the original behavior was to still re=
port sensors (with a sentinel) when PMF is unavailable, this changes that c=
ontract.

**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 =3D 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 assignment=
s are safe.

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

```c
for (i =3D 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 ha=
rdware 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 `CON=
FIG_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 =3D sensors_count * sizeof(sensor);

	return 0;
```

The `out:` label with its blank line above the assignment is fine stylistic=
ally, but placing it directly above the assignment makes the flow clearer t=
hat `buffer_size` is always updated regardless of how many sensors were wri=
tten.

---
Generated by Claude Code Patch Reviewer