From d9de61cbadc4f0aa65b32fcbad2b547d2e746857 Mon Sep 17 00:00:00 2001 From: Darian Leung Date: Wed, 22 Nov 2023 00:47:38 +0800 Subject: [PATCH 1/2] change(usb/host): Remove data buffer headers from URBs This commit removes the ability to reserve a header in the data buffer of an allocated URB. The header was required for a now defunct implementation of a synchronous USB Host library API. Thus, headers are no longer required in URB data buffers. --- components/usb/hub.c | 2 +- components/usb/private_include/usb_private.h | 9 +++------ components/usb/usb_host.c | 2 +- components/usb/usb_private.c | 13 ++++++------- 4 files changed, 11 insertions(+), 15 deletions(-) diff --git a/components/usb/hub.c b/components/usb/hub.c index 519890d135..7f24a60a6d 100644 --- a/components/usb/hub.c +++ b/components/usb/hub.c @@ -938,7 +938,7 @@ esp_err_t hub_install(hub_config_t *hub_config) HUB_DRIVER_EXIT_CRITICAL(); // Allocate Hub driver object hub_driver_t *hub_driver_obj = heap_caps_calloc(1, sizeof(hub_driver_t), MALLOC_CAP_DEFAULT); - urb_t *enum_urb = urb_alloc(sizeof(usb_setup_packet_t) + ENUM_CTRL_TRANSFER_MAX_DATA_LEN, 0, 0); + urb_t *enum_urb = urb_alloc(sizeof(usb_setup_packet_t) + ENUM_CTRL_TRANSFER_MAX_DATA_LEN, 0); if (hub_driver_obj == NULL || enum_urb == NULL) { return ESP_ERR_NO_MEM; } diff --git a/components/usb/private_include/usb_private.h b/components/usb/private_include/usb_private.h index ccdcd44a23..4fb25cc4ee 100644 --- a/components/usb/private_include/usb_private.h +++ b/components/usb/private_include/usb_private.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2021 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -43,7 +43,6 @@ struct urb_s { uint32_t hcd_var; // Host Lib Layer: void *usb_host_client; // Currently only used when submitted to shared pipes (i.e., Device default pipes) - size_t usb_host_header_size; // USB Host may need the data buffer to have a transparent header bool usb_host_inflight; // Debugging variable, used to prevent re-submitting URBs already inflight // Public transfer structure. Must be last due to variable length array usb_transfer_t transfer; @@ -76,15 +75,13 @@ typedef bool (*usb_proc_req_cb_t)(usb_proc_req_source_t source, bool in_isr, voi * * - Data buffer is allocated in DMA capable memory * - The constant fields of the URB are also set - * - The data_buffer field of the URB is set to point to start of the allocated data buffer AFTER the header. To access - * the header, users need a negative offset from data_buffer. + * - The data_buffer field of the URB is set to point to start of the allocated data buffer. * * @param data_buffer_size Size of the URB's data buffer - * @param header_size Size of header to put in front of URB's data buffer * @param num_isoc_packets Number of isochronous packet descriptors * @return urb_t* URB object */ -urb_t *urb_alloc(size_t data_buffer_size, size_t header_size, int num_isoc_packets); +urb_t *urb_alloc(size_t data_buffer_size, int num_isoc_packets); /** * @brief Free a URB diff --git a/components/usb/usb_host.c b/components/usb/usb_host.c index e0182d5660..88649d2fec 100644 --- a/components/usb/usb_host.c +++ b/components/usb/usb_host.c @@ -1245,7 +1245,7 @@ exit: esp_err_t usb_host_transfer_alloc(size_t data_buffer_size, int num_isoc_packets, usb_transfer_t **transfer) { - urb_t *urb = urb_alloc(data_buffer_size, 0, num_isoc_packets); + urb_t *urb = urb_alloc(data_buffer_size, num_isoc_packets); if (urb == NULL) { return ESP_ERR_NO_MEM; } diff --git a/components/usb/usb_private.c b/components/usb/usb_private.c index 9508597641..5d082c19d0 100644 --- a/components/usb/usb_private.c +++ b/components/usb/usb_private.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2021 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -8,17 +8,16 @@ #include "usb_private.h" #include "usb/usb_types_ch9.h" -urb_t *urb_alloc(size_t data_buffer_size, size_t header_size, int num_isoc_packets) +urb_t *urb_alloc(size_t data_buffer_size, int num_isoc_packets) { urb_t *urb = heap_caps_calloc(1, sizeof(urb_t) + (sizeof(usb_isoc_packet_desc_t) * num_isoc_packets), MALLOC_CAP_DEFAULT); - uint8_t *data_buffer = heap_caps_malloc(data_buffer_size + header_size, MALLOC_CAP_DMA); + uint8_t *data_buffer = heap_caps_malloc(data_buffer_size, MALLOC_CAP_DMA); if (urb == NULL || data_buffer == NULL) { goto err; } - urb->usb_host_header_size = header_size; // Indicate that this URB's data_buffer has a header in front of it. - // Case as dummy transfer to write to initialize const fields + // Cast as dummy transfer so that we can assign to const fields usb_transfer_dummy_t *dummy_transfer = (usb_transfer_dummy_t *)&urb->transfer; - dummy_transfer->data_buffer = (uint8_t *)(data_buffer + header_size); + dummy_transfer->data_buffer = data_buffer; dummy_transfer->data_buffer_size = data_buffer_size; dummy_transfer->num_isoc_packets = num_isoc_packets; return urb; @@ -33,6 +32,6 @@ void urb_free(urb_t *urb) if (urb == NULL) { return; } - heap_caps_free((uint8_t *)(urb->transfer.data_buffer - urb->usb_host_header_size)); + heap_caps_free(urb->transfer.data_buffer); heap_caps_free(urb); } From b7c3f01ac8b5a38062c9b129e2c4c7b7242bbf61 Mon Sep 17 00:00:00 2001 From: Darian Leung Date: Wed, 22 Nov 2023 01:35:54 +0800 Subject: [PATCH 2/2] 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; }