Re: [PATCH v3 2/2] clk: en7523: add support for .set_rate for en7581

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: en7523: broken SPI rate lookup in en7581_get_vals_for_rate()
- [High] clk: en7523: unchecked divider_get_val() error in en7581_get_vals_for_rate()
--

commit 9938b50fa7a98aecdd2b2c8ce0469da6eedd9312
Author: Christian Marangi <ansuelsmth@gmail.com>

clk: en7523: add support for .set_rate for en7581

This commit adds .determine_rate and .set_rate callbacks for the Airoha
EN7581 clock controller using standard clock divider helpers and divider
tables for the BUS, SPI, and SLIC clocks.

> diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
> index 6640e814d36a4..cf0b6ccacede9 100644
> --- a/drivers/clk/clk-en7523.c
> +++ b/drivers/clk/clk-en7523.c

[ ... ]

> @@ -338,6 +426,7 @@ static const struct en_clk_desc en7581_base_clks[] = {
>  		.div_shift = 8,
>  		.div_val0 = 40,
>  		.div_step = 2,
> +		.div_table = spi_div,
>  	}, {
>  		.id = EN7523_CLK_NPU,
>  		.name = "npu",
> @@ -775,11 +864,127 @@ static unsigned long en7581_recalc_rate(struct clk_hw *hw,
>  		return 0;
>  	}
>  
> -	return rate / en7523_get_div(desc, val);
> +	if (desc->div_bits) {
> +		val >>= desc->div_shift;
> +		val &= (1 << desc->div_bits) - 1;
> +
> +		rate = divider_recalc_rate(hw, rate, val,
> +					   desc->div_table,
> +					   desc->div_flags,
> +					   desc->div_bits);
> +	}
> +
> +	return rate;
> +}
> +
> +static int en7581_get_vals_for_rate(struct clk_hw *hw,
> +				    const struct en_clk_desc *desc,
> +				    unsigned long rate,
> +				    u32 *base_val, u32 *div_val)
> +{
> +	struct device *dev = clk_hw_get_dev(hw);
> +	int tmp_base_val = 0;
> +	int tmp_div_val = 0;
> +
> +	if (!desc->base_bits && !desc->div_bits)
> +		return -EINVAL;
> +
> +	/* Search requested rate with every matching base rate + divisor */
> +	for (; tmp_base_val < desc->n_base_values; tmp_base_val++) {
> +		unsigned long parent_rate = desc->base_values[tmp_base_val];
> +		unsigned long tmp_rate = parent_rate;

[Severity: High]
Does en7581_get_vals_for_rate() in drivers/clk/clk-en7523.c always fail with
-EINVAL when called for EN7523_CLK_SPI via en7581_determine_rate() or
en7581_set_rate()?

In en7581_base_clks, EN7523_CLK_SPI defines a fixed .base_value = 400000000
and leaves .base_bits and .n_base_values as 0:

	{
		.id = EN7523_CLK_SPI,
		.name = "spi",

		.base_reg = REG_SPI_CLK_DIV_SEL,

		.base_value = 400000000,

		.div_bits = 5,
		.div_shift = 8,
		.div_val0 = 40,
		.div_step = 2,
		.div_table = spi_div,
	},

When en7581_get_vals_for_rate() is called for EN7523_CLK_SPI,
desc->n_base_values is 0, so the for loop never executes and the check
tmp_base_val >= desc->n_base_values is immediately true, returning -EINVAL
for every SPI clock rate.

Should en7581_get_vals_for_rate() handle clocks where desc->base_bits is 0
by testing a single iteration with parent_rate = desc->base_value (similar
to how en7523_get_base_rate() handles !desc->base_bits)?

> +
> +		/*
> +		 * Simulate rate with every divisor supported
> +		 * 1. Search the divisor for the requested rate
> +		 * 2. Simulate the final rate with the new divisor
> +		 */
> +		if (desc->div_bits) {
> +			tmp_div_val = divider_get_val(rate, parent_rate,
> +						      desc->div_table,
> +						      desc->div_bits,
> +						      desc->div_flags);
> +			tmp_rate = divider_recalc_rate(hw, parent_rate,
> +						       tmp_div_val,
> +						       desc->div_table,
> +						       desc->div_flags,
> +						       desc->div_bits);
> +		}

[Severity: High]
What happens in en7581_get_vals_for_rate() in drivers/clk/clk-en7523.c when
divider_get_val() returns -EINVAL because DIV_ROUND_UP_ULL(parent_rate, rate)
is not in desc->div_table?

Because tmp_div_val is passed to divider_recalc_rate() without checking for
a negative error code, _get_div() looks up (unsigned int)-EINVAL in
desc->div_table, fails to find a match, and returns 0. Since
CLK_DIVIDER_ALLOW_ZERO is not set in desc->div_flags, divider_recalc_rate()
triggers a WARN() and returns parent_rate:

drivers/clk/clk-divider.c:divider_recalc_rate() {
	div = _get_div(table, val, flags, width);
	if (!div) {
		WARN(!(flags & CLK_DIVIDER_ALLOW_ZERO),
			"%s: Zero divisor and CLK_DIVIDER_ALLOW_ZERO not set\n",
			clk_hw_get_name(hw));
		return parent_rate;
	}
}

This causes two problems on clocks using a clk_div_table (EN7523_CLK_BUS,
EN7523_CLK_SLIC, and EN7523_CLK_SPI):

First, evaluating a valid rate that is derived from a subsequent entry in
desc->base_values[] (such as 67.5 MHz on EN7523_CLK_BUS from 540 MHz / 8, or
a rate on EN7523_CLK_SLIC derived from slic_base[1] = 3.125 MHz) will fail
divider_get_val() on the first base rate and trigger a kernel WARN() before
moving on to the next base rate.

Second, if a caller requests a rate equal to parent_rate on a clock where
divisor 1 is not supported (for example, 600 MHz or 540 MHz on EN7523_CLK_BUS
where divisor 1 is n/a in bus_div, or 100 MHz or 3.125 MHz on
EN7523_CLK_SLIC), divider_recalc_rate() returns parent_rate, which matches
tmp_rate == rate. en7581_get_vals_for_rate() then succeeds with *div_val set
to (u32)-EINVAL, and en7581_set_rate() writes the low bits of -EINVAL into
the hardware divider register via regmap_update_bits().

Should en7581_get_vals_for_rate() check if (tmp_div_val < 0) and continue to
the next base rate before calling divider_recalc_rate()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105150207.23023-1-ansuelsmth@gmail.com?part=2