Hello, I already created a ticket which blocks the 4.11 release for this bug:
https://devel.rtems.org/ticket/2502 I sent a new version of the patch to one of your previous threads some days ago: https://lists.rtems.org/pipermail/devel/2016-January/013289.html There is still a bug in it, since we must disable interrupts in the is pending function, but the general approach should work. ----- Am 11. Jan 2016 um 20:05 schrieb Marcos Díaz marcos.d...@tallertechnologies.com: > I made a fast search: > These BSPs use *rtems_timecounter_simple_downcounter*: > arm/shared/armv7m > m68k/mcf52235 > m68k/mcf5225x > m68k/mcf5329 > m68k/uC5282 > powerpc/mpc55xxevb > sparc/erc32 > sparc/leon2 > sparc/leon3 > sparc/shared > > These use *rtems_timecounter_simple_upcounter*: > powerpc/mpc55xxevb > arm/lpc22xx > > We should search a little more for the other changes but at least these > BSPs will be affected. Maybe we should wait Sebastian's opinion, since he > suggested the change in timecounter's signatures. > > On Mon, Jan 11, 2016 at 3:44 PM, Joel Sherrill <j...@rtems.org> wrote: > >> >> >> 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 >>> >>> >> > > > -- > > ______________________________ > > <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
