usb: fix TT bugs and class-specific functional descriptor discovery #38
Conversation
Summary of ChangesHello @adamgreloch, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses critical bugs related to USB Transaction Translators (TT) and improves the discovery of class-specific functional descriptors. These changes are essential for the proper operation of USB devices, specifically enabling the CYW4373E dongle by ensuring correct handling of hub communications and descriptor parsing, which were previously problematic. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
adamgreloch
force-pushed
the
adamgreloch/tt-fix
branch
from
65e7fc4 to
3113f25
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses bugs related to TT hubs and class-specific functional descriptor discovery in the USB subsystem. The changes include adding a generic descriptor structure, modifying the configuration descriptor parsing logic, and adding a workaround for TT hubs. The review focuses on correctness and potential issues arising from the introduced changes.
JIRA: RTOS-1121
Unit Test Results9 462 tests +1 504 8 873 ✅ +1 439 50m 56s ⏱️ + 10m 23s Results for commit ee3e540. ± Comparison against base commit 91853a6. This pull request removes 207 and adds 1711 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
adamgreloch
force-pushed
the
adamgreloch/tt-fix
branch
from
3113f25 to
4610e09
Compare
adamgreloch
force-pushed
the
adamgreloch/tt-fix
branch
from
4610e09 to
c005cb3
Compare
usb/hub.c
Outdated
| condWait(hub_common.cond, hub_common.lock, HUB_TT_POLL_DELAY_MS); | ||
|
|
||
| /* | ||
| * hub_ttStatus may lead to usb_devEnumerate call which may call hub_conf that |
There was a problem hiding this comment.
Can you show the problematic path that leads to the mutex lock? I can see another path leading to hub_ttRemove().
It appears that this lock protects both hub_common.events and hub_common.tts lists. Would introduction of separate locks for these lists fix this issue?
There was a problem hiding this comment.
hub_ttStatus -> hub_portstatus -> hub_connectstatus -> hub_devConnected -> usb_devEnumerate -> hub_conf -> hub_ttAdd
The problem comes from the fact that hub_portstatus can be called from hub_ttStatus or from normal device discovery below, where it is assumed not to have the lock.
Actually, now I see that this workaround is still flawed (but to the other side), as hub_ttStatus now accesses hub_common.tts unsafely, so the mutex lock/unlock should be moved to hub_ttStatus for accessing the item from hub_common.tts.
A separate lock could help here a bit, but there is still a recursion with mutex over the same resource when calling hub_ttStatus. We would need a reentrant lock. Or deeply rework this architecture, but since this is just a workaround for x86 interrupt pipe problems, it may be easier to solve the root cause than that.
There was a problem hiding this comment.
Moved the unlock/lock inside the hub_ttStatus and extended the comment
adamgreloch
force-pushed
the
adamgreloch/tt-fix
branch
from
c005cb3 to
d88f49c
Compare
When tested on imxrt1170-evkb with Laird/Sterling LWB5+ plugged in (internal TT hub + two internal devices), the workaround for the lack of interrupt pipe notifications was not neeeded. It seems that ia32 is the only platform suffering from this issue. JIRA: RTOS-1121
JIRA: RTOS-1121
Returning failure on bind causes the discovery of subsequent devices on a given hub to fail. This is still yet to be fixed with proper orphans management. JIRA: RTOS-1121
adamgreloch
force-pushed
the
adamgreloch/tt-fix
branch
from
d88f49c to
ee3e540
Compare
|
Please combine two first commits. |
| * part of L2 TT workaround, the problem may be easier to solve itself in the root | ||
| */ | ||
| mutexUnlock(hub_common.lock); | ||
| hub_ttStatus(); |
There was a problem hiding this comment.
Another approach that avoids the deadlock is to iterate over all hubs and inject an artificial event for each, rather than invoking hub_portstatus().
Description
Various bug fixes that came up when probing the CYW4373E dongle composed of a TT hub and 3 internal devices, some of which contained class-specific functional descriptors that were incorrectly parsed.
Motivation and Context
These changes make it possible to run the CYW4373E driver (phoenix-rtos/phoenix-rtos-devices#596).
Types of changes
How Has This Been Tested?
Checklist:
Special treatment