Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

Arnd Bergmann
2006-09-27T11:38:19Z

On Wednesday 27 September 2006 12:04, Aubrey wrote:
> inline static void default_idle(void)
> {
>        int flag;
>
>        while (!need_resched()) {
>                leds_switch(LED_OFF);
>                local_irq_save(flag);
>                if ( likely(!need_resched()) {
> #if defined (ANOMALY_05000244) && defined (CONFIG_BLKFIN_CACHE)
>                      __asm__("nop; nop;\n");
> #endif
>                      __asm__(".align 64;\n STI %0; IDLE;\n"
>                              : %0 (flag): :"cc");
>                }
>                local_irq_restore(flag);
>                leds_switch(LED_ON);
>        }
> }======================================
>
> Here, according to design, it's not possible that interrupt occurs
> between "STI %0"(enable interrupt) and "IDLE".
>
> __asm__(".align 64; STI %0; IDLE;" : %0 (x):  :"cc");
>
> Robin can explain more details.

Ok, looks good now. Just a few details that don't impact the
functionality:

- Always use 'static inline', not 'inline static', because of C99
- In the kernel, it's more common to use 'asm' than '__asm__'.
- It should probably be 'asm volatile', since gcc might notice
that the output (flag) is not used anywhere and it can therefore
eliminate the asm.
- Usually, I recommend using local_irq_disable() instead of
local_irq_save(flags) when you know that interrupts are enabled
before. It uses one less local variable, which makes it more
efficient on some architectures.
- I'd insert the two NOPs unconditionally here for better
readability.