From 13523c95b42c77b4263ef98ce054962bcb338636 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Fri, 21 Dec 2018 12:49:33 +0800 Subject: [PATCH 1/4] freertos: check that mutex is released by owner task Mutex type semaphores should be acquired and released by the same task. Add a check to xQueueGenericSend for this condition. --- components/freertos/Kconfig | 7 +++++++ .../include/freertos/FreeRTOSConfig.h | 6 ++++++ components/freertos/queue.c | 6 ++++++ .../freertos/test/test_freertos_mutex.c | 21 +++++++++++++++++++ 4 files changed, 40 insertions(+) create mode 100644 components/freertos/test/test_freertos_mutex.c diff --git a/components/freertos/Kconfig b/components/freertos/Kconfig index 8ec085e140..8063c266bc 100644 --- a/components/freertos/Kconfig +++ b/components/freertos/Kconfig @@ -419,4 +419,11 @@ menu "FreeRTOS" abort the application. This option is also required for GDB backtraces and C++ exceptions to work correctly inside top-level task functions. + config FREERTOS_CHECK_MUTEX_GIVEN_BY_OWNER + bool "Check that mutex semaphore is given by owner task" + default y + help + If enabled, assert that when a mutex semaphore is given, the task giving the + semaphore is the task which is currently holding the mutex. + endmenu diff --git a/components/freertos/include/freertos/FreeRTOSConfig.h b/components/freertos/include/freertos/FreeRTOSConfig.h index 80185f9e04..cc6cdad90b 100644 --- a/components/freertos/include/freertos/FreeRTOSConfig.h +++ b/components/freertos/include/freertos/FreeRTOSConfig.h @@ -314,5 +314,11 @@ extern void vPortCleanUpTCB ( void *pxTCB ); #endif /* def __ASSEMBLER__ */ #endif +#if CONFIG_FREERTOS_CHECK_MUTEX_GIVEN_BY_OWNER +#define configCHECK_MUTEX_GIVEN_BY_OWNER 1 +#else +#define configCHECK_MUTEX_GIVEN_BY_OWNER 0 +#endif + #endif /* FREERTOS_CONFIG_H */ diff --git a/components/freertos/queue.c b/components/freertos/queue.c index f6b63de5e6..eccd1a016a 100644 --- a/components/freertos/queue.c +++ b/components/freertos/queue.c @@ -724,6 +724,12 @@ Queue_t * const pxQueue = ( Queue_t * ) xQueue; configASSERT( !( ( xTaskGetSchedulerState() == taskSCHEDULER_SUSPENDED ) && ( xTicksToWait != 0 ) ) ); } #endif + #if ( configUSE_MUTEXES == 1 && configCHECK_MUTEX_GIVEN_BY_OWNER == 1) + { + configASSERT(pxQueue->uxQueueType != queueQUEUE_IS_MUTEX || pxQueue->pxMutexHolder == NULL || xTaskGetCurrentTaskHandle() == pxQueue->pxMutexHolder); + } + #endif + /* This function relaxes the coding standard somewhat to allow return diff --git a/components/freertos/test/test_freertos_mutex.c b/components/freertos/test/test_freertos_mutex.c new file mode 100644 index 0000000000..51697984e7 --- /dev/null +++ b/components/freertos/test/test_freertos_mutex.c @@ -0,0 +1,21 @@ +#include "freertos/FreeRTOS.h" +#include "freertos/task.h" +#include "freertos/semphr.h" +#include "unity.h" +#include "esp_ipc.h" +#include "test_utils.h" + +static void mutex_release_task(void* arg) +{ + SemaphoreHandle_t mutex = (SemaphoreHandle_t) arg; + xSemaphoreGive(mutex); + TEST_FAIL_MESSAGE("should not be reached"); +} + +TEST_CASE("mutex released not by owner causes an assert", "[freertos][reset=abort,SW_CPU_RESET]") +{ + SemaphoreHandle_t mutex = xSemaphoreCreateMutex(); + xSemaphoreTake(mutex, portMAX_DELAY); + xTaskCreate(&mutex_release_task, "mutex_release", 2048, mutex, UNITY_FREERTOS_PRIORITY + 1, NULL); + vTaskDelay(1); +} From d539183b40ba1add2b2dd2ff4622b0ef3fd6d84b Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Thu, 24 Jan 2019 11:08:32 +0800 Subject: [PATCH 2/4] esp32: use binary semaphore instead of mutex in dport tests --- components/esp32/test/test_dport.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/components/esp32/test/test_dport.c b/components/esp32/test/test_dport.c index 4520a70aaa..418cd4202d 100644 --- a/components/esp32/test/test_dport.c +++ b/components/esp32/test/test_dport.c @@ -66,8 +66,7 @@ void run_tasks(const char *task1_description, void (* task1_func)(void *), const for (i=0; i<2; i++) { if((task1_func != NULL && i == 0) || (task2_func != NULL && i == 1)){ - exit_sema[i] = xSemaphoreCreateMutex(); - xSemaphoreTake(exit_sema[i], portMAX_DELAY); + exit_sema[i] = xSemaphoreCreateBinary(); } } From 37144dfa077729ffe28c605a5e3cb96dbd5ef2b3 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Mon, 4 Mar 2019 16:59:09 +0800 Subject: [PATCH 3/4] mdns: use binary semaphore instead of mutex when searching mdns_search_once_t::lock is used to synchronize tasks (taken by one task and given by the other) so it should not be a mutex. Convert to semaphore, and rename to indicate its purpose. --- components/mdns/mdns.c | 14 ++++++-------- components/mdns/private_include/mdns_private.h | 5 +---- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/components/mdns/mdns.c b/components/mdns/mdns.c index 84d578b3c2..40b2cc038f 100644 --- a/components/mdns/mdns.c +++ b/components/mdns/mdns.c @@ -3074,7 +3074,7 @@ static void _mdns_search_free(mdns_search_once_t * search) free(search->instance); free(search->service); free(search->proto); - vSemaphoreDelete(search->lock); + vSemaphoreDelete(search->done_semaphore); free(search); } @@ -3090,8 +3090,8 @@ static mdns_search_once_t * _mdns_search_init(const char * name, const char * se } memset(search, 0, sizeof(mdns_search_once_t)); - search->lock = xSemaphoreCreateMutex(); - if (!search->lock) { + search->done_semaphore = xSemaphoreCreateBinary(); + if (!search->done_semaphore) { free(search); return NULL; } @@ -3130,8 +3130,6 @@ static mdns_search_once_t * _mdns_search_init(const char * name, const char * se search->started_at = xTaskGetTickCount() * portTICK_PERIOD_MS; search->next = NULL; - xSemaphoreTake(search->lock, 0); - return search; } @@ -3142,7 +3140,7 @@ static void _mdns_search_finish(mdns_search_once_t * search) { search->state = SEARCH_OFF; queueDetach(mdns_search_once_t, _mdns_server->search_once, search); - xSemaphoreGive(search->lock); + xSemaphoreGive(search->done_semaphore); } /** @@ -4148,7 +4146,7 @@ void mdns_free() free(h->instance); free(h->service); free(h->proto); - vSemaphoreDelete(h->lock); + vSemaphoreDelete(h->done_semaphore); if (h->result) { mdns_query_results_free(h->result); } @@ -4543,7 +4541,7 @@ esp_err_t mdns_query(const char * name, const char * service, const char * proto _mdns_search_free(search); return ESP_ERR_NO_MEM; } - xSemaphoreTake(search->lock, portMAX_DELAY); + xSemaphoreTake(search->done_semaphore, portMAX_DELAY); *results = search->result; _mdns_search_free(search); diff --git a/components/mdns/private_include/mdns_private.h b/components/mdns/private_include/mdns_private.h index 934b9427e2..9efd23ae70 100644 --- a/components/mdns/private_include/mdns_private.h +++ b/components/mdns/private_include/mdns_private.h @@ -115,9 +115,6 @@ #define PCB_STATE_IS_ANNOUNCING(s) (s->state > PCB_PROBE_3 && s->state < PCB_RUNNING) #define PCB_STATE_IS_RUNNING(s) (s->state == PCB_RUNNING) -#define MDNS_SEARCH_LOCK() xSemaphoreTake(_mdns_server->search.lock, portMAX_DELAY) -#define MDNS_SEARCH_UNLOCK() xSemaphoreGive(_mdns_server->search.lock) - #ifndef HOOK_MALLOC_FAILED #define HOOK_MALLOC_FAILED ESP_LOGE(TAG, "Cannot allocate memory (line: %d, free heap: %d bytes)", __LINE__, esp_get_free_heap_size()); #endif @@ -318,7 +315,7 @@ typedef struct mdns_search_once_s { uint32_t started_at; uint32_t sent_at; uint32_t timeout; - SemaphoreHandle_t lock; + SemaphoreHandle_t done_semaphore; uint16_t type; uint8_t max_results; uint8_t num_results; From 1bc1fb70058d6cadd34f040209082ca12e3c932b Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Mon, 4 Mar 2019 18:51:46 +0800 Subject: [PATCH 4/4] ci: add one more unit test job --- .gitlab-ci.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 8742f5fd74..285d641a7d 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -1305,6 +1305,12 @@ UT_001_43: - ESP32_IDF - UT_T1_1 +UT_001_43: + <<: *unit_test_template + tags: + - ESP32_IDF + - UT_T1_1 + UT_002_01: <<: *unit_test_template tags: