[1/2] clocksource: imx-tpm: fix return -ETIME when delta exceeds INT_MAX

Commit Message

Frank Li July 25, 2024, 7:33 p.m. UTC

From: Jacky Bai <[e-mail]>

In tpm_set_next_event(delta), return -ETIME by wrong cast to int when delta
is larger than INT_MAX.

For example:

tpm_set_next_event(delta = 0xffff_fffe)
{
        ...
        next = tpm_read_counter(); // assume next is 0x10
        next += delta; // next will 0xffff_fffe + 0x10 = 0x1_0000_000e
        now = tpm_read_counter();  // now is 0x10
        ...

        return (int)(next - now) <= 0 ? -ETIME : 0;
                     ^^^^^^^^^^
                     0x1_0000_000e - 0x10 = 0xffff_fffe, which is -2 when
                     cast to int. So return -ETIME.
}

To fix this, introduce a 'prev' variable and check if 'now - prev' is
larger than delta.

Cc: <[e-mail]>
Fixes: 059ab7b82eec ("clocksource/drivers/imx-tpm: Add imx tpm timer support")
Signed-off-by: Jacky Bai <[e-mail]>
Reviewed-by: Peng Fan <[e-mail]>
Reviewed-by: Ye Li <[e-mail]>
Reviewed-by: Jason Liu <[e-mail]>
Signed-off-by: Frank Li <[e-mail]>
---
 drivers/clocksource/timer-imx-tpm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Frank Li Aug. 14, 2024, 4:47 p.m. UTC | #1

On Thu, Jul 25, 2024 at 03:33:54PM -0400, Frank Li wrote:
> From: Jacky Bai <[e-mail]>
>
> In tpm_set_next_event(delta), return -ETIME by wrong cast to int when delta
> is larger than INT_MAX.
>
> For example:
>
> tpm_set_next_event(delta = 0xffff_fffe)
> {
>         ...
>         next = tpm_read_counter(); // assume next is 0x10
>         next += delta; // next will 0xffff_fffe + 0x10 = 0x1_0000_000e
>         now = tpm_read_counter();  // now is 0x10
>         ...
>
>         return (int)(next - now) <= 0 ? -ETIME : 0;
>                      ^^^^^^^^^^
>                      0x1_0000_000e - 0x10 = 0xffff_fffe, which is -2 when
>                      cast to int. So return -ETIME.
> }
>
> To fix this, introduce a 'prev' variable and check if 'now - prev' is
> larger than delta.
>
> Cc: <[e-mail]>
> Fixes: 059ab7b82eec ("clocksource/drivers/imx-tpm: Add imx tpm timer support")
> Signed-off-by: Jacky Bai <[e-mail]>
> Reviewed-by: Peng Fan <[e-mail]>
> Reviewed-by: Ye Li <[e-mail]>
> Reviewed-by: Jason Liu <[e-mail]>
> Signed-off-by: Frank Li <[e-mail]>
> ---

Ping?


