Re: [PATCH] clk: sunxi-ng: Change NKMP algorithm for finding factors

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Failure to initialize output struct fields in `ccu_nkmp_find_best` leads to writing stack garbage to hardware registers.
- [High] The new algorithm divides by the parent clock rate, leading to a kernel panic if the parent rate is 0.
- [High] Regression in rate clamping: the new algorithm fails to return the closest valid rate when the requested rate is outside the achievable bounds.
- [Medium] The new approximate rate search algorithm skips valid and better clock multipliers by only evaluating the truncated `_n`.
--

--- Patch [1]: [PATCH] clk: sunxi-ng: Change NKMP algorithm for finding factors ---
commit d6ad51cc524475bf9f5a9b1a358305bd319886bf
Author: Ondrej Jirman <[email protected]>

clk: sunxi-ng: Change NKMP algorithm for finding factors

This patch optimizes the NKMP algorithm for finding clock factors by reducing 
unnecessary iterations. It introduces an approach that respects physical SoC 
restrictions and avoids brute-forcing all possibilities.

> diff --git a/drivers/clk/sunxi-ng/ccu_nkmp.c b/drivers/clk/sunxi-ng/ccu_nkmp.c
> index 25efb5b376075..c39b198914316 100644
> --- a/drivers/clk/sunxi-ng/ccu_nkmp.c
> +++ b/drivers/clk/sunxi-ng/ccu_nkmp.c

[ ... ]

> @@ -32,40 +32,70 @@ static unsigned long ccu_nkmp_calc_rate(unsigned long parent,
>  static unsigned long ccu_nkmp_find_best(unsigned long parent, unsigned long rate,
>  					struct _ccu_nkmp *nkmp)
>  {
> +	unsigned long _k, _m, _p;
> +	unsigned long max_p = rate >= 240000000 ? 1 : nkmp->max_p;
> +	unsigned approx;
>  	unsigned long best_rate = 0;

[ ... ]

> +	for (approx = 0; approx <= 1; approx++) {
> +		for (_p = nkmp->min_p; _p <= max_p; _p <<= 1) {
>  			for (_m = nkmp->min_m; _m <= nkmp->max_m; _m++) {
> +				u64 nk = (u64)rate * _m * _p;
> +				unsigned long nk_rem = do_div(nk, parent);

[Severity: High]
Does this code handle the case where parent is 0?

In the Linux clock framework, an orphaned clock or one with an absent parent
will have a parent rate of 0. Performing a division on such a clock will
trigger a division-by-zero hardware exception here.

> +
> +				if (nk_rem != 0 && !approx)
> +					continue;
> +
> +				if (nk < 10 || nk > 88)
> +					continue;
> +
> +				for (_k = nkmp->min_k; _k <= nkmp->max_k; _k++) {
> +					if (approx) {
> +						u64 _n = (u64)rate * _m * _p;
> +						unsigned long tmp_rate;
> +
> +						do_div(_n, parent * _k);

[Severity: High]
Can parent * _k evaluate to 0 if parent is 0, leading to a division-by-zero
exception here as well?

[Severity: Medium]
Will calculating _n via integer division skip valid and better multipliers?

Because the target rate is often already truncated by a previous round rate
operation, calculating _n analytically can yield an _n that is exactly 1 less
than the optimal value. By failing to check _n + 1, this logic could lock in
a suboptimal configuration.

> +
> +						if (_n < nkmp->min_n || _n > nkmp->max_n)
> +							continue;

[Severity: High]
Is it intended to silently skip configurations that exceed hardware bounds
instead of clamping to nkmp->max_n?

When drivers request an unachievably high clock rate to configure a clock
to its physical maximum, this logic explicitly rejects the out-of-bounds _n.

This results in the function finding zero valid configurations and returning
a rate of 0, which breaks clock initialization for drivers relying on standard
maximum-rate clamping behavior.

> +
> +						tmp_rate = ccu_nkmp_calc_rate(parent,
> +									      _n, _k,

[ ... ]

> -	nkmp->n = best_n;
> -	nkmp->k = best_k;
> -	nkmp->m = best_m;
> -	nkmp->p = best_p;
> -
>  	return best_rate;
>  }

[Severity: High]
If a valid combination cannot be found, could this return 0 without
initializing the nkmp fields?

If the clock framework calls ccu_nkmp_set_rate() with a rate that cannot
be satisfied, returning 0 leaves the multiplier and divider fields as
uninitialized stack variables.

These random stack values are then shifted and written directly into the
hardware clock configuration registers:

drivers/clk/sunxi-ng/ccu_nkmp.c:ccu_nkmp_set_rate() {
    ...
    reg |= ((_nkmp.n - nkmp->n.offset) << nkmp->n.shift) & n_mask;
    reg |= ((_nkmp.k - nkmp->k.offset) << nkmp->k.shift) & k_mask;
    reg |= ((_nkmp.m - nkmp->m.offset) << nkmp->m.shift) & m_mask;
    reg |= (ilog2(_nkmp.p) << nkmp->p.shift) & p_mask;
    ...
}

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