[avr-libc-dev] [bug #35003] wdt_enable() has a wrong code sequence for many parts

[Luca Matteini]

URL:
  <http://savannah.nongnu.org/bugs/?35003>

                 Summary: wdt_enable() has a wrong code sequence for many
parts
                 Project: AVR C Runtime Library
            Submitted by: loucypher
            Submitted on: mer 07 dic 2011 14:29:44 GMT
                Category: Library
                Severity: 3 - Normal
                Priority: 5 - Normal
              Item Group: Header files
                  Status: None
        Percent Complete: 0%
             Assigned to: None
             Open/Closed: Open
         Discussion Lock: Any
                 Release: 1.7.*
           Fixed Release: None

    _______________________________________________________

Details:

While debugging my code I found a bug/race condition in avr/wdt.h that left me
astonished (since that code is in effect for a long time now).

Assembly code (ATtiny, ATmega, AT90 parts) for the inline wdt_enable()
possibly re-enables interrupts before the last write access to WDTCSR.
The bug is present both for STS/OUT instructions code.

Note in fact how for STS code we have

#define wdt_enable(value)   \
__asm__ __volatile__ (  \
    "in __tmp_reg__,__SREG__" "\n\t"    \
    "cli" "\n\t"    \
    "wdr" "\n\t"    \
    "sts %0,%1" "\n\t"  \
    "out __SREG__,__tmp_reg__" "\n\t"   \
    "sts %0,%2" "\n\t" \
    : /* no outputs */  \
    : "M" (_SFR_MEM_ADDR(_WD_CONTROL_REG)), \
    "r" (_BV(_WD_CHANGE_BIT) | _BV(WDE)), \
    "r" ((uint8_t) ((value & 0x08 ? _WD_PS3_MASK : 0x00) | \
        _BV(WDE) | (value & 0x07)) ) \
    : "r0"  \
)

the OUT to SREG happens right before the last STS to WDTCSR, making the code
non-atomic: the last STS to WDTCSR could be delayed (by an interrupt handler)
for many cycles, voiding its effect (has to happen within 4 cycles from the
precedent STS to WDTCSR).

The same happens for OUT instruction mode in wdt_enable(), while wdt_disable()
is always correct (OUT to SREG is last instruction).

For ATxmega parts code in wdt_enable() is correct, but I can't see a
wdt_disable() defined anywhere: am I right? Is that intentional?

~Lou





    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?35003>

_______________________________________________
  Messaggio inviato con/da Savannah
  http://savannah.nongnu.org/