From 5f9281f71592d51e85a5386c06367d9f353de933 Mon Sep 17 00:00:00 2001 From: Mike Nix Date: Sun, 1 Sep 2019 12:40:58 +0800 Subject: [PATCH 1/7] 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. --- cores/esp8266/core_esp8266_main.cpp | 18 ++++++++++++++++++ tools/sdk/lib/fix_sdk_libs.sh | 7 +++++++ 2 files changed, 25 insertions(+) diff --git a/cores/esp8266/core_esp8266_main.cpp b/cores/esp8266/core_esp8266_main.cpp index 588019f0dd..df85273124 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 +byte ets_intr_lock_stack[ETS_INTR_LOCK_NEST_MAX]; +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,17 @@ extern "C" void optimistic_yield(uint32_t interval_us) { } } +extern "C" void ets_intr_lock_nest() { + if (ets_intr_lock_stack_ptr < ETS_INTR_LOCK_NEST_MAX) + ets_intr_lock_stack[ets_intr_lock_stack_ptr++] = xt_rsil(3); +} + +extern "C" void ets_intr_unlock_nest() { + if (ets_intr_lock_stack_ptr > 0) + xt_wsr_ps(ets_intr_lock_stack[--ets_intr_lock_stack_ptr]); +} + + extern "C" void __loop_end (void) { run_scheduled_functions(); diff --git a/tools/sdk/lib/fix_sdk_libs.sh b/tools/sdk/lib/fix_sdk_libs.sh index 178f08bd2f..b67c2a124d 100755 --- a/tools/sdk/lib/fix_sdk_libs.sh +++ b/tools/sdk/lib/fix_sdk_libs.sh @@ -16,3 +16,10 @@ xtensa-lx106-elf-objcopy --redefine-sym default_hostname=wifi_station_default_ho xtensa-lx106-elf-objcopy --redefine-sym default_hostname=wifi_station_default_hostname eagle_lwip_if.o xtensa-lx106-elf-ar r libmain.a eagle_lwip_if.o user_interface.o rm -f eagle_lwip_if.o user_interface.o + +# Replace use of ROM ets_intr_(un)lock with nestable ets_intr_(un)lock_nest +for f in libmain.a libpp.a libnet80211.a; do + xtensa-lx106-elf-objcopy --redefine-sym ets_intr_lock=ets_intr_lock_nest $f; + xtensa-lx106-elf-objcopy --redefine-sym ets_intr_unlock=ets_intr_unlock_nest $f; +done + From 7fd530796c6e2ec95b46196b720b6516ef0b3b7e Mon Sep 17 00:00:00 2001 From: Mike Nix Date: Wed, 4 Sep 2019 02:21:49 +0800 Subject: [PATCH 2/7] 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. --- cores/esp8266/core_esp8266_main.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/cores/esp8266/core_esp8266_main.cpp b/cores/esp8266/core_esp8266_main.cpp index df85273124..d2cff4c4e5 100644 --- a/cores/esp8266/core_esp8266_main.cpp +++ b/cores/esp8266/core_esp8266_main.cpp @@ -64,8 +64,8 @@ static uint32_t s_micros_at_task_start; * Max nesting seen by SDK so far is 2. */ #define ETS_INTR_LOCK_NEST_MAX 7 -byte ets_intr_lock_stack[ETS_INTR_LOCK_NEST_MAX]; -byte ets_intr_lock_stack_ptr=0; +uint16_t ets_intr_lock_stack[ETS_INTR_LOCK_NEST_MAX]; +byte ets_intr_lock_stack_ptr=0; extern "C" { @@ -131,11 +131,15 @@ extern "C" void optimistic_yield(uint32_t interval_us) { extern "C" void ets_intr_lock_nest() { 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 ets_intr_unlock_nest() { if (ets_intr_lock_stack_ptr > 0) xt_wsr_ps(ets_intr_lock_stack[--ets_intr_lock_stack_ptr]); + else + xt_rsil(0); } From e5177ac6e55e1a53fe2504f88994a8e74e364f15 Mon Sep 17 00:00:00 2001 From: Mike Nix Date: Wed, 4 Sep 2019 02:31:42 +0800 Subject: [PATCH 3/7] 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. --- cores/esp8266/core_esp8266_main.cpp | 4 ++-- tools/sdk/lib/fix_sdk_libs.sh | 7 ------- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/cores/esp8266/core_esp8266_main.cpp b/cores/esp8266/core_esp8266_main.cpp index d2cff4c4e5..bf0f1d4d84 100644 --- a/cores/esp8266/core_esp8266_main.cpp +++ b/cores/esp8266/core_esp8266_main.cpp @@ -128,14 +128,14 @@ extern "C" void optimistic_yield(uint32_t interval_us) { } } -extern "C" void ets_intr_lock_nest() { +extern "C" void 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 ets_intr_unlock_nest() { +extern "C" void ets_intr_unlock() { if (ets_intr_lock_stack_ptr > 0) xt_wsr_ps(ets_intr_lock_stack[--ets_intr_lock_stack_ptr]); else diff --git a/tools/sdk/lib/fix_sdk_libs.sh b/tools/sdk/lib/fix_sdk_libs.sh index b67c2a124d..178f08bd2f 100755 --- a/tools/sdk/lib/fix_sdk_libs.sh +++ b/tools/sdk/lib/fix_sdk_libs.sh @@ -16,10 +16,3 @@ xtensa-lx106-elf-objcopy --redefine-sym default_hostname=wifi_station_default_ho xtensa-lx106-elf-objcopy --redefine-sym default_hostname=wifi_station_default_hostname eagle_lwip_if.o xtensa-lx106-elf-ar r libmain.a eagle_lwip_if.o user_interface.o rm -f eagle_lwip_if.o user_interface.o - -# Replace use of ROM ets_intr_(un)lock with nestable ets_intr_(un)lock_nest -for f in libmain.a libpp.a libnet80211.a; do - xtensa-lx106-elf-objcopy --redefine-sym ets_intr_lock=ets_intr_lock_nest $f; - xtensa-lx106-elf-objcopy --redefine-sym ets_intr_unlock=ets_intr_unlock_nest $f; -done - From 78da5b7b2ef1486547dd7cd7c4cd2844980ca035 Mon Sep 17 00:00:00 2001 From: Mike Nix Date: Wed, 4 Sep 2019 22:00:45 +0800 Subject: [PATCH 4/7] Remove ets_intr_(un)lock from the rom .ld as we no longer use them --- tools/sdk/ld/eagle.rom.addr.v6.ld | 2 -- 1 file changed, 2 deletions(-) diff --git a/tools/sdk/ld/eagle.rom.addr.v6.ld b/tools/sdk/ld/eagle.rom.addr.v6.ld index 0407fb8806..7ae5f4e804 100644 --- a/tools/sdk/ld/eagle.rom.addr.v6.ld +++ b/tools/sdk/ld/eagle.rom.addr.v6.ld @@ -119,8 +119,6 @@ PROVIDE ( ets_install_external_printf = 0x40002450 ); PROVIDE ( ets_install_putc1 = 0x4000242c ); PROVIDE ( ets_install_putc2 = 0x4000248c ); PROVIDE ( ets_install_uart_printf = 0x40002438 ); -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 ); From ce22692b7c83746742ec34fcbc1ca2fca585ad20 Mon Sep 17 00:00:00 2001 From: Mike Nix Date: Sun, 8 Sep 2019 20:08:05 +0800 Subject: [PATCH 5/7] 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. --- cores/esp8266/core_esp8266_main.cpp | 10 ++++++++++ tools/sdk/ld/eagle.rom.addr.v6.ld | 2 +- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/cores/esp8266/core_esp8266_main.cpp b/cores/esp8266/core_esp8266_main.cpp index bf0f1d4d84..ad3c3522ec 100644 --- a/cores/esp8266/core_esp8266_main.cpp +++ b/cores/esp8266/core_esp8266_main.cpp @@ -143,6 +143,16 @@ extern "C" void ets_intr_unlock() { } +extern "C" bool ets_post_rom(uint8 prio, ETSSignal sig, ETSParam par); + +extern "C" bool 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 7ae5f4e804..215c3d07b6 100644 --- a/tools/sdk/ld/eagle.rom.addr.v6.ld +++ b/tools/sdk/ld/eagle.rom.addr.v6.ld @@ -126,7 +126,7 @@ PROVIDE ( ets_memcmp = 0x400018d4 ); PROVIDE ( ets_memcpy = 0x400018b4 ); PROVIDE ( ets_memmove = 0x400018c4 ); PROVIDE ( ets_memset = 0x400018a4 ); -PROVIDE ( ets_post = 0x40000e24 ); +PROVIDE ( ets_post_rom = 0x40000e24 ); PROVIDE ( ets_printf = 0x400024cc ); PROVIDE ( ets_putc = 0x40002be8 ); PROVIDE ( ets_rtc_int_register = 0x40002a40 ); From 29da3fe04e08240885b7f0b054ebde75f501f9fe Mon Sep 17 00:00:00 2001 From: Mike Nix Date: Mon, 9 Sep 2019 21:31:09 +0800 Subject: [PATCH 6/7] Add IRAM_ATTR to ets_intr_(un)lock and ets_post wrappers. --- cores/esp8266/core_esp8266_main.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cores/esp8266/core_esp8266_main.cpp b/cores/esp8266/core_esp8266_main.cpp index ad3c3522ec..bf3993e654 100644 --- a/cores/esp8266/core_esp8266_main.cpp +++ b/cores/esp8266/core_esp8266_main.cpp @@ -128,14 +128,14 @@ extern "C" void optimistic_yield(uint32_t interval_us) { } } -extern "C" void ets_intr_lock() { +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 ets_intr_unlock() { +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 @@ -145,7 +145,7 @@ extern "C" void ets_intr_unlock() { extern "C" bool ets_post_rom(uint8 prio, ETSSignal sig, ETSParam par); -extern "C" bool ets_post(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); From 4203e3f98b5cd111dc675ec5e9d3165d847186f6 Mon Sep 17 00:00:00 2001 From: Mike Nix Date: Wed, 11 Sep 2019 00:13:35 +0800 Subject: [PATCH 7/7] Throw in a few comments and make ets_intr_lock_stack* static. --- cores/esp8266/core_esp8266_main.cpp | 8 ++++++-- tools/sdk/ld/eagle.rom.addr.v6.ld | 7 +++++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/cores/esp8266/core_esp8266_main.cpp b/cores/esp8266/core_esp8266_main.cpp index bf3993e654..61c07da909 100644 --- a/cores/esp8266/core_esp8266_main.cpp +++ b/cores/esp8266/core_esp8266_main.cpp @@ -64,8 +64,8 @@ static uint32_t s_micros_at_task_start; * Max nesting seen by SDK so far is 2. */ #define ETS_INTR_LOCK_NEST_MAX 7 -uint16_t ets_intr_lock_stack[ETS_INTR_LOCK_NEST_MAX]; -byte ets_intr_lock_stack_ptr=0; +static uint16_t ets_intr_lock_stack[ETS_INTR_LOCK_NEST_MAX]; +static byte ets_intr_lock_stack_ptr=0; extern "C" { @@ -128,6 +128,8 @@ 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); @@ -143,6 +145,8 @@ extern "C" void IRAM_ATTR ets_intr_unlock() { } +// 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) { diff --git a/tools/sdk/ld/eagle.rom.addr.v6.ld b/tools/sdk/ld/eagle.rom.addr.v6.ld index 215c3d07b6..144c72b557 100644 --- a/tools/sdk/ld/eagle.rom.addr.v6.ld +++ b/tools/sdk/ld/eagle.rom.addr.v6.ld @@ -119,6 +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 ); @@ -126,6 +130,9 @@ 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 );