From 2df9fb057dc169d3c2ed2aed5946e19458848145 Mon Sep 17 00:00:00 2001 From: Roland Dobai Date: Wed, 20 Mar 2019 15:43:31 +0100 Subject: [PATCH] VFS: Allocate socket select semaphore outside ISR --- components/lwip/port/esp32/vfs_lwip.c | 17 ++++++--- components/vfs/include/esp_vfs.h | 26 ++++++++++---- components/vfs/vfs.c | 51 ++++++++++++++------------- components/vfs/vfs_uart.c | 18 +++++----- 4 files changed, 67 insertions(+), 45 deletions(-) diff --git a/components/lwip/port/esp32/vfs_lwip.c b/components/lwip/port/esp32/vfs_lwip.c index 3610d2e17..bb0b3deb5 100644 --- a/components/lwip/port/esp32/vfs_lwip.c +++ b/components/lwip/port/esp32/vfs_lwip.c @@ -28,18 +28,26 @@ _Static_assert(MAX_FDS >= CONFIG_LWIP_MAX_SOCKETS, "MAX_FDS < CONFIG_LWIP_MAX_SOCKETS"); -static void lwip_stop_socket_select() +static void lwip_stop_socket_select(void *sem) { - sys_sem_signal(sys_thread_sem_get()); //socket_select will return + sys_sem_signal(sem); //socket_select will return } -static void lwip_stop_socket_select_isr(BaseType_t *woken) +static void lwip_stop_socket_select_isr(void *sem, BaseType_t *woken) { - if (sys_sem_signal_isr(sys_thread_sem_get()) && woken) { + if (sys_sem_signal_isr(sem) && woken) { *woken = pdTRUE; } } +static void *lwip_get_socket_select_semaphore() +{ + /* Calling this from the same process as select() will ensure that the semaphore won't be allocated from + * ISR (lwip_stop_socket_select_isr). + */ + return (void *) sys_thread_sem_get(); +} + static int lwip_fcntl_r_wrapper(int fd, int cmd, int arg) { return lwip_fcntl_r(fd, cmd, arg); @@ -62,6 +70,7 @@ void esp_vfs_lwip_sockets_register() .fcntl = &lwip_fcntl_r_wrapper, .ioctl = &lwip_ioctl_r_wrapper, .socket_select = &lwip_select, + .get_socket_select_semaphore = &lwip_get_socket_select_semaphore, .stop_socket_select = &lwip_stop_socket_select, .stop_socket_select_isr = &lwip_stop_socket_select_isr, }; diff --git a/components/vfs/include/esp_vfs.h b/components/vfs/include/esp_vfs.h index 49387dc5f..1d8abd8b3 100644 --- a/components/vfs/include/esp_vfs.h +++ b/components/vfs/include/esp_vfs.h @@ -66,6 +66,16 @@ extern "C" { */ typedef int esp_vfs_id_t; +/** + * @brief VFS semaphore type for select() + * + */ +typedef struct +{ + bool is_sem_local; /*!< type of "sem" is SemaphoreHandle_t when true, defined by socket driver otherwise */ + void *sem; /*!< semaphore instance */ +} esp_vfs_select_sem_t; + /** * @brief VFS definition structure * @@ -218,14 +228,16 @@ typedef struct #endif // CONFIG_SUPPORT_TERMIOS /** 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, SemaphoreHandle_t *signal_sem); + esp_err_t (*start_select)(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds, esp_vfs_select_sem_t 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 */ - void (*stop_socket_select)(); + void (*stop_socket_select)(void *sem); /** stop_socket_select which can be called from ISR; set only for the socket driver */ - void (*stop_socket_select_isr)(BaseType_t *woken); + void (*stop_socket_select_isr)(void *sem, BaseType_t *woken); /** end_select is called to stop the I/O multiplexing and deinitialize the environment created by start_select for the given VFS */ + void* (*get_socket_select_semaphore)(); + /** get_socket_select_semaphore returns semaphore allocated in the socket driver; set only for the socket driver */ void (*end_select)(); } esp_vfs_t; @@ -371,9 +383,9 @@ 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 + * @param sem semaphore structure which was passed to the driver by the start_select call */ -void esp_vfs_select_triggered(SemaphoreHandle_t *signal_sem); +void esp_vfs_select_triggered(esp_vfs_select_sem_t sem); /** * @brief Notification from a VFS driver about a read/write/error condition (ISR version) @@ -381,10 +393,10 @@ void esp_vfs_select_triggered(SemaphoreHandle_t *signal_sem); * 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 sem semaphore structure 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(SemaphoreHandle_t *signal_sem, BaseType_t *woken); +void esp_vfs_select_triggered_isr(esp_vfs_select_sem_t sem, BaseType_t *woken); /** * @brief Implements the VFS layer for synchronous I/O multiplexing by poll() diff --git a/components/vfs/vfs.c b/components/vfs/vfs.c index 61a8f7065..c64d831f7 100644 --- a/components/vfs/vfs.c +++ b/components/vfs/vfs.c @@ -824,6 +824,11 @@ int esp_vfs_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *errorfds return -1; } + esp_vfs_select_sem_t sel_sem = { + .is_sem_local = false, + .sem = NULL, + }; + int (*socket_select)(int, fd_set *, fd_set *, fd_set *, struct timeval *) = NULL; for (int fd = 0; fd < nfds; ++fd) { _lock_acquire(&s_fd_table_lock); @@ -844,6 +849,7 @@ int esp_vfs_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *errorfds esp_vfs_safe_fd_isset(fd, errorfds)) { const vfs_entry_t *vfs = s_vfs[vfs_index]; socket_select = vfs->vfs.socket_select; + sel_sem.sem = vfs->vfs.get_socket_select_semaphore(); } } continue; @@ -874,19 +880,14 @@ 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 ((select_sem = xSemaphoreCreateBinary()) == NULL) { + sel_sem.is_sem_local = true; + if ((sel_sem.sem = xSemaphoreCreateBinary()) == NULL) { free(vfs_fds_triple); __errno_r(r) = ENOMEM; - ESP_LOGD(TAG, "cannot create select_sem"); + ESP_LOGD(TAG, "cannot create select semaphore"); return -1; } } @@ -902,18 +903,18 @@ 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, &select_sem); + esp_err_t err = vfs->vfs.start_select(nfds, &item->readfds, &item->writefds, &item->errorfds, sel_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 (select_sem) { - vSemaphoreDelete(select_sem); - select_sem = NULL; + if (sel_sem.is_sem_local && sel_sem.sem) { + vSemaphoreDelete(sel_sem.sem); + sel_sem.sem = NULL; } free(vfs_fds_triple); __errno_r(r) = EINTR; - ESP_LOGD(TAG, "start_select failed"); + ESP_LOGD(TAG, "start_select failed: %s", esp_err_to_name(err)); return -1; } } @@ -947,16 +948,16 @@ 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(select_sem, ticks_to_wait); + xSemaphoreTake(sel_sem.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 (select_sem) { - vSemaphoreDelete(select_sem); - select_sem = NULL; + if (sel_sem.is_sem_local && sel_sem.sem) { + vSemaphoreDelete(sel_sem.sem); + sel_sem.sem = NULL; } free(vfs_fds_triple); @@ -967,10 +968,10 @@ int esp_vfs_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *errorfds return ret; } -void esp_vfs_select_triggered(SemaphoreHandle_t *signal_sem) +void esp_vfs_select_triggered(esp_vfs_select_sem_t sem) { - if (signal_sem && (*signal_sem)) { - xSemaphoreGive(*signal_sem); + if (sem.is_sem_local) { + xSemaphoreGive(sem.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 @@ -978,17 +979,17 @@ void esp_vfs_select_triggered(SemaphoreHandle_t *signal_sem) for (int i = 0; i < s_vfs_count; ++i) { const vfs_entry_t *vfs = s_vfs[i]; if (vfs != NULL && vfs->vfs.stop_socket_select != NULL) { - vfs->vfs.stop_socket_select(); + vfs->vfs.stop_socket_select(sem.sem); break; } } } } -void esp_vfs_select_triggered_isr(SemaphoreHandle_t *signal_sem, BaseType_t *woken) +void esp_vfs_select_triggered_isr(esp_vfs_select_sem_t sem, BaseType_t *woken) { - if (signal_sem && (*signal_sem)) { - xSemaphoreGiveFromISR(*signal_sem, woken); + if (sem.is_sem_local) { + xSemaphoreGiveFromISR(sem.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 @@ -996,7 +997,7 @@ void esp_vfs_select_triggered_isr(SemaphoreHandle_t *signal_sem, BaseType_t *wok for (int i = 0; i < s_vfs_count; ++i) { const vfs_entry_t *vfs = s_vfs[i]; if (vfs != NULL && vfs->vfs.stop_socket_select_isr != NULL) { - vfs->vfs.stop_socket_select_isr(woken); + vfs->vfs.stop_socket_select_isr(sem.sem, woken); break; } } diff --git a/components/vfs/vfs_uart.c b/components/vfs/vfs_uart.c index 895940a8a..fd0b028d5 100644 --- a/components/vfs/vfs_uart.c +++ b/components/vfs/vfs_uart.c @@ -62,7 +62,7 @@ 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 esp_vfs_select_sem_t _select_sem = {.sem = NULL}; static fd_set *_readfds = NULL; static fd_set *_writefds = NULL; static fd_set *_errorfds = NULL; @@ -319,25 +319,25 @@ 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(_signal_sem, task_woken); + esp_vfs_select_triggered_isr(_select_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(_signal_sem, task_woken); + esp_vfs_select_triggered_isr(_select_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(_signal_sem, task_woken); + esp_vfs_select_triggered_isr(_select_sem, task_woken); } break; } } -static esp_err_t uart_start_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds, SemaphoreHandle_t *signal_sem) +static esp_err_t uart_start_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds, esp_vfs_select_sem_t select_sem) { if (_lock_try_acquire(&s_one_select_lock)) { return ESP_ERR_INVALID_STATE; @@ -347,7 +347,7 @@ static esp_err_t uart_start_select(int nfds, fd_set *readfds, fd_set *writefds, portENTER_CRITICAL(uart_get_selectlock()); - if (_readfds || _writefds || _errorfds || _readfds_orig || _writefds_orig || _errorfds_orig || _signal_sem) { + if (_readfds || _writefds || _errorfds || _readfds_orig || _writefds_orig || _errorfds_orig || _select_sem.sem) { portEXIT_CRITICAL(uart_get_selectlock()); uart_end_select(); return ESP_ERR_INVALID_STATE; @@ -378,7 +378,7 @@ static esp_err_t uart_start_select(int nfds, fd_set *readfds, fd_set *writefds, } } - _signal_sem = signal_sem; + _select_sem = select_sem; _readfds = readfds; _writefds = writefds; @@ -398,7 +398,7 @@ static esp_err_t uart_start_select(int nfds, fd_set *readfds, fd_set *writefds, if (uart_get_buffered_data_len(i, &buffered_size) == ESP_OK && buffered_size > 0) { // signalize immediately when data is buffered FD_SET(i, _readfds); - esp_vfs_select_triggered(_signal_sem); + esp_vfs_select_triggered(_select_sem); } } } @@ -417,7 +417,7 @@ static void uart_end_select() uart_set_select_notif_callback(i, NULL); } - _signal_sem = NULL; + _select_sem.sem = NULL; _readfds = NULL; _writefds = NULL;