From d8531cb2c4aec39d917fed8d642afe86af59e1c6 Mon Sep 17 00:00:00 2001 From: Mike Nix Date: Wed, 11 Sep 2019 10:59:07 +0800 Subject: [PATCH] Ets intr lock nest (#6484) * Replace the SDK's use of ets_intr_lock/unlock with nestable versions Testing has shown that there are several paths in the SDK that result in nested calls to ets_intr_lock() / ets_intr_unlock() which may be a problem. These functions also do not preserve the enabled interrupt level and may result in code running with interrupts enabled when that is not intended. This issue has recently been fixed in the Arduino code by using xt_rsil() / xt_wsr_ps() but still exists in the Espressif SDK code. This commit is intended to fix that and should be used in addition to the above. The maximum nesting I have seen is 2 and lock/unlock calls appear to be balanced. A max of 7 levels of nesting leaves plenty of room for that to change. * make ets_intr_lock_stack uint16_t and behave like the original on over/underflow The PS register is 15 bits, we should store the whole thing as xt_wsr_ps() writes the whole thing. Also if there is an underflow, we should make sure interrupts are enabled. Same goes for overflow making sure interrupts are disabled, although this is less important. * Rename ets_intr_(un)lock_nest to ets_intr_(un)lock This saves having to modify libmain.a, libpp.a and libnet80211.a to use the nested versions. Adjusts fix_sdk_libs.sh accordingly. * Remove ets_intr_(un)lock from the rom .ld as we no longer use them * ets_post() wrapper to preserve interrupt state Add a wrapper around the ets_post code in rom to preserve the interrupt enable state. Rather than modifying the SDK libs, rename ets_post in the .ld file and call the wrapper "ets_post" to replace it. As far as I can establish, ets_post is the only rom function in use by our code or the SDK libs we use that causes calls to ets_intr_(un)lock. * Add IRAM_ATTR to ets_intr_(un)lock and ets_post wrappers. * Throw in a few comments and make ets_intr_lock_stack* static. --- cores/esp8266/core_esp8266_main.cpp | 36 +++++++++++++++++++++++++++++ tools/sdk/ld/eagle.rom.addr.v6.ld | 5 ++++ 2 files changed, 41 insertions(+) diff --git a/cores/esp8266/core_esp8266_main.cpp b/cores/esp8266/core_esp8266_main.cpp index 588019f0d..61c07da90 100644 --- a/cores/esp8266/core_esp8266_main.cpp +++ b/cores/esp8266/core_esp8266_main.cpp @@ -60,6 +60,13 @@ static os_event_t s_loop_queue[LOOP_QUEUE_SIZE]; /* Used to implement optimistic_yield */ static uint32_t s_micros_at_task_start; +/* For ets_intr_lock_nest / ets_intr_unlock_nest + * Max nesting seen by SDK so far is 2. + */ +#define ETS_INTR_LOCK_NEST_MAX 7 +static uint16_t ets_intr_lock_stack[ETS_INTR_LOCK_NEST_MAX]; +static byte ets_intr_lock_stack_ptr=0; + extern "C" { extern const uint32_t __attribute__((section(".ver_number"))) core_version = ARDUINO_ESP8266_GIT_VER; @@ -121,6 +128,35 @@ extern "C" void optimistic_yield(uint32_t interval_us) { } } + +// Replace ets_intr_(un)lock with nestable versions +extern "C" void IRAM_ATTR ets_intr_lock() { + if (ets_intr_lock_stack_ptr < ETS_INTR_LOCK_NEST_MAX) + ets_intr_lock_stack[ets_intr_lock_stack_ptr++] = xt_rsil(3); + else + xt_rsil(3); +} + +extern "C" void IRAM_ATTR ets_intr_unlock() { + if (ets_intr_lock_stack_ptr > 0) + xt_wsr_ps(ets_intr_lock_stack[--ets_intr_lock_stack_ptr]); + else + xt_rsil(0); +} + + +// Save / Restore the PS state across the rom ets_post call as the rom code +// does not implement this correctly. +extern "C" bool ets_post_rom(uint8 prio, ETSSignal sig, ETSParam par); + +extern "C" bool IRAM_ATTR ets_post(uint8 prio, ETSSignal sig, ETSParam par) { + uint32_t saved; + asm volatile ("rsr %0,ps":"=a" (saved)); + bool rc=ets_post_rom(prio, sig, par); + xt_wsr_ps(saved); + return rc; +} + extern "C" void __loop_end (void) { run_scheduled_functions(); diff --git a/tools/sdk/ld/eagle.rom.addr.v6.ld b/tools/sdk/ld/eagle.rom.addr.v6.ld index 0407fb880..144c72b55 100644 --- a/tools/sdk/ld/eagle.rom.addr.v6.ld +++ b/tools/sdk/ld/eagle.rom.addr.v6.ld @@ -119,8 +119,10 @@ PROVIDE ( ets_install_external_printf = 0x40002450 ); PROVIDE ( ets_install_putc1 = 0x4000242c ); PROVIDE ( ets_install_putc2 = 0x4000248c ); PROVIDE ( ets_install_uart_printf = 0x40002438 ); +/* permanently hide reimplemented ets_intr_*lock(), see #6484 PROVIDE ( ets_intr_lock = 0x40000f74 ); PROVIDE ( ets_intr_unlock = 0x40000f80 ); +*/ PROVIDE ( ets_isr_attach = 0x40000f88 ); PROVIDE ( ets_isr_mask = 0x40000f98 ); PROVIDE ( ets_isr_unmask = 0x40000fa8 ); @@ -128,7 +130,10 @@ PROVIDE ( ets_memcmp = 0x400018d4 ); PROVIDE ( ets_memcpy = 0x400018b4 ); PROVIDE ( ets_memmove = 0x400018c4 ); PROVIDE ( ets_memset = 0x400018a4 ); +/* renamed to ets_post_rom(), see #6484 PROVIDE ( ets_post = 0x40000e24 ); +*/ +PROVIDE ( ets_post_rom = 0x40000e24 ); PROVIDE ( ets_printf = 0x400024cc ); PROVIDE ( ets_putc = 0x40002be8 ); PROVIDE ( ets_rtc_int_register = 0x40002a40 );