From 49fba58f081390e17cdb7f482020784d64a7dee0 Mon Sep 17 00:00:00 2001 From: morris Date: Wed, 22 Jul 2026 19:02:58 +0800 Subject: [PATCH] fix(riscv_trace): protect shared RCC register access with PERIPH_RCC_ATOMIC riscv_trace_ll_enable_bus_clock and riscv_trace_ll_reset_register operate on shared HP_SYS_CLKRST registers and were called concurrently from both cores during SECONDARY init, creating RMW race conditions. Move the clock/reset logic out of the HAL layer into esp_riscv_trace_early_init, protected by PERIPH_RCC_ATOMIC() spinlock. Wrap the LL functions with macros that enforce the caller must be inside a PERIPH_RCC_ATOMIC() critical section at compile time. --- .../esp32p4/include/hal/riscv_trace_ll.h | 14 ++++++++++++-- components/esp_hal_debug_assist/riscv_trace_hal.c | 10 ---------- components/esp_riscv_trace/src/esp_riscv_trace.c | 8 ++++++++ .../test_apps/basic/sdkconfig.defaults | 1 + 4 files changed, 21 insertions(+), 12 deletions(-) diff --git a/components/esp_hal_debug_assist/esp32p4/include/hal/riscv_trace_ll.h b/components/esp_hal_debug_assist/esp32p4/include/hal/riscv_trace_ll.h index afc84f0ee6f..71b85feb29d 100644 --- a/components/esp_hal_debug_assist/esp32p4/include/hal/riscv_trace_ll.h +++ b/components/esp_hal_debug_assist/esp32p4/include/hal/riscv_trace_ll.h @@ -30,14 +30,19 @@ static inline trace_dev_t *riscv_trace_ll_get_hw(int core) *--------------------------------------------------------------------------*/ /** @brief Enable or disable the common TRACE CPU and system clocks. */ -static inline void riscv_trace_ll_enable_bus_clock(bool enable) +static inline void _riscv_trace_ll_enable_bus_clock(bool enable) { HP_SYS_CLKRST.soc_clk_ctrl0.reg_trace_cpu_clk_en = enable; HP_SYS_CLKRST.soc_clk_ctrl0.reg_trace_sys_clk_en = enable; } +#define riscv_trace_ll_enable_bus_clock(...) do { \ + (void)__DECLARE_RCC_ATOMIC_ENV; \ + _riscv_trace_ll_enable_bus_clock(__VA_ARGS__); \ +} while (0) + /** @brief Assert and release the reset of the given encoder core. */ -static inline void riscv_trace_ll_reset_register(int core) +static inline void _riscv_trace_ll_reset_register(int core) { if (core == 0) { HP_SYS_CLKRST.hp_rst_en0.reg_rst_en_coretrace0 = 1; @@ -48,6 +53,11 @@ static inline void riscv_trace_ll_reset_register(int core) } } +#define riscv_trace_ll_reset_register(...) do { \ + (void)__DECLARE_RCC_ATOMIC_ENV; \ + _riscv_trace_ll_reset_register(__VA_ARGS__); \ + } while (0) + /** @brief Enable the per-module register clock gate. */ static inline void riscv_trace_ll_enable_module_clock(trace_dev_t *hw, bool enable) { diff --git a/components/esp_hal_debug_assist/riscv_trace_hal.c b/components/esp_hal_debug_assist/riscv_trace_hal.c index 453e4a41161..87552c91b99 100644 --- a/components/esp_hal_debug_assist/riscv_trace_hal.c +++ b/components/esp_hal_debug_assist/riscv_trace_hal.c @@ -30,13 +30,6 @@ _Static_assert(RISCV_TRACE_INTR_FIFO_OVERFLOW == TRACE_FIFO_OVERFLOW_INTR_ENA, _Static_assert(RISCV_TRACE_INTR_MEM_FULL == TRACE_MEM_FULL_INTR_ENA, "RISCV_TRACE_INTR_MEM_FULL does not match TRACE_MEM_FULL_INTR_ENA"); -static void riscv_trace_hal_enable_clock_and_reset(int core_id, trace_dev_t *dev) -{ - riscv_trace_ll_enable_bus_clock(true); - riscv_trace_ll_reset_register(core_id); - riscv_trace_ll_enable_module_clock(dev, true); -} - void riscv_trace_hal_init(int core_id, const riscv_trace_hal_config_t *config, riscv_trace_hal_context_t *ctx) { HAL_ASSERT(ctx != NULL && config != NULL); @@ -45,8 +38,6 @@ void riscv_trace_hal_init(int core_id, const riscv_trace_hal_config_t *config, r ctx->dev = riscv_trace_ll_get_hw(core_id); ctx->core_id = core_id; - riscv_trace_hal_enable_clock_and_reset(core_id, ctx->dev); - /* Memory configuration */ riscv_trace_ll_set_mem_start_addr(ctx->dev, config->mem_start_addr); riscv_trace_ll_set_mem_end_addr(ctx->dev, config->mem_end_addr); @@ -118,7 +109,6 @@ void riscv_trace_hal_deinit(riscv_trace_hal_context_t *ctx) riscv_trace_ll_set_restart_ena(ctx->dev, false); riscv_trace_ll_trigger_off(ctx->dev); riscv_trace_ll_set_intr_ena(ctx->dev, 0); - riscv_trace_ll_enable_module_clock(ctx->dev, false); } uint32_t riscv_trace_hal_read_fifo_status(riscv_trace_hal_context_t *ctx) diff --git a/components/esp_riscv_trace/src/esp_riscv_trace.c b/components/esp_riscv_trace/src/esp_riscv_trace.c index 5b9448f32cd..10e2fdc0cf2 100644 --- a/components/esp_riscv_trace/src/esp_riscv_trace.c +++ b/components/esp_riscv_trace/src/esp_riscv_trace.c @@ -14,9 +14,11 @@ #include "esp_check.h" #include "esp_private/esp_cache_private.h" #include "esp_private/startup_internal.h" +#include "esp_private/periph_ctrl.h" #include "esp_cpu.h" #include "soc/soc_caps.h" #include "hal/riscv_trace_hal.h" +#include "hal/riscv_trace_ll.h" #include "esp_riscv_trace.h" #include "esp_riscv_trace_priv.h" @@ -387,6 +389,12 @@ ESP_SYSTEM_INIT_FN(esp_riscv_trace_early_init, SECONDARY, ESP_SYSTEM_INIT_ALL_CO int core_id = esp_cpu_get_core_id(); esp_riscv_trace_config_t config = esp_riscv_trace_get_user_config(core_id); + // Enable the clocks and reset the encoder core before accessing its registers. + PERIPH_RCC_ATOMIC() { + riscv_trace_ll_enable_bus_clock(true); + riscv_trace_ll_reset_register(core_id); + } + if (!is_valid_core_mask(config.core_mask)) { ESP_EARLY_LOGE(TAG, "invalid core mask"); return ESP_ERR_INVALID_ARG; diff --git a/components/esp_riscv_trace/test_apps/basic/sdkconfig.defaults b/components/esp_riscv_trace/test_apps/basic/sdkconfig.defaults index 8b345df6d85..46ed58779fa 100644 --- a/components/esp_riscv_trace/test_apps/basic/sdkconfig.defaults +++ b/components/esp_riscv_trace/test_apps/basic/sdkconfig.defaults @@ -1,3 +1,4 @@ +# CONFIG_ESP_TASK_WDT_INIT is not set CONFIG_ESP_RISCV_TRACE_ENABLE=y CONFIG_ESP_RISCV_TRACE_RESYNC_MODE_PACKET=y CONFIG_ESP_RISCV_TRACE_RESYNC_THRESHOLD=32