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: Add IOCTL to retrieve realtime NPU power estimate
Date: Tue, 03 Mar 2026 14:37:39 +1000
Message-ID: <review-patch1-20260228061109.361239-2-superm1@kernel.org>
In-Reply-To: <20260228061109.361239-2-superm1@kernel.org>
References: <20260228061109.361239-1-superm1@kernel.org> <20260228061109.361239-2-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

**Commit message nit**: The line `(Update comment to indicate power and uti=
lization)` between the Reviewed-by tags and the Signed-off-by is unusual. T=
his annotation should go below the `---` separator or in the cover letter, =
not in the permanent commit message area.

**`AIE2_GET_PMF_NPU_DATA` macro =E2=80=94 dead assignment on error** (`aie2=
_pci.h`):

```c
#define AIE2_GET_PMF_NPU_DATA(field, val)				\
({									\
	struct amd_pmf_npu_metrics _npu_metrics;			\
	int _ret;							\
									\
	_ret =3D amd_pmf_get_npu_data(&_npu_metrics);			\
	val =3D _ret ? U32_MAX : _npu_metrics.field;			\
	(_ret);								\
})
```

When `_ret` is non-zero, `val` is set to `U32_MAX`, but the caller immediat=
ely returns:

```c
ret =3D AIE2_GET_PMF_NPU_DATA(npu_power, sensor.input);
if (ret)
    return ret;
```

So the `U32_MAX` assignment is dead code. This is harmless but the macro de=
sign is misleading =E2=80=94 it suggests the caller might continue with a f=
allback value.

**`#else` stub =E2=80=94 fragile naming convention** (`aie2_pci.h`):

```c
#define SENSOR_DEFAULT_npu_power	U32_MAX
#define AIE2_GET_PMF_NPU_DATA(field, val)				\
({									\
	val =3D SENSOR_DEFAULT_##field;					\
	(-EOPNOTSUPP);							\
})
```

The token-pasting `SENSOR_DEFAULT_##field` requires a matching `#define` fo=
r every field name used. Currently only `npu_power` has one. This is brittl=
e and the whole construct is unnecessary since the macro always returns `-E=
OPNOTSUPP` and the caller returns immediately on error =E2=80=94 the value =
assigned to `val` is never used. This could be simplified to just:

```c
#define AIE2_GET_PMF_NPU_DATA(field, val) (-EOPNOTSUPP)
```

**`unitm` / `units` inconsistency** (`aie2_pci.c`):

```c
sensor.unitm =3D -3;
scnprintf(sensor.units, sizeof(sensor.units), "mW");
```

Per the UAPI documentation: `unitm` translates values via `pow(10, unitm) *=
 value`. The PMF struct has `u16 npu_power`. If the raw value is already in=
 milliwatts, then `unitm` should be `0` and `units =3D "mW"`, or `unitm =3D=
 -3` and `units =3D "W"`. Having `unitm =3D -3` with `units =3D "mW"` means=
 userspace would compute `pow(10, -3) * value` in milliwatts, yielding micr=
owatts =E2=80=94 likely not the intent. Please clarify which is correct.

**Missing `buffer_size` check parity with other handlers**: `aie2_get_clock=
_metadata()` doesn't validate `buffer_size` before `copy_to_user` =E2=80=94=
 it just copies. Here `aie2_get_sensors()` does validate. Inconsistent, but=
 the validation here is the right thing to do; just noting the discrepancy.

**No `#include <linux/limits.h>` justification**: The patch adds `#include =
<linux/limits.h>` to `aie2_pci.h`, presumably for `U32_MAX`, but `U32_MAX` =
is defined in `<linux/limits.h>` only indirectly. It's actually in `<linux/=
types.h>` or `<linux/kernel.h>` which are likely already pulled in. Minor, =
but worth verifying the include is actually needed.

---
Generated by Claude Code Patch Reviewer