>  drivers/clocksource/timer-imx-tpm.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clocksource/timer-imx-tpm.c b/drivers/clocksource/timer-imx-tpm.c
> index bd64a8a8427f3..cd23caf1e5999 100644
> --- a/drivers/clocksource/timer-imx-tpm.c
> +++ b/drivers/clocksource/timer-imx-tpm.c
> @@ -83,10 +83,10 @@ static u64 notrace tpm_read_sched_clock(void)
>  static int tpm_set_next_event(unsigned long delta,
>  				struct clock_event_device *evt)
>  {
> -	unsigned long next, now;
> +	unsigned long next, prev, now;
>
> -	next = tpm_read_counter();
> -	next += delta;
> +	prev = tpm_read_counter();
> +	next = prev + delta;
>  	writel(next, timer_base + TPM_C0V);
>  	now = tpm_read_counter();
>
> @@ -96,7 +96,7 @@ static int tpm_set_next_event(unsigned long delta,
>  	 * of writing CNT registers which may cause the min_delta event got
>  	 * missed, so we need add a ETIME check here in case it happened.
>  	 */
> -	return (int)(next - now) <= 0 ? -ETIME : 0;
> +	return (now - prev) >= delta ? -ETIME : 0;
>  }
>
>  static int tpm_set_state_oneshot(struct clock_event_device *evt)
> --
> 2.34.1
>

Daniel Lezcano Aug. 19, 2024, 1:21 p.m. UTC | #2

On 25/07/2024 21:33, Frank Li wrote:
> From: Jacky Bai <[e-mail]>
> 
> In tpm_set_next_event(delta), return -ETIME by wrong cast to int when delta
> is larger than INT_MAX.

Is it something you observed or from your understanding of the code ?

> For example:
> 
> tpm_set_next_event(delta = 0xffff_fffe)
> {
>          ...
>          next = tpm_read_counter(); // assume next is 0x10
>          next += delta; // next will 0xffff_fffe + 0x10 = 0x1_0000_000e
>          now = tpm_read_counter();  // now is 0x10
>          ...
> 
>          return (int)(next - now) <= 0 ? -ETIME : 0;
>                       ^^^^^^^^^^
>                       0x1_0000_000e - 0x10 = 0xffff_fffe, which is -2 when
>                       cast to int. So return -ETIME.
> }
> 
> To fix this, introduce a 'prev' variable and check if 'now - prev' is
> larger than delta.
> 
> Cc: <[e-mail]>
> Fixes: 059ab7b82eec ("clocksource/drivers/imx-tpm: Add imx tpm timer support")
> Signed-off-by: Jacky Bai <[e-mail]>
> Reviewed-by: Peng Fan <[e-mail]>
> Reviewed-by: Ye Li <[e-mail]>
> Reviewed-by: Jason Liu <[e-mail]>
> Signed-off-by: Frank Li <[e-mail]>
> ---
>   drivers/clocksource/timer-imx-tpm.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-imx-tpm.c b/drivers/clocksource/timer-imx-tpm.c
> index bd64a8a8427f3..cd23caf1e5999 100644
> --- a/drivers/clocksource/timer-imx-tpm.c
> +++ b/drivers/clocksource/timer-imx-tpm.c
> @@ -83,10 +83,10 @@ static u64 notrace tpm_read_sched_clock(void)
>   static int tpm_set_next_event(unsigned long delta,
>   				struct clock_event_device *evt)
>   {
> -	unsigned long next, now;
> +	unsigned long next, prev, now;
>   
> -	next = tpm_read_counter();
> -	next += delta;
> +	prev = tpm_read_counter();
> +	next = prev + delta;
>   	writel(next, timer_base + TPM_C0V);
>   	now = tpm_read_counter();
>   
> @@ -96,7 +96,7 @@ static int tpm_set_next_event(unsigned long delta,
>   	 * of writing CNT registers which may cause the min_delta event got
>   	 * missed, so we need add a ETIME check here in case it happened.
>   	 */
> -	return (int)(next - now) <= 0 ? -ETIME : 0;
> +	return (now - prev) >= delta ? -ETIME : 0;
>   }
>   
>   static int tpm_set_state_oneshot(struct clock_event_device *evt)

Frank Li Aug. 19, 2024, 6:43 p.m. UTC | #3

On Mon, Aug 19, 2024 at 03:21:36PM +0200, Daniel Lezcano wrote:
> On 25/07/2024 21:33, Frank Li wrote:
> > From: Jacky Bai <[e-mail]>
> >
> > In tpm_set_next_event(delta), return -ETIME by wrong cast to int when delta
> > is larger than INT_MAX.
>
> Is it something you observed or from your understanding of the code ?

System use this timer to wakeup from lower idle, which core and local timer
turn off. -ETIME may prevent system enter lower idle, or failure wakeup
from lower idle utill counter run overflow. I am not sure how to trigger
this problem.

Assume run at 24Mhz, 1/24us * 0x8000_0000 is around 89s. Problem trigger
only next event bigger 89s, which relative quite big number.

Anyways, the original code is wrong, it claim max delta is 32bit.

Jack bai: any additonal supplement for this?

Frank

>
> > For example:
> >
> > tpm_set_next_event(delta = 0xffff_fffe)
> > {
> >          ...
> >          next = tpm_read_counter(); // assume next is 0x10
> >          next += delta; // next will 0xffff_fffe + 0x10 = 0x1_0000_000e
> >          now = tpm_read_counter();  // now is 0x10
> >          ...
> >
> >          return (int)(next - now) <= 0 ? -ETIME : 0;
> >                       ^^^^^^^^^^
> >                       0x1_0000_000e - 0x10 = 0xffff_fffe, which is -2 when
> >                       cast to int. So return -ETIME.
> > }
> >
> > To fix this, introduce a 'prev' variable and check if 'now - prev' is
> > larger than delta.
> >
> > Cc: <[e-mail]>
> > Fixes: 059ab7b82eec ("clocksource/drivers/imx-tpm: Add imx tpm timer support")
> > Signed-off-by: Jacky Bai <[e-mail]>
> > Reviewed-by: Peng Fan <[e-mail]>
> > Reviewed-by: Ye Li <[e-mail]>
> > Reviewed-by: Jason Liu <[e-mail]>
> > Signed-off-by: Frank Li <[e-mail]>
> > ---
> >   drivers/clocksource/timer-imx-tpm.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/clocksource/timer-imx-tpm.c b/drivers/clocksource/timer-imx-tpm.c
> > index bd64a8a8427f3..cd23caf1e5999 100644
> > --- a/drivers/clocksource/timer-imx-tpm.c
> > +++ b/drivers/clocksource/timer-imx-tpm.c
> > @@ -83,10 +83,10 @@ static u64 notrace tpm_read_sched_clock(void)
> >   static int tpm_set_next_event(unsigned long delta,
> >   				struct clock_event_device *evt)
> >   {
> > -	unsigned long next, now;
> > +	unsigned long next, prev, now;
> > -	next = tpm_read_counter();
> > -	next += delta;
> > +	prev = tpm_read_counter();
> > +	next = prev + delta;
> >   	writel(next, timer_base + TPM_C0V);
> >   	now = tpm_read_counter();
> > @@ -96,7 +96,7 @@ static int tpm_set_next_event(unsigned long delta,
> >   	 * of writing CNT registers which may cause the min_delta event got
> >   	 * missed, so we need add a ETIME check here in case it happened.
> >   	 */
> > -	return (int)(next - now) <= 0 ? -ETIME : 0;
> > +	return (now - prev) >= delta ? -ETIME : 0;
> >   }
> >   static int tpm_set_state_oneshot(struct clock_event_device *evt)
>
>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog

Jacky Bai (OSS) Aug. 21, 2024, 7:20 a.m. UTC | #4

> Subject: Re: [PATCH 1/2] clocksource: imx-tpm: fix return -ETIME when delta
> exceeds INT_MAX
> 
> On Mon, Aug 19, 2024 at 03:21:36PM +0200, Daniel Lezcano wrote:
> > On 25/07/2024 21:33, Frank Li wrote:
> > > From: Jacky Bai <[e-mail]>
> > >
> > > In tpm_set_next_event(delta), return -ETIME by wrong cast to int
> > > when delta is larger than INT_MAX.
> >
> > Is it something you observed or from your understanding of the code ?
> 
> System use this timer to wakeup from lower idle, which core and local timer turn
> off. -ETIME may prevent system enter lower idle, or failure wakeup from lower
> idle utill counter run overflow. I am not sure how to trigger this problem.
> 
> Assume run at 24Mhz, 1/24us * 0x8000_0000 is around 89s. Problem trigger
> only next event bigger 89s, which relative quite big number.
> 
> Anyways, the original code is wrong, it claim max delta is 32bit.
> 
> Jack bai: any additonal supplement for this?
>

The issue can be triggered in below function, it is possible max_delta will be assigned to delta,
So the original int case logic will report wrong '-ETIME' error, then 'clockevents_program_min_delta'
Will be called to reprogram the timer, due to the tpm limitation mentioned in patch 2/2, this min_delta
program will not take effect.

int clockevents_program_event(struct clock_event_device *dev, ktime_t expires,
                        bool force) {
	...
	delta = min(delta, (int64_t) dev->max_delta_ns);
	delta = max(delta, (int64_t) dev->min_delta_ns);

	clc = ((unsigned long long) delta * dev->mult) >> dev->shift;
	rc = dev->set_next_event((unsigned long) clc, dev);

	return (rc && force) ? clockevents_program_min_delta(dev) : rc;
}

BR
> Frank
> 
> >
> > > For example:
> > >
> > > tpm_set_next_event(delta = 0xffff_fffe) {
> > >          ...
> > >          next = tpm_read_counter(); // assume next is 0x10
> > >          next += delta; // next will 0xffff_fffe + 0x10 = 0x1_0000_000e
> > >          now = tpm_read_counter();  // now is 0x10
> > >          ...
> > >
> > >          return (int)(next - now) <= 0 ? -ETIME : 0;
> > >                       ^^^^^^^^^^
> > >                       0x1_0000_000e - 0x10 = 0xffff_fffe, which is -2
> when
> > >                       cast to int. So return -ETIME.
> > > }
> > >
> > > To fix this, introduce a 'prev' variable and check if 'now - prev'
> > > is larger than delta.
> > >
> > > Cc: <[e-mail]>
> > > Fixes: 059ab7b82eec ("clocksource/drivers/imx-tpm: Add imx tpm timer
> > > support")
> > > Signed-off-by: Jacky Bai <[e-mail]>
> > > Reviewed-by: Peng Fan <[e-mail]>
> > > Reviewed-by: Ye Li <[e-mail]>
> > > Reviewed-by: Jason Liu <[e-mail]>
> > > Signed-off-by: Frank Li <[e-mail]>
> > > ---
> > >   drivers/clocksource/timer-imx-tpm.c | 8 ++++----
> > >   1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/clocksource/timer-imx-tpm.c
> > > b/drivers/clocksource/timer-imx-tpm.c
> > > index bd64a8a8427f3..cd23caf1e5999 100644
> > > --- a/drivers/clocksource/timer-imx-tpm.c
> > > +++ b/drivers/clocksource/timer-imx-tpm.c
> > > @@ -83,10 +83,10 @@ static u64 notrace tpm_read_sched_clock(void)
> > >   static int tpm_set_next_event(unsigned long delta,
> > >   				struct clock_event_device *evt)
> > >   {
> > > -	unsigned long next, now;
> > > +	unsigned long next, prev, now;
> > > -	next = tpm_read_counter();
> > > -	next += delta;
> > > +	prev = tpm_read_counter();
> > > +	next = prev + delta;
> > >   	writel(next, timer_base + TPM_C0V);
> > >   	now = tpm_read_counter();
> > > @@ -96,7 +96,7 @@ static int tpm_set_next_event(unsigned long delta,
> > >   	 * of writing CNT registers which may cause the min_delta event got
> > >   	 * missed, so we need add a ETIME check here in case it happened.
> > >   	 */
> > > -	return (int)(next - now) <= 0 ? -ETIME : 0;
> > > +	return (now - prev) >= delta ? -ETIME : 0;
> > >   }
> > >   static int tpm_set_state_oneshot(struct clock_event_device *evt)
> >
> >
> > --
> > <http://www.linaro.org/> Linaro.org │ Open source software for ARM
> > SoCs
> >
> > Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> > <http://twitter.com/#!/linaroorg> Twitter |
> > <http://www.linaro.org/linaro-blog/> Blog

Daniel Lezcano Sept. 2, 2024, 9:29 a.m. UTC | #5

On 25/07/2024 21:33, Frank Li wrote:
> From: Jacky Bai <[e-mail]>
> 
> In tpm_set_next_event(delta), return -ETIME by wrong cast to int when delta
> is larger than INT_MAX.
> 
> For example:
> 
> tpm_set_next_event(delta = 0xffff_fffe)
> {
>          ...
>          next = tpm_read_counter(); // assume next is 0x10
>          next += delta; // next will 0xffff_fffe + 0x10 = 0x1_0000_000e
>          now = tpm_read_counter();  // now is 0x10
>          ...
> 
>          return (int)(next - now) <= 0 ? -ETIME : 0;
>                       ^^^^^^^^^^
>                       0x1_0000_000e - 0x10 = 0xffff_fffe, which is -2 when
>                       cast to int. So return -ETIME.
> }
> 
> To fix this, introduce a 'prev' variable and check if 'now - prev' is
> larger than delta.
> 
> Cc: <[e-mail]>
> Fixes: 059ab7b82eec ("clocksource/drivers/imx-tpm: Add imx tpm timer support")
> Signed-off-by: Jacky Bai <[e-mail]>
> Reviewed-by: Peng Fan <[e-mail]>
> Reviewed-by: Ye Li <[e-mail]>
> Reviewed-by: Jason Liu <[e-mail]>
> Signed-off-by: Frank Li <[e-mail]>
> ---

Applied, thanks

Patch

diff --git a/drivers/clocksource/timer-imx-tpm.c b/drivers/clocksource/timer-imx-tpm.c
index bd64a8a8427f3..cd23caf1e5999 100644
--- a/drivers/clocksource/timer-imx-tpm.c
+++ b/drivers/clocksource/timer-imx-tpm.c
@@ -83,10 +83,10 @@  static u64 notrace tpm_read_sched_clock(void)
 static int tpm_set_next_event(unsigned long delta,
 				struct clock_event_device *evt)
 {
-	unsigned long next, now;
+	unsigned long next, prev, now;
 
-	next = tpm_read_counter();
-	next += delta;
+	prev = tpm_read_counter();
+	next = prev + delta;
 	writel(next, timer_base + TPM_C0V);
 	now = tpm_read_counter();
 
@@ -96,7 +96,7 @@  static int tpm_set_next_event(unsigned long delta,
 	 * of writing CNT registers which may cause the min_delta event got
 	 * missed, so we need add a ETIME check here in case it happened.
 	 */
-	return (int)(next - now) <= 0 ? -ETIME : 0;
+	return (now - prev) >= delta ? -ETIME : 0;
 }
 
 static int tpm_set_state_oneshot(struct clock_event_device *evt)