2003-07-18 Till Straumann <strauman@slac.stanford.edu>

PR 430/rtems
	* include/rtems/score/watchdog.h: _Watchdog_Ticks_since_boot should
	be a VOLATILE variable.
	* src/watchdoginsert.c: 'restart' algorithm needs to enforce
	reloading the list head in case a TICK interrupt during ISR_Flash()
	modified the list.  This is achieved by a proper VOLATILE cast.
	Also _Watchdog_Sync_count++ should be protected by _ISR_Disable
	(prevent corruption in case ISR calls watchdoginsert)
	* src/watchdogadjust.c: ISR protection added.
	* src/watchdogtickle.c: ISR protection added.
	NOTE: PowerPC BSPs using the new exception processing MUST BE UPDATED
	to maintain _ISR_Nest_level. See also PR288 which provides fixes
	for the affected BSPs distributed with RTEMS.

cpukit/score/ChangeLog

@@ -1,3 +1,19 @@
+2003-07-18	Till Straumann <strauman@slac.stanford.edu>
+
+	PR 430/rtems
+	* include/rtems/score/watchdog.h: _Watchdog_Ticks_since_boot should
+	be a VOLATILE variable.
+	* src/watchdoginsert.c: 'restart' algorithm needs to enforce
+	reloading the list head in case a TICK interrupt during ISR_Flash()
+	modified the list.  This is achieved by a proper VOLATILE cast.
+	Also _Watchdog_Sync_count++ should be protected by _ISR_Disable
+	(prevent corruption in case ISR calls watchdoginsert)
+	* src/watchdogadjust.c: ISR protection added.
+	* src/watchdogtickle.c: ISR protection added.
+	NOTE: PowerPC BSPs using the new exception processing MUST BE UPDATED
+	to maintain _ISR_Nest_level. See also PR288 which provides fixes
+	for the affected BSPs distributed with RTEMS.
+
 2003-07-07	Joel Sherrill <joel@OARcorp.com>
 
 	PR 417/rtems

cpukit/score/include/rtems/score/watchdog.h

@@ -103,7 +103,7 @@ SCORE_EXTERN volatile unsigned32  _Watchdog_Sync_count;
  *  system was booted.
  */
 
-SCORE_EXTERN Watchdog_Interval _Watchdog_Ticks_since_boot;
+SCORE_EXTERN volatile Watchdog_Interval _Watchdog_Ticks_since_boot;
 
 /*
  *  The following defines the watchdog chains which are managed

cpukit/score/src/watchdogadjust.c

@@ -37,6 +37,19 @@ void _Watchdog_Adjust(
   Watchdog_Interval            units
 )
 {
+  ISR_Level level;
+
+  _ISR_Disable( level );
+
+  /*
+   * NOTE: It is safe NOT to make 'header' a pointer
+   *       to volatile data (contrast this with watchdoginsert.c)
+   *       because we call _Watchdog_Tickle() below and
+   *       hence the compiler must not assume *header to remain
+   *       unmodified across that call.
+   *
+   *       Till Straumann, 7/2003
+   */
   if ( !_Chain_Is_empty( header ) ) {
     switch ( direction ) {
       case WATCHDOG_BACKWARD:
@@ -50,7 +63,13 @@ void _Watchdog_Adjust(
           } else {
             units -= _Watchdog_First( header )->delta_interval;
             _Watchdog_First( header )->delta_interval = 1;
+
+			_ISR_Enable( level );
+
             _Watchdog_Tickle( header );
+
+			_ISR_Disable( level );
+
             if ( _Chain_Is_empty( header ) )
               break;
           }
@@ -58,5 +77,8 @@ void _Watchdog_Adjust(
         break;
     }
   }
+
+  _ISR_Enable( level );
+
 }
 

cpukit/score/src/watchdoginsert.c

@@ -38,13 +38,28 @@ void _Watchdog_Insert(
   insert_isr_nest_level   = _ISR_Nest_level;
   the_watchdog->state = WATCHDOG_BEING_INSERTED;
 
+  _ISR_Disable( level );
+
   _Watchdog_Sync_count++;
+
 restart:
   delta_interval = the_watchdog->initial;
 
-  _ISR_Disable( level );
-
-  for ( after = _Watchdog_First( header ) ;
+  /*
+   * We CANT use _Watchdog_First() here, because a TICK interrupt
+   * could modify the chain during the _ISR_Flash() below. Hence,
+   * the header is pointing to volatile data. The _Watchdog_First()
+   * INLINE routine (but not the macro - note the subtle difference)
+   * casts away the 'volatile'...
+   *
+   * Also, this is only necessary because we call no other routine
+   * from this piece of code, hence the compiler thinks it's safe to
+   * cache *header!!
+   *
+   *  Till Straumann, 7/2003 (gcc-3.2.2 -O4 on powerpc)
+   *
+   */
+  for ( after = (Watchdog_Control *) ((volatile Chain_Control *)header)->first ;
         ;
         after = _Watchdog_Next( after ) ) {
 
@@ -75,7 +90,6 @@ restart:
 
      if ( _Watchdog_Sync_level > insert_isr_nest_level ) {
        _Watchdog_Sync_level = insert_isr_nest_level;
-       _ISR_Enable( level );
        goto restart;
      }
   }

cpukit/score/src/watchdogtickle.c

@@ -33,18 +33,32 @@ void _Watchdog_Tickle(
   Chain_Control *header
 )
 {
+  ISR_Level level;
   Watchdog_Control *the_watchdog;
+  Watchdog_States  watchdog_state;
+
+  /*
+   * See the comment in watchdoginsert.c and watchdogadjust.c
+   * about why it's safe not to declare header a pointer to 
+   * volatile data - till, 2003/7
+   */
+
+  _ISR_Disable( level );
 
   if ( _Chain_Is_empty( header ) )
-    return;
+    goto leave;
 
   the_watchdog = _Watchdog_First( header );
   the_watchdog->delta_interval--;
   if ( the_watchdog->delta_interval != 0 )
-    return;
+    goto leave;
 
   do {
-     switch( _Watchdog_Remove( the_watchdog ) ) {
+	 watchdog_state = _Watchdog_Remove( the_watchdog );
+
+	 _ISR_Enable( level );
+
+     switch( watchdog_state ) {
        case WATCHDOG_ACTIVE:
          (*the_watchdog->routine)(
            the_watchdog->id,
@@ -71,7 +85,13 @@ void _Watchdog_Tickle(
        case WATCHDOG_REMOVE_IT:
          break;
      }
+
+	 _ISR_Disable( level );
+
      the_watchdog = _Watchdog_First( header );
    } while ( !_Chain_Is_empty( header ) &&
              (the_watchdog->delta_interval == 0) );
+
+leave:
+   _ISR_Enable(level);
 }