bugAVR C Runtime Library - Bugs: bug #44140, wdt_disable() macro clobbers...

 
 

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

bug #44140: wdt_disable() macro clobbers prescaller bits and can cause unexpected resets

Submitter:  None
Submitted:  Sat 31 Jan 2015 05:05:09 PM UTC
   
 
Category:  Library Severity:  3 - Normal
Priority:  5 - Normal Item Group:  None
Status:  Fixed Assigned to:  pitchumani
Percent Complete:  100% Originator Email:  -email is unavailable-
Open/Closed:  Open Release:  Any
Fixed Release:  None

Wed 06 May 2015 11:40:00 AM UTC, comment #4: 

Remaks to

https://savannah.nongnu.org/bugs/download.php?file_id=33934

  • adds "memory" clobber to all


  • use constraint "d" with ORI reg


  • use constraint "n" with SFR_MEM_ADDR


  • use "n" with some non-address compile time constants


  • use SFR_MEM_ADDR if used in LDS / STS


  • use SFR_IO_ADDR if used in IN / OUT


This is completely untested.  In particular, the right fix for the latter 2 might be to use LDS/STS instead of IN/OUT and not adjusting operands.

Georg-Johann Lay <gjlayde>
Mon 09 Mar 2015 06:37:07 AM UTC, comment #3: 
Pitchumani <pitchumani>
Group Member
Thu 05 Mar 2015 02:56:20 PM UTC, comment #2: 

This new code resolves the unintentional watchdog reset problem. Thanks!

Anonymous
Thu 05 Mar 2015 11:35:37 AM UTC, comment #1: 

Hi Josh,

Below is the modified wdt_disable function. I have included wdr instruction before modifying watch dog. Also avoided overwriting prescaler bits.


static __inline__
__attribute__ ((__always_inline__))
void wdt_disable (void)
{
    if (_SFR_IO_REG_P (_WD_CONTROL_REG))
    {
        uint8_t register temp_reg;
        __asm__ __volatile__ (
                "in __tmp_reg__,__SREG__"    "\n\t"
                "cli"                        "\n\t"
                "wdr"                        "\n\t"
                "in  %[TEMPREG],%[WDTREG]"   "\n\t"
                "ori %[TEMPREG],%[WDCE_WDE]" "\n\t"
                "out %[WDTREG],%[TEMPREG]"   "\n\t"
                "out %[WDTREG],__zero_reg__" "\n\t"
                "out __SREG__,__tmp_reg__"   "\n\t"
                : [TEMPREG] "=r" (temp_reg)
                : [WDTREG]  "I"  (_SFR_IO_ADDR(_WD_CONTROL_REG)),
                [WDCE_WDE]  "I"  ((uint8_t)(_BV(_WD_CHANGE_BIT) | _BV(WDE)))
                : "r0"
        );
    }
    else
    {
        uint8_t register temp_reg;
        __asm__ __volatile__ (
                "in __tmp_reg__,__SREG__"    "\n\t"
                "cli"                        "\n\t"
                "wdr"                        "\n\t"
                "lds %[TEMPREG],%[WDTREG]"   "\n\t"
                "ori %[TEMPREG],%[WDCE_WDE]" "\n\t"
                "sts %[WDTREG],%[TEMPREG]"   "\n\t"
                "sts %[WDTREG],__zero_reg__" "\n\t"
                "out __SREG__,__tmp_reg__"   "\n\t"
                : [TEMPREG] "=r" (temp_reg)
                : [WDTREG]  "M"  (_SFR_MEM_ADDR(_WD_CONTROL_REG)),
                [WDCE_WDE]  "I"  ((uint8_t)(_BV(_WD_CHANGE_BIT) | _BV(WDE)))
                : "r0"
        );
    }
}


Generated assembly for tiny84a it should be like:

    in __tmp_reg__,__SREG__
    cli
    wdr                  ; watchdog reset
    in  r24,33           ; load wdt register value
    ori r24,24           ; OR (WDCE | WDE)
    out 33,r24           ; write back to wdt register
    out 33,__zero_reg__  ; trun-off wdt
    out __SREG__,__tmp_reg__


Is this change ok?

Manually verified the generated assembly for all devices that has wdt.

Pitchumani <pitchumani>
Group Member
Sat 31 Jan 2015 05:05:09 PM UTC, original submission:  

The wdt_disable() macro in wdt.h overwrites the prescaler bits as an unintended side effect when setting the watchdog change enable bit.

The relevant line in wdt_disable() macro looks like this (expanded for clarity)...

===
out WDTCSR, _BV(_WD_CHANGE_BIT) | _BV(WDE)
===

IN safely level 1 (WatchDog fuse not set), the above code writes 0's to the WDTCSR register's lower 3 bits,  which contain a prescaler for the counter. When changing to a lower prescaler, it is possible to trigger an unintended reset if the new prescaler has already expired.

The data sheet specially mentions this possibility in the comments for the example code to disable the watchdog, and uses an OR to set the relevant bits and preserve the prescaler bits...

===
; Write logical one to WDCE and WDE
; Keep old prescaler setting to prevent unintentional Watchdog Reset
in r16, WDTCSR
orir16, (1<<WDCE)|(1<<WDE)
out WDTCSR, r16
===

The problem can also be fixed with only one extra instruction/cycle and no extra register use by placing a watchdog reset before the assignment. Since interrupts are already disabled, this ensures that the new prescaler will not have already tripped. Note that the watchdog is reset when it is disabled anyway, so this should not have any side effects.

New macro with the reset strategy would look like...

===
_asm_ __volatile__ ( \
"in _tmp_reg_, _SREG_" "\n\t" \
"cli" "\n\t" \
"wdr" "\n\r" \
"out %0, %1" "\n\t" \
"out %0, _zero_reg_" "\n\t" \
"out _SREG_,__tmp_reg__" "\n\t" \
: /* no outputs */ \
: "I" (_SFR_IO_ADDR(_WD_CONTROL_REG)), \
"r" ((uint8_t)(_BV(_WD_CHANGE_BIT) | _BV(WDE))) \
: "r0" \
)
===

I have only tested this on an ATTINY84A, but the watchdog function is the same in every 8-bit AVR I've checked.

Let me know if you have any questions.

Thanks!

-josh

Anonymous

 

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

Attached Files
file #33934:  wdt-2.diff added by gjlayde (8KiB - text/x-patch - Fix several issues.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by gjlayde (Updated the item)
  • -email is unavailable- added by pitchumani (Posted a comment)
  • -email is unavailable- added by None (Submitted the item)
  •  

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-05-06 gjlayde Attached File- Added wdt-2.diff, #33934
    2015-03-09 pitchumani StatusIn Progress Fixed
    2015-03-05 pitchumani StatusNone In Progress
        Assigned toNone pitchumani

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code