Re: [PATCH v7 06/10] clk: realtek: Add support for mux clock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] `clk_regmap_mux_get_parent()` masks hardware read errors and invalid states by defaulting to parent index 0.
- [Medium] Potential signed integer overflow in `clk_regmap_mux_set_parent()` when shifting the `u8 index` parameter.
--

commit 462409aad9f9756e1384cae76faf89758f477e12
Author: Cheng-Yu Lee <cylee12@realtek.com>

clk: realtek: Add support for mux clock

This commit adds a regmap-based clk_ops implementation for Realtek mux
clocks, supporting parent selection and rate determination.

> diff --git a/drivers/clk/realtek/clk-regmap-mux.c b/drivers/clk/realtek/clk-regmap-mux.c
> new file mode 100644
> index 0000000000000..1b0b8419d651f
> --- /dev/null
> +++ b/drivers/clk/realtek/clk-regmap-mux.c
[ ... ]
> +static u8 clk_regmap_mux_get_parent(struct clk_hw *hw)
> +{
> +	struct clk_regmap_mux *clkm = to_clk_regmap_mux(hw);
> +	int num_parents = clk_hw_get_num_parents(hw);
> +	u32 val;
> +	int ret;
> +
> +	ret = regmap_read(clkm->clkr.regmap, clkm->mux_ofs, &val);
> +	if (ret)
> +		return 0;
> +
> +	val = (val >> clkm->shift) & clkm->mask;
> +
> +	return val >= num_parents ? 0 : val;

Does returning 0 on hardware read errors or invalid states mask the error
from the clock framework?

If 0 is returned here, the framework might cache 0 as the active parent. If
a consumer later requests to set the parent to 0, the framework could
incorrectly assume the hardware is already in the correct state and skip the
set_parent call entirely.

Could this return an out-of-bounds index (like -EINVAL cast to u8) instead,
so the framework recognizes the parent as unknown and forces a hardware
update when a valid parent is requested?

> +}
> +
> +static int clk_regmap_mux_set_parent(struct clk_hw *hw, u8 index)
> +{
> +	struct clk_regmap_mux *clkm = to_clk_regmap_mux(hw);
> +
> +	return regmap_update_bits(clkm->clkr.regmap, clkm->mux_ofs,
> +				  clkm->mask << clkm->shift, index << clkm->shift);

Could the shift operation index << clkm->shift cause a signed integer
overflow?

Since index is a u8, it is implicitly promoted to a signed 32-bit int. If
the clock mux is configured at the most significant bit of a 32-bit
register (where clkm->shift is 31), shifting a non-zero index into the sign
bit triggers undefined behavior.

This might trigger a shift-out-of-bounds splat, which could lead to a panic
on systems with panic_on_warn enabled.

Would it be safer to explicitly cast index to an unsigned type, such as
(u32)index << clkm->shift?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508111641.3192177-1-eleanor.lin@realtek.com?part=6