[PATCH v2] MIPS: Alchemy: fix wait function

[PATCH v2] MIPS: Alchemy: fix wait function

[Manuel Lauss]

Only an interrupt can wake the core from 'wait', enable interrupts
locally before executing 'wait'.

Signed-off-by: Manuel Lauss <manuel.lauss@gmail.com>
---
Ralf made me aware of the race in between enabling interrupts and
entering wait.  While this patch does not eliminate it, it shrinks it
to 1 instruction.  It's not perfect, but lets Alchemy boot until a
more sophisticated solution (like __r4k_wait) can be implemented
without having to duplicate the interrupt exception handler.

 arch/mips/kernel/idle.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/mips/kernel/idle.c b/arch/mips/kernel/idle.c
index 3b09b88..1d37b4b 100644
--- a/arch/mips/kernel/idle.c
+++ b/arch/mips/kernel/idle.c
@@ -93,9 +93,9 @@ static void rm7k_wait_irqoff(void)
 }
 
 /*
- * The Au1xxx wait is available only if using 32khz counter or
- * external timer source, but specifically not CP0 Counter.
- * alchemy/common/time.c may override cpu_wait!
+ * Au1 'wait' is only useful when the 32kHz counter is used as timer,
+ * since coreclock (and the cp0 counter) stops upon executing it. Only an
+ * interrupt can wake it, so they must be enabled before entering idle modes.
  */
 static void au1k_wait(void)
 {
@@ -103,8 +103,10 @@ static void au1k_wait(void)
 	"	.set	mips3			\n"
 	"	cache	0x14, 0(%0)		\n"
 	"	cache	0x14, 32(%0)		\n"
+	"	mfc0	$8, $12			\n"
+	"	ori	$8, $8, 1		\n"
 	"	sync				\n"
-	"	nop				\n"
+	"	mtc0	$8, $12			\n"	/* enable irqs */
 	"	wait				\n"
 	"	nop				\n"
 	"	nop				\n"
@@ -112,7 +114,6 @@ static void au1k_wait(void)
 	"	nop				\n"
 	"	.set	mips0			\n"
 	: : "r" (au1k_wait));
-	local_irq_enable();
 }
 
 static int __initdata nowait;
-- 
1.8.2.1

Re: [PATCH v2] MIPS: Alchemy: fix wait function

[Maciej W. Rozycki]

On Thu, 23 May 2013, Manuel Lauss wrote:

> Only an interrupt can wake the core from 'wait', enable interrupts
> locally before executing 'wait'.
> 
> Signed-off-by: Manuel Lauss <manuel.lauss@gmail.com>
> ---
> Ralf made me aware of the race in between enabling interrupts and
> entering wait.  While this patch does not eliminate it, it shrinks it
> to 1 instruction.  It's not perfect, but lets Alchemy boot until a
> more sophisticated solution (like __r4k_wait) can be implemented
> without having to duplicate the interrupt exception handler.

 I suggest double-checking with Alchemy documentation, but I doubt there 
is a race here, the write-back pipeline stage of MTC0 should overlap with 
the execution stage of WAIT, so assuming interrupts were previously 
disabled there should be no window between setting CP0.Status.IE and 
executing WAIT that would permit an interrrupt exception to be taken.  

 There is a bug in your change however.

>  arch/mips/kernel/idle.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/mips/kernel/idle.c b/arch/mips/kernel/idle.c
> index 3b09b88..1d37b4b 100644
> --- a/arch/mips/kernel/idle.c
> +++ b/arch/mips/kernel/idle.c
> @@ -93,9 +93,9 @@ static void rm7k_wait_irqoff(void)
>  }
>  
>  /*
> - * The Au1xxx wait is available only if using 32khz counter or
> - * external timer source, but specifically not CP0 Counter.
> - * alchemy/common/time.c may override cpu_wait!
> + * Au1 'wait' is only useful when the 32kHz counter is used as timer,
> + * since coreclock (and the cp0 counter) stops upon executing it. Only an
> + * interrupt can wake it, so they must be enabled before entering idle modes.
>   */
>  static void au1k_wait(void)
>  {
> @@ -103,8 +103,10 @@ static void au1k_wait(void)
>  	"	.set	mips3			\n"
>  	"	cache	0x14, 0(%0)		\n"
>  	"	cache	0x14, 32(%0)		\n"
> +	"	mfc0	$8, $12			\n"
> +	"	ori	$8, $8, 1		\n"
>  	"	sync				\n"
> -	"	nop				\n"
> +	"	mtc0	$8, $12			\n"	/* enable irqs */
>  	"	wait				\n"
>  	"	nop				\n"
>  	"	nop				\n"
> @@ -112,7 +114,6 @@ static void au1k_wait(void)
>  	"	nop				\n"
>  	"	.set	mips0			\n"
>  	: : "r" (au1k_wait));
> -	local_irq_enable();
>  }
>  
>  static int __initdata nowait;

 You can't just take $8 away under the feet of GCC without telling the 
