Merge branch 'bugfix/atomic_gptimer_fsm' into 'master'

gptimer: fix race condition between start and stop

See merge request espressif/esp-idf!22620

Author: suda-morris

components/driver/CMakeLists.txt

@@ -75,6 +75,7 @@ endif()
 # GPTimer related source files
 if(CONFIG_SOC_GPTIMER_SUPPORTED)
     list(APPEND srcs "gptimer/gptimer.c"
+                     "gptimer/gptimer_priv.c"
                      "deprecated/timer_legacy.c")
 endif()
 

components/driver/gptimer/README.md

@@ -0,0 +1,46 @@
+# GPTimer Driver Design
+
+## State Transition
+
+> State transition is achieved by using the primitives provided by `<stdatomic.h>`.
+
+```mermaid
+stateDiagram-v2
+    [*] --> init: gptimer_new_timer
+    init --> enable: gptimer_enable
+    enable --> init: gptimer_disable
+    enable --> run: gptimer_start*
+    run --> enable: gptimer_stop*
+    init --> [*]: gptimer_del_timer
+```
+
+Other functions won't change the driver state. The functions above labeled with `*` are allowed to be used in the interrupt context.
+
+## Concurrency
+
+There might be race conditions when the user calls the APIs from a thread and interrupt at the same time. e.g. a Task is just running the `gptimer_start`, and suddenly an interrupt occurs, where the user calls `gptimer_stop` for the same timer handle. Which is possible to make a "stopped" timer continue to run if the interrupt is returned before the Task.
+
+```mermaid
+stateDiagram-v2
+    state Race-Condition {
+        Thread --> gptimer_start
+        state gptimer_start {
+            state is_enabled <<choice>>
+            [*] --> is_enabled: Enabled?
+            is_enabled --> run_wait: yes
+            is_enabled --> [*] : no
+            run_wait --> run: call HAL/LL functions to start timer
+            }
+        --
+        Interrupt --> gptimer_stop
+        state gptimer_stop {
+            state is_running <<choice>>
+            [*] --> is_running: Running?
+            is_running --> enable_wait: yes
+            is_running --> [*] : no
+            enable_wait --> enable: call HAL/LL functions to stop timer
+        }
+    }
+```
+
+By introducing a "middle" state like `run_wait` and `enable_wait`, we make sure that the timer is in a safe state before we start/stop it. And if the state is invalid, it can return an error code to the user.

components/driver/gptimer/gptimer.c

