From a40a50998e740d3319b56cfda215acbe36ac6fa2 Mon Sep 17 00:00:00 2001 From: Myk Melez Date: Wed, 19 Mar 2025 16:27:28 -0700 Subject: [PATCH 1/2] fix(usb_host): Give semaphore on attempted close of non-opened device MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If you call *usb_host_device_close()* for a device that isn't open, the function exits early, without giving back the semaphore it took, which causes any other call that tries to take that semaphore to hang indefinitely. Strangely, there's redundant handling of this condition, with two checks in a row that both handle the case where `_check_client_opened_device(client_obj, dev_addr)` returns `false`: ```c HOST_CHECK_FROM_CRIT(_check_client_opened_device(client_obj, dev_addr), ESP_ERR_NOT_FOUND); if (!_check_client_opened_device(client_obj, dev_addr)) { // Client never opened this device ret = ESP_ERR_INVALID_STATE; HOST_EXIT_CRITICAL(); goto exit; } … exit: xSemaphoreGive(p_host_lib_obj->constant.mux_lock); return ret; ``` The first line is the one that exits early, as HOST_CHECK_FROM_CRIT returns its second parameter if its first parameter is false, without giving back the semaphore (although it does exit the critical section). The subsequent block handles the exact same case, except that it ensures the semaphore is given back before returning. Currently, this block is never reached. Perhaps the first check was added, then someone noticed the issue and added the second check, but they forgot to remove the first one. In any case, this PR removes the first check, so the second check can properly handle this case by giving back the semaphore before returning. This bug appears to have been present in the initial commit of the USB Host library to the ESP-IDF repo: https://github.com/espressif/esp-idf/commit/accbaee57ceca241ca117a53f486b7f79ed528b9 Of course, if you never try to close a non-opened device, then you won't encounter it! Unfortunately, I have some code that tried to do that, which is how I found the issue. --- components/usb/usb_host.c | 1 - 1 file changed, 1 deletion(-) diff --git a/components/usb/usb_host.c b/components/usb/usb_host.c index e0844ad49f..fbfd872810 100644 --- a/components/usb/usb_host.c +++ b/components/usb/usb_host.c @@ -1008,7 +1008,6 @@ esp_err_t usb_host_device_close(usb_host_client_handle_t client_hdl, usb_device_ HOST_ENTER_CRITICAL(); uint8_t dev_addr; ESP_ERROR_CHECK(usbh_dev_get_addr(dev_hdl, &dev_addr)); - HOST_CHECK_FROM_CRIT(_check_client_opened_device(client_obj, dev_addr), ESP_ERR_NOT_FOUND); if (!_check_client_opened_device(client_obj, dev_addr)) { // Client never opened this device ret = ESP_ERR_INVALID_STATE; From 5609df40966ebd1a9c81a0dcb61c17cd67737167 Mon Sep 17 00:00:00 2001 From: "igor.masar" Date: Tue, 25 Mar 2025 23:00:56 +0100 Subject: [PATCH 2/2] fix(usb_host): Fix return code and description Changed error code from ESP_ERR_INVALID_STATE to ESP_ERR_NOT_FOUND when the client never opened the device. Updated function documentation to correctly reflect return values. --- components/usb/include/usb/usb_host.h | 5 ++--- components/usb/usb_host.c | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/components/usb/include/usb/usb_host.h b/components/usb/include/usb/usb_host.h index d5036fdad9..59793b96eb 100644 --- a/components/usb/include/usb/usb_host.h +++ b/components/usb/include/usb/usb_host.h @@ -323,9 +323,8 @@ esp_err_t usb_host_device_open(usb_host_client_handle_t client_hdl, uint8_t dev_ * @return * - ESP_OK: Device closed successfully * - ESP_ERR_INVALID_ARG: Invalid argument - * - ESP_ERR_NOT_FOUND: Device address not found among opened devices - * - ESP_ERR_INVALID_STATE: The client never opened the device, or the client has not released - * all the interfaces from the device + * - ESP_ERR_NOT_FOUND: The client never opened the device (the device address not found among opened devices) + * - ESP_ERR_INVALID_STATE: The client has not released all interfaces from the device */ esp_err_t usb_host_device_close(usb_host_client_handle_t client_hdl, usb_device_handle_t dev_hdl); diff --git a/components/usb/usb_host.c b/components/usb/usb_host.c index fbfd872810..98e8ebdaa9 100644 --- a/components/usb/usb_host.c +++ b/components/usb/usb_host.c @@ -1010,7 +1010,7 @@ esp_err_t usb_host_device_close(usb_host_client_handle_t client_hdl, usb_device_ ESP_ERROR_CHECK(usbh_dev_get_addr(dev_hdl, &dev_addr)); if (!_check_client_opened_device(client_obj, dev_addr)) { // Client never opened this device - ret = ESP_ERR_INVALID_STATE; + ret = ESP_ERR_NOT_FOUND; HOST_EXIT_CRITICAL(); goto exit; }