Revert "Merge branch 'bugfix/uart_vfs_select_threadsafe' into 'master'"

This reverts merge request !25389

components/vfs/test_apps/main/test_vfs_select.c

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2018-2023 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2018-2022 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -101,23 +101,6 @@ static void uart1_init(void)
     uart_param_config(UART_NUM_1, &uart_config);
 }
 
-static void read_task(void *param)
-{
-    char recv_message[sizeof(message)];
-    const test_task_param_t *test_task_param = param;
-    vTaskDelay(test_task_param->delay_ms / portTICK_PERIOD_MS);
-    read(test_task_param->fd, recv_message, sizeof(message));
-    if (test_task_param->sem) {
-        xSemaphoreGive(test_task_param->sem);
-    }
-    vTaskDelete(NULL);
-}
-
-static inline void start_read_task(const test_task_param_t *test_task_param)
-{
-    xTaskCreate(read_task, "read_task", 8*1024, (void *) test_task_param, 5, NULL);
-}
-
 static void send_task(void *param)
 {
     const test_task_param_t *test_task_param = param;
@@ -129,7 +112,7 @@ static void send_task(void *param)
     vTaskDelete(NULL);
 }
 
-static inline void start_write_task(const test_task_param_t *test_task_param)
+static inline void start_task(const test_task_param_t *test_task_param)
 {
     xTaskCreate(send_task, "send_task", 8*1024, (void *) test_task_param, 5, NULL);
 }
@@ -182,7 +165,7 @@ TEST_CASE("UART can do select()", "[vfs]")
         .sem = xSemaphoreCreateBinary(),
     };
     TEST_ASSERT_NOT_NULL(test_task_param.sem);
-    start_write_task(&test_task_param);
+    start_task(&test_task_param);
 
     int s = select(uart_fd + 1, &rfds, NULL, NULL, &tv);
     TEST_ASSERT_EQUAL(s, 1);
@@ -199,7 +182,7 @@ TEST_CASE("UART can do select()", "[vfs]")
     FD_SET(uart_fd, &rfds);
     FD_SET(socket_fd, &rfds);
 
-    start_write_task(&test_task_param);
+    start_task(&test_task_param);
 
     s = select(MAX(uart_fd, socket_fd) + 1, &rfds, NULL, NULL, &tv);
     TEST_ASSERT_EQUAL(s, 1);
@@ -240,7 +223,7 @@ TEST_CASE("UART can do poll() with POLLIN event", "[vfs]")
         .sem = xSemaphoreCreateBinary(),
     };
     TEST_ASSERT_NOT_NULL(test_task_param.sem);
-    start_write_task(&test_task_param);
+    start_task(&test_task_param);
 
     int s = poll(poll_fds, sizeof(poll_fds)/sizeof(poll_fds[0]), 100);
     TEST_ASSERT_EQUAL(s, 1);
@@ -258,7 +241,7 @@ TEST_CASE("UART can do poll() with POLLIN event", "[vfs]")
     poll_fds[1].fd = socket_fd;
     poll_fds[1].events = POLLIN;
 
-    start_write_task(&test_task_param);
+    start_task(&test_task_param);
 
     s = poll(poll_fds, sizeof(poll_fds)/sizeof(poll_fds[0]), 100);
     TEST_ASSERT_EQUAL(s, 1);
@@ -301,7 +284,7 @@ TEST_CASE("UART can do poll() with POLLOUT event", "[vfs]")
         .sem = xSemaphoreCreateBinary(),
     };
     TEST_ASSERT_NOT_NULL(test_task_param.sem);
-    start_write_task(&test_task_param);
+    start_task(&test_task_param);
 
     poll(poll_fds, sizeof(poll_fds)/sizeof(poll_fds[0]), 100);
     TEST_ASSERT_EQUAL(uart_fd, poll_fds[0].fd);
@@ -347,7 +330,7 @@ TEST_CASE("socket can do select()", "[vfs]")
         .sem = xSemaphoreCreateBinary(),
     };
     TEST_ASSERT_NOT_NULL(test_task_param.sem);
-    start_write_task(&test_task_param);
+    start_task(&test_task_param);
 
     const int s = select(MAX(MAX(uart_fd, socket_fd), dummy_socket_fd) + 1, &rfds, NULL, NULL, &tv);
     TEST_ASSERT_EQUAL(1, s);
