Skip to content

Commit 602941e

Browse files
roma-jamroma-jam
committed
Merge branch 'refactor/usb_host_ext_hub_port_gone' into 'master'
refactor(ext_hub): Device release (allows to run usb_host test with ext hub) Closes IDF-12173, IDF-10490, and IDF-13132 See merge request espressif/esp-idf!38176
2 parents 884e54a + f238d75 commit 602941e

File tree

7 files changed

+113
-146
lines changed

7 files changed

+113
-146
lines changed

‎components/usb/ext_hub.c‎

Lines changed: 78 additions & 118 deletions
Original file line numberDiff line numberDiff line change
@@ -312,13 +312,32 @@ static bool _device_set_actions(ext_hub_dev_t *ext_hub_dev, uint32_t action_flag
312312

313313
static esp_err_t device_enable_int_ep(ext_hub_dev_t *ext_hub_dev)
314314
{
315-
ESP_LOGD(EXT_HUB_TAG, "[%d] Enable EP IN", ext_hub_dev->constant.dev_addr);
316-
esp_err_t ret = usbh_ep_enqueue_urb(ext_hub_dev->constant.ep_in_hdl, ext_hub_dev->constant.in_urb);
317-
if (ret != ESP_OK) {
318-
ESP_LOGE(EXT_HUB_TAG, "[%d] Failed to submit in urb: %s", ext_hub_dev->constant.dev_addr, esp_err_to_name(ret));
319-
device_error(ext_hub_dev);
315+
bool call_proc_req_cb = false;
316+
bool is_active = true;
317+
ESP_LOGD(EXT_HUB_TAG, "[%d] Device ports handle complete", ext_hub_dev->constant.dev_addr);
318+
319+
// Check if the device should be released
320+
EXT_HUB_ENTER_CRITICAL();
321+
if (ext_hub_dev->dynamic.flags.waiting_release) {
322+
is_active = false;
323+
call_proc_req_cb = _device_set_actions(ext_hub_dev, DEV_ACTION_RELEASE);
320324
}
321-
return ret;
325+
EXT_HUB_EXIT_CRITICAL();
326+
327+
if (is_active) {
328+
ESP_LOGD(EXT_HUB_TAG, "[%d] Enable IN EP", ext_hub_dev->constant.dev_addr);
329+
esp_err_t ret = usbh_ep_enqueue_urb(ext_hub_dev->constant.ep_in_hdl, ext_hub_dev->constant.in_urb);
330+
if (ret != ESP_OK) {
331+
ESP_LOGE(EXT_HUB_TAG, "[%d] Failed to submit in urb: %s", ext_hub_dev->constant.dev_addr, esp_err_to_name(ret));
332+
device_error(ext_hub_dev);
333+
}
334+
return ret;
335+
}
336+
337+
if (call_proc_req_cb) {
338+
p_ext_hub_driver->constant.proc_req_cb(false, p_ext_hub_driver->constant.proc_req_cb_arg);
339+
}
340+
return ESP_OK;
322341
}
323342

324343
static void device_has_changed(ext_hub_dev_t *ext_hub_dev)
@@ -420,7 +439,6 @@ static esp_err_t device_port_free(ext_hub_dev_t *ext_hub_dev, uint8_t port_idx)
420439
EXT_HUB_CHECK(ext_hub_dev->constant.ports[port_idx] != NULL, ESP_ERR_INVALID_STATE);
421440

422441
bool call_proc_req_cb = false;
423-
bool all_ports_freed = false;
424442

425443
assert(ext_hub_dev->single_thread.maxchild != 0);
426444
assert(p_ext_hub_driver->constant.port_driver);
@@ -435,25 +453,11 @@ static esp_err_t device_port_free(ext_hub_dev_t *ext_hub_dev, uint8_t port_idx)
435453
ext_hub_dev->single_thread.maxchild--;
436454

437455
EXT_HUB_ENTER_CRITICAL();
438-
if (ext_hub_dev->dynamic.flags.is_gone) {
439-
all_ports_freed = (ext_hub_dev->single_thread.maxchild == 0);
440-
441-
if (all_ports_freed) {
442-
ext_hub_dev->dynamic.flags.waiting_children = 0;
443-
ext_hub_dev->dynamic.flags.waiting_free = 1;
444-
call_proc_req_cb = _device_set_actions(ext_hub_dev, DEV_ACTION_FREE);
445-
} else {
446-
ext_hub_dev->dynamic.flags.waiting_children = 1;
447-
}
456+
if (ext_hub_dev->dynamic.flags.waiting_free && ext_hub_dev->single_thread.maxchild == 0) {
457+
call_proc_req_cb = _device_set_actions(ext_hub_dev, DEV_ACTION_FREE);
448458
}
449459
EXT_HUB_EXIT_CRITICAL();
450460

451-
// Close the device if all children were freed
452-
if (all_ports_freed) {
453-
ESP_LOGD(EXT_HUB_TAG, "[%d] Release USBH device object", ext_hub_dev->constant.dev_addr);
454-
ESP_ERROR_CHECK(usbh_dev_close(ext_hub_dev->constant.dev_hdl));
455-
}
456-
457461
if (call_proc_req_cb) {
458462
p_ext_hub_driver->constant.proc_req_cb(false, p_ext_hub_driver->constant.proc_req_cb_arg);
459463
}
@@ -463,71 +467,60 @@ static esp_err_t device_port_free(ext_hub_dev_t *ext_hub_dev, uint8_t port_idx)
463467
return ret;
464468
}
465469

466-
static esp_err_t device_release_ports(ext_hub_dev_t *ext_hub_dev)
470+
static esp_err_t device_release_port(ext_hub_dev_t *ext_hub_dev, uint8_t port_idx)
467471
{
468-
esp_err_t ret = ESP_OK;
469-
// Mark all ports as gone
470-
for (uint8_t i = 0; i < ext_hub_dev->constant.hub_desc->bNbrPorts; i++) {
471-
// Only for ports, that were created
472-
if (ext_hub_dev->constant.ports[i] != NULL) {
473-
// Mark port as gone
474-
ret = p_ext_hub_driver->constant.port_driver->gone(ext_hub_dev->constant.ports[i]);
475-
if (ret == ESP_OK) {
476-
// Port doesn't have a device and can be recycled right now
477-
ESP_ERROR_CHECK(device_port_free(ext_hub_dev, i));
478-
} else if (ret == ESP_ERR_NOT_FINISHED) {
479-
// Port has a device and will be recycled after USBH device will be released by all clients and freed
480-
ESP_LOGE(EXT_HUB_TAG, "[%d:%d] Port is gone", ext_hub_dev->constant.dev_addr, i + 1);
481-
// Not an error case, but it's good to notify this with the error message
482-
// TODO: IDF-12173 remove the error, instead set up the flag and verify the flag while recycle
483-
ret = ESP_OK;
484-
} else {
485-
ESP_LOGE(EXT_HUB_TAG, "[%d:%d] Unable to mark port as gone: %s",
486-
ext_hub_dev->constant.dev_addr, i + 1, esp_err_to_name(ret));
487-
return ret;
488-
}
489-
} else {
490-
ESP_LOGE(EXT_HUB_TAG, "[%d:%d] Port was not created", ext_hub_dev->constant.dev_addr, i + 1);
491-
}
472+
assert(ext_hub_dev->constant.ports[port_idx] != NULL);
473+
assert(ext_hub_dev->single_thread.maxchild != 0);
474+
assert(p_ext_hub_driver->constant.port_driver);
475+
476+
// Mark port as gone
477+
esp_err_t ret = p_ext_hub_driver->constant.port_driver->gone(ext_hub_dev->constant.ports[port_idx]);
478+
if (ret == ESP_OK) {
479+
// Port doesn't have a device and can be freed right now
480+
ESP_ERROR_CHECK(device_port_free(ext_hub_dev, port_idx));
481+
} else if (ret == ESP_ERR_NOT_FINISHED) {
482+
// Port has a device and will be freed on recycle
483+
EXT_HUB_ENTER_CRITICAL();
484+
ext_hub_dev->dynamic.flags.waiting_children = 1;
485+
EXT_HUB_EXIT_CRITICAL();
486+
ret = ESP_OK;
487+
} else {
488+
ESP_LOGE(EXT_HUB_TAG, "[%d:%d] Unable to mark port as gone: %s",
489+
ext_hub_dev->constant.dev_addr, port_idx + 1, esp_err_to_name(ret));
490+
return ret;
492491
}
492+
493493
return ret;
494494
}
495495

496+
static void device_release_ports(ext_hub_dev_t *ext_hub_dev)
497+
{
498+
assert(ext_hub_dev->constant.hub_desc->bNbrPorts); // Sanity check
499+
for (uint8_t i = 0; i < ext_hub_dev->constant.hub_desc->bNbrPorts; i++) {
500+
ESP_ERROR_CHECK(device_release_port(ext_hub_dev, i));
501+
}
502+
}
503+
496504
static void device_release(ext_hub_dev_t *ext_hub_dev)
497505
{
498506
ESP_LOGD(EXT_HUB_TAG, "[%d] Device release", ext_hub_dev->constant.dev_addr);
499507

500508
EXT_HUB_ENTER_CRITICAL();
501-
assert(ext_hub_dev->dynamic.flags.waiting_release); // Device should waiting the release
502-
ext_hub_dev->dynamic.flags.is_gone = 1;
509+
assert(ext_hub_dev->dynamic.flags.waiting_release); // Sanity check
503510
ext_hub_dev->dynamic.flags.waiting_release = 0;
511+
ext_hub_dev->dynamic.flags.waiting_free = 1;
504512
EXT_HUB_EXIT_CRITICAL();
505513

506-
// Release IN EP
507-
ESP_ERROR_CHECK(usbh_ep_command(ext_hub_dev->constant.ep_in_hdl, USBH_EP_CMD_HALT));
508-
509514
switch (ext_hub_dev->single_thread.state) {
510-
case EXT_HUB_STATE_ATTACHED:
511-
// Device has no configured ports, release the USBH device object
512-
ESP_LOGD(EXT_HUB_TAG, "[%d] Release USBH device object", ext_hub_dev->constant.dev_addr);
513-
ESP_ERROR_CHECK(usbh_dev_close(ext_hub_dev->constant.dev_hdl));
514-
break;
515515
case EXT_HUB_STATE_CONFIGURED:
516516
case EXT_HUB_STATE_SUSPENDED:
517517
assert(ext_hub_dev->constant.hub_desc != NULL); // Device should have a Hub descriptor
518518
assert(p_ext_hub_driver->constant.port_driver); // Port driver should be available
519-
520-
// Release ports if device has them
521-
if (ext_hub_dev->constant.hub_desc->bNbrPorts) {
522-
ESP_ERROR_CHECK(device_release_ports(ext_hub_dev));
523-
}
519+
device_release_ports(ext_hub_dev);
524520
break;
525521
default:
526-
// Should never occur
527-
abort();
528522
break;
529523
}
530-
531524
ext_hub_dev->single_thread.state = EXT_HUB_STATE_RELEASED;
532525
}
533526

@@ -771,17 +764,18 @@ static void device_free(ext_hub_dev_t *ext_hub_dev)
771764
TAILQ_REMOVE(&p_ext_hub_driver->dynamic.ext_hubs_tailq, ext_hub_dev, dynamic.tailq_entry);
772765
EXT_HUB_EXIT_CRITICAL();
773766

767+
ESP_ERROR_CHECK(usbh_ep_free(ext_hub_dev->constant.ep_in_hdl));
768+
ESP_ERROR_CHECK(usbh_dev_close(ext_hub_dev->constant.dev_hdl));
769+
774770
if (ext_hub_dev->constant.hub_desc) {
775771
heap_caps_free(ext_hub_dev->constant.hub_desc);
776772
}
777773
if (ext_hub_dev->constant.ports) {
778774
heap_caps_free(ext_hub_dev->constant.ports);
779775
}
780776

781-
ESP_ERROR_CHECK(usbh_ep_free(ext_hub_dev->constant.ep_in_hdl));
782777
urb_free(ext_hub_dev->constant.ctrl_urb);
783778
urb_free(ext_hub_dev->constant.in_urb);
784-
785779
heap_caps_free(ext_hub_dev);
786780
}
787781

@@ -1387,6 +1381,7 @@ esp_err_t ext_hub_dev_gone(uint8_t dev_addr)
13871381
ESP_LOGD(EXT_HUB_TAG, "[%d] Device gone", ext_hub_dev->constant.dev_addr);
13881382

13891383
EXT_HUB_ENTER_CRITICAL();
1384+
ext_hub_dev->dynamic.flags.is_gone = 1;
13901385
if (ext_hub_dev->dynamic.flags.waiting_release ||
13911386
ext_hub_dev->dynamic.flags.waiting_children) {
13921387
EXT_HUB_EXIT_CRITICAL();
@@ -1396,7 +1391,7 @@ esp_err_t ext_hub_dev_gone(uint8_t dev_addr)
13961391
}
13971392

13981393
if ((ext_hub_dev->single_thread.maxchild == 0) && !ext_hub_dev->dynamic.flags.in_pending_list) {
1399-
// Not in pending list and doesn't have any children
1394+
// Not in pending list and doesn't have any children release and free right away
14001395
ext_hub_dev->dynamic.flags.waiting_free = 1;
14011396
ext_hub_dev->dynamic.flags.waiting_release = 1;
14021397
call_proc_req_cb = _device_set_actions(ext_hub_dev, DEV_ACTION_RELEASE |
@@ -1420,10 +1415,9 @@ esp_err_t ext_hub_dev_gone(uint8_t dev_addr)
14201415
return ret;
14211416
}
14221417

1423-
esp_err_t ext_hub_all_free(void)
1418+
void ext_hub_mark_all_free(void)
14241419
{
14251420
bool call_proc_req_cb = false;
1426-
bool wait_for_free = false;
14271421

14281422
EXT_HUB_ENTER_CRITICAL();
14291423
for (int i = 0; i < 2; i++) {
@@ -1438,9 +1432,13 @@ esp_err_t ext_hub_all_free(void)
14381432
while (hub_curr != NULL) {
14391433
hub_next = TAILQ_NEXT(hub_curr, dynamic.tailq_entry);
14401434
hub_curr->dynamic.flags.waiting_release = 1;
1441-
call_proc_req_cb = _device_set_actions(hub_curr, DEV_ACTION_RELEASE);
1442-
// At least one hub should be released
1443-
wait_for_free = true;
1435+
uint32_t action_flags = DEV_ACTION_EP1_FLUSH | DEV_ACTION_EP1_DEQUEUE;
1436+
// If device in the IDLE stage, add the release action
1437+
// otherwise, device will be released when the stage changed to IDLE
1438+
if (hub_curr->single_thread.stage == EXT_HUB_STAGE_IDLE) {
1439+
action_flags |= DEV_ACTION_RELEASE;
1440+
}
1441+
call_proc_req_cb = _device_set_actions(hub_curr, action_flags);
14441442
hub_curr = hub_next;
14451443
}
14461444
}
@@ -1449,8 +1447,6 @@ esp_err_t ext_hub_all_free(void)
14491447
if (call_proc_req_cb) {
14501448
p_ext_hub_driver->constant.proc_req_cb(false, p_ext_hub_driver->constant.proc_req_cb_arg);
14511449
}
1452-
1453-
return (wait_for_free) ? ESP_ERR_NOT_FINISHED : ESP_OK;
14541450
}
14551451

14561452
esp_err_t ext_hub_status_handle_complete(ext_hub_handle_t ext_hub_hdl)
@@ -1575,66 +1571,30 @@ esp_err_t ext_hub_port_recycle(ext_hub_handle_t ext_hub_hdl, uint8_t port_num)
15751571
EXT_HUB_CHECK(ext_hub_hdl != NULL, ESP_ERR_INVALID_ARG);
15761572
ext_hub_dev_t *ext_hub_dev = (ext_hub_dev_t *)ext_hub_hdl;
15771573

1578-
esp_err_t ret;
15791574
uint8_t port_idx = port_num - 1;
1580-
bool free_port = false;
15811575
bool release_port = false;
1582-
15831576
EXT_HUB_CHECK(port_idx < ext_hub_dev->constant.hub_desc->bNbrPorts, ESP_ERR_INVALID_SIZE);
15841577
EXT_HUB_CHECK(p_ext_hub_driver->constant.port_driver != NULL, ESP_ERR_NOT_SUPPORTED);
15851578
EXT_HUB_CHECK(ext_hub_dev->single_thread.state == EXT_HUB_STATE_CONFIGURED ||
15861579
ext_hub_dev->single_thread.state == EXT_HUB_STATE_RELEASED, ESP_ERR_INVALID_STATE);
15871580

15881581
EXT_HUB_ENTER_CRITICAL();
1589-
if (ext_hub_dev->dynamic.flags.waiting_release) {
1582+
if (ext_hub_dev->dynamic.flags.waiting_release ||
1583+
ext_hub_dev->dynamic.flags.waiting_children) {
15901584
release_port = true;
1591-
} else if (ext_hub_dev->dynamic.flags.waiting_children) {
1592-
assert(ext_hub_dev->dynamic.flags.waiting_release == 0); // Device should be already released
1593-
assert(ext_hub_dev->dynamic.flags.is_gone == 1); // Device should be gone by now
1594-
free_port = true;
15951585
}
15961586
EXT_HUB_EXIT_CRITICAL();
15971587

1598-
if (!release_port && !free_port) {
1588+
esp_err_t ret;
1589+
if (release_port) {
1590+
ret = device_release_port(ext_hub_dev, port_idx);
1591+
} else {
15991592
// Parent still present, recycle the port
16001593
ret = p_ext_hub_driver->constant.port_driver->recycle(ext_hub_dev->constant.ports[port_idx]);
16011594
if (ret != ESP_OK) {
16021595
ESP_LOGE(EXT_HUB_TAG, "[%d:%d] Unable to recycle the port: %s", ext_hub_dev->constant.dev_addr, port_num, esp_err_to_name(ret));
1603-
goto exit;
1604-
}
1605-
} else {
1606-
if (release_port) {
1607-
// Notify the port that parent is not available anymore and port should be recycled then freed
1608-
ret = p_ext_hub_driver->constant.port_driver->gone(ext_hub_dev->constant.ports[port_idx]);
1609-
if (ret == ESP_OK) {
1610-
ESP_LOGD(EXT_HUB_TAG, "[%d:%d] Port doesn't have a device and can be freed right now",
1611-
ext_hub_dev->constant.dev_addr,
1612-
port_num);
1613-
assert(free_port == false);
1614-
free_port = true;
1615-
} else if (ret == ESP_ERR_NOT_FINISHED) {
1616-
// Port has a device and will be recycled after USBH device will be released by all clients and freed
1617-
ESP_LOGE(EXT_HUB_TAG, "[%d:%d] Port is gone",
1618-
ext_hub_dev->constant.dev_addr,
1619-
port_num);
1620-
// Logically, recycling logic are finished for the Hub Driver, return ESP_OK to free the node
1621-
assert(free_port == false);
1622-
} else {
1623-
ESP_LOGE(EXT_HUB_TAG, "[%d:%d] Unable to mark port as gone: %s",
1624-
ext_hub_dev->constant.dev_addr, port_num, esp_err_to_name(ret));
1625-
}
1626-
}
1627-
1628-
if (free_port) {
1629-
ret = device_port_free(ext_hub_dev, port_idx);
1630-
if (ret != ESP_OK) {
1631-
goto exit;
1632-
}
16331596
}
16341597
}
1635-
1636-
ret = ESP_OK;
1637-
exit:
16381598
return ret;
16391599
}
16401600

‎components/usb/ext_port.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -717,7 +717,7 @@ static void handle_port_state(ext_port_t *ext_port)
717717
switch (curr_state) {
718718
case USB_PORT_STATE_NOT_CONFIGURED:
719719
new_state = USB_PORT_STATE_POWERED_OFF;
720-
ESP_ERROR_CHECK(port_request_status(ext_port));
720+
port_request_status(ext_port);
721721
need_handling = true;
722722
break;
723723
case USB_PORT_STATE_POWERED_OFF:

‎components/usb/hub.c‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -771,7 +771,7 @@ esp_err_t hub_port_disable(usb_device_handle_t parent_dev_hdl, uint8_t parent_po
771771
return ret;
772772
}
773773

774-
esp_err_t hub_notify_new_dev(uint8_t dev_addr)
774+
esp_err_t hub_dev_new(uint8_t dev_addr)
775775
{
776776
HUB_DRIVER_ENTER_CRITICAL();
777777
HUB_DRIVER_CHECK_FROM_CRIT(p_hub_driver_obj != NULL, ESP_ERR_INVALID_STATE);
@@ -794,15 +794,15 @@ esp_err_t hub_notify_new_dev(uint8_t dev_addr)
794794
}
795795
}
796796
// Close device
797-
usbh_dev_close(dev_hdl);
797+
ESP_ERROR_CHECK(usbh_dev_close(dev_hdl));
798798
}
799-
// Logic should not stop the flow, so no error to return
800-
ret = ESP_OK;
799+
// Nothing to do, while Hubs support is not enabled
800+
ret = ESP_ERR_NOT_SUPPORTED;
801801
#endif // ENABLE_USB_HUBS
802802
return ret;
803803
}
804804

805-
esp_err_t hub_notify_dev_gone(uint8_t dev_addr)
805+
esp_err_t hub_dev_gone(uint8_t dev_addr)
806806
{
807807
HUB_DRIVER_ENTER_CRITICAL();
808808
HUB_DRIVER_CHECK_FROM_CRIT(p_hub_driver_obj != NULL, ESP_ERR_INVALID_STATE);
@@ -813,7 +813,7 @@ esp_err_t hub_notify_dev_gone(uint8_t dev_addr)
813813
ret = ext_hub_dev_gone(dev_addr);
814814
#else
815815
// Nothing to do, while Hubs support is not enabled
816-
ret = ESP_OK;
816+
ret = ESP_ERR_NOT_SUPPORTED;
817817
#endif // ENABLE_USB_HUBS
818818
return ret;
819819
}
@@ -825,7 +825,9 @@ esp_err_t hub_notify_all_free(void)
825825
HUB_DRIVER_CHECK_FROM_CRIT(p_hub_driver_obj != NULL, ESP_ERR_INVALID_STATE);
826826
HUB_DRIVER_EXIT_CRITICAL();
827827

828-
return ext_hub_all_free();
828+
ext_hub_mark_all_free();
829+
830+
return ESP_OK;
829831
}
830832
#endif // ENABLE_USB_HUBS
831833

‎components/usb/private_include/ext_hub.h‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -131,11 +131,8 @@ esp_err_t ext_hub_dev_gone(uint8_t dev_addr);
131131
* Entry:
132132
* - should be called within Hub Driver when USB Host library need to be uninstalled
133133
*
134-
* @return
135-
* - ESP_OK: All devices freed
136-
* - ESP_ERR_NOT_FINISHED: Operation not finished: devices waiting children to be freed.
137134
*/
138-
esp_err_t ext_hub_all_free(void);
135+
void ext_hub_mark_all_free(void);
139136

140137
/**
141138
* @brief The External Hub or Ports statuses change completed

0 commit comments

Comments
 (0)