[POWERPC] Fix off-by-one error in setting decrementer on Book E

[POWERPC] Fix off-by-one error in setting decrementer on Book E

[Paul Mackerras]

The decrementer in Book E and 4xx processors interrupts on the
transition from 1 to 0, rather than on the 0 to -1 transition as on
64-bit server and 32-bit "classic" (6xx/7xx/7xxx) processors.

This fixes the problem by making set_dec subtract 1 from the count for
server and classic processors.  Since set_dec already had a bunch of
ifdefs to handle different processor types, there is no net increase
in ugliness. :)

This also removes a redundant call to set the decrementer to
0x7fffffff - it was already set to that earlier in timer_interrupt.

Signed-off-by: Paul Mackerras <paulus@samba.org>
---
diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index 9eb3284..5e253d6 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -586,7 +586,7 @@ void timer_interrupt(struct pt_regs * regs)
 		/* not time for this event yet */
 		now = per_cpu(decrementer_next_tb, cpu) - now;
 		if (now <= DECREMENTER_MAX)
-			set_dec((unsigned int)now - 1);
+			set_dec((int)now);
 		return;
 	}
 	old_regs = set_irq_regs(regs);
@@ -601,10 +601,8 @@ void timer_interrupt(struct pt_regs * regs)
 
 	if (evt->event_handler)
 		evt->event_handler(evt);
-	else
-		evt->set_next_event(DECREMENTER_MAX, evt);
 
 #ifdef CONFIG_PPC_ISERIES
 	if (firmware_has_feature(FW_FEATURE_ISERIES) && hvlpevent_is_pending())
@@ -826,9 +824,6 @@ static int decrementer_set_next_event(unsigned long evt,
 				      struct clock_event_device *dev)
 {
 	__get_cpu_var(decrementer_next_tb) = get_tb_or_rtc() + evt;
-	/* The decrementer interrupts on the 0 -> -1 transition */
-	if (evt)
-		--evt;
 	set_dec(evt);
 	return 0;
 }
diff --git a/include/asm-powerpc/time.h b/include/asm-powerpc/time.h
index f058955..eed64bd 100644
--- a/include/asm-powerpc/time.h
+++ b/include/asm-powerpc/time.h
@@ -183,6 +183,7 @@ static inline void set_dec(int val)
 #elif defined(CONFIG_8xx_CPU6)
 	set_dec_cpu6(val);
 #else
+	--val;	/* classic decrementer interrupts when dec goes negative */
 #ifdef CONFIG_PPC_ISERIES
 	int cur_dec;
 

Re: [POWERPC] Fix off-by-one error in setting decrementer on Book E

[Paul Mackerras]

I wrote:

> @@ -601,10 +601,8 @@ void timer_interrupt(struct pt_regs * regs)

That should be

@@ -601,8 +601,6 @@ void timer_interrupt(struct pt_regs * regs)

of course.

Paul.

Re: [POWERPC] Fix off-by-one error in setting decrementer on Book E

[Olof Johansson]

Hi Paul,

On Mon, Oct 29, 2007 at 01:57:17PM +1100, Paul Mackerras wrote:

> diff --git a/include/asm-powerpc/time.h b/include/asm-powerpc/time.h
> index f058955..eed64bd 100644
> --- a/include/asm-powerpc/time.h
> +++ b/include/asm-powerpc/time.h
> @@ -183,6 +183,7 @@ static inline void set_dec(int val)
>  #elif defined(CONFIG_8xx_CPU6)
>  	set_dec_cpu6(val);
>  #else
> +	--val;	/* classic decrementer interrupts when dec goes negative */
>  #ifdef CONFIG_PPC_ISERIES
>  	int cur_dec;

This will mix code and declarations, I think some toolchains complain
about that?

It doesn't look like cur_dec is really needed, the call can be inlined
directly where it's used.


-Olof

Re: [POWERPC] Fix off-by-one error in setting decrementer on Book E

[Sergei Shtylyov]

Hello.

Paul Mackerras wrote:

> The decrementer in Book E and 4xx processors interrupts on the
> transition from 1 to 0, rather than on the 0 to -1 transition as on
> 64-bit server and 32-bit "classic" (6xx/7xx/7xxx) processors.

> This fixes the problem by making set_dec subtract 1 from the count for
> server and classic processors.  Since set_dec already had a bunch of
> ifdefs to handle different processor types, there is no net increase
> in ugliness. :)

> This also removes a redundant call to set the decrementer to
> 0x7fffffff - it was already set to that earlier in timer_interrupt.

