From b7c3f01ac8b5a38062c9b129e2c4c7b7242bbf61 Mon Sep 17 00:00:00 2001 From: Darian Leung Date: Wed, 22 Nov 2023 01:35:54 +0800 Subject: [PATCH] change(usb/host): Remove some handler function event flags This commit removes internal event flags in the USB Host Library event handling functions (i.e., usb_host_lib_handle_events() and usb_host_client_handle_events()). Previously, these flags were added to reduce the number of times semaphores were given. However, these flags were removed as the performance gain is negligible and made the logic more complicated. For usb_host_client_handle_events(), the following flags were removed: - Remove 'events_pending' flag. The semaphore is now always given - Remove 'blocked' flag. The 'handling_events' flag is already sufficient - Critical sections are now shortened due to simplication of semaphore usage. For usb_host_lib_handle_events(), the following flags were removed: - Remove 'process_pending' flag. The semaphore is now always given - Renamed 'blocked' flag to 'handling_events' --- components/usb/usb_host.c | 162 ++++++++++++++++---------------------- 1 file changed, 70 insertions(+), 92 deletions(-) diff --git a/components/usb/usb_host.c b/components/usb/usb_host.c index 88649d2fec..cfea392a43 100644 --- a/components/usb/usb_host.c +++ b/components/usb/usb_host.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -94,11 +94,9 @@ struct client_s { TAILQ_HEAD(tailhead_done_ctrl_xfers, urb_s) done_ctrl_xfer_tailq; union { struct { - uint32_t events_pending: 1; uint32_t handling_events: 1; - uint32_t blocked: 1; uint32_t taking_mux: 1; - uint32_t reserved4: 4; + uint32_t reserved6: 6; uint32_t num_intf_claimed: 8; uint32_t reserved16: 16; }; @@ -128,10 +126,8 @@ typedef struct { uint32_t lib_event_flags; union { struct { - uint32_t process_pending: 1; uint32_t handling_events: 1; - uint32_t blocked: 1; - uint32_t reserved5: 5; + uint32_t reserved7: 7; uint32_t num_clients: 8; uint32_t reserved16: 16; }; @@ -176,24 +172,16 @@ static inline bool _check_client_opened_device(client_t *client_obj, uint8_t dev static bool _unblock_client(client_t *client_obj, bool in_isr) { - bool send_sem; - if (!client_obj->dynamic.flags.events_pending && !client_obj->dynamic.flags.handling_events) { - client_obj->dynamic.flags.events_pending = 1; - send_sem = true; - } else { - send_sem = false; - } + bool yield; HOST_EXIT_CRITICAL_SAFE(); - bool yield = false; - if (send_sem) { - if (in_isr) { - BaseType_t xTaskWoken = pdFALSE; - xSemaphoreGiveFromISR(client_obj->constant.event_sem, &xTaskWoken); - yield = (xTaskWoken == pdTRUE); - } else { - xSemaphoreGive(client_obj->constant.event_sem); - } + if (in_isr) { + BaseType_t xTaskWoken = pdFALSE; + xSemaphoreGiveFromISR(client_obj->constant.event_sem, &xTaskWoken); + yield = (xTaskWoken == pdTRUE); + } else { + xSemaphoreGive(client_obj->constant.event_sem); + yield = false; } HOST_ENTER_CRITICAL_SAFE(); @@ -202,24 +190,16 @@ static bool _unblock_client(client_t *client_obj, bool in_isr) static bool _unblock_lib(bool in_isr) { - bool send_sem; - if (!p_host_lib_obj->dynamic.flags.process_pending && !p_host_lib_obj->dynamic.flags.handling_events) { - p_host_lib_obj->dynamic.flags.process_pending = 1; - send_sem = true; - } else { - send_sem = false; - } + bool yield; HOST_EXIT_CRITICAL_SAFE(); - bool yield = false; - if (send_sem) { - if (in_isr) { - BaseType_t xTaskWoken = pdFALSE; - xSemaphoreGiveFromISR(p_host_lib_obj->constant.event_sem, &xTaskWoken); - yield = (xTaskWoken == pdTRUE); - } else { - xSemaphoreGive(p_host_lib_obj->constant.event_sem); - } + if (in_isr) { + BaseType_t xTaskWoken = pdFALSE; + xSemaphoreGiveFromISR(p_host_lib_obj->constant.event_sem, &xTaskWoken); + yield = (xTaskWoken == pdTRUE); + } else { + xSemaphoreGive(p_host_lib_obj->constant.event_sem); + yield = false; } HOST_ENTER_CRITICAL_SAFE(); @@ -515,45 +495,49 @@ esp_err_t usb_host_uninstall(void) esp_err_t usb_host_lib_handle_events(TickType_t timeout_ticks, uint32_t *event_flags_ret) { - esp_err_t ret; - uint32_t event_flags = 0; + // Check arguments and state + HOST_CHECK(p_host_lib_obj != NULL, ESP_ERR_INVALID_STATE); + + esp_err_t ret = (timeout_ticks == 0) ? ESP_OK : ESP_ERR_TIMEOUT; // We don't want to return ESP_ERR_TIMEOUT if we aren't blocking + uint32_t event_flags; HOST_ENTER_CRITICAL(); - if (!p_host_lib_obj->dynamic.flags.process_pending) { - // There is currently processing that needs to be done. Wait for some processing - HOST_EXIT_CRITICAL(); - BaseType_t sem_ret = xSemaphoreTake(p_host_lib_obj->constant.event_sem, timeout_ticks); - if (sem_ret == pdFALSE) { - ret = ESP_ERR_TIMEOUT; - goto exit; - } - HOST_ENTER_CRITICAL(); - } - // Read and clear process pending flags - uint32_t process_pending_flags = p_host_lib_obj->dynamic.process_pending_flags; - p_host_lib_obj->dynamic.process_pending_flags = 0; + // Set handling_events flag. This prevents the host library from being uninstalled p_host_lib_obj->dynamic.flags.handling_events = 1; - while (process_pending_flags) { + HOST_EXIT_CRITICAL(); + + while (1) { + // Loop until there are no more events + if (xSemaphoreTake(p_host_lib_obj->constant.event_sem, timeout_ticks) == pdFALSE) { + // Timed out waiting for semaphore or currently no events + break; + } + + // Read and clear process pending flags + HOST_ENTER_CRITICAL(); + uint32_t process_pending_flags = p_host_lib_obj->dynamic.process_pending_flags; + p_host_lib_obj->dynamic.process_pending_flags = 0; HOST_EXIT_CRITICAL(); + if (process_pending_flags & PROCESS_REQUEST_PENDING_FLAG_USBH) { ESP_ERROR_CHECK(usbh_process()); } if (process_pending_flags & PROCESS_REQUEST_PENDING_FLAG_HUB) { ESP_ERROR_CHECK(hub_process()); } - HOST_ENTER_CRITICAL(); - // Read and clear process pending flags again, and loop back if there is more to process - process_pending_flags = p_host_lib_obj->dynamic.process_pending_flags; - p_host_lib_obj->dynamic.process_pending_flags = 0; + + ret = ESP_OK; + // Set timeout_ticks to 0 so that we can check for events again without blocking + timeout_ticks = 0; } - p_host_lib_obj->dynamic.flags.process_pending = 0; + + HOST_ENTER_CRITICAL(); p_host_lib_obj->dynamic.flags.handling_events = 0; + // Read and clear any event flags event_flags = p_host_lib_obj->dynamic.lib_event_flags; p_host_lib_obj->dynamic.lib_event_flags = 0; HOST_EXIT_CRITICAL(); - ret = ESP_OK; -exit: if (event_flags_ret != NULL) { *event_flags_ret = event_flags; } @@ -709,7 +693,6 @@ esp_err_t usb_host_client_deregister(usb_host_client_handle_t client_hdl) !TAILQ_EMPTY(&client_obj->dynamic.idle_ep_tailq) || !TAILQ_EMPTY(&client_obj->dynamic.done_ctrl_xfer_tailq) || client_obj->dynamic.flags.handling_events || - client_obj->dynamic.flags.blocked || client_obj->dynamic.flags.taking_mux || client_obj->dynamic.flags.num_intf_claimed != 0 || client_obj->dynamic.num_done_ctrl_xfer != 0 || @@ -746,28 +729,26 @@ exit: esp_err_t usb_host_client_handle_events(usb_host_client_handle_t client_hdl, TickType_t timeout_ticks) { + // Check arguments and state HOST_CHECK(client_hdl != NULL, ESP_ERR_INVALID_ARG); - esp_err_t ret; + HOST_CHECK(p_host_lib_obj != NULL, ESP_ERR_INVALID_STATE); + + esp_err_t ret = (timeout_ticks == 0) ? ESP_OK : ESP_ERR_TIMEOUT; // We don't want to return ESP_ERR_TIMEOUT if we aren't blocking client_t *client_obj = (client_t *)client_hdl; HOST_ENTER_CRITICAL(); - if (!client_obj->dynamic.flags.events_pending) { - // There are currently no events, wait for one to occur - client_obj->dynamic.flags.blocked = 1; - HOST_EXIT_CRITICAL(); - BaseType_t sem_ret = xSemaphoreTake(client_obj->constant.event_sem, timeout_ticks); - HOST_ENTER_CRITICAL(); - client_obj->dynamic.flags.blocked = 0; - if (sem_ret == pdFALSE) { - HOST_EXIT_CRITICAL(); - // Timed out waiting for semaphore - ret = ESP_ERR_TIMEOUT; - goto exit; - } - } - // Mark that we're processing events + // Set handling_events flag. This prevents the client from being deregistered client_obj->dynamic.flags.handling_events = 1; - while (client_obj->dynamic.flags.handling_events) { + HOST_EXIT_CRITICAL(); + + while (1) { + // Loop until there are no more events + if (xSemaphoreTake(client_obj->constant.event_sem, timeout_ticks) == pdFALSE) { + // Timed out waiting for semaphore or currently no events + break; + } + + HOST_ENTER_CRITICAL(); // Handle pending endpoints if (!TAILQ_EMPTY(&client_obj->dynamic.pending_ep_tailq)) { _handle_pending_ep(client_obj); @@ -784,29 +765,26 @@ esp_err_t usb_host_client_handle_events(usb_host_client_handle_t client_hdl, Tic urb->transfer.callback(&urb->transfer); HOST_ENTER_CRITICAL(); } + HOST_EXIT_CRITICAL(); + // Handle event messages while (uxQueueMessagesWaiting(client_obj->constant.event_msg_queue) > 0) { - HOST_EXIT_CRITICAL(); // Dequeue the event message and call the client event callback usb_host_client_event_msg_t event_msg; BaseType_t queue_ret = xQueueReceive(client_obj->constant.event_msg_queue, &event_msg, 0); assert(queue_ret == pdTRUE); client_obj->constant.event_callback(&event_msg, client_obj->constant.callback_arg); - HOST_ENTER_CRITICAL(); - } - // Check each event again to see any new events occurred - if (TAILQ_EMPTY(&client_obj->dynamic.pending_ep_tailq) && - client_obj->dynamic.num_done_ctrl_xfer == 0 && - uxQueueMessagesWaiting(client_obj->constant.event_msg_queue) == 0) { - // All pending endpoints and event messages handled - client_obj->dynamic.flags.events_pending = 0; - client_obj->dynamic.flags.handling_events = 0; } + + ret = ESP_OK; + // Set timeout_ticks to 0 so that we can check for events again without blocking + timeout_ticks = 0; } + + HOST_ENTER_CRITICAL(); + client_obj->dynamic.flags.handling_events = 0; HOST_EXIT_CRITICAL(); - ret = ESP_OK; -exit: return ret; }