]> granicus.if.org Git - esp-idf/commitdiff
VFS: esp_vfs_select() needs to be callable from concurrent tasks
authorRoland Dobai <dobai.roland@gmail.com>
Tue, 29 May 2018 09:01:25 +0000 (11:01 +0200)
committerRoland Dobai <dobai.roland@gmail.com>
Tue, 29 May 2018 09:01:25 +0000 (11:01 +0200)
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
components/vfs/test/test_vfs_select.c
components/vfs/vfs.c
components/vfs/vfs_uart.c

index d06dd223fc702deb28e714802b12f6415381f964..61b01b76f1ccfdeeece463577f406b422d6d9cb5 100644 (file)
@@ -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"
index be74ebe910173f96bef728a5e4716fa388ea4188..f9927a54f6042e0a0d6dd2beda6d94a7a88d715c 100644 (file)
@@ -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);
 }
index cd9a2aceca466bb5e14f325076a182d782698a60..dfcb07aa8945715a89c4cfa30d5c02ad5fe52495 100644 (file)
@@ -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
index c1c12066cbaebbac7b28f6fbd65cc14f6598b815..4b4762c344fa169c86c014a2e817073ec5725fbf 100644 (file)
@@ -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()