alex/esp-idf
1
0
Fork 0
mirror of https://github.com/espressif/esp-idf.git synced 2025-08-09 12:35:28 +00:00
Code Issues Packages Projects Releases Wiki Activity

gptimer: fix race condition between start and stop

Browse Source 
Added state transition in gptimer_start/stop functions.
So that it's not possible to make a stopped timer continue to run
because of race condition.
This commit is contained in:
morris
2023-03-06 13:32:35 +08:00
parent ed97d230c8
commit 2d52334e5d
11 changed files with 153 additions and 74 deletions
Download Patch File Download Diff File Expand all files Collapse all files

46
components/driver/gptimer/README.md Normal file
View File

@@ -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.

71
components/driver/gptimer/gptimer.c
View File

@@ -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 @@ err:
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;
}

25
components/driver/gptimer/gptimer_priv.c Normal file
View File

@@ -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;
}

12
components/driver/gptimer/gptimer_priv.h
View File

@@ -1,5 +1,5 @@
/*
* SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD
* SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD
*
* SPDX-License-Identifier: Apache-2.0
*/
@@ -7,6 +7,7 @@
#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;

42
components/driver/gptimer/include/driver/gptimer.h
View File

@@ -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,15 +217,16 @@ 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);
@@ -227,15 +234,16 @@ 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);