From 03bb2774d9201aa56e408a760ccf865a20d9c965 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Fri, 29 May 2020 21:52:48 +0200 Subject: [PATCH 1/5] spi_flash: don't call vTaskDelay in non-os context Fixes regression in core dump, when a crash happens in interrupt context. --- components/spi_flash/esp_flash_api.c | 4 +++- components/spi_flash/flash_ops.c | 22 ++++++++++++++----- components/spi_flash/include/esp_flash.h | 3 +++ components/spi_flash/include/esp_spi_flash.h | 5 +++++ components/spi_flash/spi_flash_os_func_app.c | 14 +++++++++++- components/spi_flash/spi_flash_os_func_noos.c | 1 + 6 files changed, 42 insertions(+), 7 deletions(-) diff --git a/components/spi_flash/esp_flash_api.c b/components/spi_flash/esp_flash_api.c index 5bcdc20b0..11c5c9b53 100644 --- a/components/spi_flash/esp_flash_api.c +++ b/components/spi_flash/esp_flash_api.c @@ -372,7 +372,9 @@ esp_err_t IRAM_ATTR esp_flash_erase_region(esp_flash_t *chip, uint32_t start, ui no_yield_time_us += (esp_timer_get_time() - start_time_us); if (no_yield_time_us / 1000 >= CONFIG_SPI_FLASH_ERASE_YIELD_DURATION_MS) { no_yield_time_us = 0; - vTaskDelay(CONFIG_SPI_FLASH_ERASE_YIELD_TICKS); + if (chip->os_func->yield) { + chip->os_func->yield(chip->os_func_data); + } } #endif } diff --git a/components/spi_flash/flash_ops.c b/components/spi_flash/flash_ops.c index a561cb4b5..09b14bed3 100644 --- a/components/spi_flash/flash_ops.c +++ b/components/spi_flash/flash_ops.c @@ -82,6 +82,7 @@ static spi_flash_counters_t s_flash_stats; static esp_err_t spi_flash_translate_rc(esp_rom_spiflash_result_t rc); static bool is_safe_write_address(size_t addr, size_t size); +static void spi_flash_os_yield(void); const DRAM_ATTR spi_flash_guard_funcs_t g_flash_guard_default_ops = { .start = spi_flash_disable_interrupts_caches_and_other_cpu, @@ -89,18 +90,20 @@ const DRAM_ATTR spi_flash_guard_funcs_t g_flash_guard_default_ops = { .op_lock = spi_flash_op_lock, .op_unlock = spi_flash_op_unlock, #if !CONFIG_SPI_FLASH_DANGEROUS_WRITE_ALLOWED - .is_safe_write_address = is_safe_write_address + .is_safe_write_address = is_safe_write_address, #endif + .yield = spi_flash_os_yield, }; const DRAM_ATTR spi_flash_guard_funcs_t g_flash_guard_no_os_ops = { .start = spi_flash_disable_interrupts_caches_and_other_cpu_no_os, .end = spi_flash_enable_interrupts_caches_no_os, - .op_lock = 0, - .op_unlock = 0, + .op_lock = NULL, + .op_unlock = NULL, #if !CONFIG_SPI_FLASH_DANGEROUS_WRITE_ALLOWED - .is_safe_write_address = 0 + .is_safe_write_address = NULL, #endif + .yield = NULL, }; static const spi_flash_guard_funcs_t *s_flash_guard_ops; @@ -185,6 +188,13 @@ static inline void IRAM_ATTR spi_flash_guard_op_unlock(void) } } +static void IRAM_ATTR spi_flash_os_yield(void) +{ +#ifdef CONFIG_SPI_FLASH_YIELD_DURING_ERASE + vTaskDelay(CONFIG_SPI_FLASH_ERASE_YIELD_TICKS); +#endif +} + #ifdef CONFIG_SPI_FLASH_USE_LEGACY_IMPL static esp_rom_spiflash_result_t IRAM_ATTR spi_flash_unlock(void) { @@ -263,7 +273,9 @@ esp_err_t IRAM_ATTR spi_flash_erase_range(size_t start_addr, size_t size) no_yield_time_us += (esp_timer_get_time() - start_time_us); if (no_yield_time_us / 1000 >= CONFIG_SPI_FLASH_ERASE_YIELD_DURATION_MS) { no_yield_time_us = 0; - vTaskDelay(CONFIG_SPI_FLASH_ERASE_YIELD_TICKS); + if (s_flash_guard_ops && s_flash_guard_ops->yield) { + s_flash_guard_ops->yield(); + } } #endif } diff --git a/components/spi_flash/include/esp_flash.h b/components/spi_flash/include/esp_flash.h index a60bfe437..ccea0d072 100644 --- a/components/spi_flash/include/esp_flash.h +++ b/components/spi_flash/include/esp_flash.h @@ -50,6 +50,9 @@ typedef struct { /** Delay for at least 'us' microseconds. Called in between 'start' and 'end'. */ esp_err_t (*delay_us)(void *arg, unsigned us); + + /** Yield to other tasks. Called during erase operations. */ + esp_err_t (*yield)(void *arg); } esp_flash_os_functions_t; /** @brief Structure to describe a SPI flash chip connected to the system. diff --git a/components/spi_flash/include/esp_spi_flash.h b/components/spi_flash/include/esp_spi_flash.h index 9e8b19e86..59d3679da 100644 --- a/components/spi_flash/include/esp_spi_flash.h +++ b/components/spi_flash/include/esp_spi_flash.h @@ -341,6 +341,10 @@ typedef void (*spi_flash_op_unlock_func_t)(void); * @brief Function to protect SPI flash critical regions corruption. */ typedef bool (*spi_flash_is_safe_write_address_t)(size_t addr, size_t size); +/** + * @brief Function to yield to the OS during erase operation. + */ +typedef void (*spi_flash_os_yield_t)(void); /** * Structure holding SPI flash access critical sections management functions. @@ -381,6 +385,7 @@ typedef struct { #if !CONFIG_SPI_FLASH_DANGEROUS_WRITE_ALLOWED spi_flash_is_safe_write_address_t is_safe_write_address; /**< checks flash write addresses.*/ #endif + spi_flash_os_yield_t yield; /**< yield to the OS during flash erase */ } spi_flash_guard_funcs_t; /** diff --git a/components/spi_flash/spi_flash_os_func_app.c b/components/spi_flash/spi_flash_os_func_app.c index af1ee8aa1..ceb046ec6 100644 --- a/components/spi_flash/spi_flash_os_func_app.c +++ b/components/spi_flash/spi_flash_os_func_app.c @@ -17,6 +17,8 @@ #include "esp_spi_flash.h" //for ``g_flash_guard_default_ops`` #include "esp_flash.h" #include "esp_flash_partitions.h" +#include "freertos/FreeRTOS.h" +#include "freertos/task.h" #include "hal/spi_types.h" #include "sdkconfig.h" @@ -101,6 +103,14 @@ static IRAM_ATTR esp_err_t delay_us(void *arg, unsigned us) return ESP_OK; } +static IRAM_ATTR esp_err_t spi_flash_os_yield(void *arg) +{ +#ifdef CONFIG_SPI_FLASH_YIELD_DURING_ERASE + vTaskDelay(CONFIG_SPI_FLASH_ERASE_YIELD_TICKS); +#endif + return ESP_OK; +} + static IRAM_ATTR esp_err_t main_flash_region_protected(void* arg, size_t start_addr, size_t size) { if (((spi1_app_func_arg_t*)arg)->no_protect || esp_partition_main_flash_region_safe(start_addr, size)) { @@ -117,14 +127,16 @@ static DRAM_ATTR spi1_app_func_arg_t main_flash_arg = {}; static const DRAM_ATTR esp_flash_os_functions_t esp_flash_spi1_default_os_functions = { .start = spi1_start, .end = spi1_end, - .delay_us = delay_us, .region_protected = main_flash_region_protected, + .delay_us = delay_us, + .yield = spi_flash_os_yield, }; static const esp_flash_os_functions_t esp_flash_spi23_default_os_functions = { .start = spi_start, .end = spi_end, .delay_us = delay_us, + .yield = spi_flash_os_yield }; static spi_bus_lock_dev_handle_t register_dev(int host_id) diff --git a/components/spi_flash/spi_flash_os_func_noos.c b/components/spi_flash/spi_flash_os_func_noos.c index 851a2d6ab..d565395e0 100644 --- a/components/spi_flash/spi_flash_os_func_noos.c +++ b/components/spi_flash/spi_flash_os_func_noos.c @@ -76,6 +76,7 @@ const DRAM_ATTR esp_flash_os_functions_t esp_flash_noos_functions = { .end = end, .delay_us = delay_us, .region_protected = NULL, + .yield = NULL, }; esp_err_t IRAM_ATTR esp_flash_app_disable_os_functions(esp_flash_t* chip) From 614a580bbb5a13f96595ae9ff792ecfd36d42150 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Fri, 29 May 2020 22:23:54 +0200 Subject: [PATCH 2/5] freertos, soc: don't lower INTLEVEL when entering critical sections This fixes the issue where XTOS_SET_INTLEVEL would lower INTLEVEL from 4 to 3, when eTaskGetState is invoked during the core dump, triggered from the interrupt watchdog. --- components/freertos/xtensa/include/freertos/portmacro.h | 2 +- components/soc/include/soc/spinlock.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/components/freertos/xtensa/include/freertos/portmacro.h b/components/freertos/xtensa/include/freertos/portmacro.h index 979ffecab..6b824e419 100644 --- a/components/freertos/xtensa/include/freertos/portmacro.h +++ b/components/freertos/xtensa/include/freertos/portmacro.h @@ -144,7 +144,7 @@ static inline uint32_t xPortGetCoreID(void); // They can be called from interrupts too. // WARNING: Only applies to current CPU. See notes above. static inline unsigned portENTER_CRITICAL_NESTED(void) { - unsigned state = XTOS_SET_INTLEVEL(XCHAL_EXCM_LEVEL); + unsigned state = XTOS_SET_MIN_INTLEVEL(XCHAL_EXCM_LEVEL); portbenchmarkINTERRUPT_DISABLE(); return state; } diff --git a/components/soc/include/soc/spinlock.h b/components/soc/include/soc/spinlock.h index fd6ec4e33..547fe6240 100644 --- a/components/soc/include/soc/spinlock.h +++ b/components/soc/include/soc/spinlock.h @@ -67,7 +67,7 @@ static inline bool __attribute__((always_inline)) spinlock_acquire(spinlock_t *l uint32_t core_id, other_core_id; assert(lock); - irq_status = XTOS_SET_INTLEVEL(XCHAL_EXCM_LEVEL); + irq_status = XTOS_SET_MIN_INTLEVEL(XCHAL_EXCM_LEVEL); if(timeout != SPINLOCK_WAIT_FOREVER){ RSR(CCOUNT, ccount_start); @@ -139,7 +139,7 @@ static inline void __attribute__((always_inline)) spinlock_release(spinlock_t *l uint32_t core_id; assert(lock); - irq_status = XTOS_SET_INTLEVEL(XCHAL_EXCM_LEVEL); + irq_status = XTOS_SET_MIN_INTLEVEL(XCHAL_EXCM_LEVEL); RSR(PRID, core_id); assert(core_id == lock->owner); // This is a mutex we didn't lock, or it's corrupt From 8c09968adc929decd12bf4e9ec359fc17a0750da Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Fri, 29 May 2020 22:33:44 +0200 Subject: [PATCH 3/5] test_apps: add coredump tests for int_wdt --- tools/test_apps/system/panic/app_test.py | 32 ++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tools/test_apps/system/panic/app_test.py b/tools/test_apps/system/panic/app_test.py index e4c82e80d..765cbbaaf 100644 --- a/tools/test_apps/system/panic/app_test.py +++ b/tools/test_apps/system/panic/app_test.py @@ -128,6 +128,22 @@ def test_coredump_uart_abort(env, extra_data): # TODO: check the contents of core dump output +@panic_test() +def test_coredump_uart_int_wdt(env, extra_data): + with get_dut(env, "coredump_uart", "test_int_wdt") as dut: + dut.expect_gme("Interrupt wdt timeout on CPU0") + dut.expect_reg_dump(0) + dut.expect("Backtrace:") + dut.expect_none("CORRUPTED", "Guru Meditation") + dut.expect_reg_dump(1) + dut.expect("Backtrace:") + dut.expect_elf_sha256() + dut.expect_none("CORRUPTED", "Guru Meditation") + dut.expect("Rebooting...") + dut.process_coredump_uart() + # TODO: check the contents of core dump output + + @panic_test() def test_coredump_flash_abort(env, extra_data): with get_dut(env, "coredump_flash", "test_abort") as dut: @@ -140,5 +156,21 @@ def test_coredump_flash_abort(env, extra_data): # TODO: check the contents of core dump output +@panic_test() +def test_coredump_flash_int_wdt(env, extra_data): + with get_dut(env, "coredump_flash", "test_int_wdt") as dut: + dut.expect_gme("Interrupt wdt timeout on CPU0") + dut.expect_reg_dump(0) + dut.expect("Backtrace:") + dut.expect_none("CORRUPTED", "Guru Meditation") + dut.expect_reg_dump(1) + dut.expect("Backtrace:") + dut.expect_elf_sha256() + dut.expect_none("CORRUPTED", "Guru Meditation") + dut.expect("Rebooting...") + dut.process_coredump_flash() + # TODO: check the contents of core dump output + + if __name__ == '__main__': run_all(__file__) From c0ed9349b0e5eaf21a3c9cbdb002d385c1831797 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Fri, 29 May 2020 22:35:28 +0200 Subject: [PATCH 4/5] test_apps: add build test for !CONFIG_SPI_FLASH_YIELD_DURING_ERASE --- tools/test_apps/system/build_test/sdkconfig.ci.no_flash_delay | 1 + 1 file changed, 1 insertion(+) create mode 100644 tools/test_apps/system/build_test/sdkconfig.ci.no_flash_delay diff --git a/tools/test_apps/system/build_test/sdkconfig.ci.no_flash_delay b/tools/test_apps/system/build_test/sdkconfig.ci.no_flash_delay new file mode 100644 index 000000000..5977e1a49 --- /dev/null +++ b/tools/test_apps/system/build_test/sdkconfig.ci.no_flash_delay @@ -0,0 +1 @@ +CONFIG_SPI_FLASH_YIELD_DURING_ERASE=n From f4ea9d4cea6bc707f9cb85c9b350bd3377bfc45f Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Tue, 2 Jun 2020 18:51:16 +0200 Subject: [PATCH 5/5] freertos: increase configMINIMAL_STACK_SIZE when building with -O0 FreeRTOS scheduler uses additional stack space, as in some functions variables are placed onto the stack instead of registers. This issue resulted in occasional stack overflows in dport task, when compiling at -O0 optimization level. - Increase the configMINIMAL_STACK_SIZE to 1kB. - Enable the watchpoint at the end of stack in CI startup test for this optimization level. --- .../freertos/xtensa/include/freertos/FreeRTOSConfig.h | 9 ++++++--- tools/test_apps/system/startup/sdkconfig.ci.opt_o0 | 1 + 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/components/freertos/xtensa/include/freertos/FreeRTOSConfig.h b/components/freertos/xtensa/include/freertos/FreeRTOSConfig.h index 51cbcbce2..128d62190 100644 --- a/components/freertos/xtensa/include/freertos/FreeRTOSConfig.h +++ b/components/freertos/xtensa/include/freertos/FreeRTOSConfig.h @@ -181,11 +181,14 @@ int xt_clock_freq(void) __attribute__((deprecated)); #define configMAX_PRIORITIES ( 25 ) #endif -#ifndef CONFIG_APPTRACE_ENABLE -#define configMINIMAL_STACK_SIZE 768 -#else +#if defined(CONFIG_APPTRACE_ENABLE) /* apptrace module requires at least 2KB of stack per task */ #define configMINIMAL_STACK_SIZE 2048 +#elif defined(CONFIG_COMPILER_OPTIMIZATION_NONE) +/* with optimizations disabled, scheduler uses additional stack */ +#define configMINIMAL_STACK_SIZE 1024 +#else +#define configMINIMAL_STACK_SIZE 768 #endif #ifndef configIDLE_TASK_STACK_SIZE diff --git a/tools/test_apps/system/startup/sdkconfig.ci.opt_o0 b/tools/test_apps/system/startup/sdkconfig.ci.opt_o0 index 8c66af770..d9a3a1b05 100644 --- a/tools/test_apps/system/startup/sdkconfig.ci.opt_o0 +++ b/tools/test_apps/system/startup/sdkconfig.ci.opt_o0 @@ -1 +1,2 @@ CONFIG_COMPILER_OPTIMIZATION_NONE=y +CONFIG_FREERTOS_WATCHPOINT_END_OF_STACK=y