Re: [PATCH v11 08/11] clk: realtek: Add support for MMC-tuned PLL clocks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] CCF API contract violation: determine_rate calculates dynamic rates while set_rate hardcodes fixed PLL settings.
- [Medium] Intermediate integer overflow in 32-bit clock rate calculation within determine_rate.
--

--- Patch [8]: [PATCH v11 08/11] clk: realtek: Add support for MMC-tuned PLL clocks ---
commit d6c866691778847bec596fd7ef12315587301b82
Author: Cheng-Yu Lee <cylee12@realtek.com>

clk: realtek: Add support for MMC-tuned PLL clocks

This commit adds clk_pll_mmc_ops for enable/disable, prepare, rate control,
and status operations on MMC PLL clocks. It also adds clk_pll_mmc_phase_ops
to support phase get/set operations.

> diff --git a/drivers/clk/realtek/clk-pll-mmc.c b/drivers/clk/realtek/clk-pll-mmc.c
> new file mode 100644
> index 0000000000000..8da1933a5a079
> --- /dev/null
> +++ b/drivers/clk/realtek/clk-pll-mmc.c
[ ... ]
> +static int rtk_clk_regmap_pll_mmc_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
> +{
> +	u32 val;
> +
> +	if (!req->best_parent_rate)
> +		return -EINVAL;
> +
> +	val = DIV_ROUND_CLOSEST_ULL((u64)req->rate * 4, req->best_parent_rate);
> +	val = clamp_t(u32, val, RTK_PLL_MMC_VAL_MIN, RTK_PLL_MMC_VAL_MAX);
> +	req->rate = req->best_parent_rate * val / 4;

[Severity: Medium]
Could this calculation overflow on 32-bit platforms where unsigned long is
32 bits?

With a parent rate of 27MHz and val of 255, the intermediate result of
req->best_parent_rate * val is ~6.88 billion. This exceeds U32_MAX and
truncates to ~2.59 billion before dividing by 4, returning an incorrect
target rate to the caller.

> +
> +	return 0;
> +}
> +
> +static int rtk_clk_regmap_pll_mmc_set_rate(struct clk_hw *hw, unsigned long rate,
> +					   unsigned long parent_rate)
> +{
> +	struct rtk_clk_regmap_pll_mmc *clkm = to_clk_pll_mmc(hw);
> +	u32 val = RTK_PLL_MMC_SSC_DIV_N_VAL;

[Severity: High]
Does this violate the Common Clock Framework API contract by hardcoding a fixed
multiplier here, while allowing dynamic rate calculations in
rtk_clk_regmap_pll_mmc_determine_rate()?

Because rtk_clk_regmap_pll_mmc_determine_rate() calculates requested
rates dynamically, it signals to callers that arbitrary target rates can be
achieved. However, this function ignores the requested rate and unconditionally
applies a static multiplier to the hardware.

Downstream consumers relying on clk_round_rate() or clk_set_rate() returning
the truth might miscalculate their internal dividers, as they would assume the
dynamically requested rate was successfully applied rather than the fixed rate.
 
[ ... ]

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