| Message ID | 20250120084719.63116-1-abelova@astralinux.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clk: sunxi: add explicit casting to prevent overflow | expand |
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. Since req->rate is 'signed long' and the value is a frequency it is only just possible that it exceeds 31 bits (and will be wrong on 32bit builds - but sun-9 might be 64bit only?) 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; 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;
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; > >
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; > > > > >
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;
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. 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(-)