diff mbox series

cpuidle: idle exit_latency overflow

Message ID 20231116132619.69500-1-bo.ye@mediatek.com (mailing list archive)
State New, archived
Headers show
Series cpuidle: idle exit_latency overflow | expand

Commit Message

Bo Ye Nov. 16, 2023, 1:26 p.m. UTC
From: mtk24676 <C.Cheng@mediatek.com>

In detail:
In kernel-6.1, in the __cpuidle_driver_init function in
driver/cpuidle/driver.c, there is a line of code that causes
an overflow. The line is s->exit_latency_ns = s->exit_latency
* NSEC_PER_USEC. The overflow occurs because the product of an
int type and a constant exceeds the range of the int type.

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.

Signed-off-by: mtk24676 <C.Cheng@mediatek.com>
Signed-off-by: bo.ye <bo.ye@mediatek.com>
---

Comments

AngeloGioacchino Del Regno Nov. 16, 2023, 2:20 p.m. UTC | #1
Il 16/11/23 14:26, Bo Ye ha scritto:
> From: mtk24676 <C.Cheng@mediatek.com>
> 
> In detail:
> In kernel-6.1, in the __cpuidle_driver_init function in
> driver/cpuidle/driver.c, there is a line of code that causes
> an overflow. The line is s->exit_latency_ns = s->exit_latency
> * NSEC_PER_USEC. The overflow occurs because the product of an
> int type and a constant exceeds the range of the int type.
> 
> 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.
> 
> Signed-off-by: mtk24676 <C.Cheng@mediatek.com>

It all makes sense, but "mtk24676" is not the name of a person, what's going on?

> Signed-off-by: bo.ye <bo.ye@mediatek.com>

That's the same, you perhaps wanted to write "Bo Ye", not "bo.ye".

Regards,
Angelo

> ---
> 
> diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
> index d9cda7f..631ca16 100644
> --- a/drivers/cpuidle/driver.c
> +++ b/drivers/cpuidle/driver.c
> @@ -187,7 +187,7 @@
>   			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 = (u64)s->exit_latency * NSEC_PER_USEC;
>   		else if (s->exit_latency_ns < 0)
>   			s->exit_latency_ns =  0;
>   		else
Bo Ye Nov. 16, 2023, 2:52 p.m. UTC | #2
On Thu, 2023-11-16 at 15:20 +0100, AngeloGioacchino Del Regno wrote:
> Il 16/11/23 14:26, Bo Ye ha scritto:
> > From: C Cheng <C.Cheng@mediatek.com>
> > 
> > In detail:
> > In kernel-6.1, in the __cpuidle_driver_init function in
> > driver/cpuidle/driver.c, there is a line of code that causes
> > an overflow. The line is s->exit_latency_ns = s->exit_latency
> > * NSEC_PER_USEC. The overflow occurs because the product of an
> > int type and a constant exceeds the range of the int type.
> > 
> > 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.
> > 
> > Signed-off-by: C Cheng <C.Cheng@mediatek.com>
> 
> It all makes sense, but "mtk24676" is not the name of a person,
> what's going on?
  Corrected, apologies for any inconvenience caused.
> 
> > Signed-off-by: Bo Ye <bo.ye@mediatek.com>
> 
> That's the same, you perhaps wanted to write "Bo Ye", not "bo.ye".
  Corrected, apologies for any inconvenience caused. I would greatly
appreciate it if you could recommend a formatting checking tool.
  If there is no auto checking tool, I will pay attention to these
formatting checks next time.

Best Regards,
Bosser
> 
> Regards,
> Angelo
> 
> > ---
> > 
> > diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
> > index d9cda7f..631ca16 100644
> > --- a/drivers/cpuidle/driver.c
> > +++ b/drivers/cpuidle/driver.c
> > @@ -187,7 +187,7 @@
> >   			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 = (u64)s->exit_latency *
> > NSEC_PER_USEC;
> >   		else if (s->exit_latency_ns < 0)
> >   			s->exit_latency_ns =  0;
> >   		else
> 
>
Rafael J. Wysocki Dec. 11, 2023, 8:52 p.m. UTC | #3
On Thu, Nov 16, 2023 at 2:26 PM Bo Ye <bo.ye@mediatek.com> wrote:
>
> From: mtk24676 <C.Cheng@mediatek.com>
>
> In detail:
> In kernel-6.1, in the __cpuidle_driver_init function in
> driver/cpuidle/driver.c, there is a line of code that causes
> an overflow. The line is s->exit_latency_ns = s->exit_latency
> * NSEC_PER_USEC. The overflow occurs because the product of an
> int type and a constant exceeds the range of the int type.

In general, but does it actually occur in that code?  IOW, is there
any system for which the exit latency of an idle state is so large
that it will trigger the overflow, for example?

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

Right, that's how C works.

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

True.  However, does this really happen in the particular case at hand?

If not, it would be better to say something like "For a multiplication
of two int values, it is better to use mul_u32_u32() that prevents
overflows from occurring."

> Signed-off-by: mtk24676 <C.Cheng@mediatek.com>
> Signed-off-by: bo.ye <bo.ye@mediatek.com>
> ---
>
> diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
> index d9cda7f..631ca16 100644
> --- a/drivers/cpuidle/driver.c
> +++ b/drivers/cpuidle/driver.c
> @@ -187,7 +187,7 @@
>                         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 = (u64)s->exit_latency * NSEC_PER_USEC;

mul_u32_u32()/?

>                 else if (s->exit_latency_ns < 0)
>                         s->exit_latency_ns =  0;
>                 else
diff mbox series

Patch

diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
index d9cda7f..631ca16 100644
--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -187,7 +187,7 @@ 
 			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 = (u64)s->exit_latency * NSEC_PER_USEC;
 		else if (s->exit_latency_ns < 0)
 			s->exit_latency_ns =  0;
 		else