From 87c415b11a26ca358b89377c41d183cc095b4a95 Mon Sep 17 00:00:00 2001 From: Tomas Rezucha Date: Mon, 10 Oct 2022 11:52:48 +0200 Subject: [PATCH 1/3] usb_host: Fixed incorrect opening devices from multiple clients 1. During USBH device open both queues (idle and pending) must be checked. 2. Don't overwrite already allocated endpoints --- components/usb/usbh.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/usb/usbh.c b/components/usb/usbh.c index 858ce3d0c9..7694287019 100644 --- a/components/usb/usbh.c +++ b/components/usb/usbh.c @@ -536,7 +536,7 @@ esp_err_t usbh_dev_open(uint8_t dev_addr, usb_device_handle_t *dev_hdl) goto exit; } } - TAILQ_FOREACH(dev_obj, &p_usbh_obj->dynamic.devs_idle_tailq, dynamic.tailq_entry) { + TAILQ_FOREACH(dev_obj, &p_usbh_obj->dynamic.devs_pending_tailq, dynamic.tailq_entry) { if (dev_obj->constant.address == dev_addr) { found_dev_obj = dev_obj; goto exit; @@ -783,7 +783,7 @@ esp_err_t usbh_ep_alloc(usb_device_handle_t dev_hdl, usbh_ep_config_t *ep_config if (is_in && dev_obj->mux_protected.ep_in[addr - 1] == NULL) { //Is an IN EP dev_obj->mux_protected.ep_in[addr - 1] = pipe_hdl; assigned = true; - } else if (dev_obj->mux_protected.ep_out[addr - 1] == NULL) { //Is an OUT EP + } else if (!is_in && dev_obj->mux_protected.ep_out[addr - 1] == NULL) { //Is an OUT EP dev_obj->mux_protected.ep_out[addr - 1] = pipe_hdl; assigned = true; } From 1f5dc0f5a174a37361bf90263d508b23e81f571b Mon Sep 17 00:00:00 2001 From: Darian Leung Date: Thu, 20 Oct 2022 00:04:53 +0800 Subject: [PATCH 2/3] usb_host: Add check to prevent submitting already inflight transfers This commit adds a simple flag/check in the USB Host Library that prevents users from submitting a transfer that is already inflight. - A transfer is considered inflight as soon as it is submitted by calling usb_host_transfer_submit() or usb_host_transfer_submit_control() - An inflight transfer remains inflight up until right before its callback is called by one of the USB Host Library handler functions. Closes https://github.com/espressif/esp-idf/issues/8748 --- components/usb/private_include/usb_private.h | 5 +++-- components/usb/usb_host.c | 23 ++++++++++++++++++-- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/components/usb/private_include/usb_private.h b/components/usb/private_include/usb_private.h index b245970b54..aaa34a1667 100644 --- a/components/usb/private_include/usb_private.h +++ b/components/usb/private_include/usb_private.h @@ -38,12 +38,13 @@ _Static_assert(sizeof(usb_transfer_dummy_t) == sizeof(usb_transfer_t), "usb_tran struct urb_s{ TAILQ_ENTRY(urb_s) tailq_entry; - //HCD handler pointer and variables. Must be initialized to NULL and 0 respectively + //HCD Layer: Handler pointer and variables. Must be initialized to NULL and 0 respectively void *hcd_ptr; uint32_t hcd_var; - //Host Driver layer handler + //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; }; diff --git a/components/usb/usb_host.c b/components/usb/usb_host.c index b4ced72a54..165cd400b9 100644 --- a/components/usb/usb_host.c +++ b/components/usb/usb_host.c @@ -590,6 +590,8 @@ static void _handle_pending_ep(client_t *client_obj) //Dequeue all URBs and run their transfer callback urb_t *urb = hcd_urb_dequeue(ep_obj->constant.pipe_hdl); while (urb != NULL) { + //Clear the transfer's inflight flag to indicate the transfer is no longer inflight + urb->usb_host_inflight = false; urb->transfer.callback(&urb->transfer); num_urb_dequeued++; urb = hcd_urb_dequeue(ep_obj->constant.pipe_hdl); @@ -747,6 +749,8 @@ esp_err_t usb_host_client_handle_events(usb_host_client_handle_t client_hdl, Tic TAILQ_REMOVE(&client_obj->dynamic.done_ctrl_xfer_tailq, urb, tailq_entry); client_obj->dynamic.num_done_ctrl_xfer--; HOST_EXIT_CRITICAL(); + //Clear the transfer's inflight flag to indicate the transfer is no longer inflight + urb->usb_host_inflight = false; //Call the transfer's callback urb->transfer.callback(&urb->transfer); HOST_ENTER_CRITICAL(); @@ -1297,6 +1301,9 @@ esp_err_t usb_host_transfer_submit(usb_transfer_t *transfer) USB_EP_DESC_GET_XFERTYPE(ep_obj->constant.ep_desc), USB_EP_DESC_GET_MPS(ep_obj->constant.ep_desc), transfer->bEndpointAddress & USB_B_ENDPOINT_ADDRESS_EP_DIR_MASK), ESP_ERR_INVALID_ARG); + //Check that we are not submitting a transfer already inflight + HOST_CHECK(!urb_obj->usb_host_inflight, ESP_ERR_NOT_FINISHED); + urb_obj->usb_host_inflight = true; HOST_ENTER_CRITICAL(); ep_obj->dynamic.num_urb_inflight++; HOST_EXIT_CRITICAL(); @@ -1316,6 +1323,7 @@ hcd_err: HOST_ENTER_CRITICAL(); ep_obj->dynamic.num_urb_inflight--; HOST_EXIT_CRITICAL(); + urb_obj->usb_host_inflight = false; err: return ret; } @@ -1323,6 +1331,7 @@ err: esp_err_t usb_host_transfer_submit_control(usb_host_client_handle_t client_hdl, usb_transfer_t *transfer) { HOST_CHECK(client_hdl != NULL && transfer != NULL, ESP_ERR_INVALID_ARG); + //Check that control transfer is valid HOST_CHECK(transfer->device_handle != NULL, ESP_ERR_INVALID_ARG); //Target device must be set usb_device_handle_t dev_hdl = transfer->device_handle; @@ -1332,8 +1341,18 @@ esp_err_t usb_host_transfer_submit_control(usb_host_client_handle_t client_hdl, HOST_CHECK(transfer_check(transfer, USB_TRANSFER_TYPE_CTRL, dev_info.bMaxPacketSize0, xfer_is_in), ESP_ERR_INVALID_ARG); //Control transfers must be targeted at EP 0 HOST_CHECK((transfer->bEndpointAddress & USB_B_ENDPOINT_ADDRESS_EP_NUM_MASK) == 0, ESP_ERR_INVALID_ARG); - //Save client handle into URB + urb_t *urb_obj = __containerof(transfer, urb_t, transfer); + //Check that we are not submitting a transfer already inflight + HOST_CHECK(!urb_obj->usb_host_inflight, ESP_ERR_NOT_FINISHED); + urb_obj->usb_host_inflight = true; + //Save client handle into URB urb_obj->usb_host_client = (void *)client_hdl; - return usbh_dev_submit_ctrl_urb(dev_hdl, urb_obj); + + esp_err_t ret; + ret = usbh_dev_submit_ctrl_urb(dev_hdl, urb_obj); + if (ret != ESP_OK) { + urb_obj->usb_host_inflight = false; + } + return ret; } From 0d1bc3b2b65b7177ba2d54e63360e933392f5ad4 Mon Sep 17 00:00:00 2001 From: Darian Leung Date: Thu, 20 Oct 2022 00:43:10 +0800 Subject: [PATCH 3/3] usb_host: Test host library inflight transfer resubmission check This commit updates the USB Host Library unit tests to test that resubmitting an inflight transfer will return an error. --- components/usb/test/usb_host/msc_client_async_seq.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/components/usb/test/usb_host/msc_client_async_seq.c b/components/usb/test/usb_host/msc_client_async_seq.c index d4be014b1c..16ce7bd3c4 100644 --- a/components/usb/test/usb_host/msc_client_async_seq.c +++ b/components/usb/test/usb_host/msc_client_async_seq.c @@ -192,6 +192,8 @@ void msc_client_async_seq_task(void *arg) xfer_out->num_bytes = sizeof(usb_setup_packet_t); xfer_out->bEndpointAddress = 0; TEST_ASSERT_EQUAL(ESP_OK, usb_host_transfer_submit_control(msc_obj.client_hdl, xfer_out)); + //Test that an inflight control transfer cannot be resubmitted + TEST_ASSERT_EQUAL(ESP_ERR_NOT_FINISHED, usb_host_transfer_submit_control(msc_obj.client_hdl, xfer_out)); //Next stage set from transfer callback break; } @@ -201,6 +203,8 @@ void msc_client_async_seq_task(void *arg) xfer_out->num_bytes = sizeof(mock_msc_bulk_cbw_t); xfer_out->bEndpointAddress = MOCK_MSC_SCSI_BULK_OUT_EP_ADDR; TEST_ASSERT_EQUAL(ESP_OK, usb_host_transfer_submit(xfer_out)); + //Test that an inflight transfer cannot be resubmitted + TEST_ASSERT_EQUAL(ESP_ERR_NOT_FINISHED, usb_host_transfer_submit(xfer_out)); //Next stage set from transfer callback break; } @@ -209,6 +213,8 @@ void msc_client_async_seq_task(void *arg) xfer_in->num_bytes = usb_round_up_to_mps(MOCK_MSC_SCSI_SECTOR_SIZE * msc_obj.test_param.num_sectors_per_xfer, MOCK_MSC_SCSI_BULK_EP_MPS); xfer_in->bEndpointAddress = MOCK_MSC_SCSI_BULK_IN_EP_ADDR; TEST_ASSERT_EQUAL(ESP_OK, usb_host_transfer_submit(xfer_in)); + //Test that an inflight transfer cannot be resubmitted + TEST_ASSERT_EQUAL(ESP_ERR_NOT_FINISHED, usb_host_transfer_submit(xfer_in)); //Next stage set from transfer callback break; } @@ -217,6 +223,8 @@ void msc_client_async_seq_task(void *arg) xfer_in->num_bytes = usb_round_up_to_mps(sizeof(mock_msc_bulk_csw_t), MOCK_MSC_SCSI_BULK_EP_MPS); xfer_in->bEndpointAddress = MOCK_MSC_SCSI_BULK_IN_EP_ADDR; TEST_ASSERT_EQUAL(ESP_OK, usb_host_transfer_submit(xfer_in)); + //Test that an inflight transfer cannot be resubmitted + TEST_ASSERT_EQUAL(ESP_ERR_NOT_FINISHED, usb_host_transfer_submit(xfer_in)); //Next stage set from transfer callback break; }