From 7b5508b6ff8cd890f6aa6d90cfc07917fc00c298 Mon Sep 17 00:00:00 2001 From: Sudeep Mohanty Date: Wed, 24 Jul 2024 16:56:17 +0200 Subject: [PATCH] fix(freertos): Incorrect assert in FreeRTOS port layer when not in ISR context This commit fixes an issue where in the FreeRTOS port layer would cause the portASSERT_IF_IN_ISR() assert check to fail even when the system is not in an interrupt context. --- .../riscv/include/freertos/portmacro.h | 16 ++++++++++++++++ .../FreeRTOS-Kernel-SMP/portable/riscv/port.c | 10 ++++++++-- .../xtensa/include/freertos/portmacro.h | 15 +++++++++++++++ .../portable/xtensa/port.c | 10 ++++++++-- .../riscv/include/freertos/portmacro.h | 12 ++++++++++++ .../FreeRTOS-Kernel/portable/riscv/port.c | 6 ++++++ .../xtensa/include/freertos/portmacro.h | 8 ++++---- .../FreeRTOS-Kernel/portable/xtensa/port.c | 7 ++++--- .../port/test_freertos_isinisrcontext.c | 19 ++++++++++++++++++- .../test_apps/freertos/pytest_freertos.py | 11 ++++++++++- 10 files changed, 101 insertions(+), 13 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 0c9074c510..7105a65c98 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 @@ -85,6 +85,13 @@ typedef spinlock_t portMUX_TYPE; /**< Spi BaseType_t xPortCheckIfInISR(void); +/** + * @brief Assert if in ISR context + * + * - Asserts on xPortCheckIfInISR() internally + */ +void vPortAssertIfInISR(void); + // ------------------ Critical Sections -------------------- /* @@ -148,6 +155,15 @@ void vPortCleanUpTCB ( void *pxTCB ); #define portENABLE_INTERRUPTS() vPortClearInterruptMask(1) #define portRESTORE_INTERRUPTS(x) vPortClearInterruptMask(x) +/** + * @brief Assert if in ISR context + * + * TODO: Enable once ISR safe version of vTaskEnter/ExitCritical() is implemented + * for single-core SMP FreeRTOS Kernel. (IDF-10540) + */ +// #define portASSERT_IF_IN_ISR() vPortAssertIfInISR() + + // ------------------ Critical Sections -------------------- #define portGET_TASK_LOCK() vPortTakeLock(&port_xTaskLock) diff --git a/components/freertos/FreeRTOS-Kernel-SMP/portable/riscv/port.c b/components/freertos/FreeRTOS-Kernel-SMP/portable/riscv/port.c index bfd68f8af4..447299ca04 100644 --- a/components/freertos/FreeRTOS-Kernel-SMP/portable/riscv/port.c +++ b/components/freertos/FreeRTOS-Kernel-SMP/portable/riscv/port.c @@ -279,6 +279,12 @@ BaseType_t xPortCheckIfInISR(void) return uxInterruptNesting; } +void vPortAssertIfInISR(void) +{ + /* Assert if the interrupt nesting count is > 0 */ + configASSERT(xPortCheckIfInISR() == 0); +} + // ------------------ Critical Sections -------------------- void IRAM_ATTR vPortTakeLock( portMUX_TYPE *lock ) @@ -438,7 +444,7 @@ FORCE_INLINE_ATTR UBaseType_t uxInitialiseStackTLS(UBaseType_t uxStackPointer, u #if CONFIG_FREERTOS_TASK_FUNCTION_WRAPPER static void vPortTaskWrapper(TaskFunction_t pxCode, void *pvParameters) { - __asm__ volatile(".cfi_undefined ra"); // tell to debugger that it's outermost (inital) frame + __asm__ volatile(".cfi_undefined ra"); // tell to debugger that it's outermost (initial) frame extern void __attribute__((noreturn)) panic_abort(const char *details); static char DRAM_ATTR msg[80] = "FreeRTOS: FreeRTOS Task \"\0"; pxCode(pvParameters); @@ -505,7 +511,7 @@ StackType_t *pxPortInitialiseStack(StackType_t *pxTopOfStack, TaskFunction_t pxC HIGH ADDRESS |---------------------------| <- pxTopOfStack on entry | TLS Variables | - | ------------------------- | <- Start of useable stack + | ------------------------- | <- Start of usable stack | Starting stack frame | | ------------------------- | <- pxTopOfStack on return (which is the tasks current SP) | | | diff --git a/components/freertos/FreeRTOS-Kernel-SMP/portable/xtensa/include/freertos/portmacro.h b/components/freertos/FreeRTOS-Kernel-SMP/portable/xtensa/include/freertos/portmacro.h index 4ff15d932d..83dd60e8b5 100644 --- a/components/freertos/FreeRTOS-Kernel-SMP/portable/xtensa/include/freertos/portmacro.h +++ b/components/freertos/FreeRTOS-Kernel-SMP/portable/xtensa/include/freertos/portmacro.h @@ -95,6 +95,13 @@ typedef spinlock_t portMUX_TYPE; /**< Spi BaseType_t xPortCheckIfInISR(void); +/** + * @brief Assert if in ISR context + * + * - Asserts on xPortCheckIfInISR() internally + */ +void vPortAssertIfInISR(void); + // ------------------ Critical Sections -------------------- UBaseType_t uxPortEnterCriticalFromISR( void ); @@ -152,6 +159,14 @@ void vPortCleanUpTCB ( void *pxTCB ); XTOS_SET_INTLEVEL(0); \ }) +/** + * @brief Assert if in ISR context + * + * TODO: Enable once ISR safe version of vTaskEnter/ExitCritical() is implemented + * for single-core SMP FreeRTOS Kernel. (IDF-10540) + */ +// #define portASSERT_IF_IN_ISR() vPortAssertIfInISR() + /* Note: XTOS_RESTORE_INTLEVEL() will overwrite entire PS register on XEA2. So we need ot make the value INTLEVEL field ourselves */ diff --git a/components/freertos/FreeRTOS-Kernel-SMP/portable/xtensa/port.c b/components/freertos/FreeRTOS-Kernel-SMP/portable/xtensa/port.c index 567e19fc57..ea0059c2cb 100644 --- a/components/freertos/FreeRTOS-Kernel-SMP/portable/xtensa/port.c +++ b/components/freertos/FreeRTOS-Kernel-SMP/portable/xtensa/port.c @@ -127,7 +127,7 @@ BaseType_t xPortEnterCriticalTimeout(portMUX_TYPE *lock, BaseType_t timeout) void vPortExitCriticalIDF(portMUX_TYPE *lock) { /* This function may be called in a nested manner. Therefore, we only need - * to reenable interrupts if this is the last call to exit the critical. We + * to re-enable interrupts if this is the last call to exit the critical. We * can use the nesting count to determine whether this is the last exit call. */ spinlock_release(lock); @@ -330,6 +330,12 @@ BaseType_t xPortCheckIfInISR(void) return ret; } +void vPortAssertIfInISR(void) +{ + /* Assert if the interrupt nesting count is > 0 */ + configASSERT(xPortCheckIfInISR() == 0); +} + // ------------------ Critical Sections -------------------- void vPortTakeLock( portMUX_TYPE *lock ) @@ -662,7 +668,7 @@ StackType_t * pxPortInitialiseStack( StackType_t * pxTopOfStack, | Coproc Save Area | (CPSA MUST BE FIRST) | ------------------------- | | TLS Variables | - | ------------------------- | <- Start of useable stack + | ------------------------- | <- Start of usable stack | Starting stack frame | | ------------------------- | <- pxTopOfStack on return (which is the tasks current SP) | | | diff --git a/components/freertos/FreeRTOS-Kernel/portable/riscv/include/freertos/portmacro.h b/components/freertos/FreeRTOS-Kernel/portable/riscv/include/freertos/portmacro.h index d309ef8638..a06945fe52 100644 --- a/components/freertos/FreeRTOS-Kernel/portable/riscv/include/freertos/portmacro.h +++ b/components/freertos/FreeRTOS-Kernel/portable/riscv/include/freertos/portmacro.h @@ -133,6 +133,13 @@ typedef uint32_t TickType_t; */ BaseType_t xPortInIsrContext(void); +/** + * @brief Assert if in ISR context + * + * - Asserts on xPortInIsrContext() internally + */ +void vPortAssertIfInISR(void); + /** * @brief Check if in ISR context from High priority ISRs * @@ -310,6 +317,11 @@ FORCE_INLINE_ATTR BaseType_t xPortGetCoreID(void) #define portSET_INTERRUPT_MASK_FROM_ISR() vPortSetInterruptMask() #define portCLEAR_INTERRUPT_MASK_FROM_ISR(uxSavedStatusValue) vPortClearInterruptMask(uxSavedStatusValue) +/** + * @brief Assert if in ISR context + */ +#define portASSERT_IF_IN_ISR() vPortAssertIfInISR() + // ------------------ Critical Sections -------------------- #define portENTER_CRITICAL(mux) {(void)mux; vPortEnterCritical();} diff --git a/components/freertos/FreeRTOS-Kernel/portable/riscv/port.c b/components/freertos/FreeRTOS-Kernel/portable/riscv/port.c index 65c3861493..fd6656178e 100644 --- a/components/freertos/FreeRTOS-Kernel/portable/riscv/port.c +++ b/components/freertos/FreeRTOS-Kernel/portable/riscv/port.c @@ -306,6 +306,12 @@ BaseType_t xPortInIsrContext(void) return uxInterruptNesting; } +void vPortAssertIfInISR(void) +{ + /* Assert if the interrupt nesting count is > 0 */ + configASSERT(xPortInIsrContext() == 0); +} + BaseType_t IRAM_ATTR xPortInterruptedFromISRContext(void) { /* For single core, this can be the same as xPortInIsrContext() because reading it is atomic */ diff --git a/components/freertos/FreeRTOS-Kernel/portable/xtensa/include/freertos/portmacro.h b/components/freertos/FreeRTOS-Kernel/portable/xtensa/include/freertos/portmacro.h index 03d8eba888..13fc084afa 100644 --- a/components/freertos/FreeRTOS-Kernel/portable/xtensa/include/freertos/portmacro.h +++ b/components/freertos/FreeRTOS-Kernel/portable/xtensa/include/freertos/portmacro.h @@ -166,12 +166,9 @@ typedef uint32_t TickType_t; BaseType_t xPortInIsrContext(void); /** - * @brief Asserts if in ISR context + * @brief Assert if in ISR context * * - Asserts on xPortInIsrContext() internally - * - * @note [refactor-todo] Check if this API is still required - * @note [refactor-todo] Check if this should be inlined */ void vPortAssertIfInISR(void); @@ -431,6 +428,9 @@ FORCE_INLINE_ATTR BaseType_t xPortGetCoreID(void); #define portSET_INTERRUPT_MASK_FROM_ISR() xPortSetInterruptMaskFromISR() #define portCLEAR_INTERRUPT_MASK_FROM_ISR(prev_level) vPortClearInterruptMaskFromISR(prev_level) +/** + * @brief Assert if in ISR context + */ #define portASSERT_IF_IN_ISR() vPortAssertIfInISR() /** diff --git a/components/freertos/FreeRTOS-Kernel/portable/xtensa/port.c b/components/freertos/FreeRTOS-Kernel/portable/xtensa/port.c index 7304ceaebd..d1f42bcfa6 100644 --- a/components/freertos/FreeRTOS-Kernel/portable/xtensa/port.c +++ b/components/freertos/FreeRTOS-Kernel/portable/xtensa/port.c @@ -423,7 +423,7 @@ StackType_t * pxPortInitialiseStack( StackType_t * pxTopOfStack, | Coproc Save Area | (CPSA MUST BE FIRST) | ------------------------- | | TLS Variables | - | ------------------------- | <- Start of useable stack + | ------------------------- | <- Start of usable stack | Starting stack frame | | ------------------------- | <- pxTopOfStack on return (which is the tasks current SP) | | | @@ -477,7 +477,8 @@ BaseType_t xPortInIsrContext(void) void vPortAssertIfInISR(void) { - configASSERT(xPortInIsrContext()); + /* Assert if the interrupt nesting count is > 0 */ + configASSERT(xPortInIsrContext() == 0); } BaseType_t IRAM_ATTR xPortInterruptedFromISRContext(void) @@ -517,7 +518,7 @@ BaseType_t __attribute__((optimize("-O3"))) xPortEnterCriticalTimeout(portMUX_TY void __attribute__((optimize("-O3"))) vPortExitCritical(portMUX_TYPE *mux) { /* This function may be called in a nested manner. Therefore, we only need - * to reenable interrupts if this is the last call to exit the critical. We + * to re-enable interrupts if this is the last call to exit the critical. We * can use the nesting count to determine whether this is the last exit call. */ spinlock_release(mux); diff --git a/components/freertos/test_apps/freertos/port/test_freertos_isinisrcontext.c b/components/freertos/test_apps/freertos/port/test_freertos_isinisrcontext.c index e1be84ef7f..73bf15dcd5 100644 --- a/components/freertos/test_apps/freertos/port/test_freertos_isinisrcontext.c +++ b/components/freertos/test_apps/freertos/port/test_freertos_isinisrcontext.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2022-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -62,3 +62,20 @@ TEST_CASE("xPortInIsrContext test", "[freertos]") #endif + +#if !CONFIG_FREERTOS_SMP // TODO: Enable when IDF-10540 is fixed + +static void testint_assert(void) +{ + esp_rom_printf("INT!\n"); + portASSERT_IF_IN_ISR(); +} + +TEST_CASE("port must assert if in ISR context", "[ignore]") +{ + esp_err_t err = esp_register_freertos_tick_hook_for_cpu(testint_assert, xPortGetCoreID()); + TEST_ASSERT_EQUAL_HEX32(ESP_OK, err); + vTaskDelay(100 / portTICK_PERIOD_MS); + esp_deregister_freertos_tick_hook_for_cpu(testint_assert, xPortGetCoreID()); +} +#endif // !CONFIG_FREERTOS_SMP diff --git a/components/freertos/test_apps/freertos/pytest_freertos.py b/components/freertos/test_apps/freertos/pytest_freertos.py index 8892d1a366..5857759984 100644 --- a/components/freertos/test_apps/freertos/pytest_freertos.py +++ b/components/freertos/test_apps/freertos/pytest_freertos.py @@ -1,6 +1,5 @@ # SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD # SPDX-License-Identifier: Apache-2.0 - import pytest from pytest_embedded import Dut @@ -40,3 +39,13 @@ def test_task_notify_wait_too_high_index_fails(dut: Dut) -> None: dut.expect('assert failed: xTaskGenericNotifyWait', timeout=5) dut.expect('uxIndexToWait < [0-9]+', timeout=5) dut.expect_exact('Rebooting...') + + +@pytest.mark.supported_targets +@pytest.mark.generic +@pytest.mark.parametrize('config', ['default'], indirect=True) +def test_port_must_assert_in_isr(dut: Dut) -> None: + dut.expect_exact('Press ENTER to see the list of tests.') + dut.write('\"port must assert if in ISR context\"') + dut.expect('assert failed: vPortAssertIfInISR', timeout=5) + dut.expect_exact('Rebooting...')