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.)

diff --git a/components/esp32/esp_timer.c b/components/esp32/esp_timer.c
index f747446aa75e2dc8da36a6b7cb089b393a3cab18..9fd8f72fe72718a8d228c0962bba9485925d8adf 100644 (file)
--- 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 3e08338a59d9f037e71478af8586cda2c6c39a71..365a11dbcb605fcb6a940ce8315b0aee929471c5 100644 (file)
--- 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 648b8ca5539e2ef482af1d137cd85ae7a61cac89..23e1add12486f4782a958f87e91395768db2f91b 100644 (file)
--- 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 f2094388d93bf9a189027045870942512b79e544..fa79bea8cbf04aa340d38b5d7a40c7f85e924440 100644 (file)
--- 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();
+}