From baa28b54a2787eeefa7f02d23dc63d6e08f8699b Mon Sep 17 00:00:00 2001 From: Darian Leung Date: Thu, 28 Jul 2022 14:20:16 +0800 Subject: [PATCH] esp_hw_support: Fix esp_light_sleep_start() deadlock esp_light_sleep_start() will stall the other CPU via esp_ipc_isr_stall_other_cpu(). After stalling the other CPU, will call esp_clk_... API which themselves take locks. If the other stalled CPU is holding those locks, this will result in a deadlock. This commit adds a workaround calling esp_clk_private_lock() to take the lock before stalling the other CPU. --- components/esp_hw_support/esp_clk.c | 10 ++++++++++ .../include/esp_private/esp_clk.h | 13 +++++++++++++ components/esp_hw_support/sleep_modes.c | 18 ++++++++++++++++++ components/esp_ipc/include/esp_ipc_isr.h | 1 + .../include/esp_private/esp_timer_private.h | 4 ++-- 5 files changed, 44 insertions(+), 2 deletions(-) diff --git a/components/esp_hw_support/esp_clk.c b/components/esp_hw_support/esp_clk.c index 3a10dfead9..356b048fc8 100644 --- a/components/esp_hw_support/esp_clk.c +++ b/components/esp_hw_support/esp_clk.c @@ -136,3 +136,13 @@ uint64_t esp_clk_rtc_time(void) return 0; #endif } + +void esp_clk_private_lock(void) +{ + portENTER_CRITICAL(&s_esp_rtc_time_lock); +} + +void esp_clk_private_unlock(void) +{ + portEXIT_CRITICAL(&s_esp_rtc_time_lock); +} diff --git a/components/esp_hw_support/include/esp_private/esp_clk.h b/components/esp_hw_support/include/esp_private/esp_clk.h index 5a4666fddd..8846f9272d 100644 --- a/components/esp_hw_support/include/esp_private/esp_clk.h +++ b/components/esp_hw_support/include/esp_private/esp_clk.h @@ -82,6 +82,19 @@ int esp_clk_xtal_freq(void); */ uint64_t esp_clk_rtc_time(void); +/** + * @brief obtain internal critical section used esp_clk implementation. + * + * This is used by the esp_light_sleep_start() to avoid deadlocking when it + * calls esp_clk related API after stalling the other CPU. + */ +void esp_clk_private_lock(void); + +/** + * @brief counterpart of esp_clk_private_lock + */ +void esp_clk_private_unlock(void); + #ifdef __cplusplus } #endif diff --git a/components/esp_hw_support/sleep_modes.c b/components/esp_hw_support/sleep_modes.c index 2d47241b1f..3fced84a8d 100644 --- a/components/esp_hw_support/sleep_modes.c +++ b/components/esp_hw_support/sleep_modes.c @@ -655,12 +655,29 @@ esp_err_t esp_light_sleep_start(void) s_config.ccount_ticks_record = cpu_ll_get_cycle_count(); static portMUX_TYPE light_sleep_lock = portMUX_INITIALIZER_UNLOCKED; portENTER_CRITICAL(&light_sleep_lock); + /* + Note: We are about to stall the other CPU via the esp_ipc_isr_stall_other_cpu(). However, there is a chance of + deadlock if after stalling the other CPU, we attempt to take spinlocks already held by the other CPU that is. + + Thus any functions that we call after stalling the other CPU will need to have the locks taken first to avoid + deadlock. + + Todo: IDF-5257 + */ + /* We will be calling esp_timer_private_advance inside DPORT access critical * section. Make sure the code on the other CPU is not holding esp_timer * lock, otherwise there will be deadlock. */ esp_timer_private_lock(); + /* We will be calling esp_rtc_get_time_us() below. Make sure the code on the other CPU is not holding the + * esp_rtc_get_time_us() lock, otherwise there will be deadlock. esp_rtc_get_time_us() is called via: + * + * - esp_clk_slowclk_cal_set() -> esp_rtc_get_time_us() + */ + esp_clk_private_lock(); + s_config.rtc_ticks_at_sleep_start = rtc_time_get(); uint32_t ccount_at_sleep_start = cpu_ll_get_cycle_count(); uint64_t frc_time_at_start = esp_timer_get_time(); @@ -796,6 +813,7 @@ esp_err_t esp_light_sleep_start(void) } esp_set_time_from_rtc(); + esp_clk_private_unlock(); esp_timer_private_unlock(); esp_ipc_isr_release_other_cpu(); if (!wdt_was_enabled) { diff --git a/components/esp_ipc/include/esp_ipc_isr.h b/components/esp_ipc/include/esp_ipc_isr.h index 22fa5b10b0..70290ba561 100644 --- a/components/esp_ipc/include/esp_ipc_isr.h +++ b/components/esp_ipc/include/esp_ipc_isr.h @@ -62,6 +62,7 @@ void esp_ipc_isr_asm_call_blocking(esp_ipc_isr_func_t func, void* arg); * - If the stall feature is paused using esp_ipc_isr_stall_pause(), this function will have no effect * * @note This function is not available in single-core mode. + * @note It is the caller's responsibility to avoid deadlocking on spinlocks */ void esp_ipc_isr_stall_other_cpu(void); diff --git a/components/esp_timer/include/esp_private/esp_timer_private.h b/components/esp_timer/include/esp_private/esp_timer_private.h index ecd4264111..6f32f1431e 100644 --- a/components/esp_timer/include/esp_private/esp_timer_private.h +++ b/components/esp_timer/include/esp_private/esp_timer_private.h @@ -52,11 +52,11 @@ void esp_timer_private_update_apb_freq(uint32_t apb_ticks_per_us); void esp_timer_private_advance(int64_t time_us); /** - * @brief obtain internal critical section used esp_timer implementation + * @brief obtain internal critical section used in the esp_timer implementation * This can be used when a sequence of calls to esp_timer has to be made, * and it is necessary that the state of the timer is consistent between * the calls. Should be treated in the same way as a spinlock. - * Call esp_timer_unlock to release the lock + * Call esp_timer_private_unlock to release the lock */ void esp_timer_private_lock(void);