From d36b1ca8aec854f822c7df1619dd7c45d83b4127 Mon Sep 17 00:00:00 2001 From: Graham Sanderson Date: Wed, 10 Mar 2021 12:04:04 -0600 Subject: [PATCH] hardware_timer: fix race condition whem a new timer being added becomes missed thus obviating the need for an IRQ but there is an IRQ already pending for another timer (#243) --- src/rp2_common/hardware_timer/timer.c | 27 ++++++++++++++-------- test/pico_time_test/pico_time_test.c | 32 ++++++++++++++++++++++++++- 2 files changed, 49 insertions(+), 10 deletions(-) diff --git a/src/rp2_common/hardware_timer/timer.c b/src/rp2_common/hardware_timer/timer.c index 7745484..23bea05 100644 --- a/src/rp2_common/hardware_timer/timer.c +++ b/src/rp2_common/hardware_timer/timer.c @@ -166,8 +166,9 @@ bool hardware_alarm_set_target(uint alarm_num, absolute_time_t target) { // 1) actually set the hardware timer spin_lock_t *lock = spin_lock_instance(PICO_SPINLOCK_ID_TIMER); uint32_t save = spin_lock_blocking(lock); - timer_hw->intr = 1u << alarm_num; + uint8_t old_timer_callbacks_pending = timer_callbacks_pending; timer_callbacks_pending |= (uint8_t)(1u << alarm_num); + timer_hw->intr = 1u << alarm_num; // clear any IRQ timer_hw->alarm[alarm_num] = (uint32_t) t; // Set the alarm. Writing time should arm it target_hi[alarm_num] = (uint32_t)(t >> 32u); @@ -178,18 +179,26 @@ bool hardware_alarm_set_target(uint alarm_num, absolute_time_t target) { assert(timer_hw->ints & 1u << alarm_num); } else { if (time_us_64() >= t) { - // ok well it is time now; the irq isn't being handled yet because of the spin lock - // however the other core might be in the IRQ handler itself about to do a callback - // we do the firing ourselves (and indicate to the IRQ handler if any that it shouldn't + // we are already at or past the right time; there is no point in us racing against the IRQ + // we are about to generate. note however that, if there was already a timer pending before, + // then we still let the IRQ fire, as whatever it was, is not handled by our setting missed=true here missed = true; - // disarm the timer - timer_hw->armed = 1u << alarm_num; - timer_hw->intr = 1u << alarm_num; // clear the IRQ too - // and set flag in case we're already in the IRQ handler waiting on the spinlock (on the other core) - timer_callbacks_pending &= (uint8_t)~(1u << alarm_num); + if (timer_callbacks_pending != old_timer_callbacks_pending) { + // disarm the timer + timer_hw->armed = 1u << alarm_num; + // clear the IRQ... + timer_hw->intr = 1u << alarm_num; + // ... including anything pending on the processor - perhaps unnecessary, but + // our timer flag says we aren't expecting anything. + irq_clear(harware_alarm_irq_number(alarm_num)); + // and clear our flag so that if the IRQ handler is already active (because it is on + // the other core) it will also skip doing anything + timer_callbacks_pending = old_timer_callbacks_pending; + } } } spin_unlock(lock, save); + // note at this point any pending timer IRQ can likely run } return missed; } diff --git a/test/pico_time_test/pico_time_test.c b/test/pico_time_test/pico_time_test.c index 1093dd6..2209fd5 100644 --- a/test/pico_time_test/pico_time_test.c +++ b/test/pico_time_test/pico_time_test.c @@ -64,12 +64,13 @@ static bool repeating_timer_callback(struct repeating_timer *t) { #define RESOLUTION_ALLOWANCE PICO_HARDWARE_TIMER_RESOLUTION_US #endif +int issue_195_test(void); + int main() { setup_default_uart(); alarm_pool_init_default(); PICOTEST_START(); - struct alarm_pool *pools[NUM_TIMERS]; for(uint i=0; i 0, "near the end of time should be before the end of time") PICOTEST_END_SECTION(); + if (issue_195_test()) { + return -1; + } + PICOTEST_END_TEST(); } +#define ISSUE_195_TIMER_DELAY 50 +volatile int issue_195_counter; +int64_t issue_195_callback(alarm_id_t id, void *user_data) { + issue_195_counter++; + return -ISSUE_195_TIMER_DELAY; +} + +int issue_195_test(void) { + PICOTEST_START_SECTION("Issue #195 race condition - without fix may hang on gcc 10.2.1 release builds"); + absolute_time_t t1 = get_absolute_time(); + int id = add_alarm_in_us(ISSUE_195_TIMER_DELAY, issue_195_callback, NULL, true); + for(uint i=0;i<5000;i++) { + sleep_us(100); + sleep_us(100); + uint delay = 9; // 9 seems to be the magic number (at least for reproducing on 10.2.1) + sleep_us(delay); + } + absolute_time_t t2 = get_absolute_time(); + cancel_alarm(id); + int expected_count = absolute_time_diff_us(t1, t2) / ISSUE_195_TIMER_DELAY; + printf("Timer fires approx_expected=%d actual=%d\n", expected_count, issue_195_counter); + PICOTEST_END_SECTION(); + return 0; +} +