Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Tue, 19 Dec 2023 17:30:00 +0100 | Subject | Re: [PATCH v2] fix overflow in idle exit_latency |
| |
On Tue, Dec 19, 2023 at 5:24 PM David Laight <David.Laight@aculab.com> wrote: > > From: Bo Ye > > Sent: 19 December 2023 03:15 > > > > From: C Cheng <C.Cheng@mediatek.com> > > > > In detail: > > > > In C language, when you perform a multiplication operation, if > > both operands are of int type, the multiplication operation is > > performed on the int type, and then the result is converted to > > the target type. This means that if the product of int type > > multiplication exceeds the range that int type can represent, > > an overflow will occur even if you store the result in a > > variable of int64_t type. > > > > For a multiplication of two int values, it is better to use > > mul_u32_u32() rather than s->exit_latency_ns = s->exit_latency * > > NSEC_PER_USEC to avoid potential overflow happenning. > > > > Signed-off-by: C Cheng <C.Cheng@mediatek.com> > > Signed-off-by: Bo Ye <bo.ye@mediatek.com> > > --- > > Change in v2: > > -remove Change-ID > > -correct patch author name > > -replace multiplication of two int values with mul_u32_u32 > > -refine commit message > > --- > > drivers/cpuidle/driver.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c > > index d9cda7f6ccb9..cf5873cc45dc 100644 > > --- a/drivers/cpuidle/driver.c > > +++ b/drivers/cpuidle/driver.c > > @@ -16,6 +16,7 @@ > > #include <linux/cpumask.h> > > #include <linux/tick.h> > > #include <linux/cpu.h> > > +#include <linux/math64.h> > > > > #include "cpuidle.h" > > > > @@ -187,7 +188,7 @@ static void __cpuidle_driver_init(struct cpuidle_driver *drv) > > s->target_residency = div_u64(s->target_residency_ns, NSEC_PER_USEC); > > > > if (s->exit_latency > 0) > > - s->exit_latency_ns = s->exit_latency * NSEC_PER_USEC; > > + s->exit_latency_ns = mul_u32_u32(s->exit_latency, NSEC_PER_USEC); > > Just force either side of the multiply to a 64bit unsigned type. > The compiler will then DTRT which is likely to be better code than > whatever mul_u32_u32() generates.
So why is it there?
The default implementation of mul_u32_u32() is to cast its first argument to u64 before the multiplication AFAICS.
> In any case is the 'exit_latency' ever going to be greater than 4 seconds? > In which case the 32bit multiply will never overflow.
So this is more of a theoretical thing found by some static analysis tool or similar.
> > else if (s->exit_latency_ns < 0) > > s->exit_latency_ns = 0; > > else > > --
| |