> Signed-off-by: Paul Mackerras <paulus@samba.org>

    Heh, I have alike patch but refrained from posting being busy with other
stuff and nt having any PPC target at hand... so, I'm late as usual

> ---
> diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
> index 9eb3284..5e253d6 100644
> --- a/arch/powerpc/kernel/time.c
> +++ b/arch/powerpc/kernel/time.c
> @@ -586,7 +586,7 @@ void timer_interrupt(struct pt_regs * regs)
>  		/* not time for this event yet */
>  		now = per_cpu(decrementer_next_tb, cpu) - now;
>  		if (now <= DECREMENTER_MAX)
> -			set_dec((unsigned int)now - 1);
> +			set_dec((int)now);
>  		return;
>  	}
>  	old_regs = set_irq_regs(regs);
> @@ -601,10 +601,8 @@ void timer_interrupt(struct pt_regs * regs)
>  
>  	if (evt->event_handler)
>  		evt->event_handler(evt);
> -	else
> -		evt->set_next_event(DECREMENTER_MAX, evt);
>  
>  #ifdef CONFIG_PPC_ISERIES
>  	if (firmware_has_feature(FW_FEATURE_ISERIES) && hvlpevent_is_pending())
> @@ -826,9 +824,6 @@ static int decrementer_set_next_event(unsigned long evt,
>  				      struct clock_event_device *dev)
>  {
>  	__get_cpu_var(decrementer_next_tb) = get_tb_or_rtc() + evt;
> -	/* The decrementer interrupts on the 0 -> -1 transition */
> -	if (evt)
> -		--evt;
>  	set_dec(evt);
>  	return 0;
>  }
> diff --git a/include/asm-powerpc/time.h b/include/asm-powerpc/time.h
> index f058955..eed64bd 100644
> --- a/include/asm-powerpc/time.h
> +++ b/include/asm-powerpc/time.h
> @@ -183,6 +183,7 @@ static inline void set_dec(int val)
>  #elif defined(CONFIG_8xx_CPU6)
>  	set_dec_cpu6(val);
>  #else
> +	--val;	/* classic decrementer interrupts when dec goes negative */

    I got an impression that set_dec_cpu6() needs the decremented val as well.
Plus for iSeries compiler will complain for iSeries about cur_dec being
declared after val-- statement, C++ style.  How about such variant (and the 
same for <asm-ppc/time.h>)?

Index: powerpc/include/asm-powerpc/time.h
===================================================================
--- powerpc.orig/include/asm-powerpc/time.h
+++ powerpc/include/asm-powerpc/time.h
@@ -178,16 +178,24 @@ static inline unsigned int get_dec(void)

  static inline void set_dec(int val)
  {
+       /*
+        * The "classic"  decrementer  interrupts at 0 to -1 transition, while
+        * 40x and book E decrementers interrupt  at 1 to  0 transition.
+        */
  #if defined(CONFIG_40x)
         mtspr(SPRN_PIT, val);
-#elif defined(CONFIG_8xx_CPU6)
+#else
+#if !defined(CONFIG_BOOKE)
+       val = val ? val - 1 : 0;
+#endif
+#if defined(CONFIG_8xx_CPU6)
         set_dec_cpu6(val);
-#ifdef CONFIG_PPC_ISERIES
-       int cur_dec;
-
+#if defined(CONFIG_PPC_ISERIES)
         if (firmware_has_feature(FW_FEATURE_ISERIES) &&
-                       get_lppaca()->shared_proc) {
+           get_lppaca()->shared_proc) {
+               int cur_dec;
+
                 get_lppaca()->virtual_decr = val;
                 cur_dec = get_dec();
                 if (cur_dec > val)
@@ -195,7 +203,8 @@ static inline void set_dec(int val)
         } else
  #endif
                 mtspr(SPRN_DEC, val);
-#endif /* not 40x or 8xx_CPU6 */
+#endif /* not 8xx_CPU6 */
+#endif /* not 40x */
  }

  static inline unsigned long tb_ticks_since(unsigned long tstamp)

WBR, Sergei

Re: [POWERPC] Fix off-by-one error in setting decrementer on Book E

[Kumar Gala]

On Oct 28, 2007, at 9:57 PM, Paul Mackerras wrote:

