fix(host): clamp illegal sub-8 fullspeed bulk max packet size - #3827
Open
tendrils wants to merge 2 commits into
Open
fix(host): clamp illegal sub-8 fullspeed bulk max packet size#3827tendrils wants to merge 2 commits into
tendrils wants to merge 2 commits into
Conversation
A full-speed bulk endpoint must report a wMaxPacketSize of 8, 16, 32 or 64. A USB-MIDI device was seen reporting 4 -- the size of a single USB-MIDI 1.0 event packet (cable number + code index + three data bytes), which is a plausible thing for firmware to confuse it with. tu_edpt_validate() rejects it, so tuh_edpt_open() fails and the device never enumerates. Rounds up to the smallest legal size, alongside the existing hack directly above that clamps an over-large 512 down to 64 for the same reason. Transfers may still be shorter than the advertised size -- a short packet is how bulk transfers normally terminate -- so this does not change on-the-wire behaviour, only what tu_edpt_validate() will accept.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves host-side robustness during enumeration by clamping an illegal sub-8 full-speed bulk endpoint wMaxPacketSize up to 8, allowing tu_edpt_validate() to accept descriptors from non-compliant devices (notably a USB-MIDI device reporting 4).
Changes:
- Add a full-speed bulk endpoint descriptor “hack” in
tuh_edpt_open()to forcewMaxPacketSizeto 8 when it’s non-zero but less than 8. - Emit a warning log when this descriptor correction is applied.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1347
to
+1352
| if (desc_ep->bmAttributes.xfer == TUSB_XFER_BULK && tu_edpt_packet_size(desc_ep) > 0 && | ||
| tu_edpt_packet_size(desc_ep) < 8 && tuh_speed_get(dev_addr) == TUSB_SPEED_FULL) { | ||
| TU_LOG1(" WARN: EP max packet size is %u in fullspeed, force to 8\r\n", tu_edpt_packet_size(desc_ep)); | ||
| tusb_desc_endpoint_t *hacked_ep = (tusb_desc_endpoint_t *)(uintptr_t)desc_ep; | ||
| hacked_ep->wMaxPacketSize = tu_htole16(8); | ||
| } |
Review feedback on hathach#3827. tu_edpt_packet_size() was called three times in the same block -- twice in the condition and once in the log -- where one read is clearer and obviously self-consistent. The log argument is also cast explicitly. Promoting a uint16_t to int and printing it with %u is accepted in practice, since every uint16_t value is representable in unsigned int, but the cast removes the question. No behaviour change: all three reads already happened before wMaxPacketSize is rewritten, so they could not have disagreed. Verified by compiling usbh.c through a pico-sdk RP2350 host build.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
tuh_edpt_open()rounds an illegally small full-speed bulkwMaxPacketSizeup to 8, the smallest legal value.Why
A full-speed bulk endpoint must report a
wMaxPacketSizeof 8, 16, 32 or 64. A USB-MIDI device was seen reporting 4 — the size of a single USB-MIDI 1.0 event packet (cable number + code index + three data bytes), which is a plausible thing for device firmware to confuse it with.tu_edpt_validate()correctly rejects it, sotuh_edpt_open()fails and the device never enumerates.Approach
This mirrors the existing hack immediately above it, which clamps an over-large 512 down to 64 for full speed for the same class of reason — so it extends an established pattern rather than introducing one, and is deliberately scoped the same way (full speed, bulk only).
Transfers may still be shorter than the advertised size — a short packet is how bulk transfers normally terminate — so this does not change on-the-wire behaviour, only what
tu_edpt_validate()will accept.Notes
Draft: I am aware that working around non-compliant device descriptors is a judgement call rather than an obvious win, and that the answer may reasonably be "the device is broken, fix the device". Raising it because the adjacent 512→64 clamp suggests the project is already willing to accommodate this class of bug, but I will happily close it if you would rather not accumulate more of them.
Tested against the one offending device [a Moog Slim Phatty] on RP2040; I have no second implausible-MPS device to check against.