From 094cf4d79dac4f81f105bbc27dff7f1e6cacd6e0 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Mon, 16 Oct 2017 19:16:20 +0800 Subject: [PATCH] wifi/bt coexistence: Fix disabled cache access race when writing to flash Moves the ets_timer_arm() / ets_timer_disarm() code paths to RAM Overhead is 740 bytes of IRAM, 0 bytes DRAM (For comparison: If all of esp_timer.c is moved to RAM, overhead is 1068 bytes IRAM and 480 bytes DRAM.) --- components/esp32/esp_timer.c | 22 ++++++------- components/esp32/esp_timer_esp32.c | 2 +- components/esp32/ets_timer_legacy.c | 8 ++--- components/esp32/test/test_ets_timer.c | 44 ++++++++++++++++++++++++++ 4 files changed, 60 insertions(+), 16 deletions(-) diff --git a/components/esp32/esp_timer.c b/components/esp32/esp_timer.c index f747446aa7..9fd8f72fe7 100644 --- a/components/esp32/esp_timer.c +++ b/components/esp32/esp_timer.c @@ -108,7 +108,7 @@ esp_err_t esp_timer_create(const esp_timer_create_args_t* args, return ESP_OK; } -esp_err_t esp_timer_start_once(esp_timer_handle_t timer, uint64_t timeout_us) +esp_err_t IRAM_ATTR esp_timer_start_once(esp_timer_handle_t timer, uint64_t timeout_us) { if (!is_initialized() || timer_armed(timer)) { return ESP_ERR_INVALID_STATE; @@ -121,7 +121,7 @@ esp_err_t esp_timer_start_once(esp_timer_handle_t timer, uint64_t timeout_us) return timer_insert(timer); } -esp_err_t esp_timer_start_periodic(esp_timer_handle_t timer, uint64_t period_us) +esp_err_t IRAM_ATTR esp_timer_start_periodic(esp_timer_handle_t timer, uint64_t period_us) { if (!is_initialized() || timer_armed(timer)) { return ESP_ERR_INVALID_STATE; @@ -135,7 +135,7 @@ esp_err_t esp_timer_start_periodic(esp_timer_handle_t timer, uint64_t period_us) return timer_insert(timer); } -esp_err_t esp_timer_stop(esp_timer_handle_t timer) +esp_err_t IRAM_ATTR esp_timer_stop(esp_timer_handle_t timer) { if (!is_initialized() || !timer_armed(timer)) { return ESP_ERR_INVALID_STATE; @@ -158,7 +158,7 @@ esp_err_t esp_timer_delete(esp_timer_handle_t timer) return ESP_OK; } -static esp_err_t timer_insert(esp_timer_handle_t timer) +static IRAM_ATTR esp_err_t timer_insert(esp_timer_handle_t timer) { timer_list_lock(); #if WITH_PROFILING @@ -187,7 +187,7 @@ static esp_err_t timer_insert(esp_timer_handle_t timer) return ESP_OK; } -static esp_err_t timer_remove(esp_timer_handle_t timer) +static IRAM_ATTR esp_err_t timer_remove(esp_timer_handle_t timer) { timer_list_lock(); LIST_REMOVE(timer, list_entry); @@ -202,7 +202,7 @@ static esp_err_t timer_remove(esp_timer_handle_t timer) #if WITH_PROFILING -static void timer_insert_inactive(esp_timer_handle_t timer) +static IRAM_ATTR void timer_insert_inactive(esp_timer_handle_t timer) { /* May be locked or not, depending on where this is called from. * Lock recursively. @@ -220,7 +220,7 @@ static void timer_insert_inactive(esp_timer_handle_t timer) timer_list_unlock(); } -static void timer_remove_inactive(esp_timer_handle_t timer) +static IRAM_ATTR void timer_remove_inactive(esp_timer_handle_t timer) { timer_list_lock(); LIST_REMOVE(timer, list_entry); @@ -229,17 +229,17 @@ static void timer_remove_inactive(esp_timer_handle_t timer) #endif // WITH_PROFILING -static bool timer_armed(esp_timer_handle_t timer) +static IRAM_ATTR bool timer_armed(esp_timer_handle_t timer) { return timer->alarm > 0; } -static void timer_list_lock() +static IRAM_ATTR void timer_list_lock() { portENTER_CRITICAL(&s_timer_lock); } -static void timer_list_unlock() +static IRAM_ATTR void timer_list_unlock() { portEXIT_CRITICAL(&s_timer_lock); } @@ -305,7 +305,7 @@ static void IRAM_ATTR timer_alarm_handler(void* arg) } } -static bool is_initialized() +static IRAM_ATTR bool is_initialized() { return s_timer_task != NULL; } diff --git a/components/esp32/esp_timer_esp32.c b/components/esp32/esp_timer_esp32.c index 3e08338a59..365a11dbcb 100644 --- a/components/esp32/esp_timer_esp32.c +++ b/components/esp32/esp_timer_esp32.c @@ -255,7 +255,7 @@ void esp_timer_impl_deinit() // FIXME: This value is safe for 80MHz APB frequency. // Should be modified to depend on clock frequency. -uint64_t esp_timer_impl_get_min_period_us() +uint64_t IRAM_ATTR esp_timer_impl_get_min_period_us() { return 50; } diff --git a/components/esp32/ets_timer_legacy.c b/components/esp32/ets_timer_legacy.c index 648b8ca553..23e1add124 100644 --- a/components/esp32/ets_timer_legacy.c +++ b/components/esp32/ets_timer_legacy.c @@ -43,7 +43,7 @@ #define TIMER_INITIALIZED_FIELD(p_ets_timer) ((p_ets_timer)->timer_expire) #define TIMER_INITIALIZED_VAL 0x12121212 -static bool timer_initialized(ETSTimer *ptimer) +static IRAM_ATTR bool timer_initialized(ETSTimer *ptimer) { return TIMER_INITIALIZED_FIELD(ptimer) == TIMER_INITIALIZED_VAL; } @@ -68,7 +68,7 @@ void ets_timer_setfn(ETSTimer *ptimer, ETSTimerFunc *pfunction, void *parg) } -void ets_timer_arm_us(ETSTimer *ptimer, uint32_t time_us, bool repeat_flag) +void IRAM_ATTR ets_timer_arm_us(ETSTimer *ptimer, uint32_t time_us, bool repeat_flag) { assert(timer_initialized(ptimer)); esp_timer_stop(ESP_TIMER(ptimer)); // no error check @@ -79,7 +79,7 @@ void ets_timer_arm_us(ETSTimer *ptimer, uint32_t time_us, bool repeat_flag) } } -void ets_timer_arm(ETSTimer *ptimer, uint32_t time_ms, bool repeat_flag) +void IRAM_ATTR ets_timer_arm(ETSTimer *ptimer, uint32_t time_ms, bool repeat_flag) { uint64_t time_us = 1000LL * (uint64_t) time_ms; assert(timer_initialized(ptimer)); @@ -100,7 +100,7 @@ void ets_timer_done(ETSTimer *ptimer) } } -void ets_timer_disarm(ETSTimer *ptimer) +void IRAM_ATTR ets_timer_disarm(ETSTimer *ptimer) { if (timer_initialized(ptimer)) { esp_timer_stop(ESP_TIMER(ptimer)); diff --git a/components/esp32/test/test_ets_timer.c b/components/esp32/test/test_ets_timer.c index f2094388d9..fa79bea8cb 100644 --- a/components/esp32/test/test_ets_timer.c +++ b/components/esp32/test/test_ets_timer.c @@ -7,6 +7,7 @@ #include "freertos/FreeRTOS.h" #include "freertos/task.h" #include "freertos/semphr.h" +#include "esp_spi_flash.h" TEST_CASE("ets_timer produces correct delay", "[ets_timer]") { @@ -192,3 +193,46 @@ TEST_CASE("multiple ETSTimers are ordered correctly", "[ets_timer]") #undef N } + +/* WiFi/BT coexistence will sometimes arm/disarm + timers from an ISR where flash may be disabled. */ +IRAM_ATTR TEST_CASE("ETSTimers arm & disarm run from IRAM", "[ets_timer]") +{ + void timer_func(void* arg) + { + volatile bool *b = (volatile bool *)arg; + *b = true; + } + + volatile bool flag = false; + ETSTimer timer1; + const int INTERVAL = 5; + + ets_timer_setfn(&timer1, &timer_func, (void *)&flag); + + /* arm a disabled timer, then disarm a live timer */ + + g_flash_guard_default_ops.start(); // Disables flash cache + + ets_timer_arm(&timer1, INTERVAL, false); + // redundant call is deliberate (test code path if already armed) + ets_timer_arm(&timer1, INTERVAL, false); + ets_timer_disarm(&timer1); + + g_flash_guard_default_ops.end(); // Re-enables flash cache + + TEST_ASSERT_FALSE(flag); // didn't expire yet + + /* do the same thing but wait for the timer to expire */ + + g_flash_guard_default_ops.start(); + ets_timer_arm(&timer1, INTERVAL, false); + g_flash_guard_default_ops.end(); + + vTaskDelay(2 * INTERVAL / portTICK_PERIOD_MS); + TEST_ASSERT_TRUE(flag); + + g_flash_guard_default_ops.start(); + ets_timer_disarm(&timer1); + g_flash_guard_default_ops.end(); +}