From 15b26f8841049d3cb5d50eded25639b2e491781b Mon Sep 17 00:00:00 2001 From: Roland Dobai Date: Tue, 29 May 2018 11:01:25 +0200 Subject: [PATCH] VFS: esp_vfs_select() needs to be callable from concurrent tasks This fix is for compatibility with lwip_select(). It moves the lock to UART which is the only VFS driver which is implemented and is not "thread-safe". --- components/vfs/include/esp_vfs.h | 9 ++-- components/vfs/test/test_vfs_select.c | 49 ++++++++++++++++++---- components/vfs/vfs.c | 59 +++++++++++---------------- components/vfs/vfs_uart.c | 29 ++++++++++--- 4 files changed, 94 insertions(+), 52 deletions(-) diff --git a/components/vfs/include/esp_vfs.h b/components/vfs/include/esp_vfs.h index d06dd223f..61b01b76f 100644 --- a/components/vfs/include/esp_vfs.h +++ b/components/vfs/include/esp_vfs.h @@ -175,7 +175,7 @@ typedef struct int (*access)(const char *path, int amode); }; /** start_select is called for setting up synchronous I/O multiplexing of the desired file descriptors in the given VFS */ - esp_err_t (*start_select)(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds); + esp_err_t (*start_select)(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds, SemaphoreHandle_t *signal_sem); /** socket select function for socket FDs with the functionality of POSIX select(); this should be set only for the socket VFS */ int (*socket_select)(int nfds, fd_set *readfds, fd_set *writefds, fd_set *errorfds, struct timeval *timeout); /** called by VFS to interrupt the socket_select call when select is activated from a non-socket VFS driver; set only for the socket driver */ @@ -326,8 +326,10 @@ int esp_vfs_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *errorfds * * This function is called when the VFS driver detects a read/write/error * condition as it was requested by the previous call to start_select. + * + * @param signal_sem semaphore handle which was passed to the driver by the start_select call */ -void esp_vfs_select_triggered(); +void esp_vfs_select_triggered(SemaphoreHandle_t *signal_sem); /** * @brief Notification from a VFS driver about a read/write/error condition (ISR version) @@ -335,9 +337,10 @@ void esp_vfs_select_triggered(); * This function is called when the VFS driver detects a read/write/error * condition as it was requested by the previous call to start_select. * + * @param signal_sem semaphore handle which was passed to the driver by the start_select call * @param woken is set to pdTRUE if the function wakes up a task with higher priority */ -void esp_vfs_select_triggered_isr(BaseType_t *woken); +void esp_vfs_select_triggered_isr(SemaphoreHandle_t *signal_sem, BaseType_t *woken); #ifdef __cplusplus } // extern "C" diff --git a/components/vfs/test/test_vfs_select.c b/components/vfs/test/test_vfs_select.c index be74ebe91..f9927a54f 100644 --- a/components/vfs/test/test_vfs_select.c +++ b/components/vfs/test/test_vfs_select.c @@ -275,8 +275,12 @@ static void select_task(void *param) .tv_usec = 100000, }; - int s = select(1, NULL, NULL, NULL, &tv); - TEST_ASSERT_EQUAL(s, 0); //timeout + fd_set rfds; + FD_ZERO(&rfds); + FD_SET(test_task_param->fd, &rfds); + + int s = select(test_task_param->fd + 1, &rfds, NULL, NULL, &tv); + TEST_ASSERT_EQUAL(0, s); //timeout if (test_task_param->sem) { xSemaphoreGive(test_task_param->sem); @@ -284,23 +288,52 @@ static void select_task(void *param) vTaskDelete(NULL); } -TEST_CASE("concurent select() fails", "[vfs]") +TEST_CASE("concurent selects work", "[vfs]") { struct timeval tv = { .tv_sec = 0, .tv_usec = 100000,//irrelevant }; - const test_task_param_t test_task_param = { + + int uart_fd, socket_fd; + const int dummy_socket_fd = open_dummy_socket(); + init(&uart_fd, &socket_fd); + + fd_set rfds; + FD_ZERO(&rfds); + FD_SET(uart_fd, &rfds); + + test_task_param_t test_task_param = { + .fd = uart_fd, .sem = xSemaphoreCreateBinary(), }; TEST_ASSERT_NOT_NULL(test_task_param.sem); + xTaskCreate(select_task, "select_task", 4*1024, (void *) &test_task_param, 5, NULL); vTaskDelay(10 / portTICK_PERIOD_MS); //make sure the task has started and waits in select() - int s = select(1, NULL, NULL, NULL, &tv); - TEST_ASSERT_EQUAL(s, -1); //this select should fail because the other one is "waiting" - TEST_ASSERT_EQUAL(errno, EINTR); + int s = select(uart_fd + 1, &rfds, NULL, NULL, &tv); + TEST_ASSERT_EQUAL(-1, s); //this select should fail because two selects are accessing UART + //(the other one is waiting for the timeout) + TEST_ASSERT_EQUAL(EINTR, errno); - TEST_ASSERT_EQUAL(xSemaphoreTake(test_task_param.sem, 1000 / portTICK_PERIOD_MS), pdTRUE); + TEST_ASSERT_EQUAL(pdTRUE, xSemaphoreTake(test_task_param.sem, 1000 / portTICK_PERIOD_MS)); + + FD_ZERO(&rfds); + FD_SET(socket_fd, &rfds); + + test_task_param.fd = dummy_socket_fd; + + xTaskCreate(select_task, "select_task", 4*1024, (void *) &test_task_param, 5, NULL); + vTaskDelay(10 / portTICK_PERIOD_MS); //make sure the task has started and waits in select() + + s = select(socket_fd + 1, &rfds, NULL, NULL, &tv); + TEST_ASSERT_EQUAL(0, s); //this select should timeout as well as the concurrent one because + //concurrent socket select should work + + TEST_ASSERT_EQUAL(pdTRUE, xSemaphoreTake(test_task_param.sem, 1000 / portTICK_PERIOD_MS)); vSemaphoreDelete(test_task_param.sem); + + deinit(uart_fd, socket_fd); + close(dummy_socket_fd); } diff --git a/components/vfs/vfs.c b/components/vfs/vfs.c index cd9a2acec..dfcb07aa8 100644 --- a/components/vfs/vfs.c +++ b/components/vfs/vfs.c @@ -72,15 +72,6 @@ static size_t s_vfs_count = 0; static fd_table_t s_fd_table[MAX_FDS] = { [0 ... MAX_FDS-1] = FD_TABLE_ENTRY_UNUSED }; static _lock_t s_fd_table_lock; -/* Semaphore used for waiting select events from other VFS drivers when socket - * select is not used (not registered or socket FDs are not observed by the - * given call of select) - */ -static SemaphoreHandle_t s_select_sem = NULL; - -/* Lock ensuring that select is called from only one task at the time */ -static _lock_t s_one_select_lock; - static esp_err_t esp_vfs_register_common(const char* base_path, size_t len, const esp_vfs_t* vfs, void* ctx, int *vfs_index) { if (len != LEN_PATH_PREFIX_IGNORED) { @@ -799,16 +790,9 @@ int esp_vfs_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *errorfds return -1; } - if (_lock_try_acquire(&s_one_select_lock)) { - ESP_LOGD(TAG, "concurrent select is not supported"); - __errno_r(r) = EINTR; - return -1; - } - fds_triple_t *vfs_fds_triple; if ((vfs_fds_triple = calloc(s_vfs_count, sizeof(fds_triple_t))) == NULL) { __errno_r(r) = ENOMEM; - _lock_release(&s_one_select_lock); ESP_LOGD(TAG, "calloc is unsuccessful"); return -1; } @@ -863,14 +847,19 @@ int esp_vfs_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *errorfds // the global readfds, writefds and errorfds contain only socket FDs (if // there any) + /* Semaphore used for waiting select events from other VFS drivers when socket + * select is not used (not registered or socket FDs are not observed by the + * given call of select) + */ + SemaphoreHandle_t select_sem = NULL; + if (!socket_select) { // There is no socket VFS registered or select() wasn't called for // any socket. Therefore, we will use our own signalization. - if ((s_select_sem = xSemaphoreCreateBinary()) == NULL) { + if ((select_sem = xSemaphoreCreateBinary()) == NULL) { free(vfs_fds_triple); __errno_r(r) = ENOMEM; - _lock_release(&s_one_select_lock); - ESP_LOGD(TAG, "cannot create s_select_sem"); + ESP_LOGD(TAG, "cannot create select_sem"); return -1; } } @@ -886,18 +875,17 @@ int esp_vfs_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *errorfds esp_vfs_log_fd_set("readfds", &item->readfds); esp_vfs_log_fd_set("writefds", &item->writefds); esp_vfs_log_fd_set("errorfds", &item->errorfds); - esp_err_t err = vfs->vfs.start_select(nfds, &item->readfds, &item->writefds, &item->errorfds); + esp_err_t err = vfs->vfs.start_select(nfds, &item->readfds, &item->writefds, &item->errorfds, &select_sem); if (err != ESP_OK) { call_end_selects(i, vfs_fds_triple); (void) set_global_fd_sets(vfs_fds_triple, s_vfs_count, readfds, writefds, errorfds); - if (s_select_sem) { - vSemaphoreDelete(s_select_sem); - s_select_sem = NULL; + if (select_sem) { + vSemaphoreDelete(select_sem); + select_sem = NULL; } free(vfs_fds_triple); - __errno_r(r) = ENOMEM; - _lock_release(&s_one_select_lock); + __errno_r(r) = EINTR; ESP_LOGD(TAG, "start_select failed"); return -1; } @@ -932,19 +920,18 @@ int esp_vfs_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *errorfds ESP_LOGD(TAG, "timeout is %dms", timeout_ms); } ESP_LOGD(TAG, "waiting without calling socket_select"); - xSemaphoreTake(s_select_sem, ticks_to_wait); + xSemaphoreTake(select_sem, ticks_to_wait); } call_end_selects(s_vfs_count, vfs_fds_triple); // for VFSs for start_select was called before if (ret >= 0) { ret += set_global_fd_sets(vfs_fds_triple, s_vfs_count, readfds, writefds, errorfds); } - if (s_select_sem) { - vSemaphoreDelete(s_select_sem); - s_select_sem = NULL; + if (select_sem) { + vSemaphoreDelete(select_sem); + select_sem = NULL; } free(vfs_fds_triple); - _lock_release(&s_one_select_lock); ESP_LOGD(TAG, "esp_vfs_select returns %d", ret); esp_vfs_log_fd_set("readfds", readfds); @@ -953,10 +940,10 @@ int esp_vfs_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *errorfds return ret; } -void esp_vfs_select_triggered() +void esp_vfs_select_triggered(SemaphoreHandle_t *signal_sem) { - if (s_select_sem) { - xSemaphoreGive(s_select_sem); + if (signal_sem && (*signal_sem)) { + xSemaphoreGive(*signal_sem); } else { // Another way would be to go through s_fd_table and find the VFS // which has a permanent FD. But in order to avoid to lock @@ -971,10 +958,10 @@ void esp_vfs_select_triggered() } } -void esp_vfs_select_triggered_isr(BaseType_t *woken) +void esp_vfs_select_triggered_isr(SemaphoreHandle_t *signal_sem, BaseType_t *woken) { - if (s_select_sem) { - xSemaphoreGiveFromISR(s_select_sem, woken); + if (signal_sem && (*signal_sem)) { + xSemaphoreGiveFromISR(*signal_sem, woken); } else { // Another way would be to go through s_fd_table and find the VFS // which has a permanent FD. But in order to avoid to lock diff --git a/components/vfs/vfs_uart.c b/components/vfs/vfs_uart.c index c1c12066c..4b4762c34 100644 --- a/components/vfs/vfs_uart.c +++ b/components/vfs/vfs_uart.c @@ -58,6 +58,10 @@ static int s_peek_char[UART_NUM] = { NONE, NONE, NONE }; // driver is used. static bool s_non_blocking[UART_NUM]; +/* Lock ensuring that uart_select is used from only one task at the time */ +static _lock_t s_one_select_lock; + +static SemaphoreHandle_t *_signal_sem = NULL; static fd_set *_readfds = NULL; static fd_set *_writefds = NULL; static fd_set *_errorfds = NULL; @@ -303,47 +307,55 @@ static void select_notif_callback(uart_port_t uart_num, uart_select_notif_t uart case UART_SELECT_READ_NOTIF: if (FD_ISSET(uart_num, _readfds_orig)) { FD_SET(uart_num, _readfds); - esp_vfs_select_triggered_isr(task_woken); + esp_vfs_select_triggered_isr(_signal_sem, task_woken); } break; case UART_SELECT_WRITE_NOTIF: if (FD_ISSET(uart_num, _writefds_orig)) { FD_SET(uart_num, _writefds); - esp_vfs_select_triggered_isr(task_woken); + esp_vfs_select_triggered_isr(_signal_sem, task_woken); } break; case UART_SELECT_ERROR_NOTIF: if (FD_ISSET(uart_num, _errorfds_orig)) { FD_SET(uart_num, _errorfds); - esp_vfs_select_triggered_isr(task_woken); + esp_vfs_select_triggered_isr(_signal_sem, task_woken); } break; } } -static esp_err_t uart_start_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds) +static esp_err_t uart_start_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds, SemaphoreHandle_t *signal_sem) { + if (_lock_try_acquire(&s_one_select_lock)) { + return ESP_ERR_INVALID_STATE; + } + const int max_fds = MIN(nfds, UART_NUM); taskENTER_CRITICAL(uart_get_selectlock()); - if (_readfds || _writefds || _errorfds || _readfds_orig || _writefds_orig || _errorfds_orig) { + if (_readfds || _writefds || _errorfds || _readfds_orig || _writefds_orig || _errorfds_orig || _signal_sem) { taskEXIT_CRITICAL(uart_get_selectlock()); + _lock_release(&s_one_select_lock); return ESP_ERR_INVALID_STATE; } if ((_readfds_orig = malloc(sizeof(fd_set))) == NULL) { taskEXIT_CRITICAL(uart_get_selectlock()); + _lock_release(&s_one_select_lock); return ESP_ERR_NO_MEM; } if ((_writefds_orig = malloc(sizeof(fd_set))) == NULL) { taskEXIT_CRITICAL(uart_get_selectlock()); + _lock_release(&s_one_select_lock); return ESP_ERR_NO_MEM; } if ((_errorfds_orig = malloc(sizeof(fd_set))) == NULL) { taskEXIT_CRITICAL(uart_get_selectlock()); + _lock_release(&s_one_select_lock); return ESP_ERR_NO_MEM; } @@ -354,6 +366,8 @@ static esp_err_t uart_start_select(int nfds, fd_set *readfds, fd_set *writefds, } } + _signal_sem = signal_sem; + _readfds = readfds; _writefds = writefds; _errorfds = exceptfds; @@ -367,6 +381,8 @@ static esp_err_t uart_start_select(int nfds, fd_set *readfds, fd_set *writefds, FD_ZERO(exceptfds); taskEXIT_CRITICAL(uart_get_selectlock()); + // s_one_select_lock is not released on successfull exit - will be + // released in uart_end_select() return ESP_OK; } @@ -378,6 +394,8 @@ static void uart_end_select() uart_set_select_notif_callback(i, NULL); } + _signal_sem = NULL; + _readfds = NULL; _writefds = NULL; _errorfds = NULL; @@ -397,6 +415,7 @@ static void uart_end_select() _errorfds_orig = NULL; } taskEXIT_CRITICAL(uart_get_selectlock()); + _lock_release(&s_one_select_lock); } void esp_vfs_dev_uart_register()