From 0c758c855755f90aa38d12546c6b2834da8cb4f5 Mon Sep 17 00:00:00 2001 From: Darian Leung Date: Mon, 25 Oct 2021 21:42:09 +0800 Subject: [PATCH] usb: Fix how the HCD handles sudden disconnection This commit fixes how the USB Host HCD handles sudden disconnections. Bugs: - HW channels remain active when the port suddenly disconnects, and previously the channel would be disabled by setting the disabled bit, then waiting for a disabled interrupt. However, ISOC channels do not generate the disabled interrupt when the port is invalid, thus leading to tasks getting indefinitely blocked in hcd_pipe_command(). Fix: On a sudden disconnection, forcibly treat all channels as halted even if their HCCHAR.ChEna bit is still set. We do a soft reset after a port error anyways, so the channels will eventually be reset. Closes https://github.com/espressif/esp-idf/issues/7505 --- components/hal/include/hal/usbh_hal.h | 87 +++++-------- components/hal/usbh_hal.c | 30 ++--- components/usb/hcd.c | 124 +++++++++++-------- components/usb/include/usb/usb_types_stack.h | 6 +- components/usb/maintainers.md | 6 +- components/usb/test/hcd/test_hcd_common.c | 27 ++-- components/usb/test/hcd/test_hcd_isoc.c | 99 +++++++++++++++ components/usb/test/hcd/test_hcd_port.c | 14 ++- tools/ci/check_copyright_ignore.txt | 2 - 9 files changed, 240 insertions(+), 155 deletions(-) diff --git a/components/hal/include/hal/usbh_hal.h b/components/hal/include/hal/usbh_hal.h index b315f6c0b1..6360f7b3f3 100644 --- a/components/hal/include/hal/usbh_hal.h +++ b/components/hal/include/hal/usbh_hal.h @@ -1,16 +1,8 @@ -// Copyright 2020 Espressif Systems (Shanghai) PTE LTD -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. +/* + * SPDX-FileCopyrightText: 2020-2021 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ #pragma once @@ -50,17 +42,6 @@ typedef struct { uint32_t ptx_fifo_lines; /**< Size of the Periodic FIFO in terms the number of FIFO lines */ } usbh_hal_fifo_config_t; -// --------------------- HAL States ------------------------ - -/** - * @brief Channel states - */ -typedef enum { - USBH_HAL_CHAN_STATE_HALTED = 0, /**< The channel is halted. No transfer descriptor list is being executed */ - USBH_HAL_CHAN_STATE_ACTIVE, /**< The channel is active. A transfer descriptor list is being executed */ - USBH_HAL_CHAN_STATE_ERROR, /**< The channel is in the error state */ -} usbh_hal_chan_state_t; - // --------------------- HAL Events ------------------------ /** @@ -153,8 +134,7 @@ typedef struct { struct { uint32_t active: 1; /**< Debugging bit to indicate whether channel is enabled */ uint32_t halt_requested: 1; /**< A halt has been requested */ - uint32_t error_pending: 1; /**< The channel is waiting for the error to be handled */ - uint32_t reserved: 1; + uint32_t reserved: 2; uint32_t chan_idx: 4; /**< The index number of the channel */ uint32_t reserved24: 24; }; @@ -556,23 +536,6 @@ static inline void *usbh_hal_chan_get_context(usbh_hal_chan_t *chan_obj) return chan_obj->chan_ctx; } -/** - * @brief Get the current state of a channel - * - * @param chan_obj Channel object - * @return usbh_hal_chan_state_t State of the channel - */ -static inline usbh_hal_chan_state_t usbh_hal_chan_get_state(usbh_hal_chan_t *chan_obj) -{ - if (chan_obj->flags.error_pending) { - return USBH_HAL_CHAN_STATE_ERROR; - } else if (chan_obj->flags.active) { - return USBH_HAL_CHAN_STATE_ACTIVE; - } else { - return USBH_HAL_CHAN_STATE_HALTED; - } -} - /** * @brief Set the endpoint information for a particular channel * @@ -602,7 +565,7 @@ void usbh_hal_chan_set_ep_char(usbh_hal_context_t *hal, usbh_hal_chan_t *chan_ob static inline void usbh_hal_chan_set_dir(usbh_hal_chan_t *chan_obj, bool is_in) { //Cannot change direction whilst channel is still active or in error - HAL_ASSERT(!chan_obj->flags.active && !chan_obj->flags.error_pending); + HAL_ASSERT(!chan_obj->flags.active); usbh_ll_chan_set_dir(chan_obj->regs, is_in); } @@ -621,7 +584,7 @@ static inline void usbh_hal_chan_set_dir(usbh_hal_chan_t *chan_obj, bool is_in) static inline void usbh_hal_chan_set_pid(usbh_hal_chan_t *chan_obj, int pid) { //Cannot change pid whilst channel is still active or in error - HAL_ASSERT(!chan_obj->flags.active && !chan_obj->flags.error_pending); + HAL_ASSERT(!chan_obj->flags.active); //Update channel object and set the register usbh_ll_chan_set_pid(chan_obj->regs, pid); } @@ -638,7 +601,7 @@ static inline void usbh_hal_chan_set_pid(usbh_hal_chan_t *chan_obj, int pid) */ static inline uint32_t usbh_hal_chan_get_pid(usbh_hal_chan_t *chan_obj) { - HAL_ASSERT(!chan_obj->flags.active && !chan_obj->flags.error_pending); + HAL_ASSERT(!chan_obj->flags.active); return usbh_ll_chan_get_pid(chan_obj->regs); } @@ -687,6 +650,25 @@ static inline int usbh_hal_chan_get_qtd_idx(usbh_hal_chan_t *chan_obj) */ bool usbh_hal_chan_request_halt(usbh_hal_chan_t *chan_obj); +/** + * @brief Indicate that a channel is halted after a port error + * + * When a port error occurs (e.g., discconect, overcurrent): + * - Any previously active channels will remain active (i.e., they will not receive a channel interrupt) + * - Attempting to disable them using usbh_hal_chan_request_halt() will NOT generate an interrupt for ISOC channels + * (probalby something to do with the periodic scheduling) + * + * However, the channel's enable bit can be left as 1 since after a port error, a soft reset will be done anyways. + * This function simply updates the channels internal state variable to indicate it is halted (thus allowing it to be + * freed). + * + * @param chan_obj Channel object + */ +static inline void usbh_hal_chan_mark_halted(usbh_hal_chan_t *chan_obj) +{ + chan_obj->flags.active = 0; +} + /** * @brief Get a channel's error * @@ -695,22 +677,9 @@ bool usbh_hal_chan_request_halt(usbh_hal_chan_t *chan_obj); */ static inline usbh_hal_chan_error_t usbh_hal_chan_get_error(usbh_hal_chan_t *chan_obj) { - HAL_ASSERT(chan_obj->flags.error_pending); return chan_obj->error; } -/** - * @brief Clear a channel of it's error - * - * @param chan_obj Channel object - */ -static inline void usbh_hal_chan_clear_error(usbh_hal_chan_t *chan_obj) -{ - //Can only clear error when an error has occurred - HAL_ASSERT(chan_obj->flags.error_pending); - chan_obj->flags.error_pending = 0; -} - // -------------------------------------------- Transfer Descriptor List ----------------------------------------------- /** diff --git a/components/hal/usbh_hal.c b/components/hal/usbh_hal.c index 5091952de8..a0e038b0e5 100644 --- a/components/hal/usbh_hal.c +++ b/components/hal/usbh_hal.c @@ -1,16 +1,8 @@ -// Copyright 2020 Espressif Systems (Shanghai) PTE LTD -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. +/* + * SPDX-FileCopyrightText: 2020-2021 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ #include #include @@ -238,7 +230,7 @@ void usbh_hal_chan_free(usbh_hal_context_t *hal, usbh_hal_chan_t *chan_obj) } } //Can only free a channel when in the disabled state and descriptor list released - HAL_ASSERT(!chan_obj->flags.active && !chan_obj->flags.error_pending); + HAL_ASSERT(!chan_obj->flags.active); //Disable channel's interrupt usbh_ll_haintmsk_dis_chan_intr(hal->dev, 1 << chan_obj->flags.chan_idx); //Deallocate channel @@ -252,7 +244,7 @@ void usbh_hal_chan_free(usbh_hal_context_t *hal, usbh_hal_chan_t *chan_obj) void usbh_hal_chan_set_ep_char(usbh_hal_context_t *hal, usbh_hal_chan_t *chan_obj, usbh_hal_ep_char_t *ep_char) { //Cannot change ep_char whilst channel is still active or in error - HAL_ASSERT(!chan_obj->flags.active && !chan_obj->flags.error_pending); + HAL_ASSERT(!chan_obj->flags.active); //Set the endpoint characteristics of the pipe usbh_ll_chan_hcchar_init(chan_obj->regs, ep_char->dev_addr, @@ -280,7 +272,7 @@ void usbh_hal_chan_set_ep_char(usbh_hal_context_t *hal, usbh_hal_chan_t *chan_ob void usbh_hal_chan_activate(usbh_hal_chan_t *chan_obj, void *xfer_desc_list, int desc_list_len, int start_idx) { //Cannot activate a channel that has already been enabled or is pending error handling - HAL_ASSERT(!chan_obj->flags.active && !chan_obj->flags.error_pending); + HAL_ASSERT(!chan_obj->flags.active); //Set start address of the QTD list and starting QTD index usbh_ll_chan_set_dma_addr_non_iso(chan_obj->regs, xfer_desc_list, start_idx); usbh_ll_chan_set_qtd_list_len(chan_obj->regs, desc_list_len); @@ -291,12 +283,14 @@ void usbh_hal_chan_activate(usbh_hal_chan_t *chan_obj, void *xfer_desc_list, int bool usbh_hal_chan_request_halt(usbh_hal_chan_t *chan_obj) { //Cannot request halt on a channel that is pending error handling - HAL_ASSERT(chan_obj->flags.active && !chan_obj->flags.error_pending); if (usbh_ll_chan_is_active(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); usbh_ll_chan_halt(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; return true; } @@ -366,6 +360,7 @@ usbh_hal_chan_event_t usbh_hal_chan_decode_intr(usbh_hal_chan_t *chan_obj) { uint32_t chan_intrs = usbh_ll_chan_intr_read_and_clear(chan_obj->regs); usbh_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 usbh_hal_chan_request_halt() 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 & USBH_LL_INTR_CHAN_CHHLTD); //An error should have halted the channel @@ -383,7 +378,6 @@ usbh_hal_chan_event_t usbh_hal_chan_decode_intr(usbh_hal_chan_t *chan_obj) //Update flags chan_obj->error = error; chan_obj->flags.active = 0; - chan_obj->flags.error_pending = 1; //Save the error to be handled later chan_event = USBH_HAL_CHAN_EVENT_ERROR; } else if (chan_intrs & USBH_LL_INTR_CHAN_CHHLTD) { diff --git a/components/usb/hcd.c b/components/usb/hcd.c index 87be3a350a..d28b943e42 100644 --- a/components/usb/hcd.c +++ b/components/usb/hcd.c @@ -24,8 +24,6 @@ #include "usb_private.h" #include "usb/usb_types_ch9.h" -#include "esp_rom_sys.h" - // ----------------------------------------------------- Macros -------------------------------------------------------- // --------------------- Constants ------------------------- @@ -214,7 +212,8 @@ typedef struct { union { struct { uint32_t executing: 1; //The buffer is currently executing - uint32_t reserved7: 7; + uint32_t was_canceled: 1; //Buffer was done due to a cancellation (i.e., a halt request) + uint32_t reserved6: 6; uint32_t stop_idx: 8; //The descriptor index when the channel was halted hcd_pipe_event_t pipe_event: 8; //The pipe event when the buffer was done uint32_t reserved8: 8; @@ -257,7 +256,7 @@ struct pipe_obj { //Pipe status/state/events related hcd_pipe_state_t state; hcd_pipe_event_t last_event; - TaskHandle_t task_waiting_pipe_notif; //Task handle used for internal pipe events + volatile TaskHandle_t task_waiting_pipe_notif; //Task handle used for internal pipe events. Set by waiter, cleared by notifier union { struct { uint32_t waiting_halt: 1; @@ -290,7 +289,7 @@ struct port_obj { hcd_port_state_t state; usb_speed_t speed; hcd_port_event_t last_event; - TaskHandle_t task_waiting_port_notif; //Task handle used for internal port events + volatile TaskHandle_t task_waiting_port_notif; //Task handle used for internal port events. Set by waiter, cleared by notifier union { struct { uint32_t event_pending: 1; //The port has an event that needs to be handled @@ -423,13 +422,15 @@ static void _buffer_exec_cont(pipe_t *pipe); * * @param pipe Pipe object * @param stop_idx Descriptor index when the buffer stopped execution - * @param pipe_event Pipe event that caused the buffer to be complete + * @param pipe_event Pipe event that caused the buffer to be complete. Use HCD_PIPE_EVENT_NONE for halt request of disconnections + * @param canceled Whether the buffer was done due to a canceled (i.e., halt request). Must set pipe_event to HCD_PIPE_EVENT_NONE */ -static inline void _buffer_done(pipe_t *pipe, int stop_idx, hcd_pipe_event_t pipe_event) +static inline void _buffer_done(pipe_t *pipe, int stop_idx, hcd_pipe_event_t pipe_event, bool canceled) { //Store the stop_idx and pipe_event for later parsing dma_buffer_block_t *buffer_done = pipe->buffers[pipe->multi_buffer_control.rd_idx]; buffer_done->status_flags.executing = 0; + buffer_done->status_flags.was_canceled = canceled; buffer_done->status_flags.stop_idx = stop_idx; buffer_done->status_flags.pipe_event = pipe_event; pipe->multi_buffer_control.rd_idx++; @@ -474,11 +475,11 @@ static void _buffer_parse(pipe_t *pipe); * @note This should only be called on pipes do not have any currently executing buffers. * * @param pipe Pipe object - * @param cancelled Whether this flush is due to cancellation + * @param canceled Whether this flush is due to cancellation * @return true One or more buffers were flushed * @return false There were no buffers that needed to be flushed */ -static bool _buffer_flush_all(pipe_t *pipe, bool cancelled); +static bool _buffer_flush_all(pipe_t *pipe, bool canceled); // ------------------------ Pipe --------------------------- @@ -689,22 +690,28 @@ static void _internal_port_event_wait(port_t *port) //There must NOT be another thread/task already waiting for an internal event assert(port->task_waiting_port_notif == NULL); port->task_waiting_port_notif = xTaskGetCurrentTaskHandle(); - HCD_EXIT_CRITICAL(); - //Wait to be notified from ISR - ulTaskNotifyTake(pdTRUE, portMAX_DELAY); - HCD_ENTER_CRITICAL(); - port->task_waiting_port_notif = NULL; + /* We need to loop as task notifications can come from anywhere. If we this + was a port event notification, task_waiting_port_notif will have been cleared + by the notifier. */ + while (port->task_waiting_port_notif != NULL) { + HCD_EXIT_CRITICAL(); + //Wait to be notified from ISR + ulTaskNotifyTake(pdTRUE, portMAX_DELAY); + HCD_ENTER_CRITICAL(); + } } static bool _internal_port_event_notify_from_isr(port_t *port) { //There must be a thread/task waiting for an internal event assert(port->task_waiting_port_notif != NULL); - BaseType_t xTaskWoken = pdFALSE; + TaskHandle_t task_to_unblock = port->task_waiting_port_notif; + //Clear task_waiting_port_notif to indicate to the waiter that the unblock was indeed an port event notification + port->task_waiting_port_notif = NULL; //Unblock the thread/task waiting for the notification - HCD_EXIT_CRITICAL_ISR(); - vTaskNotifyGiveFromISR(port->task_waiting_port_notif, &xTaskWoken); - HCD_ENTER_CRITICAL_ISR(); + BaseType_t xTaskWoken = pdFALSE; + //Note: We don't exit the critical section to be atomic. vTaskNotifyGiveFromISR() doesn't block anyways + vTaskNotifyGiveFromISR(task_to_unblock, &xTaskWoken); return (xTaskWoken == pdTRUE); } @@ -713,28 +720,34 @@ static void _internal_pipe_event_wait(pipe_t *pipe) //There must NOT be another thread/task already waiting for an internal event assert(pipe->task_waiting_pipe_notif == NULL); pipe->task_waiting_pipe_notif = xTaskGetCurrentTaskHandle(); - HCD_EXIT_CRITICAL(); - //Wait to be notified from ISR - ulTaskNotifyTake(pdTRUE, portMAX_DELAY); - HCD_ENTER_CRITICAL(); - pipe->task_waiting_pipe_notif = NULL; + /* We need to loop as task notifications can come from anywhere. If we this + was a pipe event notification, task_waiting_pipe_notif will have been cleared + by the notifier. */ + while (pipe->task_waiting_pipe_notif != NULL) { + //Wait to be unblocked by notified + HCD_EXIT_CRITICAL(); + ulTaskNotifyTake(pdTRUE, portMAX_DELAY); + HCD_ENTER_CRITICAL(); + } } static bool _internal_pipe_event_notify(pipe_t *pipe, bool from_isr) { //There must be a thread/task waiting for an internal event assert(pipe->task_waiting_pipe_notif != NULL); + TaskHandle_t task_to_unblock = pipe->task_waiting_pipe_notif; + //Clear task_waiting_pipe_notif to indicate to the waiter that the unblock was indeed an pipe event notification + pipe->task_waiting_pipe_notif = NULL; bool ret; if (from_isr) { BaseType_t xTaskWoken = pdFALSE; - HCD_EXIT_CRITICAL_ISR(); + //Note: We don't exit the critical section to be atomic. vTaskNotifyGiveFromISR() doesn't block anyways //Unblock the thread/task waiting for the pipe notification - vTaskNotifyGiveFromISR(pipe->task_waiting_pipe_notif, &xTaskWoken); - HCD_ENTER_CRITICAL_ISR(); + vTaskNotifyGiveFromISR(task_to_unblock, &xTaskWoken); ret = (xTaskWoken == pdTRUE); } else { HCD_EXIT_CRITICAL(); - xTaskNotifyGive(pipe->task_waiting_pipe_notif); + xTaskNotifyGive(task_to_unblock); HCD_ENTER_CRITICAL(); ret = false; } @@ -836,7 +849,7 @@ static hcd_pipe_event_t _intr_hdlr_chan(pipe_t *pipe, usbh_hal_chan_t *chan_obj, event = pipe->last_event; //Mark the buffer as done int stop_idx = usbh_hal_chan_get_qtd_idx(chan_obj); - _buffer_done(pipe, stop_idx, pipe->last_event); + _buffer_done(pipe, stop_idx, pipe->last_event, false); //First check if there is another buffer we can execute. But we only want to execute if there's still a valid device if (_buffer_can_exec(pipe) && pipe->port->flags.conn_dev_ena) { //If the next buffer is filled and ready to execute, execute it @@ -854,13 +867,12 @@ static hcd_pipe_event_t _intr_hdlr_chan(pipe_t *pipe, usbh_hal_chan_t *chan_obj, case USBH_HAL_CHAN_EVENT_ERROR: { //Get and store the pipe error event usbh_hal_chan_error_t chan_error = usbh_hal_chan_get_error(chan_obj); - usbh_hal_chan_clear_error(chan_obj); pipe->last_event = pipe_decode_error_event(chan_error); event = pipe->last_event; pipe->state = HCD_PIPE_STATE_HALTED; //Mark the buffer as done with an error int stop_idx = usbh_hal_chan_get_qtd_idx(chan_obj); - _buffer_done(pipe, stop_idx, pipe->last_event); + _buffer_done(pipe, stop_idx, pipe->last_event, false); //Parse the buffer _buffer_parse(pipe); break; @@ -873,7 +885,7 @@ static hcd_pipe_event_t _intr_hdlr_chan(pipe_t *pipe, usbh_hal_chan_t *chan_obj, //Halt request event is triggered when packet is successful completed. But just treat all halted transfers as errors pipe->state = HCD_PIPE_STATE_HALTED; int stop_idx = usbh_hal_chan_get_qtd_idx(chan_obj); - _buffer_done(pipe, stop_idx, HCD_PIPE_EVENT_NONE); + _buffer_done(pipe, stop_idx, HCD_PIPE_EVENT_NONE, true); //Parse the buffer _buffer_parse(pipe); //Notify the task waiting for the pipe halt @@ -1717,17 +1729,23 @@ static void pipe_set_ep_char(const hcd_pipe_config_t *pipe_config, usb_transfer_ static esp_err_t _pipe_cmd_halt(pipe_t *pipe) { esp_err_t ret; - //Pipe must be in the active state in order to be halted - if (pipe->state != HCD_PIPE_STATE_ACTIVE) { - ret = ESP_ERR_INVALID_STATE; + + //If pipe is already halted, just return. + if (pipe->state == HCD_PIPE_STATE_HALTED) { + ret = ESP_OK; goto exit; } - //Request that the channel halts - if (!usbh_hal_chan_request_halt(pipe->chan_obj)) { - //We need to wait for channel to be halted. State will be updated in the ISR - pipe->cs_flags.waiting_halt = 1; - _internal_pipe_event_wait(pipe); + //If the pipe's port is invalid, we just mark the pipe as halted without needing to halt the underlying channel + if (pipe->port->flags.conn_dev_ena //Skip halting the underlying channel if the port is invalid + && !usbh_hal_chan_request_halt(pipe->chan_obj)) { //Check if the channel is already halted + //Channel is not halted, we need to request and wait for a haltWe need to wait for channel to be halted. + pipe->cs_flags.waiting_halt = 1; + _internal_pipe_event_wait(pipe); + //State should have been updated in the ISR + assert(pipe->state == HCD_PIPE_STATE_HALTED); } else { + //We are already halted, just need to update the state + usbh_hal_chan_mark_halted(pipe->chan_obj); pipe->state = HCD_PIPE_STATE_HALTED; } ret = ESP_OK; @@ -1743,11 +1761,11 @@ static esp_err_t _pipe_cmd_flush(pipe_t *pipe) ret = ESP_ERR_INVALID_STATE; goto exit; } - //Cannot have a currently executing buffer - assert(!pipe->multi_buffer_control.buffer_is_executing); + //If the port is still valid, we are canceling transfers. Otherwise, we are flushing due to a port error + bool canceled = pipe->port->flags.conn_dev_ena; bool call_pipe_cb; //Flush any filled buffers - call_pipe_cb = _buffer_flush_all(pipe, true); + call_pipe_cb = _buffer_flush_all(pipe, canceled); //Move all URBs from the pending tailq to the done tailq if (pipe->num_urb_pending > 0) { //Process all remaining pending URBs @@ -1755,14 +1773,14 @@ static esp_err_t _pipe_cmd_flush(pipe_t *pipe) TAILQ_FOREACH(urb, &pipe->pending_urb_tailq, tailq_entry) { //Update the URB's current state urb->hcd_var = URB_HCD_STATE_DONE; - //We are canceling the URB. Update its actual_num_bytes and status + //URBs were never executed, Update the actual_num_bytes and status urb->transfer.actual_num_bytes = 0; - urb->transfer.status = USB_TRANSFER_STATUS_CANCELED; + urb->transfer.status = (canceled) ? USB_TRANSFER_STATUS_CANCELED : USB_TRANSFER_STATUS_NO_DEVICE; if (pipe->ep_char.type == USB_PRIV_XFER_TYPE_ISOCHRONOUS) { //Update the URB's isoc packet descriptors as well for (int pkt_idx = 0; pkt_idx < urb->transfer.num_isoc_packets; pkt_idx++) { urb->transfer.isoc_packet_desc[pkt_idx].actual_num_bytes = 0; - urb->transfer.isoc_packet_desc[pkt_idx].status = USB_TRANSFER_STATUS_CANCELED; + urb->transfer.isoc_packet_desc[pkt_idx].status = (canceled) ? USB_TRANSFER_STATUS_CANCELED : USB_TRANSFER_STATUS_NO_DEVICE; } } } @@ -2011,7 +2029,6 @@ esp_err_t hcd_pipe_command(hcd_pipe_handle_t pipe_hdl, hcd_pipe_cmd_t command) pipe_t *pipe = (pipe_t *)pipe_hdl; esp_err_t ret = ESP_OK; - xSemaphoreTake(pipe->port->port_mux, portMAX_DELAY); HCD_ENTER_CRITICAL(); //Cannot execute pipe commands the pipe is already executing a command, or if the pipe or its port are no longer valid if (pipe->cs_flags.reset_lock) { @@ -2035,7 +2052,6 @@ esp_err_t hcd_pipe_command(hcd_pipe_handle_t pipe_hdl, hcd_pipe_cmd_t command) pipe->cs_flags.pipe_cmd_processing = 0; } HCD_EXIT_CRITICAL(); - xSemaphoreGive(pipe->port->port_mux); return ret; } @@ -2167,6 +2183,7 @@ static void _buffer_fill(pipe_t *pipe) //Select the inactive buffer assert(pipe->multi_buffer_control.buffer_num_to_exec <= NUM_BUFFERS); dma_buffer_block_t *buffer_to_fill = pipe->buffers[pipe->multi_buffer_control.wr_idx]; + buffer_to_fill->status_flags.val = 0; //Clear the buffer's status flags assert(buffer_to_fill->urb == NULL); bool is_in = pipe->ep_char.bEndpointAddress & USB_B_ENDPOINT_ADDRESS_EP_DIR_MASK; int mps = pipe->ep_char.mps; @@ -2419,16 +2436,13 @@ static inline void _buffer_parse_isoc(dma_buffer_block_t *buffer, bool is_in) static inline void _buffer_parse_error(dma_buffer_block_t *buffer) { - //The URB had an error, so we consider that NO bytes were transferred. Set actual_num_bytes to zero + //The URB had an error in one of its packet, or a port error), so we the entire URB an error. usb_transfer_t *transfer = &buffer->urb->transfer; transfer->actual_num_bytes = 0; - for (int i = 0; i < transfer->num_isoc_packets; i++) { - transfer->isoc_packet_desc[i].actual_num_bytes = 0; - } - //Update status of URB. Status will depend on the pipe_event + //Update the overall status of URB. Status will depend on the pipe_event switch (buffer->status_flags.pipe_event) { case HCD_PIPE_EVENT_NONE: - transfer->status = USB_TRANSFER_STATUS_CANCELED; + transfer->status = (buffer->status_flags.was_canceled) ? USB_TRANSFER_STATUS_CANCELED : USB_TRANSFER_STATUS_NO_DEVICE; break; case HCD_PIPE_EVENT_ERROR_XFER: transfer->status = USB_TRANSFER_STATUS_ERROR; @@ -2496,12 +2510,12 @@ static void _buffer_parse(pipe_t *pipe) pipe->multi_buffer_control.buffer_num_to_fill++; } -static bool _buffer_flush_all(pipe_t *pipe, bool cancelled) +static bool _buffer_flush_all(pipe_t *pipe, bool canceled) { int cur_num_to_mark_done = pipe->multi_buffer_control.buffer_num_to_exec; for (int i = 0; i < cur_num_to_mark_done; i++) { //Mark any filled buffers as done - _buffer_done(pipe, 0, HCD_PIPE_EVENT_NONE); + _buffer_done(pipe, 0, HCD_PIPE_EVENT_NONE, canceled); } int cur_num_to_parse = pipe->multi_buffer_control.buffer_num_to_parse; for (int i = 0; i < cur_num_to_parse; i++) { diff --git a/components/usb/include/usb/usb_types_stack.h b/components/usb/include/usb/usb_types_stack.h index 438c22d9ef..4305555fe7 100644 --- a/components/usb/include/usb/usb_types_stack.h +++ b/components/usb/include/usb/usb_types_stack.h @@ -68,6 +68,7 @@ typedef enum { USB_TRANSFER_STATUS_STALL, /**< The transfer was stalled */ USB_TRANSFER_STATUS_OVERFLOW, /**< The transfer as more data was sent than was requested */ USB_TRANSFER_STATUS_SKIPPED, /**< ISOC packets only. The packet was skipped due to system latency or bus overload */ + USB_TRANSFER_STATUS_NO_DEVICE, /**< The transfer failed because the target device is gone */ } usb_transfer_status_t; /** @@ -102,7 +103,10 @@ typedef struct { * split into multiple packets, and each packet is transferred at the endpoint's specified interval. * - Isochronous: Represents a stream of bytes that should be transferred to an endpoint at a fixed rate. The transfer * is split into packets according to the each isoc_packet_desc. A packet is transferred at each interval - * of the endpoint. + * of the endpoint. If an entire ISOC URB was transferred without error (skipped packets do not count as + * errors), the URB's overall status and the status of each packet descriptor will be updated, and the + * actual_num_bytes reflects the total bytes transferred over all packets. If the ISOC URB encounters an + * error, the entire URB is considered erroneous so only the overall status will updated. * * @note For Bulk/Control/Interrupt IN transfers, the num_bytes must be a integer multiple of the endpoint's MPS * @note This structure should be allocated via usb_host_transfer_alloc() diff --git a/components/usb/maintainers.md b/components/usb/maintainers.md index 81e2d00dec..8a0da5d23f 100644 --- a/components/usb/maintainers.md +++ b/components/usb/maintainers.md @@ -24,7 +24,10 @@ stop the remainder of the interrupt transfer. ## Channel interrupt on port errors -- If there are one or more channels active, and a port error interrupt occurs (such as disconnection, over-current), the active channels will not have an interrupt. Each need to be manually disabled to obtain an interrupt. +- If there are one or more channels active, and a port error interrupt occurs (such as disconnection, over-current), the active channels will not have an interrupt. +- The channels will remain active (i.e., `HCCHAR.ChEna` will still be set) +- Normal methods of disabling the channel (via setting `HCCHAR.ChDis` and waiting for an interrupt) will not work for ISOC channels (the interrupt will never be generated). +- Therefore, on port errors, just treat all the channels as halted and treat their in-flight transfers as failed. The soft reset that occurs after will reset all the channel's registers. ## Reset @@ -66,7 +69,6 @@ The HAL layer abstracts the DWC_OTG operating in Host Mode using Internal Scatte - If you need to halt the channel early (such as aborting a transfer), call `usbh_hal_chan_request_halt()` - In case of a channel error event: - Call `usbh_hal_chan_get_error()` to get the specific channel error that occurred - - You must call `usbh_hal_chan_clear_error()` after an error to clear the error and allow the channel to continue to be used. # Host Controller Driver (HCD) diff --git a/components/usb/test/hcd/test_hcd_common.c b/components/usb/test/hcd/test_hcd_common.c index 93a22d8fd9..03217d83cf 100644 --- a/components/usb/test/hcd/test_hcd_common.c +++ b/components/usb/test/hcd/test_hcd_common.c @@ -9,13 +9,11 @@ #include "freertos/FreeRTOS.h" #include "freertos/semphr.h" #include "test_utils.h" -#include "soc/gpio_pins.h" -#include "soc/gpio_sig_map.h" +#include "soc/usb_wrap_struct.h" #include "esp_intr_alloc.h" #include "esp_err.h" #include "esp_attr.h" #include "esp_rom_gpio.h" -#include "soc/usb_wrap_struct.h" #include "hcd.h" #include "usb_private.h" #include "usb/usb_types_ch9.h" @@ -139,18 +137,23 @@ int test_hcd_get_num_pipe_events(hcd_pipe_handle_t pipe_hdl) void test_hcd_force_conn_state(bool connected, TickType_t delay_ticks) { - vTaskDelay(delay_ticks); + if (delay_ticks > 0) { + //Delay of 0 ticks causes a yield. So skip if delay_ticks is 0. + vTaskDelay(delay_ticks); + } usb_wrap_dev_t *wrap = &USB_WRAP; if (connected) { - //Swap back to internal PHY that is connected to a device - wrap->otg_conf.phy_sel = 0; + //Disable test mode to return to previous internal PHY configuration + wrap->test_conf.test_enable = 0; } else { - //Set external PHY input signals to fixed voltage levels mimicking a disconnected state - esp_rom_gpio_connect_in_signal(GPIO_MATRIX_CONST_ZERO_INPUT, USB_EXTPHY_VP_IDX, false); - esp_rom_gpio_connect_in_signal(GPIO_MATRIX_CONST_ZERO_INPUT, USB_EXTPHY_VM_IDX, false); - esp_rom_gpio_connect_in_signal(GPIO_MATRIX_CONST_ONE_INPUT, USB_EXTPHY_RCV_IDX, false); - //Swap to the external PHY - wrap->otg_conf.phy_sel = 1; + /* + Mimic a disconnection by using the internal PHY's test mode. + Force Output Enable to 1 (even if the controller isn't outputting). With test_tx_dp and test_tx_dm set to 0, + this will look like a disconnection. + */ + wrap->test_conf.val = 0; + wrap->test_conf.test_usb_wrap_oe = 1; + wrap->test_conf.test_enable = 1; } } diff --git a/components/usb/test/hcd/test_hcd_isoc.c b/components/usb/test/hcd/test_hcd_isoc.c index cb9dbec971..2258a89cf5 100644 --- a/components/usb/test/hcd/test_hcd_isoc.c +++ b/components/usb/test/hcd/test_hcd_isoc.c @@ -17,6 +17,7 @@ #define NUM_PACKETS_PER_URB 3 #define ISOC_PACKET_SIZE MOCK_ISOC_EP_MPS #define URB_DATA_BUFF_SIZE (NUM_PACKETS_PER_URB * ISOC_PACKET_SIZE) +#define POST_ENQUEUE_DELAY_US 20 /* Test HCD ISOC pipe URBs @@ -78,6 +79,9 @@ TEST_CASE("Test HCD isochronous pipe URBs", "[hcd][ignore]") urb_t *urb = hcd_urb_dequeue(isoc_out_pipe); TEST_ASSERT_EQUAL(urb_list[urb_idx], urb); TEST_ASSERT_EQUAL(URB_CONTEXT_VAL, urb->transfer.context); + //Overall URB status and overall number of bytes + TEST_ASSERT_EQUAL(URB_DATA_BUFF_SIZE, urb->transfer.actual_num_bytes); + TEST_ASSERT_EQUAL(USB_TRANSFER_STATUS_COMPLETED, urb->transfer.status); for (int pkt_idx = 0; pkt_idx < NUM_PACKETS_PER_URB; pkt_idx++) { TEST_ASSERT_EQUAL(USB_TRANSFER_STATUS_COMPLETED, urb->transfer.isoc_packet_desc[pkt_idx].status); } @@ -92,3 +96,98 @@ TEST_CASE("Test HCD isochronous pipe URBs", "[hcd][ignore]") test_hcd_wait_for_disconn(port_hdl, false); test_hcd_teardown(port_hdl); } + +/* +Test a port sudden disconnect with an active ISOC pipe + +Purpose: Test that when sudden disconnection happens on an HCD port, the ISOC pipe will + - Remain active after the HCD_PORT_EVENT_SUDDEN_DISCONN port event + - ISOC pipe can be halted + - ISOC pipe can be flushed (and transfers status are updated accordingly) + +Procedure: + - Setup HCD and wait for connection + - Allocate default pipe and enumerate the device + - Allocate an isochronous pipe and multiple URBs. Each URB should contain multiple packets to test HCD's ability to + schedule an URB across multiple intervals. + - Enqueue those URBs + - Trigger a disconnect after a short delay + - Check that HCD_PORT_EVENT_SUDDEN_DISCONN event is generated. Handle that port event. + - Check that both pipes remain in the HCD_PIPE_STATE_ACTIVE after the port error. + - Check that both pipes pipe can be halted. + - Check that the default pipe can be flushed. A HCD_PIPE_EVENT_URB_DONE event should be generated for the ISOC pipe + because it had enqueued URBs. + - Check that all URBs can be dequeued and their status is updated + - Free both pipes + - Teardown +*/ +TEST_CASE("Test HCD isochronous pipe sudden disconnect", "[hcd][ignore]") +{ + hcd_port_handle_t port_hdl = test_hcd_setup(); //Setup the HCD and port + usb_speed_t port_speed = test_hcd_wait_for_conn(port_hdl); //Trigger a connection + //The MPS of the ISOC OUT pipe is quite large, so we need to bias the FIFO sizing + TEST_ASSERT_EQUAL(ESP_OK, hcd_port_set_fifo_bias(port_hdl, HCD_PORT_FIFO_BIAS_PTX)); + vTaskDelay(pdMS_TO_TICKS(100)); //Short delay send of SOF (for FS) or EOPs (for LS) + + //Enumerate and reset device + hcd_pipe_handle_t default_pipe = test_hcd_pipe_alloc(port_hdl, NULL, 0, port_speed); //Create a default pipe (using a NULL EP descriptor) + uint8_t dev_addr = test_hcd_enum_device(default_pipe); + + //Create ISOC OUT pipe to non-existent device + hcd_pipe_handle_t isoc_out_pipe = test_hcd_pipe_alloc(port_hdl, &mock_isoc_out_ep_desc, dev_addr + 1, port_speed); + //Create URBs + urb_t *urb_list[NUM_URBS]; + //Initialize URBs + for (int urb_idx = 0; urb_idx < NUM_URBS; urb_idx++) { + urb_list[urb_idx] = test_hcd_alloc_urb(NUM_PACKETS_PER_URB, URB_DATA_BUFF_SIZE); + urb_list[urb_idx]->transfer.num_bytes = URB_DATA_BUFF_SIZE; + urb_list[urb_idx]->transfer.context = URB_CONTEXT_VAL; + for (int pkt_idx = 0; pkt_idx < NUM_PACKETS_PER_URB; pkt_idx++) { + urb_list[urb_idx]->transfer.isoc_packet_desc[pkt_idx].num_bytes = ISOC_PACKET_SIZE; + //Each packet will consist of the same byte, but each subsequent packet's byte will increment (i.e., packet 0 transmits all 0x0, packet 1 transmits all 0x1) + memset(&urb_list[urb_idx]->transfer.data_buffer[pkt_idx * ISOC_PACKET_SIZE], (urb_idx * NUM_URBS) + pkt_idx, ISOC_PACKET_SIZE); + } + } + //Enqueue URBs + for (int i = 0; i < NUM_URBS; i++) { + TEST_ASSERT_EQUAL(ESP_OK, hcd_urb_enqueue(isoc_out_pipe, urb_list[i])); + } + //Add a short delay to let the transfers run for a bit + esp_rom_delay_us(POST_ENQUEUE_DELAY_US); + test_hcd_force_conn_state(false, 0); + //Disconnect event should have occurred. Handle the port event + test_hcd_expect_port_event(port_hdl, HCD_PORT_EVENT_DISCONNECTION); + TEST_ASSERT_EQUAL(HCD_PORT_EVENT_DISCONNECTION, hcd_port_handle_event(port_hdl)); + TEST_ASSERT_EQUAL(HCD_PORT_STATE_RECOVERY, hcd_port_get_state(port_hdl)); + printf("Sudden disconnect\n"); + + //Both pipes should still be active + TEST_ASSERT_EQUAL(HCD_PIPE_STATE_ACTIVE, hcd_pipe_get_state(default_pipe)); + TEST_ASSERT_EQUAL(HCD_PIPE_STATE_ACTIVE, hcd_pipe_get_state(isoc_out_pipe)); + //Halt both pipes + TEST_ASSERT_EQUAL(ESP_OK, hcd_pipe_command(default_pipe, HCD_PIPE_CMD_HALT)); + TEST_ASSERT_EQUAL(ESP_OK, hcd_pipe_command(isoc_out_pipe, HCD_PIPE_CMD_HALT)); + TEST_ASSERT_EQUAL(HCD_PIPE_STATE_HALTED, hcd_pipe_get_state(default_pipe)); + TEST_ASSERT_EQUAL(HCD_PIPE_STATE_HALTED, hcd_pipe_get_state(isoc_out_pipe)); + //Flush both pipes. ISOC pipe should return completed URBs + TEST_ASSERT_EQUAL(ESP_OK, hcd_pipe_command(default_pipe, HCD_PIPE_CMD_FLUSH)); + TEST_ASSERT_EQUAL(ESP_OK, hcd_pipe_command(isoc_out_pipe, HCD_PIPE_CMD_FLUSH)); + + //Dequeue ISOC URBs + for (int urb_idx = 0; urb_idx < NUM_URBS; urb_idx++) { + urb_t *urb = hcd_urb_dequeue(isoc_out_pipe); + TEST_ASSERT_EQUAL(urb_list[urb_idx], urb); + TEST_ASSERT_EQUAL(URB_CONTEXT_VAL, urb->transfer.context); + //The URB has either completed entirely or is marked as no_device + TEST_ASSERT(urb->transfer.status == USB_TRANSFER_STATUS_COMPLETED || urb->transfer.status == USB_TRANSFER_STATUS_NO_DEVICE); + } + + //Free URB list and pipe + for (int i = 0; i < NUM_URBS; i++) { + test_hcd_free_urb(urb_list[i]); + } + test_hcd_pipe_free(isoc_out_pipe); + test_hcd_pipe_free(default_pipe); + //Cleanup + test_hcd_teardown(port_hdl); +} diff --git a/components/usb/test/hcd/test_hcd_port.c b/components/usb/test/hcd/test_hcd_port.c index e7267de5f1..05ba751994 100644 --- a/components/usb/test/hcd/test_hcd_port.c +++ b/components/usb/test/hcd/test_hcd_port.c @@ -32,7 +32,7 @@ Procedure: - Start transfers but trigger a disconnect after a short delay - Check that HCD_PORT_EVENT_SUDDEN_DISCONN event is generated. Handle that port event. - Check that the default pipe remains in the HCD_PIPE_STATE_ACTIVE after the port error. - - Check that the default pipe can be halted, a HCD_PIPE_EVENT_URB_DONE event should be generated + - Check that the default pipe can be halted. - Check that the default pipe can be flushed, a HCD_PIPE_EVENT_URB_DONE event should be generated - Check that all URBs can be dequeued. - Free default pipe @@ -66,7 +66,7 @@ TEST_CASE("Test HCD port sudden disconnect", "[hcd][ignore]") //Add a short delay to let the transfers run for a bit esp_rom_delay_us(POST_ENQUEUE_DELAY_US); test_hcd_force_conn_state(false, 0); - //Disconnect event should have occurred. Handle the event + //Disconnect event should have occurred. Handle the port event test_hcd_expect_port_event(port_hdl, HCD_PORT_EVENT_DISCONNECTION); TEST_ASSERT_EQUAL(HCD_PORT_EVENT_DISCONNECTION, hcd_port_handle_event(port_hdl)); TEST_ASSERT_EQUAL(HCD_PORT_STATE_RECOVERY, hcd_port_get_state(port_hdl)); @@ -75,17 +75,17 @@ TEST_CASE("Test HCD port sudden disconnect", "[hcd][ignore]") //We should be able to halt then flush the pipe TEST_ASSERT_EQUAL(HCD_PIPE_STATE_ACTIVE, hcd_pipe_get_state(default_pipe)); TEST_ASSERT_EQUAL(ESP_OK, hcd_pipe_command(default_pipe, HCD_PIPE_CMD_HALT)); - test_hcd_expect_pipe_event(default_pipe, HCD_PIPE_EVENT_URB_DONE); + printf("Pipe halted\n"); TEST_ASSERT_EQUAL(HCD_PIPE_STATE_HALTED, hcd_pipe_get_state(default_pipe)); TEST_ASSERT_EQUAL(ESP_OK, hcd_pipe_command(default_pipe, HCD_PIPE_CMD_FLUSH)); test_hcd_expect_pipe_event(default_pipe, HCD_PIPE_EVENT_URB_DONE); - printf("Pipe halted and flushed\n"); + printf("Pipe flushed\n"); //Dequeue URBs for (int i = 0; i < NUM_URBS; i++) { urb_t *urb = hcd_urb_dequeue(default_pipe); TEST_ASSERT_EQUAL(urb_list[i], urb); - TEST_ASSERT(urb->transfer.status == USB_TRANSFER_STATUS_COMPLETED || urb->transfer.status == USB_TRANSFER_STATUS_CANCELED); + TEST_ASSERT(urb->transfer.status == USB_TRANSFER_STATUS_COMPLETED || urb->transfer.status == USB_TRANSFER_STATUS_NO_DEVICE); if (urb->transfer.status == USB_TRANSFER_STATUS_COMPLETED) { //We must have transmitted at least the setup packet, but device may return less than bytes requested TEST_ASSERT_GREATER_OR_EQUAL(sizeof(usb_setup_packet_t), urb->transfer.actual_num_bytes); @@ -259,7 +259,9 @@ TEST_CASE("Test HCD port disable", "[hcd][ignore]") for (int i = 0; i < NUM_URBS; i++) { urb_t *urb = hcd_urb_dequeue(default_pipe); TEST_ASSERT_EQUAL(urb_list[i], urb); - TEST_ASSERT(urb->transfer.status == USB_TRANSFER_STATUS_COMPLETED || urb->transfer.status == USB_TRANSFER_STATUS_CANCELED); + TEST_ASSERT(urb->transfer.status == USB_TRANSFER_STATUS_COMPLETED || //The transfer completed before the pipe halted + urb->transfer.status == USB_TRANSFER_STATUS_CANCELED || //The transfer was stopped mid-way by the halt + urb->transfer.status == USB_TRANSFER_STATUS_NO_DEVICE); //The transfer was never started if (urb->transfer.status == USB_TRANSFER_STATUS_COMPLETED) { //We must have transmitted at least the setup packet, but device may return less than bytes requested TEST_ASSERT_GREATER_OR_EQUAL(sizeof(usb_setup_packet_t), urb->transfer.actual_num_bytes); diff --git a/tools/ci/check_copyright_ignore.txt b/tools/ci/check_copyright_ignore.txt index b52fd65b49..b60ed13e75 100644 --- a/tools/ci/check_copyright_ignore.txt +++ b/tools/ci/check_copyright_ignore.txt @@ -1414,7 +1414,6 @@ components/hal/include/hal/uart_types.h components/hal/include/hal/uhci_types.h components/hal/include/hal/usb_hal.h components/hal/include/hal/usb_types_private.h -components/hal/include/hal/usbh_hal.h components/hal/include/hal/wdt_hal.h components/hal/include/hal/wdt_types.h components/hal/interrupt_controller_hal.c @@ -1450,7 +1449,6 @@ components/hal/twai_hal_iram.c components/hal/uart_hal.c components/hal/uart_hal_iram.c components/hal/usb_hal.c -components/hal/usbh_hal.c components/hal/wdt_hal_iram.c components/heap/heap_caps_init.c components/heap/heap_private.h