Re: [PATCH v2 03/12] clk: divider: Introduce CLK_DIVIDER_EVEN_INTEGERS_NO_OFFSET flag

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] divider_ro_determine_rate() incorrectly returns -EINVAL for zero divisors when CLK_DIVIDER_ALLOW_ZERO is set, breaking valid pass-through states.

Pre-existing issues:
- [High] The pre-existing CLK_DIVIDER_EVEN_INTEGERS flag suffers from the exact same odd-divisor truncation bug described in the commit, but was left unfixed.
- [High] _div_round_closest() triggers a division-by-zero panic when the requested rate exceeds the parent rate.
--

--- Patch [3]: [PATCH v2 03/12] clk: divider: Introduce CLK_DIVIDER_EVEN_INTEGERS_NO_OFFSET flag ---
commit 2989085a3446885e07078514557377e58a8cade1
Author: Luo Jie <[email protected]>

clk: divider: Introduce CLK_DIVIDER_EVEN_INTEGERS_NO_OFFSET flag

This patch adds a new CLK_DIVIDER_EVEN_INTEGERS_NO_OFFSET flag for hardware
dividers that derive an even divisor directly from the raw register value 
(2 * val). It updates the divisor conversion functions, extends validation 
to reject odd divisors, and ensures rounding maps to even divisors to 
prevent truncation errors.

> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> index b3b485d23ea85..f062d77ee106b 100644
> --- a/drivers/clk/clk-divider.c
> +++ b/drivers/clk/clk-divider.c

[ ... ]

> @@ -181,6 +187,8 @@ static bool _is_valid_div(const struct clk_div_table *table, unsigned int div,
>  {
>  	if (flags & CLK_DIVIDER_POWER_OF_TWO)
>  		return is_power_of_2(div);
> +	if (flags & CLK_DIVIDER_EVEN_INTEGERS_NO_OFFSET)
> +		return div >= 2 && !(div & 1);

[Severity: High]
This is a pre-existing issue, but since this patch aims to prevent odd
divisors from being silently decoded into the wrong even value for the new
flag, shouldn't the pre-existing CLK_DIVIDER_EVEN_INTEGERS flag also reject
odd divisors here?

If _is_valid_div() allows an odd divisor (like 3) for
CLK_DIVIDER_EVEN_INTEGERS, _get_val() will map it to 0, causing the
hardware to run at a potentially out-of-spec faster rate (parent / 2).

>  	if (table)
>  		return _is_valid_table_div(table, div);

[ ... ]

> @@ -230,6 +238,8 @@ static int _div_round_up(const struct clk_div_table *table,
>  
>  	if (flags & CLK_DIVIDER_POWER_OF_TWO)
>  		div = __roundup_pow_of_two(div);
> +	else if (flags & CLK_DIVIDER_EVEN_INTEGERS_NO_OFFSET)
> +		div = max(2, (div + 1) & ~1);

[Severity: High]
This is a pre-existing issue, but does the original
CLK_DIVIDER_EVEN_INTEGERS flag also need this rounding logic to force even
integers?

>  	if (table)
>  		div = _round_up_table(table, div);

[ ... ]

> @@ -249,6 +259,9 @@ static int _div_round_closest(const struct clk_div_table *table,
>  	if (flags & CLK_DIVIDER_POWER_OF_TWO) {
>  		up = __roundup_pow_of_two(up);
>  		down = __rounddown_pow_of_two(down);
> +	} else if (flags & CLK_DIVIDER_EVEN_INTEGERS_NO_OFFSET) {
> +		up = max(2, (up + 1) & ~1);
> +		down = max(2, down & ~1);
>  	} else if (table) {

[Severity: High]
This isn't a bug introduced by this patch, but can down evaluate to 0
here and cause a division-by-zero panic?

Looking earlier in _div_round_closest(), down is computed as:

    down = parent_rate / rate;

If a caller requests a rate strictly greater than parent_rate, down
evaluates to 0. When later passed to DIV_ROUND_UP_ULL() in this function:

    down_rate = DIV_ROUND_UP_ULL((u64)parent_rate, down);

Does this result in a kernel panic?

>  		up = _round_up_table(table, up);
>  		down = _round_down_table(table, down);

[ ... ]

> @@ -372,6 +387,13 @@ int divider_ro_determine_rate(struct clk_hw *hw, struct clk_rate_request *req,
>  
>  	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 -EINVAL;
> +	}
> +

[Severity: High]
Does this unconditionally return -EINVAL for a zero divisor, even when
CLK_DIVIDER_ALLOW_ZERO is set?

If the hardware register contains 0 and CLK_DIVIDER_ALLOW_ZERO is set
(which implies a pass-through clock), this correctly suppresses the
WARN, but still fails the rate determination by returning -EINVAL
instead of successfully propagating the parent rate.

>  	/* Even a read-only clock can propagate a rate change */
>  	if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
>  		if (!req->best_parent_hw)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3