From a3501dfed173a84d889490d7cff725b70fd8c6f6 Mon Sep 17 00:00:00 2001 From: HiFiPhile Date: Thu, 12 Nov 2026 11:33:42 +0100 Subject: [PATCH 1/2] usbh: resend a failed enumeration request before resetting the port A device that does not answer its first request 10 ms after the port reset never enumerated (#3876): the retry reset the port again and waited the same 10 ms, so it failed the same way. Follow Linux's hub enumeration: - Wait 50 ms after the port reset (hub_port_reset: 10 + 40) and 10 ms after SET_ADDRESS (hub_port_init) instead of the USB 2.0 minimums of 10 and 2 ms. - When the device fails an enumeration request, send it again after 100 ms without a port reset, up to 3 tries (usb_get_descriptor, GET_DESCRIPTOR_TRIES with msleep(100)). A reset would restart the device's recovery. A timed-out request is not resent, the device already NAKed it for CFG_TUH_CONTROL_TIMEOUT_MS. Only then does CFG_TUH_ENUM_ATTEMPT_MAX reset the port and start over. The first request now reaches the device 50, 150 and 250 ms after the reset within one attempt. --- docs/reference/class/host.rst | 5 +- src/host/usbh.c | 56 ++- test/unit-test/project.yml | 4 + .../host/usbh/test_usbh_enum_xfer_retry.c | 384 ++++++++++++++++++ 4 files changed, 445 insertions(+), 4 deletions(-) create mode 100644 test/unit-test/test/host/usbh/test_usbh_enum_xfer_retry.c diff --git a/docs/reference/class/host.rst b/docs/reference/class/host.rst index 37c7898bfd..cf9ae2ac75 100644 --- a/docs/reference/class/host.rst +++ b/docs/reference/class/host.rst @@ -69,7 +69,10 @@ Common configuration options * - ``CFG_TUH_ENUM_ATTEMPT_MAX`` - ``2`` - Maximum enumeration attempts per attachment after core enumeration - transfers fail or time out; ``1`` disables retries. + transfers fail or time out; ``1`` disables retries. Each attempt resets + the port. Within an attempt, a request the device fails is first sent + again up to twice, 100 ms apart, without a port reset; a timed-out + request is not. * - ``CFG_TUH_ENUM_RESET_ROOT_POST_DELAY_MS`` - ``2`` - Delay in milliseconds after a root-port reset before reading the port diff --git a/src/host/usbh.c b/src/host/usbh.c index 06452c23fe..bb4e2b40a8 100644 --- a/src/host/usbh.c +++ b/src/host/usbh.c @@ -221,6 +221,7 @@ TU_FIFO_DEF(_usbh_pending_ctrl_q, CFG_TUH_CONTROL_PENDING_QUEUE_SZ * sizeof(usbh typedef struct { uint8_t enumerating_daddr; // device address of the device being enumerated uint8_t enum_attempt; // enumeration attempts used for the current attach + uint8_t enum_xfer_retry; // resends used for the current enumeration request, see enum_xfer_retry() uint8_t attach_debouncing_bm; // bitmask for roothub port attach debouncing uint8_t attach_teardown; // an attach event is tearing down what was on its port uint8_t root_retry_count; // root port re-enumerations used for the current attachment @@ -228,6 +229,7 @@ typedef struct { tuh_bus_info_t dev0_bus; // bus info for dev0 in enumeration usbh_ctrl_xfer_info_t ctrl_xfer_info; // control transfer usbh_call_after_t call_after; + usbh_pending_ctrl_t enum_xfer; // the failed enumeration request enum_xfer_retry() sends again // Per-daddr generation counter — bumped on usbh_device_close() to identify stale pending control transfer uint8_t daddr_gen[TOTAL_DEVICES + 1]; #if CFG_TUSB_OS_HAS_SCHEDULER @@ -1871,8 +1873,13 @@ enum { // USB 2.0 specs 7.1.7 for timing ENUM_RESET_ROOT_DELAY_MS = 50, // T(DRSTr) minimum 50 ms for reset from root port ENUM_RESET_ROOT_POST_DELAY_MS = CFG_TUH_ENUM_RESET_ROOT_POST_DELAY_MS, // delay after root port reset before getting speed/status ENUM_RESET_HUB_DELAY_MS = 20, // T(DRST) 10-20 ms for hub reset - ENUM_RESET_RECOVERY_DELAY_MS = 10, // T(RSTRCY) minimum 10 ms for reset recovery - ENUM_SET_ADDRESS_RECOVERY_DELAY_MS = 2, // USB 2.0 Spec 9.2.6.3 min is 2 ms + ENUM_RESET_RECOVERY_DELAY_MS = 50, // T(RSTRCY) minimum 10 ms; 50 as Linux (hub_port_reset), many devices need more + ENUM_SET_ADDRESS_RECOVERY_DELAY_MS = 10, // USB 2.0 Spec 9.2.6.3 min is 2 ms; 10 as Linux (hub_port_init) + ENUM_XFER_RETRY_DELAY_MS = 100, // gap before a failed enumeration request is sent again, as Linux +}; + +enum { + ENUM_XFER_ATTEMPT_MAX = 3, // tries of one enumeration request before a port reset, as Linux usb_get_descriptor() }; enum { @@ -1903,6 +1910,7 @@ static uint8_t enum_get_new_address(bool is_hub); static bool enum_parse_configuration_desc(uint8_t dev_addr, const tusb_desc_configuration_t *desc_cfg); static void enum_full_complete(bool success); static bool enum_retry(void); +static bool enum_xfer_retry(const tuh_xfer_t *xfer); static void process_enumeration(tuh_xfer_t *xfer); enum { @@ -1913,6 +1921,7 @@ enum { ENUM_AFTER_RESET_HUB_DELAY_RETRY, ENUM_AFTER_RESET_RECOVERY_DELAY, ENUM_AFTER_SET_ADDRESS_RECOVERY_DELAY, + ENUM_AFTER_XFER_RETRY_DELAY, }; // process async delay in enumeration @@ -2009,6 +2018,22 @@ static void enum_delay_async(uintptr_t state) { break; } + case ENUM_AFTER_XFER_RETRY_DELAY: { + const usbh_pending_ctrl_t *saved = &_usbh_data.enum_xfer; + tuh_xfer_t retry_xfer = { + .daddr = saved->daddr, + .ep_addr = 0, + .setup = &saved->setup, + .buffer = saved->buffer, + .complete_cb = process_enumeration, + .user_data = saved->user_data, + }; + if (!tuh_control_xfer(&retry_xfer)) { + enum_full_complete(false); + } + break; + } + default: break; } @@ -2021,17 +2046,19 @@ static void enum_new_device(hcd_event_t *event) { dev0_bus->hub_addr = event->connection.hub_addr; dev0_bus->hub_port = event->connection.hub_port; _usbh_data.enum_attempt = 0; + _usbh_data.enum_xfer_retry = 0; usbh_defer_func_ms_async(ENUM_DEBOUNCING_DELAY_MS, enum_delay_async, ENUM_AFTER_DEBOUNCING_DELAY); } // process device enumeration static void process_enumeration(tuh_xfer_t *xfer) { if (XFER_RESULT_FAILED == xfer->result || XFER_RESULT_TIMEOUT == xfer->result) { - if (!enum_retry()) { + if (!enum_xfer_retry(xfer) && !enum_retry()) { enum_full_complete(false); } return; } + _usbh_data.enum_xfer_retry = 0; // the device answered: the next request gets its own tries const uint8_t daddr = xfer->daddr; const uintptr_t state = xfer->user_data; @@ -2533,6 +2560,7 @@ static bool enum_retry(void) { } enum_teardown(false); _usbh_data.enum_attempt++; + _usbh_data.enum_xfer_retry = 0; TU_LOG_USBH("Enumeration failed, retry attempt %u/%u\r\n", _usbh_data.enum_attempt + 1, CFG_TUH_ENUM_ATTEMPT_MAX); _usbh_data.enumerating_daddr = 0; usbh_defer_func_ms_async(ENUM_DEBOUNCING_DELAY_MS, enum_delay_async, ENUM_AFTER_DEBOUNCING_DELAY); @@ -2542,4 +2570,26 @@ static bool enum_retry(void) { #endif } +// Device failed an enumeration request (after the immediate USBH_CONTROL_RETRY_MAX resends): send it again after +// ENUM_XFER_RETRY_DELAY_MS without a port reset, so a device still recovering from the reset gets more time; a reset +// would restart its recovery. A timeout is not retried, the device already NAKed for CFG_TUH_CONTROL_TIMEOUT_MS. +// False (nothing changed) when the request's tries are used up or an unplug already invalidated the device. +static bool enum_xfer_retry(const tuh_xfer_t *xfer) { + if (XFER_RESULT_FAILED != xfer->result || _usbh_data.enumerating_daddr == TUSB_INDEX_INVALID_8 || + _usbh_data.enum_xfer_retry + 1 >= ENUM_XFER_ATTEMPT_MAX) { + return false; + } + _usbh_data.enum_xfer_retry++; + TU_LOG_USBH("Enumeration request failed, try %u/%u in %u ms\r\n", _usbh_data.enum_xfer_retry + 1, + ENUM_XFER_ATTEMPT_MAX, ENUM_XFER_RETRY_DELAY_MS); + + usbh_pending_ctrl_t *saved = &_usbh_data.enum_xfer; + saved->setup = *xfer->setup; + saved->buffer = xfer->buffer; + saved->user_data = xfer->user_data; + saved->daddr = xfer->daddr; + usbh_defer_func_ms_async(ENUM_XFER_RETRY_DELAY_MS, enum_delay_async, ENUM_AFTER_XFER_RETRY_DELAY); + return true; +} + #endif diff --git a/test/unit-test/project.yml b/test/unit-test/project.yml index 731b6cd0d5..51e871b1d0 100644 --- a/test/unit-test/project.yml +++ b/test/unit-test/project.yml @@ -203,6 +203,10 @@ - CFG_TUH_DEVICE_MAX=3 - CFG_TUH_HUB=2 - CFG_TUSB_DEBUG=0 + :test_usbh_enum_xfer_retry: + - CFG_TUD_ENABLED=0 + - CFG_TUH_ENABLED=1 + - CFG_TUSB_DEBUG=0 :test_midi2_device_gtb_default: - CFG_TUD_MSC=0 - CFG_TUD_MIDI2=1 diff --git a/test/unit-test/test/host/usbh/test_usbh_enum_xfer_retry.c b/test/unit-test/test/host/usbh/test_usbh_enum_xfer_retry.c new file mode 100644 index 0000000000..3736d35bf5 --- /dev/null +++ b/test/unit-test/test/host/usbh/test_usbh_enum_xfer_retry.c @@ -0,0 +1,384 @@ +/* + * SPDX-FileCopyrightText: Copyright (c) 2026 TinyUSB contributors + * SPDX-License-Identifier: MIT + */ + +#include "unity.h" + +// Include the production implementation to drive the enumeration state machine directly. +#include "host/usbh.c" + +TEST_SOURCE_FILE("tusb.c") +TEST_SOURCE_FILE("tusb_fifo.c") + +enum { + RHPORT = 0, +}; + +static uint32_t now_ms; +static bool port_connected; +static uint32_t port_reset_count; +static uint32_t setup_send_count; +static uint8_t setup_send_daddr; + +//--------------------------------------------------------------------+ +// HCD stubs +//--------------------------------------------------------------------+ +uint32_t tusb_time_millis_api(void) { + return now_ms; +} + +bool hcd_init(uint8_t rhport, const tusb_rhport_init_t *rh_init) { + (void) rhport; + (void) rh_init; + return true; +} + +void hcd_int_handler(uint8_t rhport, bool in_isr) { + (void) rhport; + (void) in_isr; +} + +void hcd_int_enable(uint8_t rhport) { + (void) rhport; +} + +void hcd_int_disable(uint8_t rhport) { + (void) rhport; +} + +bool hcd_port_connect_status(uint8_t rhport) { + return rhport == RHPORT && port_connected; +} + +void hcd_port_reset(uint8_t rhport) { + (void) rhport; + port_reset_count++; +} + +void hcd_port_reset_end(uint8_t rhport) { + (void) rhport; +} + +tusb_speed_t hcd_port_speed_get(uint8_t rhport) { + (void) rhport; + return TUSB_SPEED_FULL; +} + +void hcd_device_close(uint8_t rhport, uint8_t daddr) { + (void) rhport; + (void) daddr; +} + +bool hcd_setup_send(uint8_t rhport, uint8_t daddr, const uint8_t setup_packet[8]) { + (void) rhport; + (void) setup_packet; + setup_send_count++; + setup_send_daddr = daddr; + return true; +} + +bool hcd_edpt_open(uint8_t rhport, uint8_t daddr, const tusb_desc_endpoint_t *ep_desc) { + (void) rhport; + (void) daddr; + (void) ep_desc; + return true; +} + +bool hcd_edpt_close(uint8_t rhport, uint8_t daddr, uint8_t ep_addr) { + (void) rhport; + (void) daddr; + (void) ep_addr; + return true; +} + +bool hcd_edpt_xfer(uint8_t rhport, uint8_t daddr, uint8_t ep_addr, uint8_t *buffer, uint16_t buflen) { + (void) rhport; + (void) daddr; + (void) ep_addr; + (void) buffer; + (void) buflen; + return true; +} + +bool hcd_edpt_abort_xfer(uint8_t rhport, uint8_t daddr, uint8_t ep_addr) { + (void) rhport; + (void) daddr; + (void) ep_addr; + return true; +} + +bool hcd_edpt_clear_stall(uint8_t rhport, uint8_t daddr, uint8_t ep_addr) { + (void) rhport; + (void) daddr; + (void) ep_addr; + return true; +} + +//--------------------------------------------------------------------+ +// Helpers +//--------------------------------------------------------------------+ +// Length in ms of the pending enumeration delay +static uint32_t pending_delay_ms(void) { + TEST_ASSERT_EQUAL_PTR(enum_delay_async, _usbh_data.call_after.func); + return _usbh_data.call_after.at_ms - now_ms - 1; +} + +// Run the pending enumeration delay as tuh_task_ext() does once it is due +static void fire_delay(void) { + const tusb_defer_func_t func = _usbh_data.call_after.func; + TEST_ASSERT_NOT_NULL(func); + now_ms = _usbh_data.call_after.at_ms; + _usbh_data.call_after.func = NULL; + func(_usbh_data.call_after.arg); +} + +// Handle the next queued host event as tuh_task_ext() does +static void process_next_event(void) { + hcd_event_t event; + TEST_ASSERT_TRUE(osal_queue_receive(_usbh_q, &event, 0)); + TEST_ASSERT_TRUE(usbh_process_event(&event, false)); +} + +// Attach a device to the root port and run debouncing and the port reset up to the reset recovery +static void attach_to_reset_recovery(void) { + port_connected = true; + hcd_event_device_attach(RHPORT, false); + process_next_event(); + while (_usbh_data.call_after.arg != ENUM_AFTER_RESET_RECOVERY_DELAY) { + fire_delay(); + } +} + +// Send the first request: GET_DESCRIPTOR(device, 8) to address 0 +static void send_first_request(void) { + attach_to_reset_recovery(); + fire_delay(); + TEST_ASSERT_EQUAL(CONTROL_STAGE_SETUP, _usbh_data.ctrl_xfer_info.stage); + TEST_ASSERT_EQUAL_UINT8(TUSB_REQ_GET_DESCRIPTOR, _usbh_epbuf.request.bRequest); + TEST_ASSERT_EQUAL_UINT16(8, _usbh_epbuf.request.wLength); +} + +// The device does not answer the request in flight: the hcd fails it, usbh resends it at once until +// USBH_CONTROL_RETRY_MAX, then completes it as FAILED +static void request_fails(void) { + const uint8_t daddr = _usbh_data.ctrl_xfer_info.daddr; + setup_send_count = 0; + while (_usbh_data.ctrl_xfer_info.stage != CONTROL_STAGE_IDLE) { + usbh_control_xfer_cb(daddr, 0, XFER_RESULT_FAILED, 0); + } + TEST_ASSERT_EQUAL_UINT32(USBH_CONTROL_RETRY_MAX, setup_send_count); +} + +// The device answers the request in flight with the given data +static void request_succeeds(const void *data, uint16_t len) { + const uint8_t daddr = _usbh_data.ctrl_xfer_info.daddr; + const bool in = _usbh_epbuf.request.bmRequestType_bit.direction == TUSB_DIR_IN; + usbh_control_xfer_cb(daddr, 0, XFER_RESULT_SUCCESS, 0); // SETUP + if (_usbh_epbuf.request.wLength > 0) { + if (in) { + memcpy(_usbh_data.ctrl_xfer_info.buffer, data, len); + } + usbh_control_xfer_cb(daddr, in ? 0x80 : 0x00, XFER_RESULT_SUCCESS, len); // DATA + } + usbh_control_xfer_cb(daddr, in ? 0x00 : 0x80, XFER_RESULT_SUCCESS, 0); // STATUS +} + +static const tusb_desc_device_t desc_device = { + .bLength = sizeof(tusb_desc_device_t), + .bDescriptorType = TUSB_DESC_DEVICE, + .bcdUSB = 0x0200, + .bDeviceClass = 0, + .bDeviceSubClass = 0, + .bDeviceProtocol = 0, + .bMaxPacketSize0 = 64, + .idVendor = 0xCafe, + .idProduct = 0x4000, + .bcdDevice = 0x0100, + .iManufacturer = 0, + .iProduct = 0, + .iSerialNumber = 0, + .bNumConfigurations = 1, +}; + +//--------------------------------------------------------------------+ +// Setup +//--------------------------------------------------------------------+ +void setUp(void) { + tu_memclr(&_usbh_data, sizeof(_usbh_data)); + tu_memclr(_usbh_devices, sizeof(_usbh_devices)); + now_ms = 1000; + port_connected = false; + port_reset_count = 0; + setup_send_count = 0; + setup_send_daddr = TUSB_INDEX_INVALID_8; + _usbh_data.enumerating_daddr = TUSB_INDEX_INVALID_8; + _usbh_q = osal_queue_create(&_usbh_qdef); + tu_fifo_clear(&_usbh_pending_ctrl_q); +} + +void tearDown(void) { +} + +//--------------------------------------------------------------------+ +// Tests +//--------------------------------------------------------------------+ +// The first request waits 50 ms after the port reset, as Linux, not the 10 ms USB 2.0 minimum (#3876) +void test_reset_recovery_is_50ms(void) { + attach_to_reset_recovery(); + TEST_ASSERT_EQUAL_UINT32(1, port_reset_count); + TEST_ASSERT_EQUAL_UINT32(50, pending_delay_ms()); +} + +// A failed request is sent again after the retry gap, with no port reset in between: the device keeps recovering from +// the reset it already had +void test_failed_request_is_sent_again_without_port_reset(void) { + send_first_request(); + const tusb_control_request_t request = _usbh_epbuf.request; + + request_fails(); + TEST_ASSERT_EQUAL(ENUM_AFTER_XFER_RETRY_DELAY, _usbh_data.call_after.arg); + TEST_ASSERT_EQUAL_UINT32(ENUM_XFER_RETRY_DELAY_MS, pending_delay_ms()); + + fire_delay(); + TEST_ASSERT_EQUAL(CONTROL_STAGE_SETUP, _usbh_data.ctrl_xfer_info.stage); + TEST_ASSERT_EQUAL_UINT8(0, setup_send_daddr); + TEST_ASSERT_EQUAL_MEMORY(&request, &_usbh_epbuf.request, sizeof(request)); + TEST_ASSERT_EQUAL_UINT32(1, port_reset_count); + TEST_ASSERT_EQUAL_UINT8(0, _usbh_data.enum_attempt); +} + +// A device silent for up to 250 ms after the reset answers within the first attempt: tries at 50, 150 and 250 ms +void test_tries_span_250ms_after_reset(void) { + attach_to_reset_recovery(); + const uint32_t reset_done_ms = now_ms; + fire_delay(); + TEST_ASSERT_EQUAL_UINT32(50, now_ms - reset_done_ms - 1); + + for (uint32_t i = 1; i < ENUM_XFER_ATTEMPT_MAX; i++) { + request_fails(); + fire_delay(); + TEST_ASSERT_EQUAL(CONTROL_STAGE_SETUP, _usbh_data.ctrl_xfer_info.stage); + } + TEST_ASSERT_EQUAL_UINT32(50 + 1 + (ENUM_XFER_ATTEMPT_MAX - 1) * (ENUM_XFER_RETRY_DELAY_MS + 1), + now_ms - reset_done_ms); + TEST_ASSERT_EQUAL_UINT32(1, port_reset_count); +} + +// Once the request's tries are used up, the attempt fails and the next one starts with a port reset +void test_last_try_failing_resets_port(void) { + send_first_request(); + for (uint32_t i = 1; i < ENUM_XFER_ATTEMPT_MAX; i++) { + request_fails(); + fire_delay(); + } + request_fails(); + + TEST_ASSERT_EQUAL(ENUM_AFTER_DEBOUNCING_DELAY, _usbh_data.call_after.arg); + TEST_ASSERT_EQUAL_UINT8(1, _usbh_data.enum_attempt); + TEST_ASSERT_EQUAL_UINT8(0, _usbh_data.enum_xfer_retry); + + // the next attempt's request gets all its tries again + while (_usbh_data.call_after.arg != ENUM_AFTER_RESET_RECOVERY_DELAY) { + fire_delay(); + } + TEST_ASSERT_EQUAL_UINT32(2, port_reset_count); + fire_delay(); + request_fails(); + TEST_ASSERT_EQUAL(ENUM_AFTER_XFER_RETRY_DELAY, _usbh_data.call_after.arg); +} + +// A timeout is not sent again: the device already NAKed it for CFG_TUH_CONTROL_TIMEOUT_MS +void test_timeout_is_not_sent_again(void) { + send_first_request(); + now_ms += CFG_TUH_CONTROL_TIMEOUT_MS + 1; + control_xfer_timeout_expired(); + + TEST_ASSERT_EQUAL(ENUM_AFTER_DEBOUNCING_DELAY, _usbh_data.call_after.arg); + TEST_ASSERT_EQUAL_UINT8(1, _usbh_data.enum_attempt); +} + +// A request the device answers gives the next request its own tries, and SET_ADDRESS waits 10 ms to settle, as Linux +void test_answer_restores_tries_and_set_address_recovery_is_10ms(void) { + send_first_request(); + request_fails(); + fire_delay(); + TEST_ASSERT_EQUAL_UINT8(1, _usbh_data.enum_xfer_retry); + + request_succeeds(&desc_device, 8); + TEST_ASSERT_EQUAL_UINT8(0, _usbh_data.enum_xfer_retry); + TEST_ASSERT_EQUAL(CONTROL_STAGE_SETUP, _usbh_data.ctrl_xfer_info.stage); + TEST_ASSERT_EQUAL_UINT8(TUSB_REQ_SET_ADDRESS, _usbh_epbuf.request.bRequest); + const uint8_t new_addr = (uint8_t) tu_le16toh(_usbh_epbuf.request.wValue); + + // SET_ADDRESS fails too: it gets its own tries + request_fails(); + TEST_ASSERT_EQUAL(ENUM_AFTER_XFER_RETRY_DELAY, _usbh_data.call_after.arg); + fire_delay(); + TEST_ASSERT_EQUAL_UINT8(TUSB_REQ_SET_ADDRESS, _usbh_epbuf.request.bRequest); + TEST_ASSERT_EQUAL_UINT16(new_addr, tu_le16toh(_usbh_epbuf.request.wValue)); + + request_succeeds(NULL, 0); + TEST_ASSERT_EQUAL(ENUM_AFTER_SET_ADDRESS_RECOVERY_DELAY, _usbh_data.call_after.arg); + TEST_ASSERT_EQUAL_UINT32(10, pending_delay_ms()); + TEST_ASSERT_EQUAL_UINT8(new_addr, _usbh_data.enumerating_daddr); + + // a request to the new address is sent again to that address + fire_delay(); + TEST_ASSERT_EQUAL_UINT8(new_addr, setup_send_daddr); + request_fails(); + fire_delay(); + TEST_ASSERT_EQUAL_UINT8(new_addr, setup_send_daddr); + TEST_ASSERT_EQUAL_UINT8(TUSB_REQ_GET_DESCRIPTOR, _usbh_epbuf.request.bRequest); + TEST_ASSERT_EQUAL_UINT16(sizeof(tusb_desc_device_t), _usbh_epbuf.request.wLength); +} + +// An unplug during the retry gap cancels the resend +void test_unplug_during_gap_cancels_resend(void) { + send_first_request(); + request_fails(); + TEST_ASSERT_EQUAL(ENUM_AFTER_XFER_RETRY_DELAY, _usbh_data.call_after.arg); + + port_connected = false; + hcd_event_device_remove(RHPORT, false); + process_next_event(); + + TEST_ASSERT_NULL(_usbh_data.call_after.func); + TEST_ASSERT_EQUAL_UINT8(TUSB_INDEX_INVALID_8, _usbh_data.enumerating_daddr); + TEST_ASSERT_TRUE(osal_queue_empty(_usbh_q)); +} + +// A request failed by an unplug is not sent again +void test_request_failed_by_unplug_is_not_sent_again(void) { + send_first_request(); + + port_connected = false; + hcd_event_device_remove(RHPORT, false); + process_next_event(); // the unplug fails the request in flight + + TEST_ASSERT_NULL(_usbh_data.call_after.func); + TEST_ASSERT_EQUAL(CONTROL_STAGE_IDLE, _usbh_data.ctrl_xfer_info.stage); + TEST_ASSERT_TRUE(osal_queue_empty(_usbh_q)); +} + +// Another device's control transfer holds the slot when the gap ends: the resend is queued and sent once it is free +void test_resend_waits_for_busy_control_slot(void) { + send_first_request(); + const tusb_control_request_t request = _usbh_epbuf.request; + request_fails(); + + // another device's transfer takes the slot during the gap + _usbh_data.ctrl_xfer_info.stage = CONTROL_STAGE_DATA; + _usbh_data.ctrl_xfer_info.daddr = 1; + setup_send_count = 0; + fire_delay(); + TEST_ASSERT_EQUAL_UINT32(0, setup_send_count); + TEST_ASSERT_EQUAL_UINT8(0, _usbh_data.enumerating_daddr); + + _usbh_data.ctrl_xfer_info.stage = CONTROL_STAGE_IDLE; + control_xfer_dispatch_pending(); + TEST_ASSERT_EQUAL_UINT32(1, setup_send_count); + TEST_ASSERT_EQUAL_UINT8(0, setup_send_daddr); + TEST_ASSERT_EQUAL_MEMORY(&request, &_usbh_epbuf.request, sizeof(request)); + TEST_ASSERT_EQUAL_PTR(process_enumeration, _usbh_data.ctrl_xfer_info.complete_cb); +} From 7877af326445046553e932b34ac0c662926e0dae Mon Sep 17 00:00:00 2001 From: HiFiPhile Date: Thu, 12 Nov 2026 12:39:33 +0100 Subject: [PATCH 2/2] usbh: drop an earlier attempt's enumeration request completion An enumeration request queued behind another device's control transfer outlives an unplug. When the pending queue drains, it completes the stale entry as FAILED, and process_enumeration() took that failure as the next device's: it resent the old request or reset the port under the new enumeration. Tag every enumeration request's user_data with an attempt generation, bumped when an enumeration attempt starts, and drop completions of an earlier attempt. Also: - a resend that cannot be submitted falls back to enum_retry() before ending the enumeration - enum_xfer_retry() fails when its delay cannot be scheduled --- src/host/usbh.c | 71 +++++++++++-------- .../host/usbh/test_usbh_enum_xfer_retry.c | 52 ++++++++++++++ 2 files changed, 95 insertions(+), 28 deletions(-) diff --git a/src/host/usbh.c b/src/host/usbh.c index bb4e2b40a8..57aa7187ef 100644 --- a/src/host/usbh.c +++ b/src/host/usbh.c @@ -222,6 +222,7 @@ typedef struct { uint8_t enumerating_daddr; // device address of the device being enumerated uint8_t enum_attempt; // enumeration attempts used for the current attach uint8_t enum_xfer_retry; // resends used for the current enumeration request, see enum_xfer_retry() + uint8_t enum_gen; // bumped when an enumeration attempt starts, carried by its requests, see enum_ud() uint8_t attach_debouncing_bm; // bitmask for roothub port attach debouncing uint8_t attach_teardown; // an attach event is tearing down what was on its port uint8_t root_retry_count; // root port re-enumerations used for the current attachment @@ -1913,6 +1914,13 @@ static bool enum_retry(void); static bool enum_xfer_retry(const tuh_xfer_t *xfer); static void process_enumeration(tuh_xfer_t *xfer); +// user_data of an enumeration request: the state that handles its completion, tagged with the attempt that sent it. +// A request queued behind another device's control transfer outlives an unplug; the pending queue then completes it +// as FAILED, and that must not count against the attempt enumerating the next device. +TU_ATTR_ALWAYS_INLINE static inline uintptr_t enum_ud(uintptr_t state) { + return state | ((uintptr_t) _usbh_data.enum_gen << 8); +} + enum { ENUM_AFTER_DEBOUNCING_DELAY, ENUM_AFTER_RESET_ROOT_DELAY, @@ -1934,7 +1942,7 @@ static void enum_delay_async(uintptr_t state) { // connected via hub TU_VERIFY(dev0_bus->hub_port != 0, ); if (!hub_port_get_status(dev0_bus->hub_addr, dev0_bus->hub_port, NULL, process_enumeration, - ENUM_HUB_RERSET)) { + enum_ud(ENUM_HUB_RERSET))) { enum_full_complete(false); } } else @@ -1971,7 +1979,7 @@ static void enum_delay_async(uintptr_t state) { tuh_xfer_t xfer; xfer.daddr = 0; xfer.result = XFER_RESULT_SUCCESS; - xfer.user_data = ENUM_ADDR0_DEVICE_DESC; + xfer.user_data = enum_ud(ENUM_ADDR0_DEVICE_DESC); process_enumeration(&xfer); break; @@ -1980,8 +1988,8 @@ static void enum_delay_async(uintptr_t state) { case ENUM_AFTER_RESET_HUB_DELAY_RETRY: // get status after reset complete to check for reset change if (!hub_port_get_status(dev0_bus->hub_addr, dev0_bus->hub_port, NULL, process_enumeration, - state == ENUM_AFTER_RESET_HUB_DELAY ? ENUM_HUB_CLEAR_RESET - : ENUM_HUB_CLEAR_RESET_RETRY)) { + enum_ud(state == ENUM_AFTER_RESET_HUB_DELAY ? ENUM_HUB_CLEAR_RESET + : ENUM_HUB_CLEAR_RESET_RETRY))) { enum_full_complete(false); } break; @@ -1996,7 +2004,7 @@ static void enum_delay_async(uintptr_t state) { } // Get first 8 bytes of device descriptor for control endpoint size TU_LOG_USBH("Get 8 byte of Device Descriptor\r\n"); - if (!tuh_descriptor_get_device(0, _usbh_epbuf.ctrl, 8, process_enumeration, ENUM_SET_ADDR)) { + if (!tuh_descriptor_get_device(0, _usbh_epbuf.ctrl, 8, process_enumeration, enum_ud(ENUM_SET_ADDR))) { enum_full_complete(false); } break; @@ -2012,7 +2020,7 @@ static void enum_delay_async(uintptr_t state) { } TU_LOG_USBH("Get Device Descriptor\r\n"); if (!tuh_descriptor_get_device(new_addr, _usbh_epbuf.ctrl, sizeof(tusb_desc_device_t), process_enumeration, - ENUM_GET_STRING_LANGUAGE_ID_LEN)) { + enum_ud(ENUM_GET_STRING_LANGUAGE_ID_LEN))) { enum_full_complete(false); } break; @@ -2028,7 +2036,7 @@ static void enum_delay_async(uintptr_t state) { .complete_cb = process_enumeration, .user_data = saved->user_data, }; - if (!tuh_control_xfer(&retry_xfer)) { + if (!tuh_control_xfer(&retry_xfer) && !enum_retry()) { enum_full_complete(false); } break; @@ -2047,11 +2055,17 @@ static void enum_new_device(hcd_event_t *event) { dev0_bus->hub_port = event->connection.hub_port; _usbh_data.enum_attempt = 0; _usbh_data.enum_xfer_retry = 0; + _usbh_data.enum_gen++; usbh_defer_func_ms_async(ENUM_DEBOUNCING_DELAY_MS, enum_delay_async, ENUM_AFTER_DEBOUNCING_DELAY); } // process device enumeration static void process_enumeration(tuh_xfer_t *xfer) { + if ((uint8_t) (xfer->user_data >> 8) != _usbh_data.enum_gen) { + TU_LOG_USBH("Drop a request of an earlier enumeration attempt\r\n"); + return; + } + if (XFER_RESULT_FAILED == xfer->result || XFER_RESULT_TIMEOUT == xfer->result) { if (!enum_xfer_retry(xfer) && !enum_retry()) { enum_full_complete(false); @@ -2061,7 +2075,7 @@ static void process_enumeration(tuh_xfer_t *xfer) { _usbh_data.enum_xfer_retry = 0; // the device answered: the next request gets its own tries const uint8_t daddr = xfer->daddr; - const uintptr_t state = xfer->user_data; + const uintptr_t state = xfer->user_data & 0xFFu; usbh_device_t *dev = get_device(daddr); tuh_bus_info_t *dev0_bus = &_usbh_data.dev0_bus; if (daddr > 0) { @@ -2085,7 +2099,7 @@ static void process_enumeration(tuh_xfer_t *xfer) { is_enum_failed = true; } else { is_enum_failed = !hub_port_reset(dev0_bus->hub_addr, dev0_bus->hub_port, process_enumeration, - ENUM_HUB_RESET_COMPLETE); + enum_ud(ENUM_HUB_RESET_COMPLETE)); } break; } @@ -2107,7 +2121,7 @@ static void process_enumeration(tuh_xfer_t *xfer) { if (1 == port_status.change.reset) { // Acknowledge Port Reset Change is_enum_failed = !hub_port_clear_reset_change(dev0_bus->hub_addr, dev0_bus->hub_port, process_enumeration, - ENUM_HUB_CLEAR_RESET_COMPLETE); + enum_ud(ENUM_HUB_CLEAR_RESET_COMPLETE)); } else if (state == ENUM_HUB_CLEAR_RESET) { // retry one more time if reset change not set yet usbh_defer_func_ms_async(ENUM_RESET_HUB_DELAY_MS, enum_delay_async, ENUM_AFTER_RESET_HUB_DELAY_RETRY); @@ -2154,7 +2168,7 @@ static void process_enumeration(tuh_xfer_t *xfer) { new_dev->bus_info = *dev0_bus; new_dev->desc_device.bMaxPacketSize0 = desc_device->bMaxPacketSize0; - is_enum_failed = !tuh_address_set(0, new_addr, process_enumeration, ENUM_GET_DEVICE_DESC); + is_enum_failed = !tuh_address_set(0, new_addr, process_enumeration, enum_ud(ENUM_GET_DEVICE_DESC)); break; } @@ -2181,14 +2195,14 @@ static void process_enumeration(tuh_xfer_t *xfer) { tuh_enum_descriptor_device_cb(daddr, desc_device); // callback is_enum_failed = !tuh_descriptor_get_string_langid(daddr, _usbh_epbuf.ctrl, 2, - process_enumeration, ENUM_GET_STRING_LANGUAGE_ID); + process_enumeration, enum_ud(ENUM_GET_STRING_LANGUAGE_ID)); break; } case ENUM_GET_STRING_LANGUAGE_ID: { const uint8_t str_len = xfer->buffer[0]; is_enum_failed = !tuh_descriptor_get_string_langid(daddr, _usbh_epbuf.ctrl, str_len, - process_enumeration, ENUM_GET_STRING_MANUFACTURER_LEN); + process_enumeration, enum_ud(ENUM_GET_STRING_MANUFACTURER_LEN)); break; } @@ -2199,7 +2213,7 @@ static void process_enumeration(tuh_xfer_t *xfer) { } if (dev->desc_device.iManufacturer != 0) { is_enum_failed = !tuh_descriptor_get_string(daddr, dev->desc_device.iManufacturer, langid, _usbh_epbuf.ctrl, 2, - process_enumeration, ENUM_GET_STRING_MANUFACTURER); + process_enumeration, enum_ud(ENUM_GET_STRING_MANUFACTURER)); break; } TU_ATTR_FALLTHROUGH; @@ -2210,7 +2224,7 @@ static void process_enumeration(tuh_xfer_t *xfer) { langid = tu_le16toh(xfer->setup->wIndex); // langid from length's request const uint8_t str_len = xfer->buffer[0]; is_enum_failed = !tuh_descriptor_get_string(daddr, dev->desc_device.iManufacturer, langid, _usbh_epbuf.ctrl, - str_len, process_enumeration, ENUM_GET_STRING_PRODUCT_LEN); + str_len, process_enumeration, enum_ud(ENUM_GET_STRING_PRODUCT_LEN)); break; } TU_ATTR_FALLTHROUGH; @@ -2222,7 +2236,7 @@ static void process_enumeration(tuh_xfer_t *xfer) { langid = tu_le16toh(xfer->setup->wIndex); // get langid from previous setup packet if not fall through } is_enum_failed = !tuh_descriptor_get_string(daddr, dev->desc_device.iProduct, langid, _usbh_epbuf.ctrl, 2, - process_enumeration, ENUM_GET_STRING_PRODUCT); + process_enumeration, enum_ud(ENUM_GET_STRING_PRODUCT)); break; } TU_ATTR_FALLTHROUGH; @@ -2233,7 +2247,7 @@ static void process_enumeration(tuh_xfer_t *xfer) { langid = tu_le16toh(xfer->setup->wIndex); // langid from length's request const uint8_t str_len = xfer->buffer[0]; is_enum_failed = !tuh_descriptor_get_string(daddr, dev->desc_device.iProduct, langid, _usbh_epbuf.ctrl, str_len, - process_enumeration, ENUM_GET_STRING_SERIAL_LEN); + process_enumeration, enum_ud(ENUM_GET_STRING_SERIAL_LEN)); break; } TU_ATTR_FALLTHROUGH; @@ -2245,7 +2259,7 @@ static void process_enumeration(tuh_xfer_t *xfer) { langid = tu_le16toh(xfer->setup->wIndex); // get langid from previous setup packet if not fall through } is_enum_failed = !tuh_descriptor_get_string(daddr, dev->desc_device.iSerialNumber, langid, _usbh_epbuf.ctrl, 2, - process_enumeration, ENUM_GET_STRING_SERIAL); + process_enumeration, enum_ud(ENUM_GET_STRING_SERIAL)); break; } TU_ATTR_FALLTHROUGH; @@ -2256,7 +2270,7 @@ static void process_enumeration(tuh_xfer_t *xfer) { langid = tu_le16toh(xfer->setup->wIndex); // langid from length's request const uint8_t str_len = xfer->buffer[0]; is_enum_failed = !tuh_descriptor_get_string(daddr, dev->desc_device.iSerialNumber, langid, _usbh_epbuf.ctrl, - str_len, process_enumeration, ENUM_GET_9BYTE_CONFIG_DESC); + str_len, process_enumeration, enum_ud(ENUM_GET_9BYTE_CONFIG_DESC)); break; } TU_ATTR_FALLTHROUGH; @@ -2267,7 +2281,7 @@ static void process_enumeration(tuh_xfer_t *xfer) { uint8_t const config_idx = 0; TU_LOG_USBH("Get Configuration[%u] Descriptor (9 bytes)\r\n", config_idx); is_enum_failed = !tuh_descriptor_get_configuration(daddr, config_idx, _usbh_epbuf.ctrl, 9, - process_enumeration, ENUM_GET_FULL_CONFIG_DESC); + process_enumeration, enum_ud(ENUM_GET_FULL_CONFIG_DESC)); break; } @@ -2285,7 +2299,7 @@ static void process_enumeration(tuh_xfer_t *xfer) { uint8_t const config_idx = (uint8_t) (tu_le16toh(xfer->setup->wValue) & 0xFF); TU_LOG_USBH("Get Configuration[%u] Descriptor\r\n", config_idx); is_enum_failed = !tuh_descriptor_get_configuration(daddr, config_idx, _usbh_epbuf.ctrl, total_len, - process_enumeration, ENUM_SET_CONFIG); + process_enumeration, enum_ud(ENUM_SET_CONFIG)); break; } @@ -2306,7 +2320,7 @@ static void process_enumeration(tuh_xfer_t *xfer) { } if (tuh_enum_descriptor_configuration_cb(daddr, config_idx, (const tusb_desc_configuration_t*) _usbh_epbuf.ctrl)) { - is_enum_failed = !tuh_configuration_set(daddr, desc_config->bConfigurationValue, process_enumeration, ENUM_CONFIG_DRIVER); + is_enum_failed = !tuh_configuration_set(daddr, desc_config->bConfigurationValue, process_enumeration, enum_ud(ENUM_CONFIG_DRIVER)); } else { config_idx++; if (config_idx >= dev->desc_device.bNumConfigurations) { @@ -2316,7 +2330,7 @@ static void process_enumeration(tuh_xfer_t *xfer) { } TU_LOG_USBH("Get Configuration[%u] Descriptor (9 bytes)\r\n", config_idx); is_enum_failed = !tuh_descriptor_get_configuration(daddr, config_idx, _usbh_epbuf.ctrl, 9, - process_enumeration, ENUM_GET_FULL_CONFIG_DESC); + process_enumeration, enum_ud(ENUM_GET_FULL_CONFIG_DESC)); } break; } @@ -2561,6 +2575,7 @@ static bool enum_retry(void) { enum_teardown(false); _usbh_data.enum_attempt++; _usbh_data.enum_xfer_retry = 0; + _usbh_data.enum_gen++; TU_LOG_USBH("Enumeration failed, retry attempt %u/%u\r\n", _usbh_data.enum_attempt + 1, CFG_TUH_ENUM_ATTEMPT_MAX); _usbh_data.enumerating_daddr = 0; usbh_defer_func_ms_async(ENUM_DEBOUNCING_DELAY_MS, enum_delay_async, ENUM_AFTER_DEBOUNCING_DELAY); @@ -2579,16 +2594,16 @@ static bool enum_xfer_retry(const tuh_xfer_t *xfer) { _usbh_data.enum_xfer_retry + 1 >= ENUM_XFER_ATTEMPT_MAX) { return false; } - _usbh_data.enum_xfer_retry++; - TU_LOG_USBH("Enumeration request failed, try %u/%u in %u ms\r\n", _usbh_data.enum_xfer_retry + 1, - ENUM_XFER_ATTEMPT_MAX, ENUM_XFER_RETRY_DELAY_MS); - usbh_pending_ctrl_t *saved = &_usbh_data.enum_xfer; saved->setup = *xfer->setup; saved->buffer = xfer->buffer; saved->user_data = xfer->user_data; saved->daddr = xfer->daddr; - usbh_defer_func_ms_async(ENUM_XFER_RETRY_DELAY_MS, enum_delay_async, ENUM_AFTER_XFER_RETRY_DELAY); + TU_VERIFY(usbh_defer_func_ms_async(ENUM_XFER_RETRY_DELAY_MS, enum_delay_async, ENUM_AFTER_XFER_RETRY_DELAY)); + + _usbh_data.enum_xfer_retry++; + TU_LOG_USBH("Enumeration request failed, try %u/%u in %u ms\r\n", _usbh_data.enum_xfer_retry + 1, + ENUM_XFER_ATTEMPT_MAX, ENUM_XFER_RETRY_DELAY_MS); return true; } diff --git a/test/unit-test/test/host/usbh/test_usbh_enum_xfer_retry.c b/test/unit-test/test/host/usbh/test_usbh_enum_xfer_retry.c index 3736d35bf5..c1617f3e18 100644 --- a/test/unit-test/test/host/usbh/test_usbh_enum_xfer_retry.c +++ b/test/unit-test/test/host/usbh/test_usbh_enum_xfer_retry.c @@ -382,3 +382,55 @@ void test_resend_waits_for_busy_control_slot(void) { TEST_ASSERT_EQUAL_MEMORY(&request, &_usbh_epbuf.request, sizeof(request)); TEST_ASSERT_EQUAL_PTR(process_enumeration, _usbh_data.ctrl_xfer_info.complete_cb); } + +// A resend queued behind another device's transfer outlives an unplug, and the pending queue later completes it as +// FAILED. That failure belongs to the earlier attempt: it must not count against the next device's enumeration, whose +// own first request was queued behind it. +void test_stale_queued_resend_does_not_affect_next_device(void) { + send_first_request(); + request_fails(); + + // another device's transfer holds the slot: the resend is queued + _usbh_data.ctrl_xfer_info.stage = CONTROL_STAGE_DATA; + _usbh_data.ctrl_xfer_info.daddr = 1; + fire_delay(); + + port_connected = false; + hcd_event_device_remove(RHPORT, false); + process_next_event(); + TEST_ASSERT_EQUAL_UINT8(TUSB_INDEX_INVALID_8, _usbh_data.enumerating_daddr); + + // the next device attaches; its first request is queued behind the stale resend + attach_to_reset_recovery(); + fire_delay(); + TEST_ASSERT_EQUAL_UINT8(0, _usbh_data.enumerating_daddr); + TEST_ASSERT_NULL(_usbh_data.call_after.func); + + setup_send_count = 0; + _usbh_data.ctrl_xfer_info.stage = CONTROL_STAGE_IDLE; + control_xfer_dispatch_pending(); + + // the stale resend is dropped, the new device's request goes out and its attempt is untouched + TEST_ASSERT_EQUAL_UINT32(1, setup_send_count); + TEST_ASSERT_EQUAL(CONTROL_STAGE_SETUP, _usbh_data.ctrl_xfer_info.stage); + TEST_ASSERT_EQUAL_PTR(process_enumeration, _usbh_data.ctrl_xfer_info.complete_cb); + TEST_ASSERT_EQUAL_UINT8(0, _usbh_data.enum_attempt); + TEST_ASSERT_EQUAL_UINT8(0, _usbh_data.enum_xfer_retry); + TEST_ASSERT_NULL(_usbh_data.call_after.func); + TEST_ASSERT_TRUE(tu_fifo_empty(&_usbh_pending_ctrl_q)); +} + +// The resend cannot even be queued (slot busy, pending queue full): the attempt falls back to a port reset +void test_resend_not_queued_resets_port(void) { + send_first_request(); + request_fails(); + + _usbh_data.ctrl_xfer_info.stage = CONTROL_STAGE_DATA; + _usbh_data.ctrl_xfer_info.daddr = 1; + const usbh_pending_ctrl_t filler = {.daddr = 1}; + while (tu_fifo_write_n(&_usbh_pending_ctrl_q, &filler, sizeof(filler)) > 0) {} + + fire_delay(); + TEST_ASSERT_EQUAL(ENUM_AFTER_DEBOUNCING_DELAY, _usbh_data.call_after.arg); + TEST_ASSERT_EQUAL_UINT8(1, _usbh_data.enum_attempt); +}