Re: [PATCH] clk: sunxi-ng: nkm: avoid 32-bit overflow in rate calculations

["Jernej Škrabec" <jernej.skrabec@gmail.com>]

Dne nedelja, 1. november 2026 ob 21:33:36 Srednjeevropski standardni čas je Romain TISSERAND napisal(a):
> The N-K-M rate code computes parent * n * k / m in an unsigned long,
> which wraps on 32-bit ARM as soon as parent * n * k passes 4.29 GHz.
> 
> On the A33, pll-mipi (parent pll-video at 297 MHz) with N = 14, K = 2,
> M = 14 runs at 594 MHz, but 297 MHz * 14 * 2 wraps around to
> 4021032704 and the code computes 287.2 MHz. ccu_nkm_find_best() then
> picks these factors for a 288 MHz request, and clk_get_rate() reports
> 287.2 MHz while the PLL runs at twice that. On a PocketGo S30 (A33,
> ILI9488 panel on one MIPI-DSI lane) the lane ended up at about
> 594 Mbit/s instead of 288 and the picture broke up.
> 
> Do the multiplications in 64 bits, saturating at ULONG_MAX, in
> ccu_nkm_recalc_rate(), in both best-rate searches, and for the parent
> rate request of the parent-adjusting search, where rate * m overflows
> the same way.
> 
> Without the saturation, a candidate above 4.29 GHz in the search would
> be truncated again on return and could look like a valid low rate.
> 
> 64-bit kernels are not affected. On 32-bit ones, every N-K-M clock can
> hit it (A31, A23, A33, R40, H3, V3s and F1C100s use them), whenever the
> parent rate times N * K goes above 2^32.
> 
> Fixes: df6561e60244 ("clk: sunxi-ng: Add N-K-M Factor clock")
> Fixes: 3492e4f6db5f ("clk: sunxi-ng: nkm: consider alternative parent rates when determining rate")
> Signed-off-by: Romain TISSERAND <romain.tisserand@gmail.com>

Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej

> ---
> Notes:
> - Testing: built with W=1 for arm (multi_v7_defconfig with the A31, A23,
>   A33, R40 and H3 CCUs) and arm64 (defconfig), SUNXI_CCU=y and =m, no
>   warnings; on arm the 64-bit divisions go through __do_div64 (no libgcc
>   helper). Tested on a PocketGo S30 (A33) running 6.18.54 with this
>   change, without the ULONG_MAX saturation (same results for rates that
>   fit, which is the only case the hardware hits): before, pll-mipi was
>   programmed with N = 14, K = 2, M = 14 (594 MHz measured as a 2x lane
>   rate) while clk_get_rate() returned 287216621; after, N = 15, K = 1,
>   M = 16, 278437500 Hz in both the register and clk_get_rate(), and the
>   panel picture is right. This version, with the saturation, then on
>   the same board on 7.3.1: pll-mipi again programmed with N = 15, K = 1,
>   M = 16 (278.4 MHz from the 297 MHz pll-video0) and the picture is
>   right. Not tested on the other SoCs.
> - Tool disclosure: this patch was prepared with Claude Opus 5.5
>   (Anthropic) as a coding assistant, in an interactive session where I
>   asked it to bring up the PocketGo S30 display. It found the mismatch
>   between the pll-mipi register and the rate the clock framework
>   reported, traced it to this overflow and drafted the fix and the
>   message. I reviewed and understand the change and tested it as
>   described above.
> 
>  drivers/clk/sunxi-ng/ccu_nkm.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.c b/drivers/clk/sunxi-ng/ccu_nkm.c
> index 401fbb7524..e5a8f22d50 100644
> --- a/drivers/clk/sunxi-ng/ccu_nkm.c
> +++ b/drivers/clk/sunxi-ng/ccu_nkm.c
> @@ -6,6 +6,7 @@
>  
>  #include <linux/clk-provider.h>
>  #include <linux/io.h>
> +#include <linux/math64.h>
>  
>  #include "ccu_gate.h"
>  #include "ccu_nkm.h"
> @@ -30,6 +31,26 @@ static bool ccu_nkm_is_valid_rate(struct ccu_common *common, unsigned long paren
>  	return true;
>  }
>  
> +static unsigned long ccu_nkm_calc_rate(unsigned long parent,
> +				       unsigned long n, unsigned long k,
> +				       unsigned long m)
> +{
> +	u64 rate = (u64)parent * n * k;
> +
> +	do_div(rate, m);
> +	return min_t(u64, rate, ULONG_MAX);
> +}
> +
> +static unsigned long ccu_nkm_calc_parent_rate(unsigned long rate,
> +					      unsigned long n, unsigned long k,
> +					      unsigned long m)
> +{
> +	u64 parent = (u64)rate * m;
> +
> +	do_div(parent, n * k);
> +	return min_t(u64, parent, ULONG_MAX);
> +}
> +
>  static unsigned long ccu_nkm_find_best_with_parent_adj(struct ccu_common *common,
>  						       struct clk_hw *parent_hw,
>  						       unsigned long *parent, unsigned long rate,
> @@ -44,12 +65,13 @@ static unsigned long ccu_nkm_find_best_with_parent_adj(struct ccu_common *common
>  			for (_m = nkm->min_m; _m <= nkm->max_m; _m++) {
>  				unsigned long tmp_rate, tmp_parent;
>  
> -				tmp_parent = clk_hw_round_rate(parent_hw, rate * _m / (_n * _k));
> +				tmp_parent = ccu_nkm_calc_parent_rate(rate, _n, _k, _m);
> +				tmp_parent = clk_hw_round_rate(parent_hw, tmp_parent);
>  
>  				if (!ccu_nkm_is_valid_rate(common, tmp_parent, _n, _m))
>  					continue;
>  
> -				tmp_rate = tmp_parent * _n * _k / _m;
> +				tmp_rate = ccu_nkm_calc_rate(tmp_parent, _n, _k, _m);
>  
>  				if (ccu_is_better_rate(common, rate, tmp_rate, best_rate) ||
>  				    (tmp_parent == *parent && tmp_rate == best_rate)) {
> @@ -87,7 +109,7 @@ static unsigned long ccu_nkm_find_best(unsigned long parent, unsigned long rate,
>  
>  				unsigned long tmp_rate;
>  
> -				tmp_rate = parent * _n * _k / _m;
> +				tmp_rate = ccu_nkm_calc_rate(parent, _n, _k, _m);
>  
>  				if (ccu_is_better_rate(common, rate, tmp_rate, best_rate)) {
>  					best_rate = tmp_rate;
> @@ -154,7 +176,7 @@ static unsigned long ccu_nkm_recalc_rate(struct clk_hw *hw,
>  	if (!m)
>  		m++;
>  
> -	rate = parent_rate * n  * k / m;
> +	rate = ccu_nkm_calc_rate(parent_rate, n, k, m);
>  
>  	if (nkm->common.features & CCU_FEATURE_FIXED_POSTDIV)
>  		rate /= nkm->fixed_post_div;
> 
> base-commit: 5243bd55dfe63a5f806622878f6fbdd78172fac9
>