@@ -136,7 +136,8 @@ esp_err_t gptimer_new_timer(const gptimer_config_t *config, gptimer_handle_t *re
     portEXIT_CRITICAL(&group->spinlock);
     // initialize other members of timer
     timer->spinlock = (portMUX_TYPE)portMUX_INITIALIZER_UNLOCKED;
-    timer->fsm = GPTIMER_FSM_INIT; // put the timer into init state
+    // put the timer driver to the init state
+    atomic_init(&timer->fsm, GPTIMER_FSM_INIT);
     timer->direction = config->direction;
     timer->flags.intr_shared = config->flags.intr_shared;
     ESP_LOGD(TAG, "new gptimer (%d,%d) at %p, resolution=%"PRIu32"Hz", group_id, timer_id, timer, timer->resolution_hz);
@@ -153,7 +154,7 @@ esp_err_t gptimer_new_timer(const gptimer_config_t *config, gptimer_handle_t *re
 esp_err_t gptimer_del_timer(gptimer_handle_t timer)
 {
     ESP_RETURN_ON_FALSE(timer, ESP_ERR_INVALID_ARG, TAG, "invalid argument");
-    ESP_RETURN_ON_FALSE(timer->fsm == GPTIMER_FSM_INIT, ESP_ERR_INVALID_STATE, TAG, "timer not in init state");
+    ESP_RETURN_ON_FALSE(atomic_load(&timer->fsm) == GPTIMER_FSM_INIT, ESP_ERR_INVALID_STATE, TAG, "timer not in init state");
     gptimer_group_t *group = timer->group;
     gptimer_clock_source_t clk_src = timer->clk_src;
     int group_id = group->group_id;
@@ -231,7 +232,7 @@ esp_err_t gptimer_register_event_callbacks(gptimer_handle_t timer, const gptimer
 
     // lazy install interrupt service
     if (!timer->intr) {
-        ESP_RETURN_ON_FALSE(timer->fsm == GPTIMER_FSM_INIT, ESP_ERR_INVALID_STATE, TAG, "timer not in init state");
+        ESP_RETURN_ON_FALSE(atomic_load(&timer->fsm) == GPTIMER_FSM_INIT, ESP_ERR_INVALID_STATE, TAG, "timer not in init state");
         // if user wants to control the interrupt allocation more precisely, we can expose more flags in `gptimer_config_t`
         int isr_flags = timer->flags.intr_shared ? ESP_INTR_FLAG_SHARED | GPTIMER_INTR_ALLOC_FLAGS : GPTIMER_INTR_ALLOC_FLAGS;
         ESP_RETURN_ON_ERROR(esp_intr_alloc_intrstatus(timer_group_periph_signals.groups[group_id].timer_irq_id[timer_id], isr_flags,
@@ -283,63 +284,79 @@ esp_err_t gptimer_set_alarm_action(gptimer_handle_t timer, const gptimer_alarm_c
 esp_err_t gptimer_enable(gptimer_handle_t timer)
 {
     ESP_RETURN_ON_FALSE(timer, ESP_ERR_INVALID_ARG, TAG, "invalid argument");
-    ESP_RETURN_ON_FALSE(timer->fsm == GPTIMER_FSM_INIT, ESP_ERR_INVALID_STATE, TAG, "timer not in init state");
+    gptimer_fsm_t expected_fsm = GPTIMER_FSM_INIT;
+    ESP_RETURN_ON_FALSE(atomic_compare_exchange_strong(&timer->fsm, &expected_fsm, GPTIMER_FSM_ENABLE),
+                        ESP_ERR_INVALID_STATE, TAG, "timer not in init state");
 
     // acquire power manager lock
     if (timer->pm_lock) {
         ESP_RETURN_ON_ERROR(esp_pm_lock_acquire(timer->pm_lock), TAG, "acquire pm_lock failed");
     }
+
     // enable interrupt service
     if (timer->intr) {
         ESP_RETURN_ON_ERROR(esp_intr_enable(timer->intr), TAG, "enable interrupt service failed");
     }
 
-    timer->fsm = GPTIMER_FSM_ENABLE;
     return ESP_OK;
 }
 
 esp_err_t gptimer_disable(gptimer_handle_t timer)
 {
     ESP_RETURN_ON_FALSE(timer, ESP_ERR_INVALID_ARG, TAG, "invalid argument");
-    ESP_RETURN_ON_FALSE(timer->fsm == GPTIMER_FSM_ENABLE, ESP_ERR_INVALID_STATE, TAG, "timer not in enable state");
+    gptimer_fsm_t expected_fsm = GPTIMER_FSM_ENABLE;
+    ESP_RETURN_ON_FALSE(atomic_compare_exchange_strong(&timer->fsm, &expected_fsm, GPTIMER_FSM_INIT),
+                        ESP_ERR_INVALID_STATE, TAG, "timer not in enable state");
 
     // disable interrupt service
     if (timer->intr) {
         ESP_RETURN_ON_ERROR(esp_intr_disable(timer->intr), TAG, "disable interrupt service failed");
     }
+
     // release power manager lock
     if (timer->pm_lock) {
         ESP_RETURN_ON_ERROR(esp_pm_lock_release(timer->pm_lock), TAG, "release pm_lock failed");
     }
 
-    timer->fsm = GPTIMER_FSM_INIT;
     return ESP_OK;
 }
 
 esp_err_t gptimer_start(gptimer_handle_t timer)
 {
     ESP_RETURN_ON_FALSE_ISR(timer, ESP_ERR_INVALID_ARG, TAG, "invalid argument");
-    ESP_RETURN_ON_FALSE_ISR(timer->fsm == GPTIMER_FSM_ENABLE, ESP_ERR_INVALID_STATE, TAG, "timer not enabled yet");
 
-    portENTER_CRITICAL_SAFE(&timer->spinlock);
-    timer_ll_enable_counter(timer->hal.dev, timer->timer_id, true);
-    timer_ll_enable_alarm(timer->hal.dev, timer->timer_id, timer->flags.alarm_en);
-    portEXIT_CRITICAL_SAFE(&timer->spinlock);
+    gptimer_fsm_t expected_fsm = GPTIMER_FSM_ENABLE;
+    if (atomic_compare_exchange_strong(&timer->fsm, &expected_fsm, GPTIMER_FSM_RUN_WAIT)) {
+        // the register used by the following LL functions are shared with other API,
+        // which is possible to run along with this function, so we need to protect
+        portENTER_CRITICAL_SAFE(&timer->spinlock);
+        timer_ll_enable_counter(timer->hal.dev, timer->timer_id, true);
+        timer_ll_enable_alarm(timer->hal.dev, timer->timer_id, timer->flags.alarm_en);
+        portEXIT_CRITICAL_SAFE(&timer->spinlock);
+    } else {
+        ESP_RETURN_ON_FALSE_ISR(false, ESP_ERR_INVALID_STATE, TAG, "timer is not enabled yet");
+    }
 
+    atomic_store(&timer->fsm, GPTIMER_FSM_RUN);
     return ESP_OK;
 }
 
 esp_err_t gptimer_stop(gptimer_handle_t timer)
 {
     ESP_RETURN_ON_FALSE_ISR(timer, ESP_ERR_INVALID_ARG, TAG, "invalid argument");
-    ESP_RETURN_ON_FALSE_ISR(timer->fsm == GPTIMER_FSM_ENABLE, ESP_ERR_INVALID_STATE, TAG, "timer not enabled yet");
 
-    // disable counter, alarm, auto-reload
-    portENTER_CRITICAL_SAFE(&timer->spinlock);
-    timer_ll_enable_counter(timer->hal.dev, timer->timer_id, false);
-    timer_ll_enable_alarm(timer->hal.dev, timer->timer_id, false);
-    portEXIT_CRITICAL_SAFE(&timer->spinlock);
+    gptimer_fsm_t expected_fsm = GPTIMER_FSM_RUN;
+    if (atomic_compare_exchange_strong(&timer->fsm, &expected_fsm, GPTIMER_FSM_ENABLE_WAIT)) {
+        // disable counter, alarm, auto-reload
+        portENTER_CRITICAL_SAFE(&timer->spinlock);
+        timer_ll_enable_counter(timer->hal.dev, timer->timer_id, false);
+        timer_ll_enable_alarm(timer->hal.dev, timer->timer_id, false);
+        portEXIT_CRITICAL_SAFE(&timer->spinlock);
+    } else {
+        ESP_RETURN_ON_FALSE_ISR(false, ESP_ERR_INVALID_STATE, TAG, "timer is not running");
+    }
 
+    atomic_store(&timer->fsm, GPTIMER_FSM_ENABLE);
     return ESP_OK;
 }
 
@@ -498,21 +515,3 @@ IRAM_ATTR static void gptimer_default_isr(void *args)
         portYIELD_FROM_ISR();
     }
 }
-
-////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
-///// The Following APIs are for internal use only (e.g. unit test)    /////////////////////////////////////////////////
-////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
-
-esp_err_t gptimer_get_intr_handle(gptimer_handle_t timer, intr_handle_t *ret_intr_handle)
-{
-    ESP_RETURN_ON_FALSE(timer && ret_intr_handle, ESP_ERR_INVALID_ARG, TAG, "invalid argument");
-    *ret_intr_handle = timer->intr;
-    return ESP_OK;
-}
-
-esp_err_t gptimer_get_pm_lock(gptimer_handle_t timer, esp_pm_lock_handle_t *ret_pm_lock)
-{
-    ESP_RETURN_ON_FALSE(timer && ret_pm_lock, ESP_ERR_INVALID_ARG, TAG, "invalid argument");
-    *ret_pm_lock = timer->pm_lock;
-    return ESP_OK;
-}

components/driver/gptimer/gptimer_priv.c

@@ -0,0 +1,25 @@
+/*
+ * SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#include "esp_check.h"
+#include "esp_private/gptimer.h"
+#include "gptimer_priv.h"
+
+static const char *TAG = "gptimer";
+
+esp_err_t gptimer_get_intr_handle(gptimer_handle_t timer, intr_handle_t *ret_intr_handle)
+{
+    ESP_RETURN_ON_FALSE(timer && ret_intr_handle, ESP_ERR_INVALID_ARG, TAG, "invalid argument");
+    *ret_intr_handle = timer->intr;
+    return ESP_OK;
+}
+
+esp_err_t gptimer_get_pm_lock(gptimer_handle_t timer, esp_pm_lock_handle_t *ret_pm_lock)
+{
+    ESP_RETURN_ON_FALSE(timer && ret_pm_lock, ESP_ERR_INVALID_ARG, TAG, "invalid argument");
+    *ret_pm_lock = timer->pm_lock;
+    return ESP_OK;
+}

components/driver/gptimer/gptimer_priv.h

@@ -1,12 +1,13 @@
 /*
- * SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
 
 #pragma once
 
 #include <stdint.h>
+#include <stdatomic.h>
 #include "sdkconfig.h"
 #include "freertos/FreeRTOS.h"
 #include "esp_err.h"
@@ -45,8 +46,11 @@ typedef struct gptimer_group_t {
 } gptimer_group_t;
 
 typedef enum {
-    GPTIMER_FSM_INIT,
-    GPTIMER_FSM_ENABLE,
+    GPTIMER_FSM_INIT,        // Timer is initialized, but not enabled
+    GPTIMER_FSM_ENABLE,      // Timer is enabled, but is not running
+    GPTIMER_FSM_ENABLE_WAIT, // Timer is in the middle of the enable process (Intermediate state)
+    GPTIMER_FSM_RUN,         // Timer is in running
+    GPTIMER_FSM_RUN_WAIT,    // Timer is in the middle of the run process (Intermediate state)
 } gptimer_fsm_t;
 
 struct gptimer_t {
@@ -57,7 +61,7 @@ struct gptimer_t {
     uint64_t alarm_count;
     gptimer_count_direction_t direction;
     timer_hal_context_t hal;
-    gptimer_fsm_t fsm;
+    _Atomic gptimer_fsm_t fsm;
     intr_handle_t intr;
     portMUX_TYPE spinlock; // to protect per-timer resources concurrent accessed by task and ISR handler
     gptimer_alarm_cb_t on_alarm;

components/driver/gptimer/include/driver/gptimer.h

@@ -32,7 +32,7 @@ typedef struct {
 /**
  * @brief Create a new General Purpose Timer, and return the handle
  *
- * @note The newly created timer is put in the init state.
+ * @note The newly created timer is put in the "init" state.
  *
  * @param[in] config GPTimer configuration
  * @param[out] ret_timer Returned timer handle
@@ -48,8 +48,7 @@ esp_err_t gptimer_new_timer(const gptimer_config_t *config, gptimer_handle_t *re
 /**
  * @brief Delete the GPTimer handle
  *
- * @note A timer can't be in the enable state when this function is invoked.
- *       See also `gptimer_disable` for how to disable a timer.
+ * @note A timer must be in the "init" state before it can be deleted.
  *
  * @param[in] timer Timer handle created by `gptimer_new_timer`
  * @return
@@ -65,7 +64,8 @@ esp_err_t gptimer_del_timer(gptimer_handle_t timer);
  *
  * @note When updating the raw count of an active timer, the timer will immediately start counting from the new value.
  * @note This function is allowed to run within ISR context
- * @note This function is allowed to be executed when Cache is disabled, by enabling `CONFIG_GPTIMER_CTRL_FUNC_IN_IRAM`
+ * @note If `CONFIG_GPTIMER_CTRL_FUNC_IN_IRAM` is enabled, this function will be placed in the IRAM by linker,
+ *       makes it possible to execute even when the Flash Cache is disabled.
  *
  * @param[in] timer Timer handle created by `gptimer_new_timer`
  * @param[in] value Count value to be set
@@ -82,7 +82,8 @@ esp_err_t gptimer_set_raw_count(gptimer_handle_t timer, uint64_t value);
  * @note This function will trigger a software capture event and then return the captured count value.
  * @note With the raw count value and the resolution returned from `gptimer_get_resolution`, you can convert the count value into seconds.
  * @note This function is allowed to run within ISR context
- * @note This function is allowed to be executed when Cache is disabled, by enabling `CONFIG_GPTIMER_CTRL_FUNC_IN_IRAM`
+ * @note If `CONFIG_GPTIMER_CTRL_FUNC_IN_IRAM` is enabled, this function will be placed in the IRAM by linker,
+ *       makes it possible to execute even when the Flash Cache is disabled.
  *
  * @param[in] timer Timer handle created by `gptimer_new_timer`
  * @param[out] value Returned GPTimer count value
@@ -96,7 +97,8 @@ esp_err_t gptimer_get_raw_count(gptimer_handle_t timer, uint64_t *value);
 /**
  * @brief Return the real resolution of the timer
  *
- * @note usually the timer resolution is same as what you configured in the `gptimer_config_t::resolution_hz`, but for some unstable clock source (e.g. RC_FAST), which needs a calibration, the real resolution may be different from the configured one.
+ * @note usually the timer resolution is same as what you configured in the `gptimer_config_t::resolution_hz`,
+ *       but some unstable clock source (e.g. RC_FAST) will do a calibration, the real resolution can be different from the configured one.
  *
  * @param[in] timer Timer handle created by `gptimer_new_timer`
  * @param[out] out_resolution Returned timer resolution, in Hz
@@ -110,9 +112,10 @@ esp_err_t gptimer_get_resolution(gptimer_handle_t timer, uint32_t *out_resolutio
 /**
  * @brief Get GPTimer captured count value
  *
- * @note The capture action can be issued either by external event or by software (see also `gptimer_get_raw_count`).
+ * @note The capture action can be issued either by ETM event or by software (see also `gptimer_get_raw_count`).
  * @note This function is allowed to run within ISR context
- * @note This function is allowed to be executed when Cache is disabled, by enabling `CONFIG_GPTIMER_CTRL_FUNC_IN_IRAM`
+ * @note If `CONFIG_GPTIMER_CTRL_FUNC_IN_IRAM` is enabled, this function will be placed in the IRAM by linker,
+ *       makes it possible to execute even when the Flash Cache is disabled.
  *
  * @param[in] timer Timer handle created by `gptimer_new_timer`
  * @param[out] value Returned captured count value
@@ -165,7 +168,8 @@ typedef struct {
  * @brief Set alarm event actions for GPTimer.
  *
  * @note This function is allowed to run within ISR context, so that user can set new alarm action immediately in the ISR callback.
- * @note This function is allowed to be executed when Cache is disabled, by enabling `CONFIG_GPTIMER_CTRL_FUNC_IN_IRAM`
+ * @note If `CONFIG_GPTIMER_CTRL_FUNC_IN_IRAM` is enabled, this function will be placed in the IRAM by linker,
+ *       makes it possible to execute even when the Flash Cache is disabled.
  *
  * @param[in] timer Timer handle created by `gptimer_new_timer`
  * @param[in] config Alarm configuration, especially, set config to NULL means disabling the alarm function
@@ -179,7 +183,7 @@ esp_err_t gptimer_set_alarm_action(gptimer_handle_t timer, const gptimer_alarm_c
 /**
  * @brief Enable GPTimer
  *
- * @note This function will transit the timer state from init to enable.
+ * @note This function will transit the timer state from "init" to "enable".
  * @note This function will enable the interrupt service, if it's lazy installed in `gptimer_register_event_callbacks`.
  * @note This function will acquire a PM lock, if a specific source clock (e.g. APB) is selected in the `gptimer_config_t`, while `CONFIG_PM_ENABLE` is enabled.
  * @note Enable a timer doesn't mean to start it. See also `gptimer_start` for how to make the timer start counting.
@@ -196,7 +200,9 @@ esp_err_t gptimer_enable(gptimer_handle_t timer);
 /**
  * @brief Disable GPTimer
  *
- * @note This function will do the opposite work to the `gptimer_enable`
+ * @note This function will transit the timer state from "enable" to "init".
+ * @note This function will disable the interrupt service if it's installed.
+ * @note This function will release the PM lock if it's acquired in the `gptimer_enable`.
  * @note Disable a timer doesn't mean to stop it. See also `gptimer_stop` for how to make the timer stop counting.
  *
  * @param[in] timer Timer handle created by `gptimer_new_timer`
@@ -211,31 +217,33 @@ esp_err_t gptimer_disable(gptimer_handle_t timer);
 /**
  * @brief Start GPTimer (internal counter starts counting)
  *
- * @note This function should be called when the timer is in the enable state (i.e. after calling `gptimer_enable`)
+ * @note This function will transit the timer state from "enable" to "run".
  * @note This function is allowed to run within ISR context
- * @note This function will be placed into IRAM if `CONFIG_GPTIMER_CTRL_FUNC_IN_IRAM` is on, so that it's allowed to be executed when Cache is disabled
+ * @note If `CONFIG_GPTIMER_CTRL_FUNC_IN_IRAM` is enabled, this function will be placed in the IRAM by linker,
+ *       makes it possible to execute even when the Flash Cache is disabled.
  *
  * @param[in] timer Timer handle created by `gptimer_new_timer`
  * @return
  *      - ESP_OK: Start GPTimer successfully
  *      - ESP_ERR_INVALID_ARG: Start GPTimer failed because of invalid argument
- *      - ESP_ERR_INVALID_STATE: Start GPTimer failed because the timer is not enabled yet
+ *      - ESP_ERR_INVALID_STATE: Start GPTimer failed because the timer is not enabled or is already in running
  *      - ESP_FAIL: Start GPTimer failed because of other error
  */
 esp_err_t gptimer_start(gptimer_handle_t timer);
 
 /**
  * @brief Stop GPTimer (internal counter stops counting)
  *
- * @note This function should be called when the timer is in the enable state (i.e. after calling `gptimer_enable`)
+ * @note This function will transit the timer state from "run" to "enable".
  * @note This function is allowed to run within ISR context
- * @note This function will be placed into IRAM if `CONFIG_GPTIMER_CTRL_FUNC_IN_IRAM` is on, so that it's allowed to be executed when Cache is disabled
+ * @note If `CONFIG_GPTIMER_CTRL_FUNC_IN_IRAM` is enabled, this function will be placed in the IRAM by linker,
+ *       makes it possible to execute even when the Flash Cache is disabled.
  *
  * @param[in] timer Timer handle created by `gptimer_new_timer`
  * @return
  *      - ESP_OK: Stop GPTimer successfully
  *      - ESP_ERR_INVALID_ARG: Stop GPTimer failed because of invalid argument
- *      - ESP_ERR_INVALID_STATE: Stop GPTimer failed because the timer is not enabled yet
+ *      - ESP_ERR_INVALID_STATE: Stop GPTimer failed because the timer is not in running.
  *      - ESP_FAIL: Stop GPTimer failed because of other error
  */
 esp_err_t gptimer_stop(gptimer_handle_t timer);