alex/esp-idf
alex/esp-idf
1
0
Fork 0
mirror of https://github.com/espressif/esp-idf.git synced 2024-10-05 20:47:46 -04:00
Code Issues Actions 2 Packages Projects Releases Wiki Activity

gdma: fix potential cocurrency issue

Browse Source 
Alloc handle memory first then hook, we can benifit:
1. Don't have to do malloc in a critical section
2. Don't have to do esp_intr_free in a critical section
This commit is contained in:
morris 2021-01-14 20:34:30 +08:00
parent 19d92ef6b2
commit 914ba4914a
2 changed files with 60 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

91
components/driver/gdma.c
View File

@ -97,8 +97,8 @@ static gdma_group_t *gdma_acquire_group_handle(int group_id);
static void gdma_release_group_handle(gdma_group_t *group);
static gdma_pair_t *gdma_acquire_pair_handle(gdma_group_t *group, int pair_id);
static void gdma_release_pair_handle(gdma_pair_t *pair);
static void gdma_uninstall_pair(gdma_pair_t *pair);
static void gdma_uninstall_group(gdma_group_t *group);
static void gdma_uninstall_pair(gdma_pair_t *pair);
static esp_err_t gdma_del_tx_channel(gdma_channel_t *dma_channel);
static esp_err_t gdma_del_rx_channel(gdma_channel_t *dma_channel);
static esp_err_t gdma_install_interrupt(gdma_pair_t *pair);
@ -312,6 +312,8 @@ esp_err_t gdma_register_tx_event_callbacks(gdma_channel_handle_t dma_chan, gdma_
tx_chan->on_trans_eof = cbs->on_trans_eof;
tx_chan->user_data = user_data;
DMA_CHECK(esp_intr_enable(pair->intr) == ESP_OK, "enable interrupt failed", err, ESP_FAIL);
err:
return ret_code;
}
@ -337,6 +339,8 @@ esp_err_t gdma_register_rx_event_callbacks(gdma_channel_handle_t dma_chan, gdma_
rx_chan->on_recv_eof = cbs->on_recv_eof;
rx_chan->user_data = user_data;
DMA_CHECK(esp_intr_enable(pair->intr) == ESP_OK, "enable interrupt failed", err, ESP_FAIL);
err:
return ret_code;
}
@ -429,33 +433,35 @@ static void gdma_uninstall_group(gdma_group_t *group)
static gdma_group_t *gdma_acquire_group_handle(int group_id)
{
gdma_group_t *group = NULL;
bool new_group = false;
gdma_group_t *group = NULL;
gdma_group_t *pre_alloc_group = calloc(1, sizeof(gdma_group_t));
if (!pre_alloc_group) {
goto out;
}
portENTER_CRITICAL(&s_platform.spinlock);
if (!s_platform.groups[group_id]) {
// lazy install group
group = calloc(1, sizeof(gdma_group_t));
if (group) {
new_group = true;
s_platform.groups[group_id] = group; // register to platform
group->group_id = group_id;
group->spinlock = (portMUX_TYPE)portMUX_INITIALIZER_UNLOCKED;
periph_module_enable(gdma_periph_signals.groups[group_id].module); // enable APB to access GDMA registers
gdma_hal_init(&group->hal, group_id); // initialize HAL context
gdma_ll_enable_clock(group->hal.dev, true); // enable gdma clock
}
new_group = true;
group = pre_alloc_group;
s_platform.groups[group_id] = group; // register to platform
group->group_id = group_id;
group->spinlock = (portMUX_TYPE)portMUX_INITIALIZER_UNLOCKED;
periph_module_enable(gdma_periph_signals.groups[group_id].module); // enable APB to access GDMA registers
gdma_hal_init(&group->hal, group_id); // initialize HAL context
gdma_ll_enable_clock(group->hal.dev, true); // enable gdma clock
} else {
group = s_platform.groups[group_id];
}
if (group) {
// someone acquired the group handle means we have a new object that refer to this group
group->ref_count++;
}
// someone acquired the group handle means we have a new object that refer to this group
group->ref_count++;
portEXIT_CRITICAL(&s_platform.spinlock);
if (new_group) {
ESP_LOGD(TAG, "new group (%d) at %p", group->group_id, group);
} else {
free(pre_alloc_group);
}
out:
return group;
}
@ -486,6 +492,12 @@ static void gdma_uninstall_pair(gdma_pair_t *pair)
do_deinitialize = true;
group->pairs[pair_id] = NULL; // deregister from pair
group->ref_count--; // decrease reference count, because this pair won't refer to the group
if (pair->intr) {
// disable interrupt handler (but not freed, esp_intr_free is a blocking API, we can't use it in a critical section)
esp_intr_disable(pair->intr);
gdma_ll_enable_interrupt(group->hal.dev, pair->pair_id, UINT32_MAX, false); // disable all interupt events
gdma_ll_clear_interrupt_status(group->hal.dev, pair->pair_id, UINT32_MAX); // clear all pending events
}
}
portEXIT_CRITICAL(&group->spinlock);
}
@ -502,32 +514,34 @@ static void gdma_uninstall_pair(gdma_pair_t *pair)
static gdma_pair_t *gdma_acquire_pair_handle(gdma_group_t *group, int pair_id)
{
gdma_pair_t *pair = NULL;
bool new_pair = false;
gdma_pair_t *pair = NULL;
gdma_pair_t *pre_alloc_pair = calloc(1, sizeof(gdma_pair_t));
if (!pre_alloc_pair) {
goto out;
}
portENTER_CRITICAL(&group->spinlock);
if (!group->pairs[pair_id]) {
// lazy install pair
pair = calloc(1, sizeof(gdma_pair_t));
if (pair) {
new_pair = true;
group->pairs[pair_id] = pair; // register to group
group->ref_count++; // pair obtains a reference to group
pair->group = group;
pair->pair_id = pair_id;
pair->spinlock = (portMUX_TYPE)portMUX_INITIALIZER_UNLOCKED;
}
new_pair = true;
pair = pre_alloc_pair;
group->pairs[pair_id] = pair; // register to group
group->ref_count++; // pair obtains a reference to group
pair->group = group;
pair->pair_id = pair_id;
pair->spinlock = (portMUX_TYPE)portMUX_INITIALIZER_UNLOCKED;
} else {
pair = group->pairs[pair_id];
}
if (pair) {
// someone acquired the pair handle means we have a new object that refer to this pair
pair->ref_count++;
}
// someone acquired the pair handle means we have a new object that refer to this pair
pair->ref_count++;
portEXIT_CRITICAL(&group->spinlock);
if (new_pair) {
ESP_LOGD(TAG, "new pair (%d,%d) at %p", group->group_id, pair->pair_id, pair);
} else {
free(pre_alloc_pair);
}
out:
return pair;
}
@ -619,22 +633,29 @@ static esp_err_t gdma_install_interrupt(gdma_pair_t *pair)
{
esp_err_t ret_code = ESP_OK;
gdma_group_t *group = pair->group;
int isr_flags = 0;
bool do_install_isr = false;
// pre-alloc a interrupt handle, shared with other handle, with handler disabled
// This is used to prevent potential concurrency between interrupt install and uninstall
int isr_flags = ESP_INTR_FLAG_SHARED | ESP_INTR_FLAG_INTRDISABLED;
intr_handle_t intr = NULL;
ret_code = esp_intr_alloc(gdma_periph_signals.groups[group->group_id].pairs[pair->pair_id].irq_id, isr_flags, gdma_default_isr, pair, &intr);
DMA_CHECK(ret_code == ESP_OK, "alloc interrupt failed", err, ret_code);
if (!pair->intr) {
portENTER_CRITICAL(&pair->spinlock);
if (!pair->intr) {
do_install_isr = true;
ret_code = esp_intr_alloc(gdma_periph_signals.groups[group->group_id].pairs[pair->pair_id].irq_id, isr_flags, gdma_default_isr, pair, &pair->intr);
pair->intr = intr;
gdma_ll_enable_interrupt(group->hal.dev, pair->pair_id, UINT32_MAX, false); // disable all interupt events
gdma_ll_clear_interrupt_status(group->hal.dev, pair->pair_id, UINT32_MAX); // clear all pending events
}
portEXIT_CRITICAL(&pair->spinlock);
}
if (do_install_isr) {
DMA_CHECK(ret_code == ESP_OK, "alloc interrupt failed", err, ret_code);
ESP_LOGD(TAG, "install interrupt service for pair (%d,%d)", group->group_id, pair->pair_id);
} else {
// interrupt handle has been installed before, so removed this one
esp_intr_free(intr);
}
err:

4
components/driver/test/test_gdma.c
View File

@ -10,11 +10,14 @@ TEST_CASE("GDMA channel allocation", "[gdma]")
gdma_channel_handle_t tx_channels[SOC_GDMA_PAIRS_PER_GROUP] = {};
gdma_channel_handle_t rx_channels[SOC_GDMA_PAIRS_PER_GROUP] = {};
channel_config.direction = GDMA_CHANNEL_DIRECTION_TX;
gdma_tx_event_callbacks_t tx_cbs = {};
gdma_rx_event_callbacks_t rx_cbs = {};
// install TX channels for different peripherals
for (int i = 0; i < SOC_GDMA_PAIRS_PER_GROUP; i++) {
TEST_ESP_OK(gdma_new_channel(&channel_config, &tx_channels[i]));
TEST_ESP_OK(gdma_connect(tx_channels[i], GDMA_MAKE_TRIGGER(GDMA_TRIG_PERIPH_M2M, 0)));
TEST_ESP_OK(gdma_register_tx_event_callbacks(tx_channels[i], &tx_cbs, NULL));
};
TEST_ASSERT_EQUAL(ESP_ERR_NOT_FOUND, gdma_new_channel(&channel_config, &tx_channels[0]));
@ -23,6 +26,7 @@ TEST_CASE("GDMA channel allocation", "[gdma]")
for (int i = 0; i < SOC_GDMA_PAIRS_PER_GROUP; i++) {
TEST_ESP_OK(gdma_new_channel(&channel_config, &rx_channels[i]));
TEST_ESP_OK(gdma_connect(rx_channels[i], GDMA_MAKE_TRIGGER(GDMA_TRIG_PERIPH_M2M, 0)));
TEST_ESP_OK(gdma_register_rx_event_callbacks(rx_channels[i], &rx_cbs, NULL));
}
TEST_ASSERT_EQUAL(ESP_ERR_NOT_FOUND, gdma_new_channel(&channel_config, &rx_channels[0]));