Is sleepmgr_sleep() wrong implemented in ASF?

Go To Last Post
2 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hello Forum,

 

I assume the implementation of sleepmgr_sleep() of ASF is wrong. The problem is that before __WFI() the mcu's interrupts are enabled. This allows a race condition in case an interrupts occurs right before _WFI is reached. The interrupts should be disabled instead, the __WFI() will still work as intended.

Have a look at the Linux kernel block, where the problem is explained in more detail.

http://comments.gmane.org/gmane....

        > In default_idle() we disable interrupts and then go idle.  Where do
        > those interrupts get enabled again so that we wake up when interrupts
        > come in?

        We don't - most non-brain damaged CPUs will wake up with the interrupt masked at the CPU.  There's a good reason why this is so.

        If you're intending to idle until the next interrupt, you have to do some preparation.  During that preparation, an interrupt may become
        active.  Such an interrupt may be a wake up event that you're looking for.

        No matter how good your code is, if you don't disable interrupts, you will always have a race between preparing to go to sleep and actually
        going to sleep, which results in lost wake up events.

        This is why all ARM CPUs I'm aware of will wake up even if they are masked at the core CPU (CPSR I bit.)
        Anything else and you should forget using idle mode.

 

static inline void system_sleep(void)

{

  __DSB();

  __WFI();

}

static inline void sleepmgr_sleep(const enum sleepmgr_mode sleep_mode)

{

  Assert(sleep_mode != SLEEPMGR_ACTIVE);

#ifdef CONFIG_SLEEPMGR_ENABLE

  cpu_irq_disable();

 

  /* Enter the sleep mode. */

  system_set_sleepmode((enum system_sleepmode)(sleep_mode - 1));

  cpu_irq_enable();  // <<<<< race condition starts here!

  system_sleep();

#else

  UNUSED(sleep_mode);

  cpu_irq_enable();

#endif /* CONFIG_SLEEPMGR_ENABLE */

}

 

IMHO correct would be:

 

static inline void sleepmgr_sleep(const enum sleepmgr_mode sleep_mode)

{

  Assert(sleep_mode != SLEEPMGR_ACTIVE);

#ifdef CONFIG_SLEEPMGR_ENABLE

  cpu_irq_disable();

 

  /* Enter the sleep mode. */

  system_set_sleepmode((enum system_sleepmode)(sleep_mode - 1));

  system_sleep();

  cpu_irq_enable();

#else

  UNUSED(sleep_mode);

  cpu_irq_enable();

#endif /* CONFIG_SLEEPMGR_ENABLE */

}

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hello,

 

After you call:

system_sleep();

the MCU is "gone". You suggest to enable interrupts after the MCU was awoken (by the interrupts that are potentially disabled?). So, first interrupts should be enabled and the then - sleep activated.

 

I think the idea behind __WFI() instruction is that it can not be interrupted (atomic instruction it is called, if I am not mistaken).

 

I hope I understand it right.

 

Regards