Re: [PATCH v7 07/10] clk: realtek: Add support for MMC-tuned PLL clocks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] `set_phrt0` uses an incorrect mask in `regmap_update_bits`, breaking clock enable/disable.
- [High] `clk_pll_mmc_set_rate` ignores the requested rate and hardcodes the hardware configuration.
- [High] `clk_pll_mmc_recalc_rate` improperly propagates negative error codes as unsigned clock rates.
- [Medium] Clock rate calculation in `clk_pll_mmc_recalc_rate` overflows 32-bit integers.
- [Medium] `clk_pll_mmc_phase_get_phase` invokes undefined behavior via invalid pointer subtraction.
--

commit 8ee4cf67f3b23e7f96645f8fb8d616be9378d545
Author: Cheng-Yu Lee <cylee12@realtek.com>

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

Add clk_pll_mmc_ops for enable/disable, prepare, rate control, and status
operations on MMC PLL clocks.

> diff --git a/drivers/clk/realtek/clk-pll-mmc.c b/drivers/clk/realtek/clk-pll-mmc.c
> new file mode 100644
> index 0000000000000..998ab853ffbb7
> --- /dev/null
> +++ b/drivers/clk/realtek/clk-pll-mmc.c

[ ... ]

> @@ -0,0 +1,453 @@
> +// SPDX-License-Identifier: GPL-2.0-only

[ ... ]

> +static inline int set_phrt0(struct clk_pll_mmc *clkm, u32 val)
> +{
> +	return regmap_update_bits(clkm->clkr.regmap, clkm->pll_ofs + PLL_EMMC1_OFFSET,
> +				  PLL_PHRT0_MASK, val << PLL_PHRT0_SHIFT);
> +}

regmap_update_bits internally performs a bitwise AND of the value and the
mask. Since PLL_PHRT0_MASK is BIT(0) and the value is shifted by
PLL_PHRT0_SHIFT (which is 1), will this operation always evaluate to 0?

Should the mask be passed as PLL_PHRT0_MASK << PLL_PHRT0_SHIFT so the clock
can actually be enabled and disabled?

[ ... ]

> +static int clk_pll_mmc_phase_get_phase(struct clk_hw *hw)
> +{
> +	struct clk_hw *hwp;
> +	struct clk_pll_mmc *clkm;
> +	int phase_id;
> +	int ret;
> +	u32 val;
> +
> +	hwp = clk_hw_get_parent(hw);
> +	if (!hwp)
> +		return -ENOENT;
> +
> +	clkm = to_clk_pll_mmc(hwp);
> +	phase_id = (hw - &clkm->phase0_hw) ? 1 : 0;

Does subtracting pointers to distinct struct members invoke undefined
behavior here, since they do not belong to the same array?

Could this be replaced with an equality check like
(hw == &clkm->phase0_hw) ? 0 : 1, similar to how it is handled in
clk_pll_mmc_phase_set_phase?

[ ... ]

> +static unsigned long clk_pll_mmc_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
> +{
> +	struct clk_pll_mmc *clkm = to_clk_pll_mmc(hw);
> +	u32 val, ext_f;
> +	int ret;
> +
> +	ret = get_ssc_div_n(clkm, &val);
> +	if (ret)
> +		return ret;
> +
> +	ret = get_ssc_div_ext_f(clkm, &ext_f);
> +	if (ret)
> +		return ret;

If get_ssc_div_n or get_ssc_div_ext_f fail and return a negative error code,
does this function return that error code directly? Since the return type
is unsigned long, could this result in an astronomically large positive
clock rate being reported to the Common Clock Framework?

> +
> +	return parent_rate / 4 * (val + 2) + (parent_rate / 4 * ext_f) / 8192;
> +}

On 32-bit architectures, could the expression (parent_rate / 4 * ext_f)
overflow if parent_rate is a typical clock frequency like 24 MHz and ext_f
is up to 8191? Should this math be performed using u64 to prevent silent
overflow?

[ ... ]

> +static int clk_pll_mmc_set_rate(struct clk_hw *hw, unsigned long rate, unsigned long parent_rate)
> +{
> +	struct clk_pll_mmc *clkm = to_clk_pll_mmc(hw);
> +	u32 val = PLL_MMC_SSC_DIV_N_VAL;
> +	int ret;

Does this completely ignore the requested rate and parent_rate parameters
and always program the hardware with the hardcoded PLL_MMC_SSC_DIV_N_VAL?

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