compiler, it may be storing some data there across the asm.  Rather than 
picking an arbitrary register as a clobber I suggest using an output 
register constraint associated with a scratch variable (depending on 
register usage GCC may possibly be able to reuse the same register both 
for input and for output).

  Maciej

Re: [PATCH v2] MIPS: Alchemy: fix wait function

[Manuel Lauss]

On Sat, Jun 8, 2013 at 8:18 PM, Maciej W. Rozycki <macro@linux-mips.org> wrote:
> On Thu, 23 May 2013, Manuel Lauss wrote:
>
>> Only an interrupt can wake the core from 'wait', enable interrupts
>> locally before executing 'wait'.
>>
>> Signed-off-by: Manuel Lauss <manuel.lauss@gmail.com>
>> ---
>> Ralf made me aware of the race in between enabling interrupts and
>> entering wait.  While this patch does not eliminate it, it shrinks it
>> to 1 instruction.  It's not perfect, but lets Alchemy boot until a
>> more sophisticated solution (like __r4k_wait) can be implemented
>> without having to duplicate the interrupt exception handler.
>
>  I suggest double-checking with Alchemy documentation, but I doubt there
> is a race here, the write-back pipeline stage of MTC0 should overlap with
> the execution stage of WAIT, so assuming interrupts were previously
> disabled there should be no window between setting CP0.Status.IE and
> executing WAIT that would permit an interrrupt exception to be taken.

That was my thinking as well.  The Alchemy manuals (at least the ones I have)
don't specify the stage where c0 writes complete or 'wait' executes, but the
wording on how and when exceptions are raised makes me believe the
scheme above is race-free.


>  There is a bug in your change however.
[...]
>  You can't just take $8 away under the feet of GCC without telling the
> compiler, it may be storing some data there across the asm.  Rather than
> picking an arbitrary register as a clobber I suggest using an output
> register constraint associated with a scratch variable (depending on
> register usage GCC may possibly be able to reuse the same register both
> for input and for output).

I've fixed that, and sent out a new patch.

Thank you very much!
        Manuel

Re: [PATCH v2] MIPS: Alchemy: fix wait function

[Ralf Baechle]

On Thu, May 23, 2013 at 03:28:36PM +0200, Manuel Lauss wrote:

> Only an interrupt can wake the core from 'wait', enable interrupts
> locally before executing 'wait'.
> 
> Signed-off-by: Manuel Lauss <manuel.lauss@gmail.com>
> ---
> Ralf made me aware of the race in between enabling interrupts and
> entering wait.  While this patch does not eliminate it, it shrinks it
> to 1 instruction.  It's not perfect, but lets Alchemy boot until a
> more sophisticated solution (like __r4k_wait) can be implemented
> without having to duplicate the interrupt exception handler.

It doesn't shrink it - interrupts will be pending from the time they
were disabled to the point where they get re-enabled.  That can be quite
a few cycles and so the likelyhood for hitting the race is not that low.

  Ralf

Re: [PATCH v2] MIPS: Alchemy: fix wait function

[Maciej W. Rozycki]

On Mon, 10 Jun 2013, Ralf Baechle wrote:

> > Only an interrupt can wake the core from 'wait', enable interrupts
> > locally before executing 'wait'.
> > 
> > Signed-off-by: Manuel Lauss <manuel.lauss@gmail.com>
> > ---
> > Ralf made me aware of the race in between enabling interrupts and
> > entering wait.  While this patch does not eliminate it, it shrinks it
> > to 1 instruction.  It's not perfect, but lets Alchemy boot until a
> > more sophisticated solution (like __r4k_wait) can be implemented
> > without having to duplicate the interrupt exception handler.
> 
> It doesn't shrink it - interrupts will be pending from the time they
> were disabled to the point where they get re-enabled.  That can be quite
> a few cycles and so the likelyhood for hitting the race is not that low.

 My understanding has been the race is that an interrupt can be taken 
between the MTC0 instruction used to enable interrupts and the WAIT 
instruction and the handler can disable interrupts back.  That's not going 
to be a problem if the interrupt is taken "a few cycles" later, because by 
then WAIT will have already been executed.

 Have I been missing something here?

  Maciej