Merge branch 'bugfix/flash_op_deadlock' into 'master'

spi_flash: fix race condition in s_flash_op_complete access

Flash operation complete flag was cleared by the CPU initiating flash operation. If the other core was running an ISR, then IPC task could be late to enter the loop to check `s_flash_op_complete` by the time next flash operation started. This would cause a deadlock, as the IPC task would still be waiting for `s_flash_op_complete` to be set (which was already cleared by the next flash operation), while the flash operation task would be blocked waiting for IPC task to set `s_flash_op_can_start`.

If the flag is cleared on the CPU waiting on this flag, then the race condition can not happen.

See merge request !615

components/spi_flash/cache_utils.c

@@ -69,9 +69,12 @@ void IRAM_ATTR spi_flash_op_block_func(void* arg)
     uint32_t cpuid = (uint32_t) arg;
     // Disable cache so that flash operation can start
     spi_flash_disable_cache(cpuid, &s_flash_op_cache_state[cpuid]);
+    // s_flash_op_complete flag is cleared on *this* CPU, otherwise the other
+    // CPU may reset the flag back to false before IPC task has a chance to check it
+    // (if it is preempted by an ISR taking non-trivial amount of time)
+    s_flash_op_complete = false;
     s_flash_op_can_start = true;
     while (!s_flash_op_complete) {
-        // until we have a way to use interrupts for inter-CPU communication,
         // busy loop here and wait for the other CPU to finish flash operation
     }
     // Flash operation is complete, re-enable cache
@@ -105,7 +108,6 @@ void IRAM_ATTR spi_flash_disable_interrupts_caches_and_other_cpu()
         // Signal to the spi_flash_op_block_task on the other CPU that we need it to
         // disable cache there and block other tasks from executing.
         s_flash_op_can_start = false;
-        s_flash_op_complete = false;
         esp_err_t ret = esp_ipc_call(other_cpuid, &spi_flash_op_block_func, (void*) other_cpuid);
         assert(ret == ESP_OK);
         while (!s_flash_op_can_start) {

components/spi_flash/test/test_spi_flash.c

@@ -6,6 +6,8 @@
 #include <unity.h>
 #include <esp_spi_flash.h>
 #include <esp_attr.h>
+#include "driver/timer.h"
+#include "esp_intr_alloc.h"
 
 struct flash_test_ctx {
     uint32_t offset;
@@ -87,3 +89,81 @@ TEST_CASE("flash write and erase work both on PRO CPU and on APP CPU", "[spi_fla
     vSemaphoreDelete(done);
 }
 
+
+
+typedef struct {
+    size_t buf_size;
+    uint8_t* buf;
+    size_t flash_addr;
+    size_t repeat_count;
+    SemaphoreHandle_t done;
+} read_task_arg_t;
+
+
+typedef struct {
+    size_t delay_time_us;
+    size_t repeat_count;
+} block_task_arg_t;
+
+static void IRAM_ATTR timer_isr(void* varg) {
+    block_task_arg_t* arg = (block_task_arg_t*) varg;
+    TIMERG0.int_clr_timers.t0 = 1;
+    TIMERG0.hw_timer[0].config.alarm_en = 1;
+    ets_delay_us(arg->delay_time_us);
+    arg->repeat_count++;
+}
+
+static void read_task(void* varg) {
+    read_task_arg_t* arg = (read_task_arg_t*) varg;
+    for (size_t i = 0; i < arg->repeat_count; ++i) {
+        ESP_ERROR_CHECK( spi_flash_read(arg->flash_addr, arg->buf, arg->buf_size) );
+    }
+    xSemaphoreGive(arg->done);
+    vTaskDelay(1);
+    vTaskDelete(NULL);
+}
+
+TEST_CASE("spi flash functions can run along with IRAM interrupts", "[spi_flash]")
+{
+    const size_t size = 128;
+    read_task_arg_t read_arg = {
+            .buf_size = size,
+            .buf = (uint8_t*) malloc(size),
+            .flash_addr = 0,
+            .repeat_count = 1000,
+            .done = xSemaphoreCreateBinary()
+    };
+
+    timer_config_t config = {
+            .alarm_en = true,
+            .counter_en = false,
+            .intr_type = TIMER_INTR_LEVEL,
+            .counter_dir = TIMER_COUNT_UP,
+            .auto_reload = true,
+            .divider = 80
+    };
+
+    block_task_arg_t block_arg = {
+            .repeat_count = 0,
+            .delay_time_us = 100
+    };
+
+    ESP_ERROR_CHECK( timer_init(TIMER_GROUP_0, TIMER_0, &config) );
+    timer_pause(TIMER_GROUP_0, TIMER_0);
+    ESP_ERROR_CHECK( timer_set_alarm_value(TIMER_GROUP_0, TIMER_0, 120) );
+    intr_handle_t handle;
+    ESP_ERROR_CHECK( timer_isr_register(TIMER_GROUP_0, TIMER_0, &timer_isr, &block_arg, ESP_INTR_FLAG_IRAM, &handle) );
+    timer_set_counter_value(TIMER_GROUP_0, TIMER_0, 0);
+    timer_enable_intr(TIMER_GROUP_0, TIMER_0);
+    timer_start(TIMER_GROUP_0, TIMER_0);
+
+    xTaskCreatePinnedToCore(read_task, "r", 2048, &read_arg, 3, NULL, 1);
+    xSemaphoreTake(read_arg.done, portMAX_DELAY);
+
+    timer_pause(TIMER_GROUP_0, TIMER_0);
+    timer_disable_intr(TIMER_GROUP_0, TIMER_0);
+    esp_intr_free(handle);
+    vSemaphoreDelete(read_arg.done);
+    free(read_arg.buf);
+}
+