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

[Arnd Bergmann]

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.

	Arnd <><
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/