Re: [PATCH] clk: sunxi: add explicit casting to prevent overflow

[Andre Przywara <andre.przywara@arm.com>]

On Wed, 22 Jan 2025 22:58:05 +0000
David Laight <david.laight.linux@gmail.com> wrote:

Hi,

please note that this is all practically irrelevant:
- PLL4 is PLL_PERIPH0, which is meant to be fixed to 960MHz. Linux
  would not change this frequency.
- the Allwinner A80 is both old and quite rare/obscure: the most
  prominent board (Cubieboard4) was broken for a while and nobody
  noticed
- this "allwinner,sun9i-a80-pll4-clk" clock is not used by any DT
  in the kernel, so it's effectively dead code

But just for sports:

> On Mon, 20 Jan 2025 11:47:16 +0300
> Anastasia Belova <abelova@astralinux.ru> wrote:
> 
> > If n = 255, the result of multiplication of n and 24000000
> > may not fit int type. Add explicit casting to prevent overflow.
> > 
> > Found by Linux Verification Center (linuxtesting.org) with SVACE.  
> 
> You need to read and understand the code before writing any patches.
> The '>> p' and '/ (m + 1)' are both just conditional 'divide by 2'.
> So can be done before the multiply.

Well, normally you would try to multiply first, then divide, to avoid
losing precision. In this case it's fine, since it's just dividing by 2
or 4, and 24E6 is dividable by both, so no loss. But the formula in the
data sheet is written as "24MHz*N/(Input_div+1)/(Output_div+1)", which
matches the code (somewhat).
So I think it's indeed better to divide first here, to avoid using
heavy library based 64-bit mul/div algorithms, just for this one corner
case, but it would need a comment, to point to the problem and avoid
people "fixing it back".

> Since req->rate is 'signed long' and the value is a frequency it is

struct factors_request.rate is "unsigned long"

> only just possible that it exceeds 31 bits (and will be wrong on 32bit
> builds - but sun-9 might be 64bit only?)

The A80 has Cortex-A7 cores, so it's 32-bit only. The SoC can address
more than 4GB, but that's not relevant here.
 
> In any case it would be sensible to force an unsigned divide.
> So perhaps:
> 	unsigned int n = DIV_ROUND_UP(req->rate, 6000000ul);
> 	...
> 	req->rate = ((24000000ul >> p) / (m + 1)) * n;

Yeah, I don't think we need the "long" qualifier, but this looks like
indeed the best solution, just with an added comment. And we probably
want to change the type of "p" and "m" to u8 on the way, to match the
struct and make them unsigned as well.

Cheers,
Andre
 

> 
> David
> 
> > 
> > Fixes: 6424e0aeebc4 ("clk: sunxi: rewrite sun9i_a80_get_pll4_factors()")
> > Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
> > ---
> >  drivers/clk/sunxi/clk-sun9i-core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/sunxi/clk-sun9i-core.c b/drivers/clk/sunxi/clk-sun9i-core.c
> > index d93c7a53c6c0..70fbd7390d96 100644
> > --- a/drivers/clk/sunxi/clk-sun9i-core.c
> > +++ b/drivers/clk/sunxi/clk-sun9i-core.c
> > @@ -50,7 +50,7 @@ static void sun9i_a80_get_pll4_factors(struct factors_request *req)
> >  	else if (n < 12)
> >  		n = 12;
> >  
> > -	req->rate = ((24000000 * n) >> p) / (m + 1);
> > +	req->rate = ((24000000ULL * n) >> p) / (m + 1);
> >  	req->n = n;
> >  	req->m = m;
> >  	req->p = p;  
> 
>