Crash_Override/OVMS3-idf
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

VFS: esp_vfs_select() needs to be callable from concurrent tasks

Browse source 
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".
This commit is contained in:
Roland Dobai 2018-05-29 11:01:25 +02:00
parent bdbcc492cf
commit 15b26f8841
4 changed files with 94 additions and 52 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

9
components/vfs/include/esp_vfs.h
View file

@ -175,7 +175,7 @@ typedef struct
int (*access)(const char *path, int amode); 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 */ /** 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 */ /** 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); 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 */ /** 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 * 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. * 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) * @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 * 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. * 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 * @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 #ifdef __cplusplus
} // extern "C" } // extern "C"

49
components/vfs/test/test_vfs_select.c
View file

@ -275,8 +275,12 @@ static void select_task(void *param)
.tv_usec = 100000, .tv_usec = 100000,
}; };
int s = select(1, NULL, NULL, NULL, &tv); fd_set rfds;
TEST_ASSERT_EQUAL(s, 0); //timeout 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) { if (test_task_param->sem) {
xSemaphoreGive(test_task_param->sem); xSemaphoreGive(test_task_param->sem);
@ -284,23 +288,52 @@ static void select_task(void *param)
vTaskDelete(NULL); vTaskDelete(NULL);
} }
TEST_CASE("concurent select() fails", "[vfs]") TEST_CASE("concurent selects work", "[vfs]")
{ {
struct timeval tv = { struct timeval tv = {
.tv_sec = 0, .tv_sec = 0,
.tv_usec = 100000,//irrelevant .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(), .sem = xSemaphoreCreateBinary(),
}; };
TEST_ASSERT_NOT_NULL(test_task_param.sem); TEST_ASSERT_NOT_NULL(test_task_param.sem);
xTaskCreate(select_task, "select_task", 4*1024, (void *) &test_task_param, 5, NULL); 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() vTaskDelay(10 / portTICK_PERIOD_MS); //make sure the task has started and waits in select()
int s = select(1, NULL, NULL, NULL, &tv); int s = select(uart_fd + 1, &rfds, NULL, NULL, &tv);
TEST_ASSERT_EQUAL(s, -1); //this select should fail because the other one is "waiting" TEST_ASSERT_EQUAL(-1, s); //this select should fail because two selects are accessing UART
TEST_ASSERT_EQUAL(errno, EINTR); //(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); vSemaphoreDelete(test_task_param.sem);
deinit(uart_fd, socket_fd);
close(dummy_socket_fd);
} }

59
components/vfs/vfs.c
View 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 fd_table_t s_fd_table[MAX_FDS] = { [0 ... MAX_FDS-1] = FD_TABLE_ENTRY_UNUSED };
static _lock_t s_fd_table_lock; 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) 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) { 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; 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; fds_triple_t *vfs_fds_triple;
if ((vfs_fds_triple = calloc(s_vfs_count, sizeof(fds_triple_t))) == NULL) { if ((vfs_fds_triple = calloc(s_vfs_count, sizeof(fds_triple_t))) == NULL) {
__errno_r(r) = ENOMEM; __errno_r(r) = ENOMEM;
_lock_release(&s_one_select_lock);
ESP_LOGD(TAG, "calloc is unsuccessful"); ESP_LOGD(TAG, "calloc is unsuccessful");
return -1; 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 // the global readfds, writefds and errorfds contain only socket FDs (if
// there any) // 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) { if (!socket_select) {
// There is no socket VFS registered or select() wasn't called for // There is no socket VFS registered or select() wasn't called for
// any socket. Therefore, we will use our own signalization. // any socket. Therefore, we will use our own signalization.
if ((s_select_sem = xSemaphoreCreateBinary()) == NULL) { if ((select_sem = xSemaphoreCreateBinary()) == NULL) {
free(vfs_fds_triple); free(vfs_fds_triple);
__errno_r(r) = ENOMEM; __errno_r(r) = ENOMEM;
_lock_release(&s_one_select_lock); ESP_LOGD(TAG, "cannot create select_sem");
ESP_LOGD(TAG, "cannot create s_select_sem");
return -1; 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("readfds", &item->readfds);
esp_vfs_log_fd_set("writefds", &item->writefds); esp_vfs_log_fd_set("writefds", &item->writefds);
esp_vfs_log_fd_set("errorfds", &item->errorfds); 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) { if (err != ESP_OK) {
call_end_selects(i, vfs_fds_triple); call_end_selects(i, vfs_fds_triple);
(void) set_global_fd_sets(vfs_fds_triple, s_vfs_count, readfds, writefds, errorfds); (void) set_global_fd_sets(vfs_fds_triple, s_vfs_count, readfds, writefds, errorfds);
if (s_select_sem) { if (select_sem) {
vSemaphoreDelete(s_select_sem); vSemaphoreDelete(select_sem);
s_select_sem = NULL; select_sem = NULL;
} }
free(vfs_fds_triple); free(vfs_fds_triple);
__errno_r(r) = ENOMEM; __errno_r(r) = EINTR;
_lock_release(&s_one_select_lock);
ESP_LOGD(TAG, "start_select failed"); ESP_LOGD(TAG, "start_select failed");
return -1; 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, "timeout is %dms", timeout_ms);
} }
ESP_LOGD(TAG, "waiting without calling socket_select"); 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 call_end_selects(s_vfs_count, vfs_fds_triple); // for VFSs for start_select was called before
if (ret >= 0) { if (ret >= 0) {
ret += set_global_fd_sets(vfs_fds_triple, s_vfs_count, readfds, writefds, errorfds); ret += set_global_fd_sets(vfs_fds_triple, s_vfs_count, readfds, writefds, errorfds);
} }
if (s_select_sem) { if (select_sem) {
vSemaphoreDelete(s_select_sem); vSemaphoreDelete(select_sem);
s_select_sem = NULL; select_sem = NULL;
} }
free(vfs_fds_triple); free(vfs_fds_triple);
_lock_release(&s_one_select_lock);
ESP_LOGD(TAG, "esp_vfs_select returns %d", ret); ESP_LOGD(TAG, "esp_vfs_select returns %d", ret);
esp_vfs_log_fd_set("readfds", readfds); 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; return ret;
} }
void esp_vfs_select_triggered() void esp_vfs_select_triggered(SemaphoreHandle_t *signal_sem)
{ {
if (s_select_sem) { if (signal_sem && (*signal_sem)) {
xSemaphoreGive(s_select_sem); xSemaphoreGive(*signal_sem);
} else { } else {
// Another way would be to go through s_fd_table and find the VFS // 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 // 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) { if (signal_sem && (*signal_sem)) {
xSemaphoreGiveFromISR(s_select_sem, woken); xSemaphoreGiveFromISR(*signal_sem, woken);
} else { } else {
// Another way would be to go through s_fd_table and find the VFS // 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 // which has a permanent FD. But in order to avoid to lock

29
components/vfs/vfs_uart.c
View file

@ -58,6 +58,10 @@ static int s_peek_char[UART_NUM] = { NONE, NONE, NONE };
// driver is used. // driver is used.
static bool s_non_blocking[UART_NUM]; 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 *_readfds = NULL;
static fd_set *_writefds = NULL; static fd_set *_writefds = NULL;
static fd_set *_errorfds = 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: case UART_SELECT_READ_NOTIF:
if (FD_ISSET(uart_num, _readfds_orig)) { if (FD_ISSET(uart_num, _readfds_orig)) {
FD_SET(uart_num, _readfds); FD_SET(uart_num, _readfds);
esp_vfs_select_triggered_isr(task_woken); esp_vfs_select_triggered_isr(_signal_sem, task_woken);
} }
break; break;
case UART_SELECT_WRITE_NOTIF: case UART_SELECT_WRITE_NOTIF:
if (FD_ISSET(uart_num, _writefds_orig)) { if (FD_ISSET(uart_num, _writefds_orig)) {
FD_SET(uart_num, _writefds); FD_SET(uart_num, _writefds);
esp_vfs_select_triggered_isr(task_woken); esp_vfs_select_triggered_isr(_signal_sem, task_woken);
} }
break; break;
case UART_SELECT_ERROR_NOTIF: case UART_SELECT_ERROR_NOTIF:
if (FD_ISSET(uart_num, _errorfds_orig)) { if (FD_ISSET(uart_num, _errorfds_orig)) {
FD_SET(uart_num, _errorfds); FD_SET(uart_num, _errorfds);
esp_vfs_select_triggered_isr(task_woken); esp_vfs_select_triggered_isr(_signal_sem, task_woken);
} }
break; 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); const int max_fds = MIN(nfds, UART_NUM);
taskENTER_CRITICAL(uart_get_selectlock()); 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()); taskEXIT_CRITICAL(uart_get_selectlock());
_lock_release(&s_one_select_lock);
return ESP_ERR_INVALID_STATE; return ESP_ERR_INVALID_STATE;
} }
if ((_readfds_orig = malloc(sizeof(fd_set))) == NULL) { if ((_readfds_orig = malloc(sizeof(fd_set))) == NULL) {
taskEXIT_CRITICAL(uart_get_selectlock()); taskEXIT_CRITICAL(uart_get_selectlock());
_lock_release(&s_one_select_lock);
return ESP_ERR_NO_MEM; return ESP_ERR_NO_MEM;
} }
if ((_writefds_orig = malloc(sizeof(fd_set))) == NULL) { if ((_writefds_orig = malloc(sizeof(fd_set))) == NULL) {
taskEXIT_CRITICAL(uart_get_selectlock()); taskEXIT_CRITICAL(uart_get_selectlock());
_lock_release(&s_one_select_lock);
return ESP_ERR_NO_MEM; return ESP_ERR_NO_MEM;
} }
if ((_errorfds_orig = malloc(sizeof(fd_set))) == NULL) { if ((_errorfds_orig = malloc(sizeof(fd_set))) == NULL) {
taskEXIT_CRITICAL(uart_get_selectlock()); taskEXIT_CRITICAL(uart_get_selectlock());
_lock_release(&s_one_select_lock);
return ESP_ERR_NO_MEM; 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; _readfds = readfds;
_writefds = writefds; _writefds = writefds;
_errorfds = exceptfds; _errorfds = exceptfds;
@ -367,6 +381,8 @@ static esp_err_t uart_start_select(int nfds, fd_set *readfds, fd_set *writefds,
FD_ZERO(exceptfds); FD_ZERO(exceptfds);
taskEXIT_CRITICAL(uart_get_selectlock()); 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; return ESP_OK;
} }
@ -378,6 +394,8 @@ static void uart_end_select()
uart_set_select_notif_callback(i, NULL); uart_set_select_notif_callback(i, NULL);
} }
_signal_sem = NULL;
_readfds = NULL; _readfds = NULL;
_writefds = NULL; _writefds = NULL;
_errorfds = NULL; _errorfds = NULL;
@ -397,6 +415,7 @@ static void uart_end_select()
_errorfds_orig = NULL; _errorfds_orig = NULL;
} }
taskEXIT_CRITICAL(uart_get_selectlock()); taskEXIT_CRITICAL(uart_get_selectlock());
_lock_release(&s_one_select_lock);
} }
void esp_vfs_dev_uart_register() void esp_vfs_dev_uart_register()