From d2860b58b06f02303a30a7ff505f3fae2c21ba25 Mon Sep 17 00:00:00 2001 From: Scott Shawcroft Date: Thu, 19 Aug 2021 12:18:13 -0700 Subject: [PATCH] Check background pending before sleep There is a race between when we run background tasks and when we sleep. If an interrupt happens between the two, then we may delay executing the background task. On some ports we checked this for TinyUSB already. On iMX RT, we didn't which caused USB issues. This PR makes it more generic for all background tasks including USB. Fixes #5086 and maybe others. --- ports/atmel-samd/supervisor/port.c | 5 ++--- ports/esp32s2/supervisor/port.c | 5 ++++- ports/mimxrt10xx/Makefile | 4 +++- ports/mimxrt10xx/linking/common.ld | 1 - ports/mimxrt10xx/supervisor/port.c | 16 +++++++++++----- ports/nrf/supervisor/port.c | 19 ++++++++++++------- ports/raspberrypi/supervisor/port.c | 5 ++--- ports/stm/supervisor/port.c | 10 ++++++++-- shared-bindings/storage/__init__.c | 8 ++++++++ supervisor/background_callback.h | 6 ++++++ supervisor/shared/background_callback.c | 6 +++++- supervisor/shared/usb/usb.c | 2 +- 12 files changed, 62 insertions(+), 25 deletions(-) diff --git a/ports/atmel-samd/supervisor/port.c b/ports/atmel-samd/supervisor/port.c index 4aa9daa3e9..3ced6efde6 100644 --- a/ports/atmel-samd/supervisor/port.c +++ b/ports/atmel-samd/supervisor/port.c @@ -80,12 +80,11 @@ #include "shared_timers.h" #include "reset.h" +#include "supervisor/background_callback.h" #include "supervisor/shared/safe_mode.h" #include "supervisor/shared/stack.h" #include "supervisor/shared/tick.h" -#include "tusb.h" - #if CIRCUITPY_GAMEPADSHIFT #include "shared-module/gamepadshift/__init__.h" #endif @@ -628,7 +627,7 @@ void port_idle_until_interrupt(void) { } #endif common_hal_mcu_disable_interrupts(); - if (!tud_task_event_ready() && sleep_ok && !_woken_up) { + if (!background_callback_pending() && sleep_ok && !_woken_up) { __DSB(); __WFI(); } diff --git a/ports/esp32s2/supervisor/port.c b/ports/esp32s2/supervisor/port.c index 1fca60a1c3..de6941ed70 100644 --- a/ports/esp32s2/supervisor/port.c +++ b/ports/esp32s2/supervisor/port.c @@ -48,6 +48,7 @@ #include "common-hal/watchdog/WatchDogTimer.h" #include "common-hal/socketpool/Socket.h" #include "common-hal/wifi/__init__.h" +#include "supervisor/background_callback.h" #include "supervisor/memory.h" #include "supervisor/shared/tick.h" #include "shared-bindings/rtc/__init__.h" @@ -329,7 +330,9 @@ void port_interrupt_after_ticks(uint32_t ticks) { // On the ESP we use FreeRTOS notifications instead of interrupts so this is a // bit of a misnomer. void port_idle_until_interrupt(void) { - xTaskNotifyWait(0x01, 0x01, NULL, portMAX_DELAY); + if (!background_callback_pending()) { + xTaskNotifyWait(0x01, 0x01, NULL, portMAX_DELAY); + } } // Wrap main in app_main that the IDF expects. diff --git a/ports/mimxrt10xx/Makefile b/ports/mimxrt10xx/Makefile index c112902639..310b5d6f31 100644 --- a/ports/mimxrt10xx/Makefile +++ b/ports/mimxrt10xx/Makefile @@ -81,11 +81,13 @@ CFLAGS += -Os -ftree-vrp -DNDEBUG -ffreestanding CFLAGS += -DCFG_TUSB_MCU=OPT_MCU_MIMXRT10XX -DCFG_TUD_MIDI_RX_BUFSIZE=512 -DCFG_TUD_CDC_RX_BUFSIZE=512 -DCFG_TUD_MIDI_TX_BUFSIZE=512 -DCFG_TUD_CDC_TX_BUFSIZE=512 -DCFG_TUD_MSC_BUFSIZE=1024 #Debugging/Optimization +# Never set -fno-inline because we use inline to move small functions into routines that must be +# in RAM. If inlining is disallowed, then we may end up calling a function in flash when we cannot. ifeq ($(DEBUG), 1) # You may want to disable -flto if it interferes with debugging. # CFLAGS += -flto -flto-partition=none # You may want to enable these flags to make setting breakpoints easier. - CFLAGS += -fno-inline -fno-ipa-sra + CFLAGS += -fno-ipa-sra else #CFLAGS += -flto -flto-partition=none endif diff --git a/ports/mimxrt10xx/linking/common.ld b/ports/mimxrt10xx/linking/common.ld index 720361aa0a..44e8d537c4 100644 --- a/ports/mimxrt10xx/linking/common.ld +++ b/ports/mimxrt10xx/linking/common.ld @@ -51,7 +51,6 @@ SECTIONS KEEP(* (.boot_hdr.dcd_data)) . = ALIGN(4); } > FLASH_IVT - image_vector_table = LOADADDR(.ivt); .text : { diff --git a/ports/mimxrt10xx/supervisor/port.c b/ports/mimxrt10xx/supervisor/port.c index e31dda173c..88a4ce5bb7 100644 --- a/ports/mimxrt10xx/supervisor/port.c +++ b/ports/mimxrt10xx/supervisor/port.c @@ -40,10 +40,11 @@ #include "common-hal/pwmio/PWMOut.h" #include "common-hal/rtc/RTC.h" #include "common-hal/busio/SPI.h" +#include "shared-bindings/microcontroller/__init__.h" #include "reset.h" -#include "tusb.h" +#include "supervisor/background_callback.h" #if CIRCUITPY_GAMEPADSHIFT #include "shared-module/gamepadshift/__init__.h" @@ -400,10 +401,15 @@ void port_idle_until_interrupt(void) { __set_FPSCR(__get_FPSCR() & ~(0x9f)); (void)__get_FPSCR(); } - NVIC_ClearPendingIRQ(SNVS_HP_WRAPPER_IRQn); - CLOCK_SetMode(kCLOCK_ModeWait); - __WFI(); - CLOCK_SetMode(kCLOCK_ModeRun); + + common_hal_mcu_disable_interrupts(); + if (!background_callback_pending()) { + NVIC_ClearPendingIRQ(SNVS_HP_WRAPPER_IRQn); + CLOCK_SetMode(kCLOCK_ModeWait); + __WFI(); + CLOCK_SetMode(kCLOCK_ModeRun); + } + common_hal_mcu_enable_interrupts(); } /** diff --git a/ports/nrf/supervisor/port.c b/ports/nrf/supervisor/port.c index 66b4bc9181..772fcde120 100644 --- a/ports/nrf/supervisor/port.c +++ b/ports/nrf/supervisor/port.c @@ -25,8 +25,10 @@ * THE SOFTWARE. */ -#include #include "supervisor/port.h" + +#include +#include "supervisor/background_callback.h" #include "supervisor/board.h" #include "nrfx/hal/nrf_clock.h" @@ -39,6 +41,8 @@ #include "nrf/power.h" #include "nrf/timers.h" +#include "nrf_nvic.h" + #include "common-hal/microcontroller/Pin.h" #include "common-hal/_bleio/__init__.h" #include "common-hal/analogio/AnalogIn.h" @@ -360,7 +364,12 @@ void port_idle_until_interrupt(void) { sd_softdevice_is_enabled(&sd_enabled); if (sd_enabled) { - sd_app_evt_wait(); + uint8_t is_nested_critical_region; + sd_nvic_critical_region_enter(&is_nested_critical_region); + if (!background_callback_pending()) { + sd_app_evt_wait(); + } + sd_nvic_critical_region_exit(is_nested_critical_region); } else { // Call wait for interrupt ourselves if the SD isn't enabled. // Note that `wfi` should be called with interrupts disabled, @@ -376,11 +385,7 @@ void port_idle_until_interrupt(void) { // function (whether or not SD is enabled) int nested = __get_PRIMASK(); __disable_irq(); - bool ok = true; - #if CIRCUITPY_USB - ok = !tud_task_event_ready(); - #endif - if (ok) { + if (!background_callback_pending()) { __DSB(); __WFI(); } diff --git a/ports/raspberrypi/supervisor/port.c b/ports/raspberrypi/supervisor/port.c index 443cfc2715..ef35c22518 100644 --- a/ports/raspberrypi/supervisor/port.c +++ b/ports/raspberrypi/supervisor/port.c @@ -27,6 +27,7 @@ #include #include +#include "supervisor/background_callback.h" #include "supervisor/board.h" #include "supervisor/port.h" @@ -54,8 +55,6 @@ #include "src/common/pico_time/include/pico/time.h" #include "src/common/pico_binary_info/include/pico/binary_info.h" -#include "tusb.h" - #include "pico/bootrom.h" #include "hardware/watchdog.h" @@ -233,7 +232,7 @@ void port_interrupt_after_ticks(uint32_t ticks) { void port_idle_until_interrupt(void) { common_hal_mcu_disable_interrupts(); - if (!tud_task_event_ready()) { + if (!background_callback_pending()) { // asm volatile ("dsb 0xF":::"memory"); // __wfi(); } diff --git a/ports/stm/supervisor/port.c b/ports/stm/supervisor/port.c index b62fe9d24a..9dc6eeb870 100644 --- a/ports/stm/supervisor/port.c +++ b/ports/stm/supervisor/port.c @@ -26,11 +26,13 @@ */ #include -#include "supervisor/port.h" +#include "supervisor/background_callback.h" #include "supervisor/board.h" +#include "supervisor/port.h" #include "lib/timeutils/timeutils.h" #include "common-hal/microcontroller/Pin.h" +#include "shared-bindings/microcontroller/__init__.h" #ifdef CIRCUITPY_AUDIOPWMIO #include "common-hal/audiopwmio/PWMAudioOut.h" @@ -366,7 +368,11 @@ void port_idle_until_interrupt(void) { if (stm32_peripherals_rtc_alarm_triggered(PERIPHERALS_ALARM_A)) { return; } - __WFI(); + common_hal_mcu_disable_interrupts(); + if (!background_callback_pending()) { + __WFI(); + } + common_hal_mcu_enable_interrupts(); } // Required by __libc_init_array in startup code if we are compiling using diff --git a/shared-bindings/storage/__init__.c b/shared-bindings/storage/__init__.c index f19aaa4396..4232b93bea 100644 --- a/shared-bindings/storage/__init__.c +++ b/shared-bindings/storage/__init__.c @@ -165,7 +165,11 @@ MP_DEFINE_CONST_FUN_OBJ_0(storage_erase_filesystem_obj, storage_erase_filesystem //| ... //| STATIC mp_obj_t storage_disable_usb_drive(void) { + #if CIRCUITPY_USB_MSC if (!common_hal_storage_disable_usb_drive()) { + #else + if (true) { + #endif mp_raise_RuntimeError(translate("Cannot change USB devices now")); } return mp_const_none; @@ -186,7 +190,11 @@ MP_DEFINE_CONST_FUN_OBJ_0(storage_disable_usb_drive_obj, storage_disable_usb_dri //| ... //| STATIC mp_obj_t storage_enable_usb_drive(void) { + #if CIRCUITPY_USB_MSC if (!common_hal_storage_enable_usb_drive()) { + #else + if (true) { + #endif mp_raise_RuntimeError(translate("Cannot change USB devices now")); } return mp_const_none; diff --git a/supervisor/background_callback.h b/supervisor/background_callback.h index 535dd656be..651ac020a6 100644 --- a/supervisor/background_callback.h +++ b/supervisor/background_callback.h @@ -27,6 +27,8 @@ #ifndef CIRCUITPY_INCLUDED_SUPERVISOR_BACKGROUND_CALLBACK_H #define CIRCUITPY_INCLUDED_SUPERVISOR_BACKGROUND_CALLBACK_H +#include + /** Background callbacks are a linked list of tasks to call in the background. * * Include a member of type `background_callback_t` inside an object @@ -69,6 +71,10 @@ void background_callback_add(background_callback_t *cb, background_callback_fun * whenever the list is non-empty */ void background_callback_run_all(void); +/* True when a background callback is pending. Helpful for checking background state when + * interrupts are disabled. */ +bool background_callback_pending(void); + /* During soft reset, remove all pending callbacks and clear the critical section flag */ void background_callback_reset(void); diff --git a/supervisor/shared/background_callback.c b/supervisor/shared/background_callback.c index 0dafb5398d..a59a6e85f5 100644 --- a/supervisor/shared/background_callback.c +++ b/supervisor/shared/background_callback.c @@ -67,9 +67,13 @@ void background_callback_add(background_callback_t *cb, background_callback_fun background_callback_add_core(cb); } +bool PLACE_IN_ITCM(background_callback_pending)(void) { + return callback_head != NULL; +} + static bool in_background_callback; void PLACE_IN_ITCM(background_callback_run_all)() { - if (!callback_head) { + if (!background_callback_pending()) { return; } CALLBACK_CRITICAL_BEGIN; diff --git a/supervisor/shared/usb/usb.c b/supervisor/shared/usb/usb.c index 9a276aa6cc..8333f662b5 100644 --- a/supervisor/shared/usb/usb.c +++ b/supervisor/shared/usb/usb.c @@ -91,7 +91,7 @@ void usb_init(void) { // Set up USB defaults before any USB changes are made in boot.py void usb_set_defaults(void) { - #if CIRCUITPY_STORAGE + #if CIRCUITPY_STORAGE && CIRCUITPY_USB_MSC storage_usb_set_defaults(); #endif