Skip to content

usb: host: fix HS device enumeration and interface probing #106802

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

Open
Girinandha-M wants to merge 2 commits into
base: main
Choose a base branch
from
+11 −16
Open

usb: host: fix HS device enumeration and interface probing #106802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f076393
usb: host: assign device address before full descriptor read
Girinandha-M Mar 29, 2026
f78700d
usb: host: fix interface skipping in descriptor walking
Girinandha-M Apr 2, 2026
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
5 changes: 0 additions & 5 deletions subsys/usb/host/usbh_desc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,11 +186,6 @@ const void *usbh_desc_get_next_function(const void *const desc)
skip_num = ass_d->bInterfaceCount;
}

/* Skip the interface if the head is interface */
if (usbh_desc_is_valid_interface(head)) {
Copy link
Copy Markdown
Contributor

@jfischer-no jfischer-no Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usbh_desc_get_next_function() incorrectly sets skip_num=1
when starting from an interface descriptor.

Why incorrectly? I think that is what usbh_desc_get_next_function() should do. @josuah ?

Copy link
Copy Markdown
Contributor Author

@Girinandha-M Girinandha-M Apr 17, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Boot log WITH skip_num=1 (current code):

Claimed 2c7c:6007 iface 0
Claimed 2c7c:6007 iface 2
not matching interface 4

Interfaces 1, 3 never appear - completely skipped.

Boot log WITHOUT skip_num=1 (this fix):

Claimed 2c7c:6007 iface 0
Claimed 2c7c:6007 iface 1
Claimed 2c7c:6007 iface 2
Claimed 2c7c:6007 iface 3
not matching interface 4

All vendor-class interfaces probed.

The issue is that usbh_desc_get_next() already advances
past the current descriptor before the while loop runs.
When called with interface 0:

  1. skip_num set to 1
  2. usbh_desc_get_next() advances past interface 0
  3. Loop hits interface 1
  4. skip_num=1, decrement, SKIP interface 1
  5. Loop hits interface 2, skip_num=0, return

Interface 1 is skipped because skip_num consumes the
NEXT interface instead of the current one. For IADs this
logic is correct since bInterfaceCount covers grouped
interfaces. For plain interfaces there is nothing to skip.

Copy link
Copy Markdown
Contributor

@josuah josuah Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the long wait.
I see how this is a bug and how it was not visible until a class with only one descriptor per interface triggers it.

skip_num = 1;
}

while (true) {
/* If already on an Interface Association or Interface, this will skip it */
head = usbh_desc_get_next(head);
Expand Down
22 changes: 11 additions & 11 deletions subsys/usb/host/usbh_device.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -560,31 +560,31 @@ int usbh_device_init(struct usb_device *const udev)
goto error;
}

err = usbh_req_desc_dev(udev, sizeof(udev->dev_desc), &udev->dev_desc);
err = alloc_device_address(udev, &new_addr);
Comment thread
tmon-nordic marked this conversation as resolved.
if (err) {
LOG_ERR("Failed to read device descriptor");
LOG_ERR("Failed to allocate device address");
goto error;
}

if (!udev->dev_desc.bNumConfigurations) {
LOG_ERR("Device has no configurations, bNumConfigurations %d",
udev->dev_desc.bNumConfigurations);
err = usbh_device_set_address(udev, new_addr);
if (err) {
goto error;
}

err = alloc_device_address(udev, &new_addr);
LOG_INF("New device with address %u state %u", udev->addr, udev->state);

err = usbh_req_desc_dev(udev, sizeof(udev->dev_desc), &udev->dev_desc);
Comment on lines +569 to +576
Copy link
Copy Markdown
Contributor

@jfischer-no jfischer-no Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current sequence reads the full 18-byte device descriptor
at address 0 before assigning a device address. This causes
enumeration failure on the Quectel EG916Q-GL which returns
bNumConfigurations=0 on the second GET_DESCRIPTOR at address 0.

What does it return on third and fourth GET_DESCRIPTOR request?

reorder to assign the address after the initial 8-byte read
and before the full descriptor read.

There will be another non-compliant USB device that fails with new request order tomorrow. What will we change it to then? This absolutely does not seem like the right solution to me.

Both orderings are
permitted by the USB 2.0 spec, but the new order is strictly
more compatible and matches the enumeration sequence used by
Linux (hub_port_init) and xHCI host stacks.

More compatible to what?

Copy link
Copy Markdown
Contributor Author

@Girinandha-M Girinandha-M Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested repeated GET_DESCRIPTOR at address 0 as requested:

Quectel EG916Q-GL (modem):
GET_DESC #1 addr 0: err=0 bNumConf=0
GET_DESC #2 addr 0: err=0 bNumConf=0
GET_DESC #3 addr 0: err=0 bNumConf=0
GET_DESC #4 addr 0: err=0 bNumConf=0

ECM dongle:
GET_DESC #1 addr 0: err=0 bNumConf=2
GET_DESC #2 addr 0: err=0 bNumConf=2
GET_DESC #3 addr 0: err=0 bNumConf=2
GET_DESC #4 addr 0: err=0 bNumConf=2

The modem transfer completes with no error but consistently
returns bNumConf=0 at address 0. Retry does not help.

Verified the reorder does not break the ECM dongle:

ECM dongle (with reorder):
New device with address 1 state 2
Configuration 1 bNumInterfaces 1

Quectel modem (with reorder):
New device with address 1 state 2
Configuration 1 bNumInterfaces 6

Both devices enumerate correctly with the reorder.
SET_ADDRESS after the initial 8-byte read is a valid
state transition.

Copy link
Copy Markdown
Contributor

@jfischer-no jfischer-no Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both orderings are
permitted by the USB 2.0 spec, but the new order is strictly
more compatible and matches the enumeration sequence used by
Linux (hub_port_init) and xHCI host stacks.

Again, strictly more compatible to what? How I see it, it does not really "matches the enumeration sequence used by Linux". So what does it mean "more compatible"?

Copy link
Copy Markdown
Contributor Author

@Girinandha-M Girinandha-M Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the commit message. Removed the "Linux (hub_port_init) and xHCI" comparison and the "strictly more compatible" framing. The reorder addresses an enumeration failure on the Quectel EG916Q-GL (returns bNumConfigurations=0 on the 18-byte GET_DESCRIPTOR at address 0); ECM dongle continues to enumerate correctly with the new order.

if (err) {
LOG_ERR("Failed to allocate device address");
LOG_ERR("Failed to read device descriptor");
goto error;
}

err = usbh_device_set_address(udev, new_addr);
if (err) {
if (!udev->dev_desc.bNumConfigurations) {
LOG_ERR("Device has no configurations, bNumConfigurations %d",
udev->dev_desc.bNumConfigurations);
goto error;
}

LOG_INF("New device with address %u state %u", udev->addr, udev->state);

err = usbh_device_set_configuration(udev, 1);
if (err) {
LOG_ERR("Failed to configure new device with address %u", udev->addr);
Expand Down
Loading