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

[David Laight <david.laight.linux@gmail.com>]

On Thu, 23 Jan 2025 00:55:56 +0000
Andre Przywara <andre.przywara@arm.com> wrote:

> 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:

Doesn't surprise me ...

> 
> > 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).

That PLL can generate all sorts of frequencies due to the multiply
and divide (as well as the shift).
The code was clearly sub-optimal for arbitrary frequencies :-)
 
> 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.

I couldn't decide whether the code was for 32bit or not.
Using 'long' is pretty dubious almost everywhere.
I'm sure it is a hangover from people worried about int being 16bit.
But that has never been true for linux (or pretty much any unix since
the early 1980s).

>  
> > 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.

Maybe just mention it only need to generate 96MHz.

> 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.

Make them unsigned, but not u8.
The u8 would get promoted to signed int before any arithmetic.

	David

> 
> 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;    
> > 
> >   
>