From d69d1aafaf8b45a18757c554d5d3ab1152286cc9 Mon Sep 17 00:00:00 2001 From: Darian Leung Date: Mon, 5 Dec 2022 23:22:25 +0800 Subject: [PATCH 1/2] hal: Fix USB DWC HAL host channel halt race condition This commit fixes a race condtion bug with usb_dwc_hal_chan_request_halt() where a channel the channel is halted if it has just completed a transfer (i.e., finished processing a QTD with the "HOC" flag set) but the channel is still pending interrupt handling. In this case... - usb_dwc_hal_chan_request_halt() would simply read the channel's underlying register, determine it is not active, not set the "halt_requested" flag, and simply return true. - The caller assumes of usb_dwc_hal_chan_request_halt() will assume that the channel has halted, and may proceed to reconfigure the pipe/port - When usb_dwc_hal_chan_decode_intr() comes to process the pending interrupt it will simply return USB_DWC_HAL_CHAN_EVENT_CPLT not knowing a halt has been requested. This commit updates the implementation of usb_dwc_hal_chan_request_halt() so that a halt is properly requested even if the underlying channel has already physically halted. --- components/hal/usb_dwc_hal.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/components/hal/usb_dwc_hal.c b/components/hal/usb_dwc_hal.c index ee0663dfce7..8e26371275b 100644 --- a/components/hal/usb_dwc_hal.c +++ b/components/hal/usb_dwc_hal.c @@ -279,16 +279,21 @@ void usb_dwc_hal_chan_activate(usb_dwc_hal_chan_t *chan_obj, void *xfer_desc_lis bool usb_dwc_hal_chan_request_halt(usb_dwc_hal_chan_t *chan_obj) { - //Cannot request halt on a channel that is pending error handling - if (usb_dwc_ll_hcchar_chan_is_enabled(chan_obj->regs)) { - //If the register indicates that the channel is still active, the active flag must have been previously set - HAL_ASSERT(chan_obj->flags.active); + if (chan_obj->flags.active) { + /* + Request a halt so long as the channel's active flag is set. + - If the underlying hardware channel is already halted but the channel is pending interrupt handling, + disabling the channel will have no effect (i.e., no channel interrupt is generated). + - If the underlying channel is currently active, disabling the channel will trigger a channel interrupt. + + Regardless, setting the "halt_requested" should cause "usb_dwc_hal_chan_decode_intr()" to report the + USB_DWC_HAL_CHAN_EVENT_HALT_REQ event when channel interrupt is handled (pending or triggered). + */ usb_dwc_ll_hcchar_disable_chan(chan_obj->regs); chan_obj->flags.halt_requested = 1; return false; } else { - //We just clear the active flag here as it could still be set (if we have a pending channel interrupt) - chan_obj->flags.active = 0; + //Channel was never active to begin with, simply return true return true; } } @@ -359,6 +364,10 @@ usb_dwc_hal_chan_event_t usb_dwc_hal_chan_decode_intr(usb_dwc_hal_chan_t *chan_o usb_dwc_hal_chan_event_t chan_event; //Note: We don't assert on (chan_obj->flags.active) here as it could have been already cleared by usb_dwc_hal_chan_request_halt() + /* + Note: Do not change order of checks as some events take precedence over others. + Errors > Channel Halt Request > Transfer completed + */ if (chan_intrs & CHAN_INTRS_ERROR_MSK) { //Note: Errors are uncommon, so we check against the entire interrupt mask to reduce frequency of entering this call path HAL_ASSERT(chan_intrs & USB_DWC_LL_INTR_CHAN_CHHLTD); //An error should have halted the channel //Store the error in hal context From c2bc34ee849f4c620c07f4473db46f399e221127 Mon Sep 17 00:00:00 2001 From: Darian Leung Date: Tue, 6 Dec 2022 18:10:45 +0800 Subject: [PATCH 2/2] usb: Fix incorrect bmRequestType direction flag in USB Host Library usb_host_transfer_submit_control() uses the incorrect bmRequestType direction flag. Therefore, when doing a transfer check, all transfers were mistakenly treated as OUT transfers (only affects transfer check and not actual transfer). --- components/usb/usb_host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/usb/usb_host.c b/components/usb/usb_host.c index ebbb00b33cf..55507117248 100644 --- a/components/usb/usb_host.c +++ b/components/usb/usb_host.c @@ -1336,7 +1336,7 @@ esp_err_t usb_host_transfer_submit_control(usb_host_client_handle_t client_hdl, //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; - bool xfer_is_in = ((usb_setup_packet_t *)transfer->data_buffer)->bmRequestType & USB_BM_REQUEST_TYPE_DIR_OUT; + bool xfer_is_in = ((usb_setup_packet_t *)transfer->data_buffer)->bmRequestType & USB_BM_REQUEST_TYPE_DIR_IN; usb_device_info_t dev_info; ESP_ERROR_CHECK(usbh_dev_get_info(dev_hdl, &dev_info)); HOST_CHECK(transfer_check(transfer, USB_TRANSFER_TYPE_CTRL, dev_info.bMaxPacketSize0, xfer_is_in), ESP_ERR_INVALID_ARG);