On Mon, Jan 11, 2016 at 12:28 PM, Marcos Díaz < marcos.d...@tallertechnologies.com> wrote:
> I will issue a ticket. But I noticed that in my patch I include changes to > common code that Sebastian suggested, and this will break any other BSP > that uses rtems timecounter simple downcounter or upcounter, since these > function's signatures changed. Should we update all BSPs? Or make changes > more locally to our BSP? Meanwhile please do not apply the patch I sent. > > I would prefer not to ship BSPs with bugs but fixing them comes with risks. So I would lean to the same fix on the master and 4.11 branch fixing it where it needs to be fixed. What BSPs does this change impact? --joel > > On Mon, Jan 11, 2016 at 3:16 PM, Joel Sherrill <j...@rtems.org> wrote: > >> 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 >>> >> >> > > > -- > > ______________________________ > > <http://www.tallertechnologies.com> > > > Marcos Díaz > > Software Engineer > > > San Lorenzo 47, 3rd Floor, Office 5 > > Córdoba, Argentina > > > Phone: +54 351 4217888 / +54 351 4218211/ +54 351 7617452 > > Skype: markdiaz22 > >
_______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
