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: Sun, 12 Apr 2026 12:19:40 +1000 Message-ID: In-Reply-To: <20260408150152.2093638-1-andrzej.kacprowski@linux.intel.com> References: <20260408150152.2093638-1-andrzej.kacprowski@linux.intel.com> <20260408150152.2093638-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 **Type mismatch in `ivpu_jsm_msg_freq_config` (bug)** The function is declared in the header and defined with `u16` parameters: ```c int ivpu_jsm_msg_freq_config(struct ivpu_device *vdev, u16 min_ratio, u16 p= n_ratio, u16 max_ratio); ``` But all callers pass `u8` values (from `struct ivpu_hw_info` where `min_rat= io`, `max_ratio`, `pn_ratio`, `cfg_min_ratio`, `cfg_max_ratio` are all `u8`= ), and the payload fields are `u32`: ```c struct vpu_ipc_msg_payload_freq_config { u32 min_freq_pll_ratio; u32 pn_freq_pll_ratio; u32 max_freq_pll_ratio; u32 reserved_0; }; ``` There are three different types in play (`u8` -> `u16` -> `u32`). The `u16`= intermediate type is inconsistent with both ends. The function parameters = should likely be `u8` to match the callers and the internal representation,= since PLL ratios fit in a byte. The implicit widening to `u32` for the pay= load assignment is fine either way, but `u16` serves no purpose and creates= confusion about the valid range. **No access control on writable sysfs attributes** The `set_min_freq` and `set_max_freq` attributes are mode `0644` (via `DEVI= CE_ATTR_RW`), meaning any user can write to them: ```c static DEVICE_ATTR_RW(set_min_freq); ... static DEVICE_ATTR_RW(set_max_freq); ``` Frequency control affects power, thermal behavior, and performance. This sh= ould require `CAP_SYS_ADMIN` or at minimum a capability check in the store = functions. Compare with how other kernel frequency interfaces (e.g., cpufre= q) handle permissions. **`dpu_mhz_to_pll_ratio_lnl` used for all platforms** The inverse conversion function is named `_lnl` indicating it's LNL-specifi= c: ```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); } ``` But `ivpu_hw_btrs_cfg_max_freq_set` and `ivpu_hw_btrs_cfg_min_freq_set` cal= l it unconditionally. While the writable sysfs attributes are gated to `IVP= U_HW_IP_50XX+` (which may all be LNL-based), the btrs-level functions thems= elves have no such guard. If these functions were ever called from a non-LN= L code path, the ratio would be wrong. At minimum, add a comment explaining= the assumption. Ideally, add a platform-dispatched `pll_mhz_to_ratio()` th= at mirrors the existing `pll_ratio_to_mhz()` dispatch. Also note `PLL_REF_CLK_FREQ_MHZ / 2` is `50 / 2 =3D 25` which uses integer = division =E2=80=94 this is fine, but the formula `freq_mhz / 25` is the LNL= inverse of `ratio * 50 / 2 =3D ratio * 25`. For MTL, the inverse would be = `freq_mhz * 3 / (50 * 2) =3D freq_mhz * 3 / 100`, which is clearly differen= t. Worth ensuring no MTL path can reach this. **Misleading 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 function receives MHz and passes MHz =E2=80=94 there is no MHz-to-Hz co= nversion here. The comment is incorrect and should be removed. **Missing concurrency protection** `cfg_min_ratio` and `cfg_max_ratio` are read and written from sysfs callbac= ks and from `ivpu_hw_btrs_cfg_freq_init` during boot, without any locking: ```c vdev->hw->pll.cfg_min_ratio =3D cfg_min_ratio; vdev->hw->pll.cfg_max_ratio =3D cfg_max_ratio; ``` If two concurrent sysfs writes happen (e.g., writing `set_min_freq` and `se= t_max_freq` simultaneously), the reads of `vdev->hw->pll.cfg_min_ratio` or = `cfg_max_ratio` inside `ivpu_hw_btrs_cfg_freq_set` could race with updates.= A mutex protecting the frequency configuration would be appropriate. **`ivpu_hw_btrs_cfg_freq_set` clamps `min_ratio` against `cfg_max_ratio` bu= t not symmetrically** ```c static int ivpu_hw_btrs_cfg_freq_set(struct ivpu_device *vdev, u8 cfg_min_r= atio, u8 cfg_max_ratio) { u8 min_ratio =3D clamp_t(u8, cfg_min_ratio, vdev->hw->pll.min_ratio, cfg_m= ax_ratio); u8 pn_ratio =3D clamp_t(u8, vdev->hw->pll.pn_ratio, min_ratio, cfg_max_rat= io); ``` The `cfg_min_ratio` is clamped to `[hw min, cfg_max_ratio]`, but `cfg_max_r= atio` is not clamped against `cfg_min_ratio`. This means if someone sets `c= fg_max_ratio < cfg_min_ratio`, the clamping makes `min_ratio =3D cfg_max_ra= tio`, which is fine for the firmware message, but then `vdev->hw->pll.cfg_m= in_ratio` is stored as the *original unclamped* `cfg_min_ratio`: ```c vdev->hw->pll.cfg_min_ratio =3D cfg_min_ratio; vdev->hw->pll.cfg_max_ratio =3D cfg_max_ratio; ``` This means reading back `set_min_freq` via sysfs could show a value higher = than `set_max_freq`, even though the firmware was told something different.= The documentation says "the driver clamps set_min_freq to set_max_freq whe= n selecting the operating frequency" which matches the firmware-side behavi= or, but the sysfs readback will be confusing. Consider whether to store the= clamped value or the user-requested value, and document the choice consist= ently. **Sysfs cleanup for manually-added files** The `set_min_freq` and `set_max_freq` attributes are added via `sysfs_add_f= ile_to_group()` rather than as part of the static attribute group. While `d= evm_device_add_group` handles cleanup for the group itself, `sysfs_add_file= _to_group` does **not** have a devm variant, so these files are not automat= ically removed on driver unbind. They need explicit removal, or use `is_vis= ible` callback on the attribute group to conditionally show them. Using `is_visible` would be the cleaner approach: ```c static umode_t ivpu_freq_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n) { struct device *dev =3D kobj_to_dev(kobj); struct ivpu_device *vdev =3D to_ivpu_device(dev_get_drvdata(dev)); if ((attr =3D=3D &dev_attr_set_min_freq.attr || attr =3D=3D &dev_attr_set_max_freq.attr) && ivpu_hw_ip_gen(vdev) < IVPU_HW_IP_50XX) return 0; return attr->mode; } ``` This would let you include all attributes in the static group and avoid the= manual `sysfs_add_file_to_group` calls entirely. **`VPU_JSM_MSG_RESERVED_111E` in debug string table** ```c IVPU_CASE_TO_STR(VPU_JSM_MSG_RESERVED_111E); ``` Adding a reserved message ID to the debug string conversion is harmless but= unnecessary =E2=80=94 it will never appear in practice and clutters the de= bug output if it somehow does. Not a blocking issue. **Minor: Missing space in JSM message init** ```c struct vpu_jsm_msg req =3D { .type =3D VPU_JSM_MSG_FREQ_CONFIG}; ``` Missing space before `}` =E2=80=94 should be `VPU_JSM_MSG_FREQ_CONFIG };` p= er kernel coding style. **Minor: Early return changes in `ivpu_sysfs_init`** The patch changes the error handling from continuing past failures to retur= ning early: ```c if (ret) { ivpu_warn(vdev, "Failed to add sysfs group to device, ret %d", ret); return; } ``` Previously, failure to add one sysfs group didn't prevent other initializat= ion. Now, a failure in adding the base group prevents the freq group from b= eing added. This is a reasonable change for correctness, but it's a behavio= ral change worth noting. **Summary of required fixes:** 1. Fix the `u16` type in `ivpu_jsm_msg_freq_config` to `u8` (matching calle= rs and struct fields) 2. Add capability check for writable freq attributes or restrict permissions 3. Remove the incorrect "Convert MHz to Hz" comment 4. Fix the missing space before `}` in the JSM message init 5. Address sysfs file cleanup (prefer `is_visible` approach) 6. Consider adding a mutex for concurrent sysfs access to frequency config --- Generated by Claude Code Patch Reviewer