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

@@ -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);
 }
 
@@ -688,27 +651,33 @@ 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 Get a channel's error
+ * @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
- * @return usbh_hal_chan_error_t The type of error the channel has encountered
  */
-static inline usbh_hal_chan_error_t usbh_hal_chan_get_error(usbh_hal_chan_t *chan_obj)
+static inline void usbh_hal_chan_mark_halted(usbh_hal_chan_t *chan_obj)
 {
-    HAL_ASSERT(chan_obj->flags.error_pending);
-    return chan_obj->error;
+    chan_obj->flags.active = 0;
 }
 
 /**
- * @brief Clear a channel of it's error
+ * @brief Get a channel's error
  *
  * @param chan_obj Channel object
+ * @return usbh_hal_chan_error_t The type of error the channel has encountered
  */
-static inline void usbh_hal_chan_clear_error(usbh_hal_chan_t *chan_obj)
+static inline usbh_hal_chan_error_t usbh_hal_chan_get_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;
+    return chan_obj->error;
 }
 
 // -------------------------------------------- Transfer Descriptor List -----------------------------------------------

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 <stddef.h>
 #include <stdint.h>
@@ -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) {

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++) {

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()

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)
 

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;
     }
 }
 

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);
+}

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);