bugAVR C Runtime Library - Bugs: bug #35003, wdt_enable() has a wrong code...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #35003: wdt_enable() has a wrong code sequence for many parts

Submitter:  Luca Matteini <loucypher>
Submitted:  Wed 07 Dec 2011 02:29:44 PM UTC
   
 
Category:  Library Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Header files
Status:  Invalid Assigned to:  None
Percent Complete:  0% Open/Closed:  Closed
Release:  1.7.* Fixed Release:  None

Wed 07 Dec 2011 04:01:36 PM UTC, comment #5: 

I admit I trusted quality of the compiler for a long time, then!

I don't change my mind anyway on finding questionable the fact how coding style changes every few lines, of the same source file, based on a feature only partially documented by silicon manufacturer.
s/bug/critical review/



Luca Matteini <loucypher>
Wed 07 Dec 2011 03:20:51 PM UTC, comment #4: 

You'd be surprised to learn that the compiler has been
generating code like that for much more than 10 years now
(in every function prologue/epilogue when handling stack
frame creation/destruction).

Joerg Wunsch <joerg_wunsch>
Group administrator
Wed 07 Dec 2011 03:03:11 PM UTC, comment #3: 

Well, I wasn't aware of that!
I guess it's an extension to what happens with SEI instruction.
Note how in this last case all is clearly stated, as if the SEI instruction decoding, in itself, is to force execution of the following instruction, more than setting I flag (...)

IMHO even if that works, brings in some confusion in the sources, being the only cases relying on a feature poorly(?) documented (if any) -- all other cases are handled as in wdt_disable(), following all examples on Atmel data sheets.

Luca Matteini <loucypher>
Wed 07 Dec 2011 02:39:52 PM UTC, comment #2: 

I-bit, of course

Georg-Johann Lay <gjlayde>
Wed 07 Dec 2011 02:39:16 PM UTC, comment #1: 

AFAIK, setting the I-big for non-Xmega devices has a latency of 1 instruction, so the code looks fine.

Georg-Johann Lay <gjlayde>
Wed 07 Dec 2011 02:29:44 PM UTC, original submission:  

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

Luca Matteini <loucypher>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by joerg_wunsch (Posted a comment)
  • -email is unavailable- added by gjlayde (Posted a comment)
  • -email is unavailable- added by loucypher (Submitted the item)
  •  

    Follow 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2011-12-07 joerg_wunsch StatusNone Invalid
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code