@@ -396,7 +379,7 @@ TEST_CASE("socket can do poll()", "[vfs]")
         .sem = xSemaphoreCreateBinary(),
     };
     TEST_ASSERT_NOT_NULL(test_task_param.sem);
-    start_write_task(&test_task_param);
+    start_task(&test_task_param);
 
     int s = poll(poll_fds, sizeof(poll_fds)/sizeof(poll_fds[0]), 100);
     TEST_ASSERT_EQUAL(s, 1);
@@ -491,7 +474,8 @@ static void select_task(void *task_param)
 {
     const test_select_task_param_t *param = task_param;
 
-    select(param->maxfds, param->rdfds, param->wrfds, param->errfds, param->tv);
+    int s = select(param->maxfds, param->rdfds, param->wrfds, param->errfds, param->tv);
+    TEST_ASSERT_EQUAL(param->select_ret, s);
 
     if (param->sem) {
         xSemaphoreGive(param->sem);
@@ -505,74 +489,6 @@ static void inline start_select_task(test_select_task_param_t *param)
 }
 
 #if !CONFIG_IDF_TARGET_ESP32H2 // IDF-6782
-TEST_CASE("concurrent selects work for UART", "[vfs]")
-{
-    // This test case initiates two select tasks on the same UART FD,
-    // One task will wait for a write operation, while the other will wait for a read operation to occur.
-    // The first task will complete its operation before the second task proceeds with its operation on the same FD
-    // In this scenario, the write operation will be performed initially,
-    // followed by the subsequent continuation of the read operation.
-
-    int uart_fd, socket_fd;
-    init(&uart_fd, &socket_fd);
-
-    const test_task_param_t send_param = {
-        .fd = uart_fd,
-        .delay_ms = 0,
-        .sem = NULL,
-    };
-
-    fd_set wrfds1;
-    FD_ZERO(&wrfds1);
-    FD_SET(uart_fd, &wrfds1);
-
-    test_select_task_param_t param_write = {
-        .rdfds = NULL,
-        .wrfds = &wrfds1,
-        .errfds = NULL,
-        .maxfds = uart_fd + 1,
-        .tv = NULL,
-        .select_ret = 1,
-        .sem = xSemaphoreCreateBinary(),
-    };
-    TEST_ASSERT_NOT_NULL(param_write.sem);
-    //Start first task which will wait on select call for write operation on the UART FD
-    start_select_task(&param_write);
-
-    fd_set rdfds2;
-    FD_ZERO(&rdfds2);
-    FD_SET(uart_fd, &rdfds2);
-
-    test_select_task_param_t param_read = {
-        .rdfds = &rdfds2,
-        .wrfds = NULL,
-        .errfds = NULL,
-        .maxfds = uart_fd + 1,
-        .tv = NULL,
-        .select_ret = 2,
-        .sem = xSemaphoreCreateBinary(),
-    };
-    TEST_ASSERT_NOT_NULL(param_read.sem);
-    //Start second task which will wait on another select call for read operation on the same UART FD
-    start_select_task(&param_read);
-
-    //Start writing operation on the UART port
-    start_write_task(&send_param);
-    //Confirm the completion of the write operation
-    TEST_ASSERT_EQUAL(pdTRUE, xSemaphoreTake(param_write.sem, 1000 / portTICK_PERIOD_MS));
-    vSemaphoreDelete(param_write.sem);
-    TEST_ASSERT(FD_ISSET(uart_fd, &wrfds1));
-
-    //Start reading operation on the same UART port
-    start_read_task(&send_param);
-    //Confirm the completion of the read operation
-    TEST_ASSERT_EQUAL(pdTRUE, xSemaphoreTake(param_read.sem, 1000 / portTICK_PERIOD_MS));
-    vSemaphoreDelete(param_read.sem);
-    TEST_ASSERT(FD_ISSET(uart_fd, &rdfds2));
-
-    deinit(uart_fd, socket_fd);
-}
-
 TEST_CASE("concurrent selects work", "[vfs]")
 {
     int uart_fd, socket_fd;
@@ -648,7 +564,7 @@ TEST_CASE("concurrent selects work", "[vfs]")
             .sem = xSemaphoreCreateBinary(),
         };
         TEST_ASSERT_NOT_NULL(send_param.sem);
-        start_write_task(&send_param);        // This task will write to UART which will be detected by select()
+        start_task(&send_param);        // This task will write to UART which will be detected by select()
         start_select_task(&param);
         vTaskDelay(100 / portTICK_PERIOD_MS); //make sure the task has started and waits in select()
 

components/vfs/vfs_uart.c

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2021 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -21,7 +21,6 @@
 #include "esp_rom_uart.h"
 #include "soc/soc_caps.h"
 #include "hal/uart_ll.h"
-#include "freertos/semphr.h"
 
 #define UART_NUM SOC_UART_HP_NUM
 
@@ -56,7 +55,6 @@ static int uart_rx_char(int fd);
 // Functions for sending and receiving bytes which use UART driver
 static void uart_tx_char_via_driver(int fd, int c);
 static int uart_rx_char_via_driver(int fd);
-static SemaphoreHandle_t uart_select_mutex[UART_NUM];
 
 typedef struct {
     // Pointers to UART peripherals
@@ -152,8 +150,6 @@ static int uart_open(const char *path, int flags, int mode)
         return -1;
     }
 
-    uart_select_mutex[fd] = xSemaphoreCreateMutex();
-    xSemaphoreGive(uart_select_mutex[fd]);
     s_ctx[fd]->non_blocking = ((flags & O_NONBLOCK) == O_NONBLOCK);
 
     return fd;
@@ -302,7 +298,6 @@ static int uart_fstat(int fd, struct stat * st)
 static int uart_close(int fd)
 {
     assert(fd >=0 && fd < 3);
-    vSemaphoreDelete(uart_select_mutex[fd]);
     return 0;
 }
 
@@ -442,7 +437,6 @@ static esp_err_t uart_start_select(int nfds, fd_set *readfds, fd_set *writefds,
         esp_vfs_select_sem_t select_sem, void **end_select_args)
 {
     const int max_fds = MIN(nfds, UART_NUM);
-    int fd = -1;
     *end_select_args = NULL;
 
     for (int i = 0; i < max_fds; ++i) {
@@ -450,7 +444,6 @@ static esp_err_t uart_start_select(int nfds, fd_set *readfds, fd_set *writefds,
             if (!uart_is_driver_installed(i)) {
                 return ESP_ERR_INVALID_STATE;
             }
-            fd = i;
         }
     }
 
@@ -471,11 +464,14 @@ static esp_err_t uart_start_select(int nfds, fd_set *readfds, fd_set *writefds,
     FD_ZERO(writefds);
     FD_ZERO(exceptfds);
 
-    xSemaphoreTake(uart_select_mutex[fd], portMAX_DELAY);
     portENTER_CRITICAL(uart_get_selectlock());
 
     //uart_set_select_notif_callback sets the callbacks in UART ISR
-    uart_set_select_notif_callback(fd, select_notif_callback_isr);
+    for (int i = 0; i < max_fds; ++i) {
+        if (FD_ISSET(i, &args->readfds_orig) || FD_ISSET(i, &args->writefds_orig) || FD_ISSET(i, &args->errorfds_orig)) {
+            uart_set_select_notif_callback(i, select_notif_callback_isr);
+        }
+    }
 
     for (int i = 0; i < max_fds; ++i) {
         if (FD_ISSET(i, &args->readfds_orig)) {
@@ -504,23 +500,17 @@ static esp_err_t uart_start_select(int nfds, fd_set *readfds, fd_set *writefds,
 static esp_err_t uart_end_select(void *end_select_args)
 {
     uart_select_args_t *args = end_select_args;
-    int fd = -1;
 
     portENTER_CRITICAL(uart_get_selectlock());
     esp_err_t ret = unregister_select(args);
     for (int i = 0; i < UART_NUM; ++i) {
-        if (FD_ISSET(i, &args->readfds_orig) || FD_ISSET(i, &args->writefds_orig) || FD_ISSET(i, &args->errorfds_orig)) {
-            uart_set_select_notif_callback(i, NULL);
-            fd = i;
-            break;
-        }
+        uart_set_select_notif_callback(i, NULL);
     }
     portEXIT_CRITICAL(uart_get_selectlock());
 
     if (args) {
         free(args);
     }
-    xSemaphoreGive(uart_select_mutex[fd]);
 
     return ret;
 }