[Bug 2180] New: _TOD_Get_with_nanoseconds() is broken on SMP

[Daniel Cederman]

Hi,

We are currently experiencing this bug reported by Sebastian Huber and I 
could not find any discussion on it on the list. I'm guessing that the 
simple solution of moving the call to Clock_driver_support_at_tick() to 
the critical section of _TOD_Tickle_ticks() is not an acceptable 
solution, given that it would be a call from score to the BSP?

Best regards,
Daniel C

On 2014-06-05 15:29, bugzilla-daemon at rtems.org wrote:
> https://www.rtems.org/bugzilla/show_bug.cgi?id=2180
>
>               Bug #: 2180
>             Summary: _TOD_Get_with_nanoseconds() is broken on SMP
>      Classification: Unclassified
>             Product: RTEMS
>             Version: 4.11
>            Platform: All
>          OS/Version: RTEMS
>              Status: NEW
>            Severity: normal
>            Priority: P3
>           Component: cpukit
>          AssignedTo: joel.sherrill at oarcorp.com
>          ReportedBy: sebastian.huber at embedded-brains.de
>
>
> We have
>
> Timestamp_Control *_TOD_Get_with_nanoseconds(
>    Timestamp_Control *snapshot,
>    const Timestamp_Control *clock
> )
> {
>    TOD_Control      *tod = &_TOD;
>    ISR_lock_Context  lock_context;
>    Timestamp_Control offset;
>    Timestamp_Control now;
>    uint32_t          nanoseconds;
>
>    _TOD_Acquire( tod, &lock_context );
>      nanoseconds = ( *tod->nanoseconds_since_last_tick )();
>      now = *clock;
>    _TOD_Release( tod, &lock_context );
>
>    _Timestamp_Set( &offset, 0, nanoseconds );
>    _Timestamp_Add_to( &now, &offset );
>
>    *snapshot = now;
>
>    return snapshot;
> }
>
> and
>
> void _TOD_Tickle_ticks( void )
> {
>    TOD_Control       *tod = &_TOD;
>    ISR_lock_Context   lock_context;
>    Timestamp_Control  tick;
>    uint32_t           nanoseconds_per_tick;
>
>    nanoseconds_per_tick = rtems_configuration_get_nanoseconds_per_tick();
>
>    /* Convert the tick quantum to a timestamp */
>    _Timestamp_Set( &tick, 0, nanoseconds_per_tick );
>
>    /* Update the counter of ticks since boot */
>    _Watchdog_Ticks_since_boot += 1;
>
>    _TOD_Acquire( tod, &lock_context );
>
>    /* Update the uptime */
>    _Timestamp_Add_to( &tod->uptime, &tick );
>
>    /* Update the current TOD */
>    _Timestamp_Add_to( &tod->now, &tick );
>
>    _TOD_Release( tod, &lock_context );
>
>    _TOD.seconds_trigger += nanoseconds_per_tick;
>    if ( _TOD.seconds_trigger >= 1000000000UL ) {
>      _TOD.seconds_trigger -= 1000000000UL;
>      _Watchdog_Tickle_seconds();
>    }
> }
>
> and (standard Clock driver)
>
> #if defined(BSP_FEATURE_IRQ_EXTENSION) || \
>      (CPU_SIMPLE_VECTORED_INTERRUPTS != TRUE)
> void Clock_isr(void *arg)
> {
> #else
> rtems_isr Clock_isr(rtems_vector_number vector);
> rtems_isr Clock_isr(
>    rtems_vector_number vector
> )
> {
> #endif
>    /*
>     *  Accurate count of ISRs
>     */
>    Clock_driver_ticks += 1;
>
>    #if CLOCK_DRIVER_USE_FAST_IDLE
>      do {
>        rtems_clock_tick();
>      } while (
>        _Thread_Heir == _Thread_Executing
>          && _Thread_Executing->Start.entry_point
>            == rtems_configuration_get_idle_task()
>      );
>
>      Clock_driver_support_at_tick();
>      return;
>    #else
>      /*
>       *  Do the hardware specific per-tick action.
>       *
>       *  The counter/timer may or may not be set to automatically reload.
>       */
>      Clock_driver_support_at_tick();
>
>      #if CLOCK_DRIVER_ISRS_PER_TICK
>        /*
>         *  The driver is multiple ISRs per clock tick.
>         */
>        if ( !Clock_driver_isrs ) {
>          rtems_clock_tick();
>
>          Clock_driver_isrs = CLOCK_DRIVER_ISRS_PER_TICK;
>        }
>        Clock_driver_isrs--;
>      #else
>        /*
>         *  The driver is one ISR per clock tick.
>         */
>        rtems_clock_tick();
>      #endif
>    #endif
> }
>
>
> Suppose we are between Clock_driver_support_at_tick() and _TOD_Tickle_ticks().
> Now call _TOD_Get_with_nanoseconds() on another processor.  With most
> nanoseconds extensions we observe now a serviced hardware clock interrupt and
> the old _TOD.uptime value.
>