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/ivpu: Add support for limiting NPU frequency Date: Tue, 31 Mar 2026 17:27:17 +1000 Message-ID: In-Reply-To: <20260330083815.1806045-1-andrzej.kacprowski@linux.intel.com> References: <20260330083815.1806045-1-andrzej.kacprowski@linux.intel.com> <20260330083815.1806045-1-andrzej.kacprowski@linux.intel.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 **Bug: Duplicate function declaration in header** The header `ivpu_jsm_msg.h` has the declaration duplicated: ```c +int ivpu_jsm_msg_freq_config(struct ivpu_device *vdev, u16 min_ratio, u16 = pn_ratio, u16 max_ratio); +int ivpu_jsm_msg_freq_config(struct ivpu_device *vdev, u16 min_ratio, u16 = pn_ratio, u16 max_ratio); ``` This will generate a compiler warning. Remove the duplicate line. **Bug: Type mismatch between declaration and definition** The header declares: ```c int ivpu_jsm_msg_freq_config(struct ivpu_device *vdev, u16 min_ratio, u16 p= n_ratio, u16 max_ratio); ``` But the callers and the struct fields (`cfg_min_ratio`, `cfg_max_ratio`) us= e `u8` for ratios, while the JSM payload struct uses `u32` fields. The func= tion implementation in `ivpu_jsm_msg.c` takes `u16` parameters which are as= signed to `u32` fields =E2=80=94 this works but is inconsistent. Since the = PLL ratios are stored as `u8` in `ivpu_hw_info` and the JSM payload uses `u= 32`, the function signature should probably use `u8` to match the callers (= or at minimum be consistent with one width). **Issue: `dpu_mhz_to_pll_ratio_lnl` only works for LNL, used unconditionall= y** The reverse conversion function: ```c +static u8 dpu_mhz_to_pll_ratio_lnl(u32 freq_mhz) +{ + return clamp_t(u32, freq_mhz / (PLL_REF_CLK_FREQ_MHZ / 2), 0, U8_MAX); +} ``` This is named `_lnl` and uses the LNL formula (`freq =3D ratio * 50 / 2`, s= o `ratio =3D freq / 25`), but `ivpu_hw_btrs_cfg_max_freq_set()` and `ivpu_h= w_btrs_cfg_min_freq_set()` call it unconditionally without checking the har= dware generation. Since `set_min_freq`/`set_max_freq` are only exposed for = `IVPU_HW_IP_50XX` and newer (which are LNL+), this is probably correct in p= ractice, but it would be cleaner to either: - Add a comment explaining this is LNL-only by design, or - Add a platform-dispatching wrapper matching `pll_ratio_to_mhz` **Issue: No locking around cfg_min_ratio / cfg_max_ratio updates** In `ivpu_hw_btrs_cfg_freq_set()`, the values are updated after the JSM call= succeeds: ```c + vdev->hw->pll.cfg_min_ratio =3D cfg_min_ratio; + vdev->hw->pll.cfg_max_ratio =3D cfg_max_ratio; ``` These can be written concurrently from two sysfs store calls (one to `set_m= in_freq`, one to `set_max_freq`). The read of `cfg_min_ratio` / `cfg_max_ra= tio` in `ivpu_hw_btrs_cfg_min_freq_set` / `ivpu_hw_btrs_cfg_max_freq_set` i= s also unprotected. A mutex should guard these read-modify-write sequences,= or at minimum a comment explaining why races are acceptable. **Issue: Stale comment in set_max_freq_store** ```c + /* Convert MHz to Hz and set max frequency */ + ret =3D ivpu_hw_btrs_cfg_max_freq_set(vdev, freq_mhz); ``` The comment says "Convert MHz to Hz" but the function takes and works with = MHz directly. This is a copy-paste leftover. **Minor: Missing `const` on attribute group** ```c +static struct attribute_group ivpu_freq_attr_group =3D { ``` Should be `static const struct attribute_group` to match kernel conventions= and allow the compiler to place it in read-only data. **Design observation: sysfs_add_file_to_group for set_min/set_max** The conditional addition of `set_min_freq` and `set_max_freq` using `sysfs_= add_file_to_group()` after creating the group is functional but differs fro= m the typical kernel pattern of using `is_visible` callbacks in the attribu= te group. Using `.is_visible` would be more idiomatic and would also handle= cleanup automatically via `devm_device_add_group`. The current approach al= so doesn't have matching removal =E2=80=94 `sysfs_add_file_to_group` files = won't be removed by the `devm_device_add_group` cleanup of the parent group. **Observation: `ivpu_hw_btrs_cfg_freq_init` placement** The call to `ivpu_hw_btrs_cfg_freq_init()` is inside `if (!ivpu_fw_is_warm_= boot(vdev))` which means frequency config is only sent on cold boot. This i= s probably intentional (firmware remembers across warm boots), but worth co= nfirming =E2=80=94 if the firmware doesn't preserve frequency config across= warm boots, this would silently lose user-configured limits. **Minor: `PLL_REF_CLK_FREQ_MHZ / 2` integer division** In `dpu_mhz_to_pll_ratio_lnl`: ```c + return clamp_t(u32, freq_mhz / (PLL_REF_CLK_FREQ_MHZ / 2), 0, U8_MAX); ``` `PLL_REF_CLK_FREQ_MHZ` is 50, so `50 / 2 =3D 25`. This works fine, but the = formula isn't the exact inverse of `pll_ratio_to_mhz_lnl` which is `(ratio = * 50) / 2`. For `ratio * 50 / 2 =E2=86=92 freq`, the inverse should be `fre= q * 2 / 50` (i.e., `freq / 25`). Both produce the same result here, but `fr= eq_mhz * 2 / PLL_REF_CLK_FREQ_MHZ` would be more clearly the algebraic inve= rse. **Summary**: The critical fixes needed are the duplicate declaration in the= header and the type consistency for the JSM function signature. The lockin= g concern around `cfg_min_ratio`/`cfg_max_ratio` should also be addressed o= r explicitly justified. The rest are minor improvements. --- Generated by Claude Code Patch Reviewer