At this point, a ticket is needed for anything applied to 4.11 that is not release mechanics related.
--joel On Mon, Jan 11, 2016 at 11:47 AM, Marcos Diaz < marcos.d...@tallertechnologies.com> wrote: > Hi Sebastian, > > we did some more testing and found out what's causing the problem. Based > on what > I posted at > https://lists.rtems.org/pipermail/devel/2015-December/013235.html, > the problem arises when the ticker interrupt occurs while a task is > executing > one of the instructions that make up the following line: > > is_count_pending = (systick->csr & ARMV7M_SYSTICK_CSR_COUNTFLAG) != 0; > > which compiling with -O3 are: > > ldr.w r3, [r9] > ubfx r5, r3, #16, #1 > strb.w r5, [r8, #64] > > If the SysTick counts to 0 and the ldr executes before the interrupt > occurs, > R3 will have the CSR.COUNTFLAG bit set. Even though _ARMV7M_TC_at_tick will > clear the is_count_pending boolean, the task will still see it as true. > > The solution we found is to simply disable interrupts while reading that > flag. > We also use a local variable inside _ARMV7M_TC_is_pending instead of > directly > returning the current value of the is_count_pending global, just in case. > > Here's the patch that fixes it; this applies to the 4.11 branch. > Do we have to open a new thread for it to be applied to the mainline, or > will > this suffice? Should we open a ticket for 4.11? > > --- > .../arm/shared/armv7m/clock/armv7m-clock-config.c | 38 > +++++++++++++++------- > c/src/lib/libbsp/shared/clockdrv_shell.h | 16 ++++++++- > cpukit/rtems/src/clocktick.c | 6 +++- > cpukit/sapi/include/rtems/timecounter.h | 35 > ++++++++++++++++---- > cpukit/score/include/rtems/score/timecounter.h | 37 > +++++++++++++++++++-- > cpukit/score/src/kern_tc.c | 16 ++++----- > doc/bsp_howto/clock.t | 13 +++++++- > 7 files changed, 130 insertions(+), 31 deletions(-) > > diff --git > a/c/src/lib/libbsp/arm/shared/armv7m/clock/armv7m-clock-config.c > b/c/src/lib/libbsp/arm/shared/armv7m/clock/armv7m-clock-config.c > index e78684c..53bd7cf 100644 > --- a/c/src/lib/libbsp/arm/shared/armv7m/clock/armv7m-clock-config.c > +++ b/c/src/lib/libbsp/arm/shared/armv7m/clock/armv7m-clock-config.c > @@ -25,6 +25,8 @@ static void Clock_isr(void *arg); > > static rtems_timecounter_simple _ARMV7M_TC; > > +static bool is_count_pending = false; > + > static uint32_t _ARMV7M_TC_get(rtems_timecounter_simple *tc) > { > volatile ARMV7M_Systick *systick = _ARMV7M_Systick; > @@ -34,9 +36,20 @@ static uint32_t _ARMV7M_TC_get(rtems_timecounter_simple > *tc) > > static bool _ARMV7M_TC_is_pending(rtems_timecounter_simple *tc) > { > - volatile ARMV7M_SCB *scb = _ARMV7M_SCB; > + volatile ARMV7M_Systick *systick = _ARMV7M_Systick; > + ISR_lock_Context lock_context; > + > + _Timecounter_Acquire( &lock_context ); > + /* We use this bool since the hardware flag is reset each time it is > read. */ > + if (!is_count_pending) > + { > + is_count_pending = ((systick->csr & ARMV7M_SYSTICK_CSR_COUNTFLAG) != > 0 ); > + } > > - return ((scb->icsr & ARMV7M_SCB_ICSR_PENDSTSET) != 0); > + const bool is_pending_local = is_count_pending; > + > + _Timecounter_Release( &lock_context ); > + return is_pending_local; > } > > static uint32_t _ARMV7M_TC_get_timecount(struct timecounter *tc) > @@ -48,19 +61,23 @@ static uint32_t _ARMV7M_TC_get_timecount(struct > timecounter *tc) > ); > } > > -static void _ARMV7M_TC_tick(void) > -{ > - rtems_timecounter_simple_downcounter_tick(&_ARMV7M_TC, _ARMV7M_TC_get); > -} > - > -static void _ARMV7M_Systick_at_tick(void) > +static void _ARMV7M_TC_at_tick(rtems_timecounter_simple *tc) > { > volatile ARMV7M_Systick *systick = _ARMV7M_Systick; > - > + is_count_pending = false; > /* Clear COUNTFLAG */ > systick->csr; > } > > +static void _ARMV7M_TC_tick(void) > +{ > + rtems_timecounter_simple_downcounter_tick( > + &_ARMV7M_TC, > + _ARMV7M_TC_get, > + _ARMV7M_TC_at_tick > + ); > +} > + > static void _ARMV7M_Systick_handler(void) > { > _ARMV7M_Interrupt_service_enter(); > @@ -111,9 +128,6 @@ static void _ARMV7M_Systick_cleanup(void) > > #define Clock_driver_timecounter_tick() _ARMV7M_TC_tick() > > -#define Clock_driver_support_at_tick() \ > - _ARMV7M_Systick_at_tick() > - > #define Clock_driver_support_initialize_hardware() \ > _ARMV7M_Systick_initialize() > > diff --git a/c/src/lib/libbsp/shared/clockdrv_shell.h > b/c/src/lib/libbsp/shared/clockdrv_shell.h > index d546fb8..96b962f 100644 > --- a/c/src/lib/libbsp/shared/clockdrv_shell.h > +++ b/c/src/lib/libbsp/shared/clockdrv_shell.h > @@ -44,6 +44,13 @@ > #define Clock_driver_support_find_timer() > #endif > > +/** > + * @brief Do nothing by default. > + */ > +#ifndef Clock_driver_support_at_tick > + #define Clock_driver_support_at_tick() > +#endif > + > /* > * A specialized clock driver may use for example > rtems_timecounter_tick_simple() > * instead of the default. > @@ -108,7 +115,14 @@ rtems_isr Clock_isr( > && _Thread_Executing->Start.entry_point > == (Thread_Entry) rtems_configuration_get_idle_task() > ) { > - _Timecounter_Tick_simple(interval, (*tc->tc_get_timecount)(tc)); > + ISR_lock_Context lock_context; > + > + _Timecounter_Acquire(&lock_context); > + _Timecounter_Tick_simple( > + interval, > + (*tc->tc_get_timecount)(tc), > + &lock_context > + ); > } > > Clock_driver_support_at_tick(); > diff --git a/cpukit/rtems/src/clocktick.c b/cpukit/rtems/src/clocktick.c > index e2cd35f..a6bf26d 100644 > --- a/cpukit/rtems/src/clocktick.c > +++ b/cpukit/rtems/src/clocktick.c > @@ -23,9 +23,13 @@ > > rtems_status_code rtems_clock_tick( void ) > { > + ISR_lock_Context lock_context; > + > + _Timecounter_Acquire( &lock_context ); > _Timecounter_Tick_simple( > rtems_configuration_get_microseconds_per_tick(), > - 0 > + 0, > + &lock_context > ); > > return RTEMS_SUCCESSFUL; > diff --git a/cpukit/sapi/include/rtems/timecounter.h > b/cpukit/sapi/include/rtems/timecounter.h > index 04bc534..06b3973 100644 > --- a/cpukit/sapi/include/rtems/timecounter.h > +++ b/cpukit/sapi/include/rtems/timecounter.h > @@ -94,6 +94,13 @@ typedef struct { > } rtems_timecounter_simple; > > /** > + * @brief At tick handling done under protection of the timecounter lock. > + */ > +typedef uint32_t rtems_timecounter_simple_at_tick( > + rtems_timecounter_simple *tc > +); > + > +/** > * @brief Returns the current value of a simple timecounter. > */ > typedef uint32_t rtems_timecounter_simple_get( > @@ -199,20 +206,28 @@ RTEMS_INLINE_ROUTINE uint32_t > rtems_timecounter_simple_scale( > * > * @param[in] tc The simple timecounter. > * @param[in] get The method to get the value of the simple timecounter. > + * @param[in] at_tick The method to perform work under timecounter lock > + * protection at this tick, e.g. clear a pending flag. > */ > RTEMS_INLINE_ROUTINE void rtems_timecounter_simple_downcounter_tick( > - rtems_timecounter_simple *tc, > - rtems_timecounter_simple_get get > + rtems_timecounter_simple *tc, > + rtems_timecounter_simple_get get, > + rtems_timecounter_simple_at_tick at_tick > ) > { > + ISR_lock_Context lock_context; > uint32_t current; > > + _Timecounter_Acquire( &lock_context ); > + > + ( *at_tick )( tc ); > + > current = rtems_timecounter_simple_scale( > tc, > tc->real_interval - ( *get )( tc ) > ); > > - _Timecounter_Tick_simple( tc->binary_interval, current ); > + _Timecounter_Tick_simple( tc->binary_interval, current, &lock_context ); > } > > /** > @@ -220,17 +235,25 @@ RTEMS_INLINE_ROUTINE void > rtems_timecounter_simple_downcounter_tick( > * > * @param[in] tc The simple timecounter. > * @param[in] get The method to get the value of the simple timecounter. > + * @param[in] at_tick The method to perform work under timecounter lock > + * protection at this tick, e.g. clear a pending flag. > */ > RTEMS_INLINE_ROUTINE void rtems_timecounter_simple_upcounter_tick( > - rtems_timecounter_simple *tc, > - rtems_timecounter_simple_get get > + rtems_timecounter_simple *tc, > + rtems_timecounter_simple_get get, > + rtems_timecounter_simple_at_tick at_tick > ) > { > + ISR_lock_Context lock_context; > uint32_t current; > > + _Timecounter_Acquire( &lock_context ); > + > + ( *at_tick )( tc ); > + > current = rtems_timecounter_simple_scale( tc, ( *get )( tc ) ); > > - _Timecounter_Tick_simple( tc->binary_interval, current ); > + _Timecounter_Tick_simple( tc->binary_interval, current, &lock_context ); > } > > /** > diff --git a/cpukit/score/include/rtems/score/timecounter.h > b/cpukit/score/include/rtems/score/timecounter.h > index 0d17cc7..0e611b5 100644 > --- a/cpukit/score/include/rtems/score/timecounter.h > +++ b/cpukit/score/include/rtems/score/timecounter.h > @@ -26,6 +26,8 @@ > #include <sys/time.h> > #include <sys/timetc.h> > > +#include <rtems/score/isrlock.h> > + > #ifdef __cplusplus > extern "C" { > #endif /* __cplusplus */ > @@ -161,6 +163,31 @@ void _Timecounter_Install( struct timecounter *tc ); > void _Timecounter_Tick( void ); > > /** > + * @brief Lock to protect the timecounter mechanic. > + */ > +ISR_LOCK_DECLARE( extern, _Timecounter_Lock ) > + > +/** > + * @brief Acquires the timecounter lock. > + * > + * @param[in] lock_context The lock context. > + * > + * See _Timecounter_Tick_simple(). > + */ > +#define _Timecounter_Acquire( lock_context ) \ > + _ISR_lock_ISR_disable_and_acquire( &_Timecounter_Lock, lock_context ) > + > +/** > + * @brief Releases the timecounter lock. > + * > + * @param[in] lock_context The lock context. > + * > + * See _Timecounter_Tick_simple(). > + */ > +#define _Timecounter_Release(lock_context) \ > + _ISR_lock_Release_and_ISR_enable(&_Timecounter_Lock, lock_context) > + > +/** > * @brief Performs a simple timecounter tick. > * > * This is a special purpose tick function for simple timecounter to > support > @@ -169,8 +196,14 @@ void _Timecounter_Tick( void ); > * @param[in] delta The time in timecounter ticks elapsed since the last > call > * to _Timecounter_Tick_simple(). > * @param[in] offset The current value of the timecounter. > - */ > -void _Timecounter_Tick_simple( uint32_t delta, uint32_t offset ); > + * @param[in] lock_context The lock context of the corresponding > + * _Timecounter_Acquire(). > + */ > +void _Timecounter_Tick_simple( > + uint32_t delta, > + uint32_t offset, > + ISR_lock_Context *lock_context > +); > > /** > * @brief The wall clock time in seconds. > diff --git a/cpukit/score/src/kern_tc.c b/cpukit/score/src/kern_tc.c > index f6a1136..4257533 100644 > --- a/cpukit/score/src/kern_tc.c > +++ b/cpukit/score/src/kern_tc.c > @@ -64,7 +64,9 @@ __FBSDID("$FreeBSD r284178 2015-06-09T11:49:56Z$"); > #ifdef __rtems__ > #include <limits.h> > #include <rtems.h> > -ISR_LOCK_DEFINE(static, _Timecounter_Lock, "Timecounter"); > +ISR_LOCK_DEFINE(, _Timecounter_Lock, "Timecounter") > +#define _Timecounter_Release(lock_context) \ > + _ISR_lock_Release_and_ISR_enable(&_Timecounter_Lock, lock_context) > #define hz rtems_clock_get_ticks_per_second() > #define printf(...) > #define log(...) > @@ -1383,7 +1385,7 @@ tc_windup(void) > #ifdef __rtems__ > ISR_lock_Context lock_context; > > - _ISR_lock_ISR_disable_and_acquire(&_Timecounter_Lock, > &lock_context); > + _Timecounter_Acquire(&lock_context); > #endif /* __rtems__ */ > > /* > @@ -1538,7 +1540,7 @@ tc_windup(void) > timekeep_push_vdso(); > #endif /* __rtems__ */ > #ifdef __rtems__ > - _ISR_lock_Release_and_ISR_enable(&_Timecounter_Lock, > &lock_context); > + _Timecounter_Release(&lock_context); > #endif /* __rtems__ */ > } > > @@ -1963,14 +1965,12 @@ _Timecounter_Tick(void) > } > #ifdef __rtems__ > void > -_Timecounter_Tick_simple(uint32_t delta, uint32_t offset) > +_Timecounter_Tick_simple(uint32_t delta, uint32_t offset, > + ISR_lock_Context *lock_context) > { > struct bintime bt; > struct timehands *th; > uint32_t ogen; > - ISR_lock_Context lock_context; > - > - _ISR_lock_ISR_disable_and_acquire(&_Timecounter_Lock, > &lock_context); > > th = timehands; > ogen = th->th_generation; > @@ -1997,7 +1997,7 @@ _Timecounter_Tick_simple(uint32_t delta, uint32_t > offset) > time_second = th->th_microtime.tv_sec; > time_uptime = th->th_offset.sec; > > - _ISR_lock_Release_and_ISR_enable(&_Timecounter_Lock, > &lock_context); > + _Timecounter_Release(lock_context); > > _Watchdog_Tick(); > } > diff --git a/doc/bsp_howto/clock.t b/doc/bsp_howto/clock.t > index f58b898..2891bb2 100644 > --- a/doc/bsp_howto/clock.t > +++ b/doc/bsp_howto/clock.t > @@ -89,6 +89,13 @@ static uint32_t some_tc_get( rtems_timecounter_simple > *tc ) > return some.counter; > @} > > +static void some_tc_at_tick( rtems_timecounter_simple *tc ) > +@{ > + /* > + * Do work necessary at the clock tick interrupt, e.g. clear a pending > flag. > + */ > +@} > + > static bool some_tc_is_pending( rtems_timecounter_simple *tc ) > @{ > return some.is_pending; > @@ -105,7 +112,11 @@ static uint32_t some_tc_get_timecount( struct > timecounter *tc ) > > static void some_tc_tick( void ) > @{ > - rtems_timecounter_simple_downcounter_tick( &some_tc, some_tc_get ); > + rtems_timecounter_simple_downcounter_tick( > + &some_tc, > + some_tc_get, > + some_tc_at_tick > + ); > @} > > static void some_support_initialize_hardware( void ) > -- > 2.7.0 > > _______________________________________________ > devel mailing list > devel@rtems.org > http://lists.rtems.org/mailman/listinfo/devel >
_______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
