From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: accel/amdxdna: Read real-time clock frequencies Date: Sun, 12 Apr 2026 14:21:10 +1000 Message-ID: In-Reply-To: <20260406220526.4027917-1-lizhi.hou@amd.com> References: <20260406220526.4027917-1-lizhi.hou@amd.com> <20260406220526.4027917-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. Potential field-name swap in `npu4_update_counters` (npu4_regs.c) =E2= =80=94 needs clarification** ```c ndev->npuclk_freq =3D npu_metrics.mpnpuclk_freq; ndev->hclk_freq =3D npu_metrics.npuclk_freq; ``` >From `include/linux/amd-pmf-io.h`, the PMF struct documents: - `npuclk_freq` =E2=80=94 "NPU clock frequency [MHz]" - `mpnpuclk_freq` =E2=80=94 "MPNPU [MHz]" In the driver (`npu4_set_dpm`), the existing mapping is: ```c ndev->npuclk_freq =3D ndev->priv->dpm_clk_tbl[dpm_level].npuclk; // MP-NPU= clock ndev->hclk_freq =3D ndev->priv->dpm_clk_tbl[dpm_level].hclk; // H clock ``` So this maps PMF's `mpnpuclk_freq` =E2=86=92 driver's `npuclk_freq` (MP-NPU= ), and PMF's `npuclk_freq` =E2=86=92 driver's `hclk_freq` (H clock). The cr= oss-wiring of names is extremely confusing. Even if this mapping is hardwar= e-correct, a comment explaining why `npu_metrics.npuclk_freq` maps to `ndev= ->hclk_freq` (and not `npuclk_freq`) would prevent future maintenance mista= kes. **2. `aie2_update_counters` macro silently discards errors (aie2_pci.h)** ```c #define aie2_update_counters(ndev) \ ({ \ typeof(ndev) _ndev =3D ndev; \ if (_ndev->priv->hw_ops->update_counters) \ _ndev->priv->hw_ops->update_counters(_ndev);\ }) ``` This macro has two problems: - The return value of `update_counters()` is silently discarded. If PMF is = unavailable (`CONFIG_AMD_PMF=3Dn`), `AIE2_GET_PMF_NPU_METRICS` returns `-EO= PNOTSUPP`, and the caller proceeds to report stale/previous frequency data = to userspace with no indication it's stale. - The macro itself is a statement expression but its value is the result of= the `if` statement, which is `void`. Callers cannot check for errors even = if they wanted to. At minimum, consider making this return `int` and propagating the error, or= at least adding `dev_dbg` on failure so the silent fallback to stale data = is discoverable during debugging: ```c #define aie2_update_counters(ndev) \ ({ \ typeof(ndev) _ndev =3D ndev; \ int _ret =3D 0; \ if (_ndev->priv->hw_ops->update_counters) \ _ret =3D _ndev->priv->hw_ops->update_counters(_ndev); \ _ret; \ }) ``` **3. `npu4_update_counters` doesn't update `max_tops` (npu4_regs.c)** Compare with `npu4_set_dpm`: ```c ndev->max_tops =3D NPU4_DPM_TOPS(ndev, ndev->priv->dpm_clk_tbl[ndev->max_dp= m_level].hclk); ndev->curr_tops =3D NPU4_DPM_TOPS(ndev, ndev->hclk_freq); ``` But `npu4_update_counters` only updates `curr_tops`: ```c ndev->curr_tops =3D NPU4_DPM_TOPS(ndev, ndev->hclk_freq); ``` This is probably intentional (max TOPS shouldn't change at runtime), but `a= ie2_query_resource_info` reads `ndev->max_tops` right after calling `aie2_u= pdate_counters`. If this is deliberate, a brief comment explaining that `ma= x_tops` is set once in `set_dpm` and doesn't need runtime refresh would hel= p reviewers. **4. No locking around field updates in `npu4_update_counters`** `npu4_update_counters` modifies `ndev->npuclk_freq`, `ndev->hclk_freq`, and= `ndev->curr_tops` without any visible locking. These same fields are also = written by `npu4_set_dpm` (called via `aie2_pm_set_dpm` from ioctl paths). = If a DPM-level change races with a clock metadata query, the reported value= s could be inconsistent (e.g., `npuclk_freq` from PMF but `curr_tops` compu= ted from a DPM-table hclk). This may be an existing issue, but the new call= sites in `aie2_get_clock_metadata` and `aie2_query_resource_info` widen th= e race window. **5. Compound literal vs named constant inconsistency (npu1_regs.c)** ```c .hw_ops =3D &(const struct aie2_hw_ops) { .set_dpm =3D npu1_set_dpm, }, ``` NPU1 uses an anonymous compound literal while NPU4/5/6 use the named `npu4_= hw_ops`. For consistency and debuggability, consider defining a `npu1_hw_op= s` named constant as well. **6. Missing trailing commas (npu4_regs.c, npu5_regs.c, npu6_regs.c)** ```c .hw_ops =3D &npu4_hw_ops ``` The kernel coding style convention is to include trailing commas after the = last struct member initializer, to reduce future diff noise. The original c= ode had them. **7. NPU5 and NPU6 inherit `update_counters` (npu5_regs.c, npu6_regs.c)** By pointing `.hw_ops =3D &npu4_hw_ops`, npu5 and npu6 now also get `npu4_up= date_counters`. This means PMF metrics queries will be made for these NPU g= enerations too. Is the PMF interface guaranteed to work the same way across= NPU4/5/6? The commit message doesn't address this. **8. Patch should be split** This patch bundles a mechanical refactor (struct =E2=86=92 pointer, making = `npu4_set_dpm` static, sharing `npu4_hw_ops`) with a functional change (new= `update_counters` callback and its call sites). Kernel convention is to se= parate these: - Patch 1: Refactor `hw_ops` from embedded struct to const pointer - Patch 2: Add `update_counters` callback and PMF integration This aids bisection if the PMF integration causes issues. **9. `NPU4_DPM_TOPS` macro simplification is correct (npu4_regs.c)** The old macro: ```c #define NPU4_DPM_TOPS(ndev, dpm_level) \ ({ \ typeof(ndev) _ndev =3D ndev; \ (4096 * (_ndev)->total_col * \ (_ndev)->priv->dpm_clk_tbl[dpm_level].hclk / 1000000); \ }) ``` The new macro: ```c #define NPU4_DPM_TOPS(ndev, hclk) (4096 * (ndev)->total_col * (hclk) / 1000= 000) ``` This is a good simplification. Taking the raw hclk value as input rather th= an a DPM level index makes the macro more reusable (needed for `update_coun= ters` which gets hclk from PMF, not from the DPM table). The removal of `ty= peof` guard is safe since `ndev` is only dereferenced once and all callers = pass a simple pointer. --- Generated by Claude Code Patch Reviewer