Skip to content

host: fix enumerate racing #3080

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Apr 23, 2025
Merged

host: fix enumerate racing #3080

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9f096ac
host: fix enumerate racing
HiFiPhile Apr 12, 2025
3c4e6a7
Move decouncing delay before USB reset.
HiFiPhile Apr 18, 2025
7ba63a6
Also cleanup unaddressed device.
HiFiPhile Apr 18, 2025
940fe43
move removing dev0 to process_removing_device()
hathach Apr 22, 2025
8f9ef7d
reduce ENUM_DEBOUNCING_DELAY_MS to 200ms
hathach Apr 22, 2025
b6170c9
Compile fix.
HiFiPhile Apr 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
151 changes: 73 additions & 78 deletions src/host/usbh.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,12 +96,7 @@ typedef struct {
uint8_t rhport;
uint8_t hub_addr;
uint8_t hub_port;

struct TU_ATTR_PACKED {
uint8_t speed : 4; // packed speed to save footprint
volatile uint8_t enumerating : 1; // enumeration is in progress, false if not connected or all interfaces are configured
uint8_t TU_RESERVED : 3;
};
uint8_t speed;
} usbh_dev0_t;

typedef struct {
Expand All @@ -117,7 +112,6 @@ typedef struct {
volatile uint8_t addressed : 1; // After SET_ADDR
volatile uint8_t configured : 1; // After SET_CONFIG and all drivers are configured
volatile uint8_t suspended : 1; // Bus suspended

// volatile uint8_t removing : 1; // Physically disconnected, waiting to be processed by usbh
};

Expand Down Expand Up @@ -261,8 +255,6 @@ static inline usbh_class_driver_t const *get_driver(uint8_t drv_id) {
// sum of end device + hub
#define TOTAL_DEVICES (CFG_TUH_DEVICE_MAX + CFG_TUH_HUB)

static uint8_t _usbh_controller = TUSB_INDEX_INVALID_8;

// Device with address = 0 for enumeration
static usbh_dev0_t _dev0;

Expand All @@ -279,8 +271,7 @@ static usbh_device_t _usbh_devices[TOTAL_DEVICES];
#define _usbh_mutex NULL
#endif

// Event queue
// usbh_int_set is used as mutex in OS NONE config
// Event queue: usbh_int_set() is used as mutex in OS NONE config
OSAL_QUEUE_DEF(usbh_int_set, _usbh_qdef, CFG_TUH_TASK_QUEUE_SZ, hcd_event_t);
static osal_queue_t _usbh_q;

Expand All @@ -305,6 +296,15 @@ typedef struct {

CFG_TUH_MEM_SECTION static usbh_epbuf_t _usbh_epbuf;

typedef struct {
uint8_t controller_id; // controller ID
uint8_t enumerating_daddr; // device address of the device being enumerated
} usbh_data_t;

static usbh_data_t _usbh_data = {
.controller_id = TUSB_INDEX_INVALID_8,
};

//------------- Helper Function -------------//
TU_ATTR_ALWAYS_INLINE static inline usbh_device_t* get_device(uint8_t dev_addr) {
TU_VERIFY(dev_addr > 0 && dev_addr <= TOTAL_DEVICES, NULL);
Expand Down Expand Up @@ -334,7 +334,7 @@ bool tuh_mounted(uint8_t dev_addr) {

bool tuh_connected(uint8_t daddr) {
if (daddr == 0) {
return _dev0.enumerating; // dev0 is connected if still enumerating
return _usbh_data.enumerating_daddr == 0;
} else {
const usbh_device_t* dev = get_device(daddr);
return dev && dev->connected;
Expand All @@ -359,7 +359,7 @@ tusb_speed_t tuh_speed_get(uint8_t dev_addr) {
}

bool tuh_rhport_is_active(uint8_t rhport) {
return _usbh_controller == rhport;
return _usbh_data.controller_id == rhport;
}

bool tuh_rhport_reset_bus(uint8_t rhport, bool active) {
Expand Down Expand Up @@ -387,7 +387,7 @@ static void clear_device(usbh_device_t* dev) {
}

bool tuh_inited(void) {
return _usbh_controller != TUSB_INDEX_INVALID_8;
return _usbh_data.controller_id != TUSB_INDEX_INVALID_8;
}

bool tuh_rhport_init(uint8_t rhport, const tusb_rhport_init_t* rh_init) {
Expand Down Expand Up @@ -427,6 +427,9 @@ bool tuh_rhport_init(uint8_t rhport, const tusb_rhport_init_t* rh_init) {
tu_memclr(_usbh_devices, sizeof(_usbh_devices));
tu_memclr(&_ctrl_xfer, sizeof(_ctrl_xfer));

_usbh_data.controller_id = TUSB_INDEX_INVALID_8;
_usbh_data.enumerating_daddr = TUSB_INDEX_INVALID_8;

for (uint8_t i = 0; i < TOTAL_DEVICES; i++) {
clear_device(&_usbh_devices[i]);
}
Expand All @@ -442,7 +445,7 @@ bool tuh_rhport_init(uint8_t rhport, const tusb_rhport_init_t* rh_init) {
}

// Init host controller
_usbh_controller = rhport;
_usbh_data.controller_id = rhport;
TU_ASSERT(hcd_init(rhport, rh_init));
hcd_int_enable(rhport);

Expand All @@ -457,7 +460,7 @@ bool tuh_deinit(uint8_t rhport) {
// deinit host controller
hcd_int_disable(rhport);
hcd_deinit(rhport);
_usbh_controller = TUSB_INDEX_INVALID_8;
_usbh_data.controller_id = TUSB_INDEX_INVALID_8;

// "unplug" all devices on this rhport (hub_addr = 0, hub_port = 0)
process_removing_device(rhport, 0, 0);
Expand Down Expand Up @@ -524,20 +527,19 @@ void tuh_task_ext(uint32_t timeout_ms, bool in_isr) {

switch (event.event_id) {
case HCD_EVENT_DEVICE_ATTACH:
// due to the shared control buffer, we must complete enumerating one device before enumerating another one.
// due to the shared control buffer, we must fully complete enumerating one device first.
// TODO better to have an separated queue for newly attached devices
if (_dev0.enumerating) {
// Some device can cause multiple duplicated attach events
// drop current enumerating and start over for a proper port reset
if (event.rhport == _dev0.rhport && event.connection.hub_addr == _dev0.hub_addr &&
event.connection.hub_port == _dev0.hub_port) {
if (_usbh_data.enumerating_daddr != TUSB_INDEX_INVALID_8) {
if (event.rhport == _dev0.rhport &&
event.connection.hub_addr == _dev0.hub_addr && event.connection.hub_port == _dev0.hub_port) {
// Some device can cause multiple duplicated attach events
// drop current enumerating and start over for a proper port reset
// abort/cancel current enumeration and start new one
TU_LOG1("[%u:] USBH Device Attach (duplicated)\r\n", event.rhport);
tuh_edpt_abort_xfer(0, 0);
enum_new_device(&event);
} else {
TU_LOG_USBH("[%u:] USBH Defer Attach until current enumeration complete\r\n", event.rhport);

bool is_empty = osal_queue_empty(_usbh_q);
queue_event(&event, in_isr);

Expand All @@ -548,7 +550,7 @@ void tuh_task_ext(uint32_t timeout_ms, bool in_isr) {
}
} else {
TU_LOG1("[%u:] USBH Device Attach\r\n", event.rhport);
_dev0.enumerating = 1;
_usbh_data.enumerating_daddr = 0;
enum_new_device(&event);
}
break;
Expand Down Expand Up @@ -648,9 +650,11 @@ static void _control_blocking_complete_cb(tuh_xfer_t* xfer) {
}

TU_ATTR_ALWAYS_INLINE static inline void _control_set_xfer_stage(uint8_t stage) {
(void) osal_mutex_lock(_usbh_mutex, OSAL_TIMEOUT_WAIT_FOREVER);
_ctrl_xfer.stage = stage;
(void) osal_mutex_unlock(_usbh_mutex);
if (_ctrl_xfer.stage != stage) {
(void) osal_mutex_lock(_usbh_mutex, OSAL_TIMEOUT_WAIT_FOREVER);
_ctrl_xfer.stage = stage;
(void) osal_mutex_unlock(_usbh_mutex);
}
}

TU_ATTR_ALWAYS_INLINE static inline bool usbh_setup_send(uint8_t daddr, const uint8_t setup_packet[8]) {
Expand All @@ -666,7 +670,7 @@ TU_ATTR_ALWAYS_INLINE static inline bool usbh_setup_send(uint8_t daddr, const ui
bool tuh_control_xfer (tuh_xfer_t* xfer) {
TU_VERIFY(xfer->ep_addr == 0 && xfer->setup); // EP0 with setup packet
const uint8_t daddr = xfer->daddr;
TU_VERIFY(tuh_connected(daddr)); // Check if device is still connected (enumerating for dev0)
TU_VERIFY(tuh_connected(daddr));

// pre-check to help reducing mutex lock
TU_VERIFY(_ctrl_xfer.stage == CONTROL_STAGE_IDLE);
Expand Down Expand Up @@ -893,9 +897,9 @@ uint8_t *usbh_get_enum_buf(void) {
void usbh_int_set(bool enabled) {
// TODO all host controller if multiple are used since they shared the same event queue
if (enabled) {
hcd_int_enable(_usbh_controller);
hcd_int_enable(_usbh_data.controller_id);
} else {
hcd_int_disable(_usbh_controller);
hcd_int_disable(_usbh_data.controller_id);
}
}

Expand All @@ -904,7 +908,6 @@ void usbh_defer_func(osal_task_func_t func, void *param, bool in_isr) {
event.event_id = USBH_EVENT_FUNC_CALL;
event.func_call.func = func;
event.func_call.param = param;

queue_event(&event, in_isr);
}

Expand Down Expand Up @@ -1041,12 +1044,6 @@ TU_ATTR_FAST_FUNC void hcd_event_handler(hcd_event_t const* event, bool in_isr)
case HCD_EVENT_DEVICE_REMOVE:
// FIXME device remove from a hub need an HCD API for hcd to free up endpoint
// mark device as removing to prevent further xfer before the event is processed in usbh task

// Check if dev0 is removed
if ((event->rhport == _dev0.rhport) && (event->connection.hub_addr == _dev0.hub_addr) &&
(event->connection.hub_port == _dev0.hub_port)) {
_dev0.enumerating = 0;
}
break;

default: break;
Expand Down Expand Up @@ -1270,30 +1267,20 @@ TU_ATTR_ALWAYS_INLINE static inline bool is_hub_addr(uint8_t daddr) {
return (CFG_TUH_HUB > 0) && (daddr > CFG_TUH_DEVICE_MAX);
}

//static void mark_removing_device_isr(uint8_t rhport, uint8_t hub_addr, uint8_t hub_port) {
// for (uint8_t dev_id = 0; dev_id < TOTAL_DEVICES; dev_id++) {
// usbh_device_t *dev = &_usbh_devices[dev_id];
// uint8_t const daddr = dev_id + 1;
//
// // hub_addr = 0 means roothub, hub_port = 0 means all devices of downstream hub
// if (dev->rhport == rhport && dev->connected &&
// (hub_addr == 0 || dev->hub_addr == hub_addr) &&
// (hub_port == 0 || dev->hub_port == hub_port)) {
// if (is_hub_addr(daddr)) {
// // If the device itself is a usb hub, mark all downstream devices.
// // FIXME recursive calls
// mark_removing_device_isr(rhport, daddr, 0);
// }
//
// dev->removing = 1;
// }
// }
//}

// a device unplugged from rhport:hub_addr:hub_port
static void process_removing_device(uint8_t rhport, uint8_t hub_addr, uint8_t hub_port) {
// dev0 is unplugged
if ((_usbh_data.enumerating_daddr == 0) && (rhport == _dev0.rhport) &&
(hub_addr == _dev0.hub_addr) && (hub_port == _dev0.hub_port)) {
hcd_device_close(_dev0.rhport, 0);
if (_ctrl_xfer.daddr == 0) {
_control_set_xfer_stage(CONTROL_STAGE_IDLE);
}
_usbh_data.enumerating_daddr = TUSB_INDEX_INVALID_8;
return;
}

//------------- find the all devices (star-network) under port that is unplugged -------------//
// TODO mark as disconnected in ISR, also handle dev0
uint32_t removing_hubs = 0;
do {
for (uint8_t dev_id = 0; dev_id < TOTAL_DEVICES; dev_id++) {
Expand Down Expand Up @@ -1328,15 +1315,21 @@ static void process_removing_device(uint8_t rhport, uint8_t hub_addr, uint8_t hu
clear_device(dev);

// abort on-going control xfer on this device if any
if (_ctrl_xfer.daddr == daddr) {
if (daddr == _ctrl_xfer.daddr) {
_control_set_xfer_stage(CONTROL_STAGE_IDLE);
}

if (daddr == _usbh_data.enumerating_daddr) {
_usbh_data.enumerating_daddr = TUSB_INDEX_INVALID_8;
}
}
}

// if removing a hub, we need to remove its downstream devices
// if removing a hub, we need to remove all of its downstream devices
#if CFG_TUH_HUB
if (removing_hubs == 0) break;
if (removing_hubs == 0) {
break;
}

// find a marked hub to process
for (uint8_t h_id = 0; h_id < CFG_TUH_HUB; h_id++) {
Expand Down Expand Up @@ -1365,9 +1358,9 @@ static void process_removing_device(uint8_t rhport, uint8_t hub_addr, uint8_t hu
//--------------------------------------------------------------------+

enum {
ENUM_RESET_DELAY_MS = 50, // USB specs: 10 to 50ms
ENUM_DEBOUNCING_DELAY_MS = 450, // when plug/unplug a device, physical connection can be bouncing and may
ENUM_DEBOUNCING_DELAY_MS = 200, // when plug/unplug a device, physical connection can be bouncing and may
// generate a series of attach/detach event. This delay wait for stable connection
ENUM_RESET_DELAY_MS = 50, // USB specs: 10 to 50ms
};

enum {
Expand Down Expand Up @@ -1411,7 +1404,7 @@ static void process_enumeration(tuh_xfer_t* xfer) {

// retry if not reaching max attempt
failed_count++;
bool retry = _dev0.enumerating && (failed_count < ATTEMPT_COUNT_MAX);
bool retry = (_usbh_data.enumerating_daddr != TUSB_INDEX_INVALID_8) && (failed_count < ATTEMPT_COUNT_MAX);
if (retry) {
tusb_time_delay_ms_api(ATTEMPT_DELAY_MS); // delay a bit
TU_LOG1("Enumeration attempt %u/%u\r\n", failed_count+1, ATTEMPT_COUNT_MAX);
Expand All @@ -1421,7 +1414,6 @@ static void process_enumeration(tuh_xfer_t* xfer) {
if (!retry) {
enum_full_complete(); // complete as failed
}

return;
}
failed_count = 0;
Expand All @@ -1437,7 +1429,6 @@ static void process_enumeration(tuh_xfer_t* xfer) {
switch (state) {
#if CFG_TUH_HUB
//case ENUM_HUB_GET_STATUS_1: break;

case ENUM_HUB_CLEAR_RESET_1: {
hub_port_status_response_t port_status;
memcpy(&port_status, _usbh_epbuf.ctrl, sizeof(hub_port_status_response_t));
Expand Down Expand Up @@ -1502,14 +1493,12 @@ static void process_enumeration(tuh_xfer_t* xfer) {
usbh_device_t* new_dev = get_device(new_addr);
TU_ASSERT(new_dev,);
new_dev->addressed = 1;
_usbh_data.enumerating_daddr = new_addr;

// Close device 0
hcd_device_close(_dev0.rhport, 0);
hcd_device_close(_dev0.rhport, 0); // close dev0

// open control pipe for new address
TU_ASSERT(usbh_edpt_control_open(new_addr, new_dev->ep0_size),);
TU_ASSERT(usbh_edpt_control_open(new_addr, new_dev->ep0_size),); // open new control endpoint

// Get full device descriptor
TU_LOG_USBH("Get Device Descriptor\r\n");
TU_ASSERT(tuh_descriptor_get_device(new_addr, _usbh_epbuf.ctrl, sizeof(tusb_desc_device_t),
process_enumeration, ENUM_GET_STRING_LANGUAGE_ID_LEN),);
Expand All @@ -1528,8 +1517,7 @@ static void process_enumeration(tuh_xfer_t* xfer) {
dev->i_serial = desc_device->iSerialNumber;
dev->bNumConfigurations = desc_device->bNumConfigurations;

tuh_enum_descriptor_device_cb(daddr, desc_device);// callback

tuh_enum_descriptor_device_cb(daddr, desc_device); // callback
tuh_descriptor_get_string_langid(daddr, _usbh_epbuf.ctrl, 2,
process_enumeration, ENUM_GET_STRING_LANGUAGE_ID);
break;
Expand Down Expand Up @@ -1685,17 +1673,24 @@ static bool enum_new_device(hcd_event_t* event) {

if (_dev0.hub_addr == 0) {
// connected directly to roothub
hcd_port_reset(_dev0.rhport);

// wait until device connection is stable TODO non blocking
tusb_time_delay_ms_api(ENUM_DEBOUNCING_DELAY_MS);

// device unplugged while delaying
if (!hcd_port_connect_status(_dev0.rhport)) {
enum_full_complete();
return true;
}

hcd_port_reset(_dev0.rhport); // reset device

// Since we are in middle of rhport reset, frame number is not available yet.
// need to depend on tusb_time_millis_api()
tusb_time_delay_ms_api(ENUM_RESET_DELAY_MS);

hcd_port_reset_end(_dev0.rhport);

// wait until device connection is stable TODO non blocking
tusb_time_delay_ms_api(ENUM_DEBOUNCING_DELAY_MS);

// device unplugged while delaying
if (!hcd_port_connect_status(_dev0.rhport)) {
enum_full_complete();
Expand Down Expand Up @@ -1911,7 +1906,7 @@ void usbh_driver_set_config_complete(uint8_t dev_addr, uint8_t itf_num) {

static void enum_full_complete(void) {
// mark enumeration as complete
_dev0.enumerating = 0;
_usbh_data.enumerating_daddr = TUSB_INDEX_INVALID_8;

#if CFG_TUH_HUB
if (_dev0.hub_addr) {
Expand Down
Loading