From 92436021ab0eb2d7eb271f4bfe9f2500b8a10011 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Tue, 28 Mar 2017 01:02:58 +0800 Subject: [PATCH] spi_flash: fix race condition in s_flash_op_complete access Flash operation complete flag was cleared by the core 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. If the flag is cleared on the CPU waiting on this flag, then the race condition can not happen. --- components/spi_flash/cache_utils.c | 6 +- components/spi_flash/test/test_spi_flash.c | 80 ++++++++++++++++++++++ 2 files changed, 84 insertions(+), 2 deletions(-) diff --git a/components/spi_flash/cache_utils.c b/components/spi_flash/cache_utils.c index 6d0dd91845..b4e5fa6db5 100644 --- a/components/spi_flash/cache_utils.c +++ b/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) { diff --git a/components/spi_flash/test/test_spi_flash.c b/components/spi_flash/test/test_spi_flash.c index c7829ead2a..f3bd3dc104 100644 --- a/components/spi_flash/test/test_spi_flash.c +++ b/components/spi_flash/test/test_spi_flash.c @@ -6,6 +6,8 @@ #include #include #include +#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); +} +