EHCI bug hunt & reinit on host system error#667
Conversation
![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 refactors the EHCI USB driver to improve stability and error recovery. Key changes include replacing infinite polling loops with a timed ehci_handshake helper, implementing a controller reset mechanism (ehci_resetController) to recover from Host System Errors, utilizing atomic operations for status flags, and adding teardown routines for periodic and asynchronous lists. However, several critical issues were identified in the review: incorrect indexing of the uint16_t array desc->wData leads to an out-of-bounds write during UTF-16 encoding; modifying node->next in ehci_tearDownPeriodic corrupts shared periodic lists and causes memory leaks; the 20ms port reset handshake timeout is too short and may cause premature timeouts; and ignoring the return value of ehci_stopAsync in ehci_transferDequeue could lead to race conditions if the controller fails to stop.
adamgreloch
force-pushed
the
adamgreloch/usb-fixes
branch
from
9beec0f to
c3df6fb
Compare
TASK: RTOS-970
Lack of `else if` caused the transaction errors status (<0) to be overwritten by the success branch (>=0), causing partially received data to be reported as fully transferred. This also stops mistreating of QTD_HALTED as an error. The consequence of this bug was not visible due to it being canceled out by the above. TASK: RTOS-1335
`qh->next` was not checked for null deref, leading to `QH_PTR(NULL)`. Suppose two QHs with same period and phase are opened. Then, the first QH is inserted as the only element. When the second QH is inserted, `t` is the first QH and `t->next == NULL`, `t->period == qh->period`, leading to the else branch with `qh->next == t->next == NULL`. TASK: RTOS-1335
Suppose the periodic list has one QH and a new QH with longer period is added (same phase). Then t->period < qh->period, we arrive in the if branch and ehci->periodicNodes[qh->phase] == t. Thus, qh->next != NULL, but the qh->hw->horizontal stays marked as QH_PTR_INVALID (from ehci_qhAlloc). This is wrong, as the horizontal pointer of first element should point to the next element on the periodic list (EHCI 3.6.1). Leaving it marked with QTH_PTR_INVALID made the host controller treat the new QH as the END of the list, ignoring any QHs with longer periods. TASK: RTOS-1335
Per EHCI 2.3.9, bits CSC, PEC, OCC in PORTSC are write-1-to-clear. The ehci_resetPort haven't masked them when clearing ENA and PR, causing them to be accidentally lost. TASK: RTOS-1335
Memcpying to buf instead of wData corrupted the descriptor type, length and caused the actual langID to contain garbage on bytes 3-4. TASK: RTOS-1335
adamgreloch
force-pushed
the
adamgreloch/usb-fixes
branch
from
c3df6fb to
7628125
Compare
|
/gemini review |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review and don't encounter an error |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review and IMPORTANT: don't encounter an error |
There was a problem hiding this comment.
Code Review
This pull request refactors the EHCI USB driver to improve stability, error recovery, and thread safety. Key changes include introducing atomic operations for status tracking, replacing infinite polling loops with a timed handshake mechanism, implementing controller reset and transfer abort procedures on host system errors, and adding proper queue teardown to prevent memory leaks. The review feedback highlights several critical issues: potential race conditions in ehci_resetController due to missing locks during teardown, resource leaks in ehci_free (missing irqHandle and periodicLock destruction), signedness bypasses in port boundary checks that could allow negative port values, an inefficient usleep(1) in the handshake loop that could cause severe timeouts, and an unchecked return value for ehci_startAsync.
…ortStatus} These functions write to the t->buffer. Prior to the change, they returned 0 on success, causing the subsequent usb_transferFinished() to incorrectly set t->transferred to 0. TASK: RTOS-1335
Per USB 2.0 (9.6.7), bString is unicode*, not null-terminated and its length is calculated by subtracting 2 from bLength. *) de-facto utf-16le but this was precisely specified from usb 3.2+ (rant) TASK: RTOS-1335
adamgreloch
force-pushed
the
adamgreloch/usb-fixes
branch
from
7628125 to
51f8160
Compare
TASK: RTOS-1335
Per EHCI 4.10.2, the HC copies a QTD into the QH overlay area before executing it. Clearing QTD_ACTIVE while the HC is mid-read can cause corruption. To avoid the race, this unlinks the qh in case of async (does IAA ringing) and stops the schedule in case of periodic before modifying the QTD. TASK: RTOS-1335
adamgreloch
force-pushed
the
adamgreloch/usb-fixes
branch
from
51f8160 to
1239a06
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the EHCI USB driver to improve robustness, thread safety, and error recovery. Key changes include replacing busy-waiting loops with a timed handshake helper, introducing atomic operations for port reset changes and interrupt status, adding alignment assertions, and implementing a controller reset routine to recover from host system errors. The review feedback highlights two important issues: a missing cleanup of ehci->irqHandle in ehci_free which could lead to kernel panics, and a potential unsigned underflow in ehci_qtdsCheck if the hardware reports a remaining transfer length larger than the allocated bytes.
TASK: RTOS-1197
QTD_XACT being set does not imply that the QTD has been finished! Nothing, ever, should be inferred from the error bits before checking ACTIVE/HALTED. TASK: RTOS-1335
pipe->hcdpriv read was not guarded while it can be set to NULL by the irqThread during ehci controller reset. TASK: RTOS-1335
TASK: RTOS-1335
The QH must be linked into the hardware schedule and the transfer registered in hcd->transfers atomically. Otherwise, a concurrent HCD reset (which holds transLock during ehci_tearDownAsync) can free the newly-linked QH before the transfer is visible to ehci_transAbort. TASK: RTOS-1335
adamgreloch
force-pushed
the
adamgreloch/usb-fixes
branch
from
1239a06 to
3d64dbc
Compare
2 participants
Description
Motivation and Context
Types of changes
How Has This Been Tested?
Checklist:
Special treatment