From 71eef9a9b0cb1a15f4d15f7ef39344104c620b7d Mon Sep 17 00:00:00 2001 From: Darian Leung Date: Wed, 8 Jun 2022 16:52:32 +0800 Subject: [PATCH] freertos: Fix SMP RISC-V Port IDF Style critical sections Previously the RV port was routing IDF style critical section API to call FreeRTOS style critical section API. For example, a call to "portENTER_CRITICAL(mux)" would eventually call `vTaskEnterCritical()" via the following call flow: - portENTER_CRITICAL(mux) - vPortEnterCritical() - portSET_INTERRUPT_MASK_FROM_ISR() - vTaskEnterCritical() This commit fixes the IDF style critical section by making sure that they are completely orthogonal to FreeRTOS critical sections --- .../riscv/include/freertos/portmacro.h | 65 ++++++++----------- .../FreeRTOS-Kernel-SMP/portable/riscv/port.c | 46 +++++++------ 2 files changed, 49 insertions(+), 62 deletions(-) diff --git a/components/freertos/FreeRTOS-Kernel-SMP/portable/riscv/include/freertos/portmacro.h b/components/freertos/FreeRTOS-Kernel-SMP/portable/riscv/include/freertos/portmacro.h index 736c6da5f3..1bda71b504 100644 --- a/components/freertos/FreeRTOS-Kernel-SMP/portable/riscv/include/freertos/portmacro.h +++ b/components/freertos/FreeRTOS-Kernel-SMP/portable/riscv/include/freertos/portmacro.h @@ -377,17 +377,6 @@ static inline BaseType_t IRAM_ATTR xPortGetCoreID(void) #define portENABLE_INTERRUPTS() vPortClearInterruptMask(1) #define portRESTORE_INTERRUPTS(x) vPortClearInterruptMask(x) -#define portSET_INTERRUPT_MASK_FROM_ISR() ({ \ - unsigned int cur_level; \ - cur_level = REG_READ(INTERRUPT_CORE0_CPU_INT_THRESH_REG); \ - vTaskEnterCritical(); \ - cur_level; \ -}) -#define portCLEAR_INTERRUPT_MASK_FROM_ISR(x) ({ \ - vTaskExitCritical(); \ - portRESTORE_INTERRUPTS(x); \ -}) - // ------------------ Critical Sections -------------------- #define portGET_TASK_LOCK() vPortTakeLock(&port_xTaskLock) @@ -395,8 +384,6 @@ static inline BaseType_t IRAM_ATTR xPortGetCoreID(void) #define portGET_ISR_LOCK() vPortTakeLock(&port_xISRLock) #define portRELEASE_ISR_LOCK() vPortReleaseLock(&port_xISRLock) -#define portENTER_CRITICAL_IDF(mux) {(void)mux; vPortEnterCritical();} -#define portEXIT_CRITICAL_IDF(mux) {(void)mux; vPortExitCritical();} //Critical sections used by FreeRTOS SMP extern void vTaskEnterCritical( void ); @@ -412,33 +399,16 @@ extern void vTaskExitCritical( void ); #define portEXIT_CRITICAL(...) CHOOSE_MACRO_VA_ARG(portEXIT_CRITICAL_IDF, portEXIT_CRITICAL_SMP, ##__VA_ARGS__)(__VA_ARGS__) #endif -#define portTRY_ENTER_CRITICAL(mux, timeout) ({ \ - (void)mux; (void)timeout; \ - vPortEnterCritical(); \ - BaseType_t ret = pdPASS; \ - ret; \ +#define portSET_INTERRUPT_MASK_FROM_ISR() ({ \ + unsigned int cur_level; \ + cur_level = REG_READ(INTERRUPT_CORE0_CPU_INT_THRESH_REG); \ + vTaskEnterCritical(); \ + cur_level; \ }) -//In single-core RISC-V, we can use the same critical section API -#define portENTER_CRITICAL_ISR(mux) portENTER_CRITICAL(mux) -#define portEXIT_CRITICAL_ISR(mux) portEXIT_CRITICAL(mux) -#define portTRY_ENTER_CRITICAL_ISR(mux, timeout) portTRY_ENTER_CRITICAL(mux, timeout) - -/* [refactor-todo] on RISC-V, both ISR and non-ISR cases result in the same call. We can redefine this macro */ -#define portENTER_CRITICAL_SAFE(mux) ({ \ - if (xPortInIsrContext()) { \ - portENTER_CRITICAL_ISR(mux); \ - } else { \ - portENTER_CRITICAL(mux); \ - } \ +#define portCLEAR_INTERRUPT_MASK_FROM_ISR(x) ({ \ + vTaskExitCritical(); \ + portRESTORE_INTERRUPTS(x); \ }) -#define portEXIT_CRITICAL_SAFE(mux) ({ \ - if (xPortInIsrContext()) { \ - portEXIT_CRITICAL_ISR(mux); \ - } else { \ - portEXIT_CRITICAL(mux); \ - } \ -}) -#define portTRY_ENTER_CRITICAL_SAFE(mux, timeout) portENTER_CRITICAL_SAFE(mux, timeout) // ---------------------- Yielding ------------------------- @@ -574,6 +544,25 @@ static inline void uxPortCompareSetExtram(volatile uint32_t *addr, uint32_t comp // ------------------ Critical Sections -------------------- +/* +IDF style critical sections which are orthogonal to FreeRTOS critical sections. However, on single core, the IDF style +critical sections simply disable interrupts, thus we discard the lock and timeout arguments. +*/ +void vPortEnterCriticalIDF(void); +void vPortExitCriticalIDF(void); + +//IDF task critical sections +#define portTRY_ENTER_CRITICAL(lock, timeout) {((void) lock; (void) timeout; vPortEnterCriticalIDF(); pdPASS;)} +#define portENTER_CRITICAL_IDF(lock) ({(void) lock; vPortEnterCriticalIDF();}) +#define portEXIT_CRITICAL_IDF(lock) ({(void) lock; vPortExitCriticalIDF();}) +//IDF ISR critical sections +#define portTRY_ENTER_CRITICAL_ISR(lock, timeout) {((void) lock; (void) timeout; vPortEnterCriticalIDF(); pdPASS;)} +#define portENTER_CRITICAL_ISR(lock) ({(void) lock; vPortEnterCriticalIDF();}) +#define portEXIT_CRITICAL_ISR(lock) ({(void) lock; vPortExitCriticalIDF();}) +//IDF safe critical sections (they're the same) +#define portENTER_CRITICAL_SAFE(lock) ({(void) lock; vPortEnterCriticalIDF();}) +#define portEXIT_CRITICAL_SAFE(lock) ({(void) lock; vPortExitCriticalIDF();}) + // ---------------------- Yielding ------------------------- static inline bool IRAM_ATTR xPortCanYield(void) diff --git a/components/freertos/FreeRTOS-Kernel-SMP/portable/riscv/port.c b/components/freertos/FreeRTOS-Kernel-SMP/portable/riscv/port.c index eb340b065b..f307d2e728 100644 --- a/components/freertos/FreeRTOS-Kernel-SMP/portable/riscv/port.c +++ b/components/freertos/FreeRTOS-Kernel-SMP/portable/riscv/port.c @@ -45,21 +45,17 @@ static const char *TAG = "cpu_start"; // [refactor-todo]: might be appropriate to change in the future, but -/** - * @brief A variable is used to keep track of the critical section nesting. - * @note This variable has to be stored as part of the task context and must be initialized to a non zero value - * to ensure interrupts don't inadvertently become unmasked before the scheduler starts. - * As it is stored as part of the task context it will automatically be set to 0 when the first task is started. - */ -static UBaseType_t uxCriticalNesting = 0; -static UBaseType_t uxSavedInterruptState = 0; BaseType_t uxSchedulerRunning = 0; UBaseType_t uxInterruptNesting = 0; +portMUX_TYPE port_xTaskLock = portMUX_INITIALIZER_UNLOCKED; +portMUX_TYPE port_xISRLock = portMUX_INITIALIZER_UNLOCKED; BaseType_t xPortSwitchFlag = 0; __attribute__((aligned(16))) static StackType_t xIsrStack[configISR_STACK_SIZE]; StackType_t *xIsrStackTop = &xIsrStack[0] + (configISR_STACK_SIZE & (~((portPOINTER_SIZE_TYPE)portBYTE_ALIGNMENT_MASK))); -portMUX_TYPE port_xTaskLock = portMUX_INITIALIZER_UNLOCKED; -portMUX_TYPE port_xISRLock = portMUX_INITIALIZER_UNLOCKED; + +// Variables used for IDF style critical sections. These are orthogonal to FreeRTOS critical sections +static UBaseType_t port_uxCriticalNestingIDF = 0; +static UBaseType_t port_uxCriticalOldInterruptStateIDF = 0; /* ------------------------------------------------ IDF Compatibility -------------------------------------------------- * - These need to be defined for IDF to compile @@ -318,7 +314,7 @@ extern void esprv_intc_int_set_threshold(int); // FIXME, this function is in ROM BaseType_t xPortStartScheduler(void) { uxInterruptNesting = 0; - uxCriticalNesting = 0; + port_uxCriticalNestingIDF = 0; uxSchedulerRunning = 0; /* Setup the hardware to generate the tick. */ @@ -421,7 +417,6 @@ __attribute__((noreturn)) static void _prvTaskExitError(void) Artificially force an assert() to be triggered if configASSERT() is defined, then stop here so application writers can catch the error. */ - configASSERT(uxCriticalNesting == ~0UL); portDISABLE_INTERRUPTS(); abort(); } @@ -629,22 +624,25 @@ void vPortCleanUpTCB ( void *pxTCB ) // ------------------ Critical Sections -------------------- -void vPortEnterCritical(void) +void vPortEnterCriticalIDF(void) { - BaseType_t state = portSET_INTERRUPT_MASK_FROM_ISR(); - uxCriticalNesting++; - - if (uxCriticalNesting == 1) { - uxSavedInterruptState = state; + // Save current interrupt threshold and disable interrupts + int old_thresh = vPortSetInterruptMask(); + // Update the IDF critical nesting count + port_uxCriticalNestingIDF++; + if (port_uxCriticalNestingIDF == 1) { + // Save a copy of the old interrupt threshold + port_uxCriticalOldInterruptStateIDF = (UBaseType_t) old_thresh; } } -void vPortExitCritical(void) +void vPortExitCriticalIDF(void) { - if (uxCriticalNesting > 0) { - uxCriticalNesting--; - if (uxCriticalNesting == 0) { - portCLEAR_INTERRUPT_MASK_FROM_ISR(uxSavedInterruptState); + if (port_uxCriticalNestingIDF > 0) { + port_uxCriticalNestingIDF--; + if (port_uxCriticalNestingIDF == 0) { + // Restore the saved interrupt threshold + vPortClearInterruptMask((int)port_uxCriticalOldInterruptStateIDF); } } } @@ -712,7 +710,7 @@ void vPortYield(void) for an instant yield, and if that happens then the WFI would be waiting for the next interrupt to occur...) */ - while (uxSchedulerRunning && uxCriticalNesting == 0 && REG_READ(SYSTEM_CPU_INTR_FROM_CPU_0_REG) != 0) {} + while (uxSchedulerRunning && REG_READ(SYSTEM_CPU_INTR_FROM_CPU_0_REG) != 0) {} } }