> The decrementer in Book E and 4xx processors interrupts on the
> transition from 1 to 0, rather than on the 0 to -1 transition as on
> 64-bit server and 32-bit "classic" (6xx/7xx/7xxx) processors.
>
> This fixes the problem by making set_dec subtract 1 from the count for
> server and classic processors.  Since set_dec already had a bunch of
> ifdefs to handle different processor types, there is no net increase
> in ugliness. :)
>
> This also removes a redundant call to set the decrementer to
> 0x7fffffff - it was already set to that earlier in timer_interrupt.
>
> Signed-off-by: Paul Mackerras <paulus@samba.org>
> ---

...

> diff --git a/include/asm-powerpc/time.h b/include/asm-powerpc/time.h
> index f058955..eed64bd 100644
> --- a/include/asm-powerpc/time.h
> +++ b/include/asm-powerpc/time.h
> @@ -183,6 +183,7 @@ static inline void set_dec(int val)
>  #elif defined(CONFIG_8xx_CPU6)
>  	set_dec_cpu6(val);
>  #else
> +	--val;	/* classic decrementer interrupts when dec goes negative */
>  #ifdef CONFIG_PPC_ISERIES
>  	int cur_dec;
>

Unless I'm reading set_dec() you are getting --val on booke.

- k

Re: [POWERPC] Fix off-by-one error in setting decrementer on Book E

[Sergei Shtylyov]

Hello.

Kumar Gala wrote:

>>The decrementer in Book E and 4xx processors interrupts on the
>>transition from 1 to 0, rather than on the 0 to -1 transition as on
>>64-bit server and 32-bit "classic" (6xx/7xx/7xxx) processors.

>>This fixes the problem by making set_dec subtract 1 from the count for
>>server and classic processors.  Since set_dec already had a bunch of
>>ifdefs to handle different processor types, there is no net increase
>>in ugliness. :)

>>This also removes a redundant call to set the decrementer to
>>0x7fffffff - it was already set to that earlier in timer_interrupt.

>>Signed-off-by: Paul Mackerras <paulus@samba.org>
>>---

> ...

>>diff --git a/include/asm-powerpc/time.h b/include/asm-powerpc/time.h
>>index f058955..eed64bd 100644
>>--- a/include/asm-powerpc/time.h
>>+++ b/include/asm-powerpc/time.h
>>@@ -183,6 +183,7 @@ static inline void set_dec(int val)
>> #elif defined(CONFIG_8xx_CPU6)
>> 	set_dec_cpu6(val);
>> #else
>>+	--val;	/* classic decrementer interrupts when dec goes negative */
>> #ifdef CONFIG_PPC_ISERIES
>> 	int cur_dec;

> Unless I'm reading set_dec() you are getting --val on booke.

    You meant "misreading"?
    Indeed the patch is decrementing count for *both* book E and classic CPUs 
now, and that slipped past my attention the first time.  The patch summary 
("Fix off-by-one error in setting decrementer on Book E") also looks stange -- 
there is *no* off-by-one error on Book E, and the description correctly says 
that we're fixing off-by-one on classic/server CPUs.
     Maybe I should even go and post my patch variant instead...

WBR, Sergei

Re: [POWERPC] Fix off-by-one error in setting decrementer on Book E

[Paul Mackerras]

Sergei Shtylyov writes:

> +       /*
> +        * The "classic"  decrementer  interrupts at 0 to -1 transition, while
> +        * 40x and book E decrementers interrupt  at 1 to  0 transition.

Funky  spacing .  : )

If I take out the removed lines in the rest of your patch, I get:

> +        */
>   #if defined(CONFIG_40x)
>          mtspr(SPRN_PIT, val);
> +#else
> +#if !defined(CONFIG_BOOKE)
> +       val = val ? val - 1 : 0;
> +#endif
> +#if defined(CONFIG_8xx_CPU6)
>          set_dec_cpu6(val);
> +#if defined(CONFIG_PPC_ISERIES)

I think you're missing a #else here.

Paul.

Re: [POWERPC] Fix off-by-one error in setting decrementer on Book E

[Sergei Shtylyov]

Hello.

Paul Mackerras wrote:

> If I take out the removed lines in the rest of your patch, I get:

>>+        */
>>  #if defined(CONFIG_40x)
>>         mtspr(SPRN_PIT, val);
>>+#else
>>+#if !defined(CONFIG_BOOKE)
>>+       val = val ? val - 1 : 0;
>>+#endif
>>+#if defined(CONFIG_8xx_CPU6)
>>         set_dec_cpu6(val);
>>+#if defined(CONFIG_PPC_ISERIES)

> I think you're missing a #else here.

    You've cut off #endif's at the end.  But yes, you are correct -- I've lost 
it while pasting this hunk into mail. :-/

> Paul.

WBR, Sergei