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@xxxxxxxxx>
> ---
> 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
