Claude review: accel/ivpu: Add support for limiting NPU frequency

[Claude Code Review Bot <claude-review@example.com>]

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 pn_ratio, u16 max_ratio);
```

But all callers pass `u8` values (from `struct ivpu_hw_info` where `min_ratio`, `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 payload 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 `DEVICE_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 should require `CAP_SYS_ADMIN` or at minimum a capability check in the store functions. Compare with how other kernel frequency interfaces (e.g., cpufreq) handle permissions.

**`dpu_mhz_to_pll_ratio_lnl` used for all platforms**

The inverse conversion function is named `_lnl` indicating it's LNL-specific:

```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` call it unconditionally. While the writable sysfs attributes are gated to `IVPU_HW_IP_50XX+` (which may all be LNL-based), the btrs-level functions themselves have no such guard. If these functions were ever called from a non-LNL code path, the ratio would be wrong. At minimum, add a comment explaining the assumption. Ideally, add a platform-dispatched `pll_mhz_to_ratio()` that mirrors the existing `pll_ratio_to_mhz()` dispatch.

Also note `PLL_REF_CLK_FREQ_MHZ / 2` is `50 / 2 = 25` which uses integer division — this is fine, but the formula `freq_mhz / 25` is the LNL inverse of `ratio * 50 / 2 = ratio * 25`. For MTL, the inverse would be `freq_mhz * 3 / (50 * 2) = freq_mhz * 3 / 100`, which is clearly different. 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 = ivpu_hw_btrs_cfg_max_freq_set(vdev, freq_mhz);
```

The function receives MHz and passes MHz — there is no MHz-to-Hz conversion 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 callbacks and from `ivpu_hw_btrs_cfg_freq_init` during boot, without any locking:

```c
vdev->hw->pll.cfg_min_ratio = cfg_min_ratio;
vdev->hw->pll.cfg_max_ratio = cfg_max_ratio;
```

If two concurrent sysfs writes happen (e.g., writing `set_min_freq` and `set_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` but not symmetrically**

```c
static int ivpu_hw_btrs_cfg_freq_set(struct ivpu_device *vdev, u8 cfg_min_ratio, u8 cfg_max_ratio)
{
	u8 min_ratio = clamp_t(u8, cfg_min_ratio, vdev->hw->pll.min_ratio, cfg_max_ratio);
	u8 pn_ratio = clamp_t(u8, vdev->hw->pll.pn_ratio, min_ratio, cfg_max_ratio);
```

The `cfg_min_ratio` is clamped to `[hw min, cfg_max_ratio]`, but `cfg_max_ratio` is not clamped against `cfg_min_ratio`. This means if someone sets `cfg_max_ratio < cfg_min_ratio`, the clamping makes `min_ratio = cfg_max_ratio`, which is fine for the firmware message, but then `vdev->hw->pll.cfg_min_ratio` is stored as the *original unclamped* `cfg_min_ratio`:

```c
	vdev->hw->pll.cfg_min_ratio = cfg_min_ratio;
	vdev->hw->pll.cfg_max_ratio = 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 when selecting the operating frequency" which matches the firmware-side behavior, but the sysfs readback will be confusing. Consider whether to store the clamped value or the user-requested value, and document the choice consistently.

**Sysfs cleanup for manually-added files**

The `set_min_freq` and `set_max_freq` attributes are added via `sysfs_add_file_to_group()` rather than as part of the static attribute group. While `devm_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 automatically removed on driver unbind. They need explicit removal, or use `is_visible` 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 = kobj_to_dev(kobj);
    struct ivpu_device *vdev = to_ivpu_device(dev_get_drvdata(dev));

    if ((attr == &dev_attr_set_min_freq.attr ||
         attr == &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 — it will never appear in practice and clutters the debug output if it somehow does. Not a blocking issue.

**Minor: Missing space in JSM message init**

```c
	struct vpu_jsm_msg req = { .type = VPU_JSM_MSG_FREQ_CONFIG};
```

Missing space before `}` — should be `VPU_JSM_MSG_FREQ_CONFIG };` per kernel coding style.

**Minor: Early return changes in `ivpu_sysfs_init`**

The patch changes the error handling from continuing past failures to returning 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 initialization. Now, a failure in adding the base group prevents the freq group from being added. This is a reasonable change for correctness, but it's a behavioral change worth noting.

**Summary of required fixes:**
1. Fix the `u16` type in `ivpu_jsm_msg_freq_config` to `u8` (matching callers 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