From f839a1328ccc411eace56277a7d0ac96bc31c7e1 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Tue, 22 Oct 2019 09:43:20 +0200 Subject: [PATCH] esp_netif: added locking for netif list management, unit tests to use unique if_keys, updated comments --- components/esp_netif/README.md | 2 +- components/esp_netif/esp_netif_handlers.c | 2 +- components/esp_netif/esp_netif_objects.c | 91 +++++++++++++++++++ components/esp_netif/include/esp_netif.h | 2 +- .../esp_netif/loopback/esp_netif_loopback.c | 11 --- components/esp_netif/lwip/esp_netif_lwip.c | 30 ++---- .../private_include/esp_netif_private.h | 34 +++++++ components/esp_netif/test/test_esp_netif.c | 9 +- components/esp_wifi/include/esp_wifi_netif.h | 2 +- components/esp_wifi/src/wifi_default.c | 6 +- components/esp_wifi/src/wifi_netif.c | 4 +- 11 files changed, 148 insertions(+), 45 deletions(-) diff --git a/components/esp_netif/README.md b/components/esp_netif/README.md index b3837e7fd..ee62db678 100644 --- a/components/esp_netif/README.md +++ b/components/esp_netif/README.md @@ -57,7 +57,7 @@ Overall application interaction with communication media and network stack ### C) ESP-NETIF, former tcpip_adapter * init API (new, configure) * IO API: for passing data between IO driver and network stack -* event/actiona API (esp-netif lifecycle management) +* event/action API (esp-netif lifecycle management) - building blocks for designing event handlers * setters, getters * network stack abstraction: enabling user interaction with TCP/IP stack diff --git a/components/esp_netif/esp_netif_handlers.c b/components/esp_netif/esp_netif_handlers.c index 04d3c5f7b..c57266576 100644 --- a/components/esp_netif/esp_netif_handlers.c +++ b/components/esp_netif/esp_netif_handlers.c @@ -1,4 +1,4 @@ -// Copyright 2018 Espressif Systems (Shanghai) PTE LTD +// Copyright 2019 Espressif Systems (Shanghai) PTE LTD // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. diff --git a/components/esp_netif/esp_netif_objects.c b/components/esp_netif/esp_netif_objects.c index f188d0615..cf53416da 100644 --- a/components/esp_netif/esp_netif_objects.c +++ b/components/esp_netif/esp_netif_objects.c @@ -15,6 +15,10 @@ #include "esp_netif.h" #include "sys/queue.h" #include "esp_log.h" +#include "freertos/FreeRTOS.h" +#include "freertos/semphr.h" +#include "esp_netif_private.h" +#include // // Purpose of this module is to provide list of esp-netif structures @@ -32,15 +36,38 @@ struct slist_netifs_s { SLIST_HEAD(slisthead, slist_netifs_s) s_head = { .slh_first = NULL, }; static size_t s_esp_netif_counter = 0; +static xSemaphoreHandle s_list_lock = NULL; ESP_EVENT_DEFINE_BASE(IP_EVENT); +esp_err_t esp_netif_list_lock(void) +{ + if (s_list_lock == NULL) { + s_list_lock = xSemaphoreCreateMutex(); + if (s_list_lock == NULL) { + return ESP_ERR_NO_MEM; + } + } + xSemaphoreTake(s_list_lock, portMAX_DELAY); + return ESP_OK; +} + +void esp_netif_list_unlock(void) +{ + assert(s_list_lock); + xSemaphoreGive(s_list_lock); + if (s_esp_netif_counter == 0) { + vQueueDelete(s_list_lock); + s_list_lock = NULL; + } +} // // List manipulation functions // esp_err_t esp_netif_add_to_list(esp_netif_t *netif) { + esp_err_t ret; struct slist_netifs_s *item = calloc(1, sizeof(struct slist_netifs_s)); ESP_LOGD(TAG, "%s %p", __func__, netif); if (item == NULL) { @@ -48,9 +75,14 @@ esp_err_t esp_netif_add_to_list(esp_netif_t *netif) } item->netif = netif; + if ((ret = esp_netif_list_lock()) != ESP_OK) { + return ret; + } + SLIST_INSERT_HEAD(&s_head, item, next); ++s_esp_netif_counter; ESP_LOGD(TAG, "%s netif added successfully (total netifs: %d)", __func__, s_esp_netif_counter); + esp_netif_list_unlock(); return ESP_OK; } @@ -58,7 +90,12 @@ esp_err_t esp_netif_add_to_list(esp_netif_t *netif) esp_err_t esp_netif_remove_from_list(esp_netif_t *netif) { struct slist_netifs_s *item; + esp_err_t ret; + if ((ret = esp_netif_list_lock()) != ESP_OK) { + return ret; + } ESP_LOGV(TAG, "%s %p", __func__, netif); + SLIST_FOREACH(item, &s_head, next) { if (item->netif == netif) { SLIST_REMOVE(&s_head, item, slist_netifs_s, next); @@ -66,9 +103,11 @@ esp_err_t esp_netif_remove_from_list(esp_netif_t *netif) --s_esp_netif_counter; ESP_LOGD(TAG, "%s netif successfully removed (total netifs: %d)", __func__, s_esp_netif_counter); free(item); + esp_netif_list_unlock(); return ESP_OK; } } + esp_netif_list_unlock(); return ESP_ERR_NOT_FOUND; } @@ -78,6 +117,19 @@ size_t esp_netif_get_nr_of_ifs(void) } esp_netif_t* esp_netif_next(esp_netif_t* netif) +{ + esp_err_t ret; + esp_netif_t* result; + if ((ret = esp_netif_list_lock()) != ESP_OK) { + ESP_LOGE(TAG, "Failed to lock esp-netif list with %d", ret); + return NULL; + } + result = esp_netif_next_unsafe(netif); + esp_netif_list_unlock(); + return result; +} + +esp_netif_t* esp_netif_next_unsafe(esp_netif_t* netif) { ESP_LOGV(TAG, "%s %p", __func__, netif); struct slist_netifs_s *item; @@ -93,4 +145,43 @@ esp_netif_t* esp_netif_next(esp_netif_t* netif) } } return NULL; +} + +bool esp_netif_is_netif_listed(esp_netif_t *esp_netif) +{ + esp_err_t ret; + if ((ret = esp_netif_list_lock()) != ESP_OK) { + ESP_LOGE(TAG, "Failed to lock esp-netif list with %d", ret); + return NULL; + } + + // looking for the netif in the list of registered interfaces + esp_netif_t *it = esp_netif_next_unsafe(NULL); + do { + if (it && it == esp_netif) { + esp_netif_list_unlock(); + return true; + } + } while (NULL != (it = esp_netif_next_unsafe(it))); + esp_netif_list_unlock(); + return false; +} + +esp_netif_t *esp_netif_get_handle_from_ifkey(const char *if_key) +{ + esp_err_t ret; + if ((ret = esp_netif_list_lock()) != ESP_OK) { + ESP_LOGE(TAG, "Failed to lock esp-netif list with %d", ret); + return NULL; + } + + esp_netif_t *esp_netif = esp_netif_next_unsafe(NULL); + do { + if (esp_netif && strcmp(if_key, esp_netif_get_ifkey(esp_netif))==0) { + esp_netif_list_unlock(); + return esp_netif; + } + } while (NULL != (esp_netif = esp_netif_next_unsafe(esp_netif))); + esp_netif_list_unlock(); + return NULL; } \ No newline at end of file diff --git a/components/esp_netif/include/esp_netif.h b/components/esp_netif/include/esp_netif.h index 65623a716..b3d2f72a9 100644 --- a/components/esp_netif/include/esp_netif.h +++ b/components/esp_netif/include/esp_netif.h @@ -1,4 +1,4 @@ -// Copyright 2015-2016 Espressif Systems (Shanghai) PTE LTD +// Copyright 2019 Espressif Systems (Shanghai) PTE LTD // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. diff --git a/components/esp_netif/loopback/esp_netif_loopback.c b/components/esp_netif/loopback/esp_netif_loopback.c index 5f481052c..a434ee960 100644 --- a/components/esp_netif/loopback/esp_netif_loopback.c +++ b/components/esp_netif/loopback/esp_netif_loopback.c @@ -427,17 +427,6 @@ const char *esp_netif_get_desc(esp_netif_t *esp_netif) return esp_netif->if_desc; } -esp_netif_t *esp_netif_get_handle_from_ifkey(const char *if_key) -{ - esp_netif_t *esp_netif = esp_netif_next(NULL); - do { - if (esp_netif && strcmp(if_key, esp_netif->if_key)==0) { - return esp_netif; - } - } while (NULL != (esp_netif = esp_netif_next(esp_netif))); - return NULL; -} - uint32_t esp_netif_get_event_id(esp_netif_t *esp_netif, esp_netif_ip_event_type_t event_type) { return 0; diff --git a/components/esp_netif/lwip/esp_netif_lwip.c b/components/esp_netif/lwip/esp_netif_lwip.c index 269375e0f..3138bb8da 100644 --- a/components/esp_netif/lwip/esp_netif_lwip.c +++ b/components/esp_netif/lwip/esp_netif_lwip.c @@ -155,17 +155,12 @@ static inline esp_err_t esp_netif_lwip_ipc_call(esp_netif_api_fn fn, esp_netif_t */ static esp_netif_t* esp_netif_is_active(esp_netif_t *arg) { - esp_netif_t *esp_netif = NULL; // looking for the netif in the list of registered interfaces // as it might have already been destroyed - esp_netif_t *nif = esp_netif_next(NULL); - do { - if (nif && nif == arg) { - esp_netif = nif; - break; - } - } while (NULL != (nif = esp_netif_next(nif))); - return esp_netif; + if (esp_netif_is_netif_listed(arg)) { + return arg; + } + return NULL; } /** @@ -195,8 +190,9 @@ static void esp_netif_update_default_netif(esp_netif_t *esp_netif, esp_netif_act default: case ESP_NETIF_STOPPED: { - esp_netif_t *netif = esp_netif_next(NULL); s_last_default_esp_netif = NULL; + esp_netif_list_lock(); + esp_netif_t *netif = esp_netif_next_unsafe(NULL); while (netif) { if (esp_netif_is_netif_up(netif)) { if (s_last_default_esp_netif && esp_netif_is_netif_up(s_last_default_esp_netif)) { @@ -208,8 +204,9 @@ static void esp_netif_update_default_netif(esp_netif_t *esp_netif, esp_netif_act s_last_default_esp_netif = netif; } } - netif = esp_netif_next(netif); + netif = esp_netif_next_unsafe(netif); } + esp_netif_list_unlock(); if (s_last_default_esp_netif && esp_netif_is_netif_up(s_last_default_esp_netif)) { netif_set_default(s_last_default_esp_netif->lwip_netif); } @@ -1362,17 +1359,6 @@ const char *esp_netif_get_desc(esp_netif_t *esp_netif) return esp_netif->if_desc; } -esp_netif_t *esp_netif_get_handle_from_ifkey(const char *if_key) -{ - esp_netif_t *esp_netif = esp_netif_next(NULL); - do { - if (esp_netif && strcmp(if_key, esp_netif->if_key)==0) { - return esp_netif; - } - } while (NULL != (esp_netif = esp_netif_next(esp_netif))); - return NULL; -} - uint32_t esp_netif_get_event_id(esp_netif_t *esp_netif, esp_netif_ip_event_type_t event_type) { switch(event_type) { diff --git a/components/esp_netif/private_include/esp_netif_private.h b/components/esp_netif/private_include/esp_netif_private.h index 0519c397d..b34711d16 100644 --- a/components/esp_netif/private_include/esp_netif_private.h +++ b/components/esp_netif/private_include/esp_netif_private.h @@ -111,4 +111,38 @@ esp_err_t esp_netif_add_to_list(esp_netif_t* netif); */ esp_err_t esp_netif_remove_from_list(esp_netif_t* netif); +/** + * @brief Iterates over list of interfaces without list locking. Returns first netif if NULL given as parameter + * + * Used for bulk search loops to avoid locking and unlocking every iteration. esp_netif_list_lock and esp_netif_list_unlock + * must be used to guard the search loop + * + * @param[in] esp_netif Handle to esp-netif instance + * + * @return First netif from the list if supplied parameter is NULL, next one otherwise + */ +esp_netif_t* esp_netif_next_unsafe(esp_netif_t* netif); + +/** + * @brief Locking network interface list. Use only in connection with esp_netif_next_unsafe + * + * @return ESP_OK on success, specific mutex error if failed to lock + */ +esp_err_t esp_netif_list_lock(void); + +/** + * @brief Unlocking network interface list. Use only in connection with esp_netif_next_unsafe + * + */ +void esp_netif_list_unlock(void); + +/** + * @brief Iterates over list of registered interfaces to check if supplied netif is listed + * + * @param esp_netif network interface to check + * + * @return true if supplied interface is listed + */ +bool esp_netif_is_netif_listed(esp_netif_t *esp_netif); + #endif //_ESP_NETIF_PRIVATE_H_ diff --git a/components/esp_netif/test/test_esp_netif.c b/components/esp_netif/test/test_esp_netif.c index 329259c0b..8013cdb1c 100644 --- a/components/esp_netif/test/test_esp_netif.c +++ b/components/esp_netif/test/test_esp_netif.c @@ -2,7 +2,7 @@ #include "esp_netif.h" #include "esp_wifi.h" -TEST_CASE("esp_netif: init and destroy", "[esp_netif][leaks=0]") +TEST_CASE("esp_netif: init and destroy", "[esp_netif]") { esp_netif_config_t cfg = ESP_NETIF_DEFAULT_WIFI_STA(); esp_netif_t *esp_netif = esp_netif_new(NULL); @@ -36,12 +36,15 @@ TEST_CASE("esp_netif: get from if_key", "[esp_netif][leaks=0]") TEST_CASE("esp_netif: create and delete multiple netifs", "[esp_netif][leaks=0]") { - const int nr_of_netifs = 10; + // interface key has to be a unique identifier + const char* if_keys[] = { "if1", "if2", "if3", "if4", "if5", "if6", "if7", "if8", "if9" }; + const int nr_of_netifs = sizeof(if_keys)/sizeof(char*); esp_netif_t *netifs[nr_of_netifs]; - esp_netif_config_t cfg = ESP_NETIF_DEFAULT_WIFI_STA(); // create 10 wifi stations for (int i=0; iwifi_if == WIFI_IF_AP);