From ee48fc06463cf320b8599fdf46b952ebad2cb7d4 Mon Sep 17 00:00:00 2001 From: Adam Greloch Date: Mon, 25 May 2026 14:54:35 +0200 Subject: [PATCH 01/18] ehci: update ehci->{status,portResetChange} atomically TASK: RTOS-970 --- usb/ehci/ehci-hub.c | 5 +++-- usb/ehci/ehci.c | 33 ++++++++++++++++++--------------- usb/ehci/ehci.h | 4 ++-- 3 files changed, 23 insertions(+), 19 deletions(-) diff --git a/usb/ehci/ehci-hub.c b/usb/ehci/ehci-hub.c index 393713fa4..5e3e53667 100644 --- a/usb/ehci/ehci-hub.c +++ b/usb/ehci/ehci-hub.c @@ -14,6 +14,7 @@ #include #include +#include #include #include #include @@ -131,7 +132,7 @@ static void ehci_resetPort(hcd_t *hcd, int port) } #endif - ehci->portResetChange = 1 << port; + atomic_fetch_or(&ehci->portResetChange, 1 << port); if ((*reg & PORTSC_PSPD) == PORTSC_PSPD_HS) phy_enableHighSpeedDisconnect(hcd, 1); @@ -250,7 +251,7 @@ static int ehci_clearPortFeature(usb_dev_t *hub, int port, uint16_t wValue) *portsc = val | PORTSC_OCC; break; case USB_PORT_FEAT_C_RESET: - ehci->portResetChange &= ~(1 << port); + atomic_fetch_and(&ehci->portResetChange, ~(1 << port)); break; case USB_PORT_FEAT_ENABLE: /* Disable port */ diff --git a/usb/ehci/ehci.c b/usb/ehci/ehci.c index f18fc453f..af3ca5fec 100644 --- a/usb/ehci/ehci.c +++ b/usb/ehci/ehci.c @@ -21,6 +21,7 @@ #include #include +#include #include #include #include @@ -546,20 +547,21 @@ static int ehci_irqHandler(unsigned int n, void *data) { hcd_t *hcd = (hcd_t *)data; ehci_t *ehci = (ehci_t *)hcd->priv; - uint32_t currentStatus; + uint32_t currentStatus, handled = 0; currentStatus = *(ehci->opbase + usbsts); do { *(ehci->opbase + usbsts) = currentStatus & (EHCI_INTRMASK | USBSTS_FRI); - ehci->status |= currentStatus; + handled |= currentStatus; + atomic_fetch_or(&ehci->status, currentStatus); /* For edge triggered interrupts to prevent losing interrupts, * poll the usbsts register until it is stable */ currentStatus = *(ehci->opbase + usbsts); } while ((currentStatus & EHCI_INTRMASK) != 0); - return -!(ehci->status & EHCI_INTRMASK); + return -!(handled & EHCI_INTRMASK); } @@ -639,7 +641,7 @@ static void ehci_portStatusChanged(hcd_t *hcd) #if EHCI_DEBUG_IRQ -static void ehci_printIrq(hcd_t *hcd) +static void ehci_printIrq(hcd_t *hcd, uint32_t status) { ehci_t *ehci = (ehci_t *)hcd->priv; static char buf[30]; @@ -648,7 +650,7 @@ static void ehci_printIrq(hcd_t *hcd) i += sprintf(buf, "INT%d: ", hcd->info->irq); #define append_to_buf(interrupt) \ - if (ehci->status & (interrupt)) { \ + if (status & (interrupt)) { \ i += sprintf(buf + i, #interrupt " "); \ } append_to_buf(USBSTS_UI); @@ -665,34 +667,35 @@ static void ehci_irqThread(void *arg) { hcd_t *hcd = (hcd_t *)arg; ehci_t *ehci = (ehci_t *)hcd->priv; + uint32_t status; mutexLock(ehci->irqLock); for (;;) { condWait(ehci->irqCond, ehci->irqLock, 0); + /* + * The irqThread must clear the handler interrupt status, since otherwise it would handle + * ghost interrupts on every interrupt (irqHandler never clears ehci->status) + */ + status = atomic_exchange(&ehci->status, 0); + #if EHCI_DEBUG_IRQ - ehci_printIrq(hcd); + ehci_printIrq(hcd, status); #endif - /* The irqThread must clear the handler interrupt status, - since otherwise it would handle ghost interrupts - on every interrupt (irqHandler never clears ehci->status) */ - if (ehci->status & USBSTS_SEI) { - ehci->status &= ~USBSTS_SEI; + if (status & USBSTS_SEI) { log_error("host system error, controller halted"); /* TODO cleanup/reset after death */ continue; } - if (ehci->status & (USBSTS_UI | USBSTS_UEI)) { - ehci->status &= ~(USBSTS_UI | USBSTS_UEI); + if (status & (USBSTS_UI | USBSTS_UEI)) { mutexLock(hcd->transLock); ehci_transUpdate(hcd); mutexUnlock(hcd->transLock); } - if (ehci->status & USBSTS_PCI) { - ehci->status &= ~USBSTS_PCI; + if (status & USBSTS_PCI) { ehci_portStatusChanged(hcd); } } diff --git a/usb/ehci/ehci.h b/usb/ehci/ehci.h index 06203adcb..56400f25c 100644 --- a/usb/ehci/ehci.h +++ b/usb/ehci/ehci.h @@ -237,8 +237,8 @@ typedef struct { size_t nqtds; handle_t irqCond, irqHandle, irqLock, asyncLock, periodicLock; - volatile unsigned portResetChange; - volatile unsigned status; + _Atomic uint32_t portResetChange; + _Atomic uint32_t status; volatile uint32_t *base; volatile uint32_t *opbase; From 8ce5edb05afae4d52598786281925b3292bd9e5c Mon Sep 17 00:00:00 2001 From: Adam Greloch Date: Mon, 25 May 2026 14:59:59 +0200 Subject: [PATCH 02/18] ehci: fix QTD errors being silently treated as success 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 --- usb/ehci/ehci.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/usb/ehci/ehci.c b/usb/ehci/ehci.c index af3ca5fec..38f87f0d0 100644 --- a/usb/ehci/ehci.c +++ b/usb/ehci/ehci.c @@ -568,13 +568,13 @@ static int ehci_irqHandler(unsigned int n, void *data) static int ehci_qtdsCheck(hcd_t *hcd, usb_transfer_t *t, int *status) { ehci_qtd_t *qtds = (ehci_qtd_t *)t->hcdpriv; - int error = 0; + unsigned int error = 0; int finished = 0; *status = 0; do { ehci_qtdDump(qtds, false); - if (qtds->hw->token & (QTD_XACT | QTD_BABBLE | QTD_BUFERR | QTD_HALTED)) { + if (qtds->hw->token & (QTD_XACT | QTD_BABBLE | QTD_BUFERR)) { error++; } @@ -585,9 +585,7 @@ static int ehci_qtdsCheck(hcd_t *hcd, usb_transfer_t *t, int *status) finished = 1; *status = -error; } - - /* Finished no error */ - if (!(qtds->prev->hw->token & QTD_ACTIVE) || (qtds->prev->hw->token & QTD_HALTED)) { + else if (!(qtds->prev->hw->token & QTD_ACTIVE) || (qtds->prev->hw->token & QTD_HALTED)) { finished = 1; *status = t->size - QTD_LEN(qtds->prev->hw->token); } From 19a93eadda49c9bdd2e6f6374b92bf7f21c15eab Mon Sep 17 00:00:00 2001 From: Adam Greloch Date: Mon, 25 May 2026 15:12:03 +0200 Subject: [PATCH 03/18] ehci: fix potential null deref when inserting QH after last element `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 --- usb/ehci/ehci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/usb/ehci/ehci.c b/usb/ehci/ehci.c index 38f87f0d0..c0f5b783b 100644 --- a/usb/ehci/ehci.c +++ b/usb/ehci/ehci.c @@ -437,7 +437,7 @@ static void ehci_qhLinkPeriodic(hcd_t *hcd, ehci_qh_t *qh) /* Insert inside */ qh->next = t->next; t->next = qh; - qh->hw->horizontal = QH_PTR(qh->next); + qh->hw->horizontal = (qh->next != NULL) ? QH_PTR(qh->next) : QH_PTR_INVALID; t->hw->horizontal = QH_PTR(qh); } From 7d008ee644753c5679db5aaeacdf9dc16acdc2a0 Mon Sep 17 00:00:00 2001 From: Adam Greloch Date: Mon, 25 May 2026 15:24:05 +0200 Subject: [PATCH 04/18] ehci: fix hw horizontal pointer not being set for new first QH 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 --- usb/ehci/ehci.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/usb/ehci/ehci.c b/usb/ehci/ehci.c index c0f5b783b..b68d4ff19 100644 --- a/usb/ehci/ehci.c +++ b/usb/ehci/ehci.c @@ -437,14 +437,17 @@ static void ehci_qhLinkPeriodic(hcd_t *hcd, ehci_qh_t *qh) /* Insert inside */ qh->next = t->next; t->next = qh; - qh->hw->horizontal = (qh->next != NULL) ? QH_PTR(qh->next) : QH_PTR_INVALID; t->hw->horizontal = QH_PTR(qh); } - if (qh->next == NULL) { + if (qh->next != NULL) { + qh->hw->horizontal = QH_PTR(qh->next); + } + else { /* New last element */ qh->hw->horizontal |= QH_PTR_INVALID; } + ehci_memDmb(); mutexUnlock(ehci->periodicLock); } From 20fd2c148526f8768ea07282804effc7e3555c23 Mon Sep 17 00:00:00 2001 From: Adam Greloch Date: Mon, 25 May 2026 15:44:53 +0200 Subject: [PATCH 05/18] ehci: fix CBITS being cleared on port reset 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 --- usb/ehci/ehci-hub.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/usb/ehci/ehci-hub.c b/usb/ehci/ehci-hub.c index 5e3e53667..40d5d5ee2 100644 --- a/usb/ehci/ehci-hub.c +++ b/usb/ehci/ehci-hub.c @@ -99,7 +99,7 @@ static void ehci_resetPort(hcd_t *hcd, int port) log_debug("resetting port %d", port); tmp = *reg; - tmp &= ~(PORTSC_ENA | PORTSC_PR); + tmp &= ~(PORTSC_ENA | PORTSC_PR | PORTSC_CBITS); *reg = tmp | PORTSC_PR; #ifdef EHCI_IMX @@ -236,7 +236,9 @@ static int ehci_clearPortFeature(usb_dev_t *hub, int port, uint16_t wValue) if (port > hub->nports) return -1; + /* Prevent clearing unrelated write-1-to-clear bits */ val &= ~PORTSC_CBITS; + switch (wValue) { /* For 'change' features, ack the change */ case USB_PORT_FEAT_C_CONNECTION: From 2dff02274836473273d6f841ab1ffd5eb33afbcf Mon Sep 17 00:00:00 2001 From: Adam Greloch Date: Mon, 25 May 2026 15:58:16 +0200 Subject: [PATCH 06/18] ehci/ehci-hub: fix langID string descriptor corruption 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 --- usb/ehci/ehci-hub.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/usb/ehci/ehci-hub.c b/usb/ehci/ehci-hub.c index 40d5d5ee2..5ea3a4c64 100644 --- a/usb/ehci/ehci-hub.c +++ b/usb/ehci/ehci-hub.c @@ -308,14 +308,15 @@ static int ehci_getStringDesc(usb_dev_t *hub, int index, char *buf, size_t size) desc->bLength = len; if (index == 0) { - memcpy(buf, src, len - 2); + /* LangID */ + memcpy(desc->wData, src, len - 2); } else { - /* Unicode encode */ + /* Encode in UTF-16LE */ memset(desc->wData, 0, len - 2); - - for (i = 0; src[i] != '\0'; i++) + for (i = 0; src[i] != '\0'; i++) { desc->wData[i * 2] = src[i]; + } } return len; From 4d3af78039484902ee4a1a47ff68f67ddd0c708b Mon Sep 17 00:00:00 2001 From: Adam Greloch Date: Mon, 25 May 2026 16:34:17 +0200 Subject: [PATCH 07/18] ehci/ehci-hub: guard against wIndex=0 port reads from ehci_roothubReq TASK: RTOS-1335 --- usb/ehci/ehci-hub.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/usb/ehci/ehci-hub.c b/usb/ehci/ehci-hub.c index 5ea3a4c64..2d61c2150 100644 --- a/usb/ehci/ehci-hub.c +++ b/usb/ehci/ehci-hub.c @@ -12,6 +12,7 @@ */ +#include #include #include #include @@ -96,6 +97,8 @@ static void ehci_resetPort(hcd_t *hcd, int port) volatile uint32_t *reg = (ehci->opbase + portsc1) + (port - 1); uint32_t tmp; + assert(port > 0); + log_debug("resetting port %d", port); tmp = *reg; @@ -145,8 +148,9 @@ static int ehci_getPortStatus(usb_dev_t *hub, int port, usb_port_status_t *statu ehci_t *ehci = (ehci_t *)hcd->priv; uint32_t val; - if (port > hub->nports) + if (port <= 0 || port > hub->nports) { return -1; + } status->wPortChange = 0; status->wPortStatus = 0; @@ -205,8 +209,9 @@ static int ehci_setPortFeature(usb_dev_t *hub, int port, uint16_t wValue) { hcd_t *hcd = hub->hcd; - if (port > hub->nports) + if (port <= 0 || port > hub->nports) { return -1; + } switch (wValue) { case USB_PORT_FEAT_RESET: @@ -230,11 +235,13 @@ static int ehci_clearPortFeature(usb_dev_t *hub, int port, uint16_t wValue) { hcd_t *hcd = hub->hcd; ehci_t *ehci = (ehci_t *)hcd->priv; - volatile uint32_t *portsc = (ehci->opbase + portsc1) + (port - 1); - uint32_t val = *portsc; - if (port > hub->nports) + if (port <= 0 || port > hub->nports) { return -1; + } + + volatile uint32_t *portsc = (ehci->opbase + portsc1) + (port - 1); + uint32_t val = *portsc; /* Prevent clearing unrelated write-1-to-clear bits */ val &= ~PORTSC_CBITS; From 9a6b2e4955ebeb3f65a2fe129c490f02c0a3a645 Mon Sep 17 00:00:00 2001 From: Adam Greloch Date: Mon, 25 May 2026 17:36:39 +0200 Subject: [PATCH 08/18] ehci/ehci-hub: fix hub status not being propagated after port reset TASK: RTOS-1335 --- usb/ehci/ehci-hub.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/usb/ehci/ehci-hub.c b/usb/ehci/ehci-hub.c index 2d61c2150..45e297e9b 100644 --- a/usb/ehci/ehci-hub.c +++ b/usb/ehci/ehci-hub.c @@ -378,8 +378,9 @@ uint32_t ehci_getHubStatus(usb_dev_t *hub) for (i = 0; i < hub->nports; i++) { val = *(ehci->opbase + portsc1 + i); log_debug("(INT%d) port %d portsc: %x", hcd->info->irq, i + 1, val); - if (val & (PORTSC_CSC | PORTSC_PEC | PORTSC_OCC)) + if ((val & (PORTSC_CSC | PORTSC_PEC | PORTSC_OCC)) != 0 || (ehci->portResetChange & (1 << (i + 1))) != 0) { status |= 1 << (i + 1); + } } log_debug("(INT%d): status: %x", hcd->info->irq, status); From 855a957bdd0a447523b0b78d67a21b6b111b44a3 Mon Sep 17 00:00:00 2001 From: Adam Greloch Date: Mon, 25 May 2026 17:39:29 +0200 Subject: [PATCH 09/18] ehci/ehci-hub: return number of bytes written from ehci_{getDesc,getPortStatus} 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 --- usb/ehci/ehci-hub.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/usb/ehci/ehci-hub.c b/usb/ehci/ehci-hub.c index 45e297e9b..c41d2ced2 100644 --- a/usb/ehci/ehci-hub.c +++ b/usb/ehci/ehci-hub.c @@ -201,7 +201,7 @@ static int ehci_getPortStatus(usb_dev_t *hub, int port, usb_port_status_t *statu /* TODO: set indicator */ - return 0; + return sizeof(usb_port_status_t); } @@ -361,10 +361,11 @@ static int ehci_getDesc(usb_dev_t *hub, int type, int index, char *buf, size_t s hdesc->bNbrPorts = *(ehci->base + hcsparams) & 0xf; hdesc->variable[0] = 0; /* Device not removable */ hdesc->variable[1] = 0xff; /* PortPwrCtrlMask */ + bytes = sizeof(usb_hub_desc_t) + 2; break; } - return 0; + return bytes; } From 463ebad0de776cba4b24783867f759b6bb533508 Mon Sep 17 00:00:00 2001 From: Adam Greloch Date: Mon, 25 May 2026 17:50:47 +0200 Subject: [PATCH 10/18] ehci/ehci-hub: fix off-by-one in hub product/manufacturer string lens 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 --- usb/ehci/ehci-hub.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/usb/ehci/ehci-hub.c b/usb/ehci/ehci-hub.c index c41d2ced2..3e4de3fe8 100644 --- a/usb/ehci/ehci-hub.c +++ b/usb/ehci/ehci-hub.c @@ -295,12 +295,12 @@ static int ehci_getStringDesc(usb_dev_t *hub, int index, char *buf, size_t size) break; case 1: /* Product string */ - len = strlen(ehci_descs.product) * 2 + 3; + len = strlen(ehci_descs.product) * 2 + 2; src = ehci_descs.product; break; case 2: /* Manufacturer string */ - len = strlen(ehci_descs.manufacturer) * 2 + 3; + len = strlen(ehci_descs.manufacturer) * 2 + 2; src = ehci_descs.manufacturer; break; default: From a257b80b51c89d91a3c268d40d3c63fb771a1702 Mon Sep 17 00:00:00 2001 From: Adam Greloch Date: Thu, 28 May 2026 10:22:24 +0200 Subject: [PATCH 11/18] ehci: use a timeoutable handshake when polling the ehci registers TASK: RTOS-1335 --- usb/ehci/ehci-hub.c | 19 ++++++++++--------- usb/ehci/ehci.c | 38 +++++++++++++++++++++++--------------- usb/ehci/ehci.h | 19 +++++++++++++++++++ 3 files changed, 52 insertions(+), 24 deletions(-) diff --git a/usb/ehci/ehci-hub.c b/usb/ehci/ehci-hub.c index 3e4de3fe8..8a4bee0a0 100644 --- a/usb/ehci/ehci-hub.c +++ b/usb/ehci/ehci-hub.c @@ -105,27 +105,28 @@ static void ehci_resetPort(hcd_t *hcd, int port) tmp &= ~(PORTSC_ENA | PORTSC_PR | PORTSC_CBITS); *reg = tmp | PORTSC_PR; -#ifdef EHCI_IMX /* - * imx deviation: According to ehci documentation - * it is up to software to set the PR bit 0 after waiting 20ms + * imx deviation: According to ehci documentation, it is up to software to + * set the PR bit 0 after waiting 20ms. On IMX, it is done by the controller. */ - while (*reg & PORTSC_PR) - ; - usleep(20 * 1000); -#else + +#ifndef EHCI_IMX /* Wait for reset to complete */ usleep(50 * 1000); /* Stop the reset sequence */ *reg = tmp; +#endif /* Wait until reset sequence stops */ - while ((*reg & PORTSC_PR) != 0) - ; + if (ehci_handshake(reg, PORTSC_PR, 0, 100000) < 0) { + log_error("port reset handshake timed out"); + return; + } usleep(20 * 1000); +#ifndef EHCI_IMX tmp = *reg; log_debug("port %d reset done, status after reset=%x", port, tmp); diff --git a/usb/ehci/ehci.c b/usb/ehci/ehci.c index b68d4ff19..2bfc82d36 100644 --- a/usb/ehci/ehci.c +++ b/usb/ehci/ehci.c @@ -59,26 +59,24 @@ static inline void ehci_memDmb(void) } -static void ehci_startAsync(hcd_t *hcd) +static int ehci_startAsync(hcd_t *hcd) { ehci_t *ehci = (ehci_t *)hcd->priv; *(ehci->opbase + asynclistaddr) = va2pa((void *)ehci->asyncList->hw); *(ehci->opbase + usbcmd) |= USBCMD_ASE; ehci_memDmb(); - while ((*(ehci->opbase + usbsts) & USBSTS_AS) == 0) - ; + return ehci_handshake(ehci->opbase + usbsts, USBSTS_AS, USBSTS_AS, 20000); } -static void ehci_stopAsync(hcd_t *hcd) +static int ehci_stopAsync(hcd_t *hcd) { ehci_t *ehci = (ehci_t *)hcd->priv; *(ehci->opbase + usbcmd) &= ~USBCMD_ASE; ehci_memDmb(); - while ((*(ehci->opbase + usbsts) & USBSTS_AS) != 0) - ; + return ehci_handshake(ehci->opbase + usbsts, USBSTS_AS, 0, 20000); } @@ -487,15 +485,19 @@ static void ehci_qtdsDeactivate(ehci_qtd_t *qtds) } +/* + * TODO: propagate -ETIMEDOUT from {stop,start}Async up to pipeDestroy - + * requires hcd signature change + */ static void ehci_qhUnlinkAsync(hcd_t *hcd, ehci_qh_t *qh) { ehci_t *ehci = (ehci_t *)hcd->priv; mutexLock(ehci->asyncLock); - ehci_stopAsync(hcd); + (void)ehci_stopAsync(hcd); qh->prev->hw->horizontal = qh->hw->horizontal; - ehci_startAsync(hcd); + (void)ehci_startAsync(hcd); ehci_memDmb(); qh->prev->next = qh->next; @@ -720,6 +722,7 @@ static int ehci_qtdAdd(ehci_t *ehci, ehci_qtd_t **list, int token, size_t maxpac } +/* TODO: return -ETIMEDOUT on failed handshakes/{start,stop}Async - will require a change in signature */ static void ehci_transferDequeue(hcd_t *hcd, usb_transfer_t *t) { mutexLock(hcd->transLock); @@ -974,14 +977,16 @@ static int ehci_init(hcd_t *hcd) /* Hangs controller on imx */ *(ehci->opbase + usbcmd) &= ~(USBCMD_RUN | USBCMD_IAA); - while ((*(ehci->opbase + usbsts) & USBSTS_HCH) == 0) - ; + if (ehci_handshake(ehci->opbase + usbsts, USBSTS_HCH, USBSTS_HCH, 100000) < 0) { + return -ETIMEDOUT; + } #endif /* Reset controller */ *(ehci->opbase + usbcmd) |= USBCMD_HCRESET; - while ((*(ehci->opbase + usbcmd) & USBCMD_HCRESET) != 0) - ; + if (ehci_handshake(ehci->opbase + usbcmd, USBCMD_HCRESET, 0, 250000) < 0) { + return -ETIMEDOUT; + } #ifdef EHCI_IMX /* imx deviation: Set host mode */ @@ -1007,8 +1012,9 @@ static int ehci_init(hcd_t *hcd) *(ehci->opbase + usbcmd) &= ~(USBCMD_LRESET | USBCMD_ASE); *(ehci->opbase + usbcmd) |= (USBCMD_PSE | USBCMD_RUN); - while ((*(ehci->opbase + usbsts) & (USBSTS_HCH)) != 0) - ; + if (ehci_handshake(ehci->opbase + usbsts, USBSTS_HCH, 0, 100000) < 0) { + return -ETIMEDOUT; + } /* Route all ports to this host controller */ *(ehci->opbase + configflag) = 1; @@ -1016,7 +1022,9 @@ static int ehci_init(hcd_t *hcd) /* Allow for the hardware to catch up */ usleep(50 * 1000); - ehci_startAsync(hcd); + if (ehci_startAsync(hcd) < 0) { + return -ETIMEDOUT; + } log_debug("hc initialized"); diff --git a/usb/ehci/ehci.h b/usb/ehci/ehci.h index 56400f25c..d2817849c 100644 --- a/usb/ehci/ehci.h +++ b/usb/ehci/ehci.h @@ -257,4 +257,23 @@ int ehci_roothubReq(usb_dev_t *hub, usb_transfer_t *t); uint32_t ehci_getHubStatus(usb_dev_t *hub); +/* Poll a register until (read & mask) == done, or timeout. Returns 0 on success, -ETIMEDOUT on timeout. */ +static inline int ehci_handshake(volatile uint32_t *reg, uint32_t mask, uint32_t done, unsigned int timeoutUs) +{ + unsigned int elapsedUs = 0, sleepUs = 100; + + while (elapsedUs < timeoutUs) { + if ((*reg & mask) == done) { + return 0; + } + usleep(sleepUs); + elapsedUs += sleepUs; + } + + log_error("handshake failed: %p & 0x%x != 0x%x after %d us", reg, mask, done, timeoutUs); + + return -ETIMEDOUT; +} + + #endif /* _USB_EHCI_H_ */ From ac1a8b8f2ffaaf23e3895ff5261d67173cdc485c Mon Sep 17 00:00:00 2001 From: Adam Greloch Date: Thu, 28 May 2026 10:23:38 +0200 Subject: [PATCH 12/18] ehci: ring the IAA on QH unlink instead of async start/stop TASK: RTOS-1335 --- usb/ehci/ehci.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/usb/ehci/ehci.c b/usb/ehci/ehci.c index 2bfc82d36..f87c327cc 100644 --- a/usb/ehci/ehci.c +++ b/usb/ehci/ehci.c @@ -70,16 +70,6 @@ static int ehci_startAsync(hcd_t *hcd) } -static int ehci_stopAsync(hcd_t *hcd) -{ - ehci_t *ehci = (ehci_t *)hcd->priv; - - *(ehci->opbase + usbcmd) &= ~USBCMD_ASE; - ehci_memDmb(); - return ehci_handshake(ehci->opbase + usbsts, USBSTS_AS, 0, 20000); -} - - static void ehci_qtdLink(ehci_qtd_t *prev, ehci_qtd_t *next) { prev->hw->next = next->paddr; @@ -495,14 +485,25 @@ static void ehci_qhUnlinkAsync(hcd_t *hcd, ehci_qh_t *qh) mutexLock(ehci->asyncLock); - (void)ehci_stopAsync(hcd); qh->prev->hw->horizontal = qh->hw->horizontal; - (void)ehci_startAsync(hcd); ehci_memDmb(); qh->prev->next = qh->next; qh->next->prev = qh->prev; + /* Ring the IAA doorbell if async schedule is enabled */ + if ((*(ehci->opbase + usbsts) & USBSTS_AS) != 0) { + *(ehci->opbase + usbsts) = USBSTS_IAA; + ehci_memDmb(); + *(ehci->opbase + usbcmd) |= USBCMD_IAA; + ehci_memDmb(); + + (void)ehci_handshake(ehci->opbase + usbsts, USBSTS_IAA, USBSTS_IAA, 10000); + + *(ehci->opbase + usbsts) = USBSTS_IAA; + ehci_memDmb(); + } + mutexUnlock(ehci->asyncLock); } From 3d0b616ccf44e64a024e3b34f339bbdcd8357069 Mon Sep 17 00:00:00 2001 From: Adam Greloch Date: Mon, 25 May 2026 16:48:53 +0200 Subject: [PATCH 13/18] ehci: fix race between QTD deactivation and hardware read 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 --- usb/ehci/ehci.c | 52 +++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 48 insertions(+), 4 deletions(-) diff --git a/usb/ehci/ehci.c b/usb/ehci/ehci.c index f87c327cc..793151bc0 100644 --- a/usb/ehci/ehci.c +++ b/usb/ehci/ehci.c @@ -726,11 +726,55 @@ static int ehci_qtdAdd(ehci_t *ehci, ehci_qtd_t **list, int token, size_t maxpac /* TODO: return -ETIMEDOUT on failed handshakes/{start,stop}Async - will require a change in signature */ static void ehci_transferDequeue(hcd_t *hcd, usb_transfer_t *t) { + ehci_t *ehci = (ehci_t *)hcd->priv; + ehci_qtd_t *qtds; + mutexLock(hcd->transLock); - /* note: not tested for interrupt transfers */ - if (t->hcdpriv != NULL) - ehci_qtdsDeactivate((ehci_qtd_t *)t->hcdpriv); - ehci_transUpdate(hcd); + + if (t->hcdpriv == NULL) { + ehci_transUpdate(hcd); + mutexUnlock(hcd->transLock); + return; + } + + qtds = (ehci_qtd_t *)t->hcdpriv; + if (t->type == usb_transfer_bulk || t->type == usb_transfer_control) { + ehci_qh_t *qh = qtds->qh; + ehci_qhUnlinkAsync(hcd, qh); + + mutexLock(ehci->asyncLock); + ehci_qtdsDeactivate(qtds); + /* Clear overlay active bit so HC won't resume this QTD */ + qh->hw->token &= ~QTD_ACTIVE; + ehci_memDmb(); + mutexUnlock(ehci->asyncLock); + + ehci_transUpdate(hcd); + + ehci_qhLinkAsync(hcd, qh); + } + else if (t->type == usb_transfer_interrupt) { + mutexLock(ehci->periodicLock); + + *(ehci->opbase + usbcmd) &= ~USBCMD_PSE; + ehci_memDmb(); + (void)ehci_handshake(ehci->opbase + usbsts, USBSTS_PS, 0, 20000); + + ehci_qtdsDeactivate(qtds); + qtds->qh->hw->token &= ~QTD_ACTIVE; + ehci_memDmb(); + + mutexUnlock(ehci->periodicLock); + + ehci_transUpdate(hcd); + + mutexLock(ehci->periodicLock); + *(ehci->opbase + usbcmd) |= USBCMD_PSE; + ehci_memDmb(); + (void)ehci_handshake(ehci->opbase + usbsts, USBSTS_PS, USBSTS_PS, 20000); + mutexUnlock(ehci->periodicLock); + } + mutexUnlock(hcd->transLock); } From 20e34286a38b445388fd554db5c5262679e3405e Mon Sep 17 00:00:00 2001 From: Adam Greloch Date: Wed, 27 May 2026 12:06:01 +0200 Subject: [PATCH 14/18] ehci: reinit controller on host system error TASK: RTOS-1197 --- usb/ehci/ehci.c | 322 +++++++++++++++++++++++++++++++++++------------- usb/ehci/ehci.h | 1 + 2 files changed, 239 insertions(+), 84 deletions(-) diff --git a/usb/ehci/ehci.c b/usb/ehci/ehci.c index 793151bc0..52babc6e9 100644 --- a/usb/ehci/ehci.c +++ b/usb/ehci/ehci.c @@ -307,6 +307,7 @@ static ehci_qh_t *ehci_qhAlloc(ehci_t *ehci) qh->next = NULL; qh->prev = NULL; + qh->pipe = NULL; qh->period = 0; qh->uframe = 0; qh->phase = 0; @@ -600,13 +601,13 @@ static int ehci_qtdsCheck(hcd_t *hcd, usb_transfer_t *t, int *status) } -static void ehci_transUpdate(hcd_t *hcd) +static void ehci_transUpdate_ex(hcd_t *hcd, int abort) { ehci_qh_t *qh; ehci_qtd_t *qtd; usb_transfer_t *t, *n; int cont; - int status; + int status, finished; if ((t = hcd->transfers) == NULL) return; @@ -617,19 +618,40 @@ static void ehci_transUpdate(hcd_t *hcd) cont = 0; n = t->next; - if (ehci_qtdsCheck(hcd, t, &status)) { + if (abort == 0) { + finished = ehci_qtdsCheck(hcd, t, &status); + } + else { + finished = 1; + status = -1; + } + + if (finished != 0) { ehci_continue(hcd->priv, qh, qtd->prev); ehci_qtdsPut(hcd->priv, &qtd); LIST_REMOVE(&hcd->transfers, t); t->hcdpriv = NULL; usb_transferFinished(t, status); - if (n != t) + if (n != t) { cont = 1; + } } } while (hcd->transfers && ((t = n) != hcd->transfers || cont)); } +static void ehci_transUpdate(hcd_t *hcd) +{ + return ehci_transUpdate_ex(hcd, 0); +} + + +static void ehci_transAbort(hcd_t *hcd) +{ + return ehci_transUpdate_ex(hcd, 1); +} + + static void ehci_portStatusChanged(hcd_t *hcd) { usb_dev_t *hub = hcd->roothub; @@ -667,6 +689,9 @@ static void ehci_printIrq(hcd_t *hcd, uint32_t status) #endif +static int ehci_resetController(hcd_t *hcd); + + static void ehci_irqThread(void *arg) { hcd_t *hcd = (hcd_t *)arg; @@ -688,9 +713,35 @@ static void ehci_irqThread(void *arg) #endif if (status & USBSTS_SEI) { - log_error("host system error, controller halted"); - /* TODO cleanup/reset after death */ - continue; + log_error("host system error: USBCMD=%08x USBSTS=%08x ASYNCLISTADDR=%08x PERIODICLISTBASE=%08x PORTSC=%08x", + *(ehci->opbase + usbcmd), *(ehci->opbase + usbsts), *(ehci->opbase + asynclistaddr), *(ehci->opbase + periodiclistbase), *(ehci->opbase + portsc1)); + + /* Wait for HC to fully halt before touching transfers. The HC may still be mid-DMA */ + *(ehci->opbase + usbcmd) &= ~USBCMD_RUN; + ehci_memDmb(); + if (ehci_handshake(ehci->opbase + usbsts, USBSTS_HCH, USBSTS_HCH, 100000) < 0) { + log_error("HC did not halt after SEI"); + continue; + } + + mutexLock(hcd->transLock); + ehci_transAbort(hcd); + if (ehci_resetController(hcd) < 0) { + log_error("failed to reset the controller"); + mutexUnlock(hcd->transLock); + continue; + } + mutexUnlock(hcd->transLock); + + /* Pick up any interrupts (e.g. PCI) that fired during reinit */ + status = atomic_exchange(&ehci->status, 0); + + /* Ensure port changes are not missed */ + ehci_portStatusChanged(hcd); + + if (status == 0) { + continue; + } } if (status & (USBSTS_UI | USBSTS_UEI)) { @@ -794,6 +845,7 @@ static int ehci_transferEnqueue(hcd_t *hcd, usb_transfer_t *t, usb_pipe_t *pipe) ehci_qhConf(qh, pipe); pipe->hcdpriv = qh; + qh->pipe = pipe; if (t->type == usb_transfer_bulk || t->type == usb_transfer_control) ehci_qhLinkAsync(hcd, qh); @@ -897,30 +949,191 @@ static void ehci_pipeDestroy(hcd_t *hcd, usb_pipe_t *pipe) } +static void ehci_tearDownAsync(ehci_t *ehci) +{ + ehci_qh_t *head = ehci->asyncList; + ehci_qh_t *curr = head->next; + + while (curr != head) { + ehci_qh_t *tmp = curr; + curr = curr->next; + if (tmp->pipe != NULL) + tmp->pipe->hcdpriv = NULL; + usb_free((void *)tmp->hw, sizeof(struct qh)); + free(tmp); + } + /* Free old dummy QH */ + usb_free((void *)head->hw, sizeof(struct qh)); + free(head); + ehci->asyncList = NULL; +} + + +static void ehci_tearDownPeriodic(ehci_t *ehci) +{ + int i; + ehci_qh_t *marker = (ehci_qh_t *)(uintptr_t)1; + ehci_qh_t *freeList = marker; + + /* Collect unique QHs first using the unused 'prev' pointer as a visited marker/link, then free them */ + for (i = 0; i < EHCI_PERIODIC_SIZE; ++i) { + ehci_qh_t *node = ehci->periodicNodes[i]; + while (node != NULL) { + if (node->prev == NULL) { + /* Not yet visited. Mark and collect */ + node->prev = freeList; + freeList = node; + } + node = node->next; + } + ehci->periodicNodes[i] = NULL; + } + + while (freeList != marker) { + ehci_qh_t *tmp = freeList; + freeList = tmp->prev; + if (tmp->pipe != NULL) { + tmp->pipe->hcdpriv = NULL; + } + usb_free((void *)tmp->hw, sizeof(struct qh)); + free(tmp); + } +} + + static void ehci_free(ehci_t *ehci) { - if (ehci->periodicList != NULL) - usb_freeAligned(ehci->periodicList, EHCI_PERIODIC_SIZE * sizeof(uint32_t)); + if (ehci->irqHandle != 0) { + resourceDestroy(ehci->irqHandle); + } - if (ehci->irqCond != 0) - resourceDestroy(ehci->irqCond); + if (ehci->asyncList != NULL) { + ehci_tearDownAsync(ehci); + } - if (ehci->irqLock != 0) - resourceDestroy(ehci->irqLock); + if (ehci->periodicLock != 0) { + resourceDestroy(ehci->periodicLock); + } - if (ehci->asyncLock != 0) + if (ehci->asyncLock != 0) { resourceDestroy(ehci->asyncLock); + } + + if (ehci->irqLock != 0) { + resourceDestroy(ehci->irqLock); + } + + if (ehci->irqCond != 0) { + resourceDestroy(ehci->irqCond); + } + + if (ehci->periodicNodes != NULL) { + ehci_tearDownPeriodic(ehci); + free(ehci->periodicNodes); + } + + if (ehci->periodicList != NULL) { + usb_freeAligned(ehci->periodicList, EHCI_PERIODIC_SIZE * sizeof(uint32_t)); + } - free(ehci->periodicNodes); free(ehci); } +static int ehci_resetController(hcd_t *hcd) +{ + ehci_t *ehci = (ehci_t *)hcd->priv; + ehci_qh_t *qh; + int i; + + /* Initialize Async List with a dummy qh to optimize accesses and make them safer */ + qh = ehci_qhAlloc(ehci); + if (qh == NULL) { + log_error("Out of memory!"); + return -ENOMEM; + } + qh->hw->info[0] |= QH_HEAD; + qh->hw->horizontal = QH_PTR(qh); + + mutexLock(ehci->asyncLock); + mutexLock(ehci->periodicLock); + + if (ehci->asyncList != NULL) { + /* + * Free all QHs and invalidate their pipes' hcdpriv so that ehci_pipeDestroy (called + * later by the hub layer during device disconnect) becomes a no-op for stale QHs. + */ + ehci_tearDownAsync(ehci); + } + + ehci_tearDownPeriodic(ehci); + + for (i = 0; i < EHCI_PERIODIC_SIZE; ++i) { + ehci->periodicList[i] = QH_PTR_INVALID; + } + + LIST_ADD(&ehci->asyncList, qh); + + mutexUnlock(ehci->periodicLock); + mutexUnlock(ehci->asyncLock); + +#ifndef EHCI_IMX + /* Hangs controller on imx */ + *(ehci->opbase + usbcmd) &= ~(USBCMD_RUN | USBCMD_IAA); + + if (ehci_handshake(ehci->opbase + usbsts, USBSTS_HCH, USBSTS_HCH, 100000) < 0) { + return -ETIMEDOUT; + } +#endif + + /* Reset controller */ + *(ehci->opbase + usbcmd) |= USBCMD_HCRESET; + if (ehci_handshake(ehci->opbase + usbcmd, USBCMD_HCRESET, 0, 250000) < 0) { + return -ETIMEDOUT; + } + +#ifdef EHCI_IMX + /* imx deviation: Set host mode */ + *(ehci->opbase + usbmode) |= 3; +#else + if ((*(ehci->base + hccparams) & HCCPARAMS_64BIT_ADDRS) != 0) { + *(ehci->opbase + ctrldssegment) = 0; + } +#endif + + /* Enable interrupts */ + *(ehci->opbase + usbintr) = USBSTS_UI | USBSTS_UEI | USBSTS_SEI | USBSTS_PCI; + + /* Set periodic frame list */ + *(ehci->opbase + periodiclistbase) = va2pa(ehci->periodicList); + +#ifdef EHCI_IMX + /* imx deviation: Set frame list size (128 elements) */ + *(ehci->opbase + usbcmd) |= (3 << 2); +#endif + + /* Turn the controller on, enable periodic scheduling */ + *(ehci->opbase + usbcmd) &= ~(USBCMD_LRESET | USBCMD_ASE); + + *(ehci->opbase + usbcmd) |= (USBCMD_PSE | USBCMD_RUN); + if (ehci_handshake(ehci->opbase + usbsts, USBSTS_HCH, 0, 100000) < 0) { + return -ETIMEDOUT; + } + + /* Route all ports to this host controller */ + *(ehci->opbase + configflag) = 1; + + /* Allow for the hardware to catch up */ + usleep(50 * 1000); + + return ehci_startAsync(hcd); +} + + static int ehci_init(hcd_t *hcd) { ehci_t *ehci; - ehci_qh_t *qh; - int i, ret; + int ret; if ((ehci = calloc(1, sizeof(ehci_t))) == NULL) { log_error("Out of memory!"); @@ -971,20 +1184,6 @@ static int ehci_init(hcd_t *hcd) return -ENOMEM; } - /* Initialize Async List with a dummy qh to optimize - * accesses and make them safer */ - if ((qh = ehci_qhAlloc(ehci)) == NULL) { - log_error("Out of memory!"); - ehci_free(ehci); - return -ENOMEM; - } - qh->hw->info[0] |= QH_HEAD; - qh->hw->horizontal = QH_PTR(qh); - LIST_ADD(&ehci->asyncList, qh); - - for (i = 0; i < EHCI_PERIODIC_SIZE; ++i) - ehci->periodicList[i] = QH_PTR_INVALID; - if (((addr_t)hcd->base & (0x20 - 1)) != 0) { log_error("USBBASE not aligned to 32 bits"); ehci_free(ehci); @@ -1007,69 +1206,24 @@ static int ehci_init(hcd_t *hcd) log_debug("attaching handler to irq=%d", hcd->info->irq); ret = interrupt(hcd->info->irq, ehci_irqHandler, hcd, ehci->irqCond, &ehci->irqHandle); - if (ret < 0) { log_error("failed to set interrupt handler"); + ehci_free(ehci); return ret; } - if (beginthread(ehci_irqThread, EHCI_PRIO, ehci->stack, sizeof(ehci->stack), hcd) != 0) { + if (ehci_resetController(hcd) < 0) { ehci_free(ehci); - return -ENOMEM; - } - -#ifndef EHCI_IMX - /* Hangs controller on imx */ - *(ehci->opbase + usbcmd) &= ~(USBCMD_RUN | USBCMD_IAA); - - if (ehci_handshake(ehci->opbase + usbsts, USBSTS_HCH, USBSTS_HCH, 100000) < 0) { - return -ETIMEDOUT; - } -#endif - - /* Reset controller */ - *(ehci->opbase + usbcmd) |= USBCMD_HCRESET; - if (ehci_handshake(ehci->opbase + usbcmd, USBCMD_HCRESET, 0, 250000) < 0) { - return -ETIMEDOUT; - } - -#ifdef EHCI_IMX - /* imx deviation: Set host mode */ - *(ehci->opbase + usbmode) |= 3; -#else - if ((*(ehci->base + hccparams) & HCCPARAMS_64BIT_ADDRS) != 0) { - *(ehci->opbase + ctrldssegment) = 0; + return -1; } -#endif - - /* Enable interrupts */ - *(ehci->opbase + usbintr) = USBSTS_UI | USBSTS_UEI | USBSTS_SEI | USBSTS_PCI; - - /* Set periodic frame list */ - *(ehci->opbase + periodiclistbase) = va2pa(ehci->periodicList); -#ifdef EHCI_IMX - /* imx deviation: Set frame list size (128 elements) */ - *(ehci->opbase + usbcmd) |= (3 << 2); -#endif - - /* Turn the controller on, enable periodic scheduling */ - *(ehci->opbase + usbcmd) &= ~(USBCMD_LRESET | USBCMD_ASE); - - *(ehci->opbase + usbcmd) |= (USBCMD_PSE | USBCMD_RUN); - if (ehci_handshake(ehci->opbase + usbsts, USBSTS_HCH, 0, 100000) < 0) { - return -ETIMEDOUT; + if (beginthread(ehci_irqThread, EHCI_PRIO, ehci->stack, sizeof(ehci->stack), hcd) != 0) { + ehci_free(ehci); + return -1; } - /* Route all ports to this host controller */ - *(ehci->opbase + configflag) = 1; - - /* Allow for the hardware to catch up */ - usleep(50 * 1000); - - if (ehci_startAsync(hcd) < 0) { - return -ETIMEDOUT; - } + /* Catch any port status changes before the irqThread was running */ + ehci_portStatusChanged(hcd); log_debug("hc initialized"); diff --git a/usb/ehci/ehci.h b/usb/ehci/ehci.h index d2817849c..de94ba960 100644 --- a/usb/ehci/ehci.h +++ b/usb/ehci/ehci.h @@ -218,6 +218,7 @@ typedef struct _ehci_qh { struct _ehci_qh *prev, *next; volatile struct qh *hw; volatile struct qtd *lastQtd; + usb_pipe_t *pipe; unsigned period; /* [ms], interrupt transfer only */ unsigned phase; /* [ms], interrupt transfer only */ unsigned uframe; /* interrupt transfer and high-speed only */ From 091ef8dc36060c128b171dfac03055c45442c427 Mon Sep 17 00:00:00 2001 From: Adam Greloch Date: Tue, 16 Jun 2026 14:22:42 +0200 Subject: [PATCH 15/18] ehci: fix ehci_qtdsCheck mistreating qtds as finished 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 --- usb/ehci/ehci.c | 55 ++++++++++++++++++++++++++++++++++--------------- 1 file changed, 38 insertions(+), 17 deletions(-) diff --git a/usb/ehci/ehci.c b/usb/ehci/ehci.c index 52babc6e9..c2dfdf432 100644 --- a/usb/ehci/ehci.c +++ b/usb/ehci/ehci.c @@ -14,6 +14,7 @@ * %LICENSE% */ +#include #include #include #include @@ -240,6 +241,7 @@ static ehci_qtd_t *ehci_qtdAlloc(ehci_t *ehci, int pid, size_t maxpacksz, char * qtd->hw->token |= bytes << 16; *size -= bytes; } + qtd->bytes = bytes; return qtd; } @@ -574,30 +576,49 @@ static int ehci_irqHandler(unsigned int n, void *data) static int ehci_qtdsCheck(hcd_t *hcd, usb_transfer_t *t, int *status) { - ehci_qtd_t *qtds = (ehci_qtd_t *)t->hcdpriv; - unsigned int error = 0; - int finished = 0; + ehci_qtd_t *qtd = (ehci_qtd_t *)t->hcdpriv; + size_t left = 0, totalTransferred = 0; *status = 0; do { - ehci_qtdDump(qtds, false); - if (qtds->hw->token & (QTD_XACT | QTD_BABBLE | QTD_BUFERR)) { - error++; + ehci_memDmb(); + + ehci_qtdDump(qtd, false); + + uint32_t token = qtd->hw->token; + if ((token & (QTD_ACTIVE)) != 0) { + return 0; } - qtds = qtds->next; - } while (qtds != t->hcdpriv); + if ((token & (QTD_HALTED)) != 0) { + *status = -1; + return 1; + } - if (error > 0) { - finished = 1; - *status = -error; - } - else if (!(qtds->prev->hw->token & QTD_ACTIVE) || (qtds->prev->hw->token & QTD_HALTED)) { - finished = 1; - *status = t->size - QTD_LEN(qtds->prev->hw->token); - } + left = QTD_LEN(token); + if (QTD_PID(token) != setup_token) { + /* Exclude setup qtd from the payload size. Status qtd is fine, as it has qtd->bytes == 0 anyway */ + if (left > qtd->bytes) { + log_error("hardware reported left (%zu) > bytes (%zu)", left, qtd->bytes); + *status = -1; + return 1; + } + totalTransferred += qtd->bytes - left; + } + + if (left > 0) { + /* Short packet, abandon the rest of the chain */ + break; + } + + qtd = qtd->next; + } while (qtd != t->hcdpriv); + + *status = totalTransferred; + + assert(left > 0 || totalTransferred == t->size); - return finished; + return 1; } From 533aba1a4003a368c390a22814f3a1ee60de066e Mon Sep 17 00:00:00 2001 From: Adam Greloch Date: Fri, 10 Jul 2026 12:21:48 +0200 Subject: [PATCH 16/18] ehci: fix race on pipe->hcdpriv in ehci_pipeDestroy pipe->hcdpriv read was not guarded while it can be set to NULL by the irqThread during ehci controller reset. TASK: RTOS-1335 --- usb/ehci/ehci.c | 40 ++++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/usb/ehci/ehci.c b/usb/ehci/ehci.c index c2dfdf432..77a20f3f2 100644 --- a/usb/ehci/ehci.c +++ b/usb/ehci/ehci.c @@ -482,12 +482,9 @@ static void ehci_qtdsDeactivate(ehci_qtd_t *qtds) * TODO: propagate -ETIMEDOUT from {stop,start}Async up to pipeDestroy - * requires hcd signature change */ -static void ehci_qhUnlinkAsync(hcd_t *hcd, ehci_qh_t *qh) +/* Must be called under ehci->asyncLock */ +static void _ehci_qhUnlinkAsync(ehci_t *ehci, ehci_qh_t *qh) { - ehci_t *ehci = (ehci_t *)hcd->priv; - - mutexLock(ehci->asyncLock); - qh->prev->hw->horizontal = qh->hw->horizontal; ehci_memDmb(); @@ -506,18 +503,15 @@ static void ehci_qhUnlinkAsync(hcd_t *hcd, ehci_qh_t *qh) *(ehci->opbase + usbsts) = USBSTS_IAA; ehci_memDmb(); } - - mutexUnlock(ehci->asyncLock); } -void ehci_qhUnlinkPeriodic(hcd_t *hcd, ehci_qh_t *qh) +/* Must be called under ehci->periodicLock */ +static void _ehci_qhUnlinkPeriodic(ehci_t *ehci, ehci_qh_t *qh) { - ehci_t *ehci = (ehci_t *)hcd->priv; ehci_qh_t *tmp; int i; - mutexLock(ehci->periodicLock); /* TODO: do we have to stop the periodic queue? */ for (i = 0; i < EHCI_PERIODIC_SIZE; i++) { /* Count Qhs linked to this periodic index */ @@ -548,7 +542,6 @@ void ehci_qhUnlinkPeriodic(hcd_t *hcd, ehci_qh_t *qh) } } ehci_memDmb(); - mutexUnlock(ehci->periodicLock); } @@ -811,10 +804,11 @@ static void ehci_transferDequeue(hcd_t *hcd, usb_transfer_t *t) qtds = (ehci_qtd_t *)t->hcdpriv; if (t->type == usb_transfer_bulk || t->type == usb_transfer_control) { + mutexLock(ehci->asyncLock); ehci_qh_t *qh = qtds->qh; - ehci_qhUnlinkAsync(hcd, qh); - mutexLock(ehci->asyncLock); + _ehci_qhUnlinkAsync(ehci, qh); + ehci_qtdsDeactivate(qtds); /* Clear overlay active bit so HC won't resume this QTD */ qh->hw->token &= ~QTD_ACTIVE; @@ -940,16 +934,26 @@ static void ehci_pipeDestroy(hcd_t *hcd, usb_pipe_t *pipe) usb_transfer_t *t; ehci_qh_t *qh; ehci_qtd_t *qtds; + ehci_t *ehci = (ehci_t *)hcd->priv; - if (pipe->hcdpriv == NULL) + mutexLock(ehci->asyncLock); + if (pipe->hcdpriv == NULL) { + mutexUnlock(ehci->asyncLock); return; + } qh = (ehci_qh_t *)pipe->hcdpriv; - if (pipe->type == usb_transfer_bulk || pipe->type == usb_transfer_control) - ehci_qhUnlinkAsync(hcd, qh); - else if (pipe->type == usb_transfer_interrupt) - ehci_qhUnlinkPeriodic(hcd, qh); + if (pipe->type == usb_transfer_bulk || pipe->type == usb_transfer_control) { + _ehci_qhUnlinkAsync(ehci, qh); + } + else if (pipe->type == usb_transfer_interrupt) { + mutexLock(ehci->periodicLock); + _ehci_qhUnlinkPeriodic(ehci, qh); + mutexUnlock(ehci->periodicLock); + } + + mutexUnlock(ehci->asyncLock); mutexLock(hcd->transLock); t = hcd->transfers; From 6d1a0ef1fc4fb0f6ed8ca9c200bcc2cca1faa57e Mon Sep 17 00:00:00 2001 From: Adam Greloch Date: Fri, 10 Jul 2026 15:56:19 +0200 Subject: [PATCH 17/18] ehci: add qh/qtd assertions TASK: RTOS-1335 --- usb/ehci/ehci.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/usb/ehci/ehci.c b/usb/ehci/ehci.c index 77a20f3f2..0f96eb4ba 100644 --- a/usb/ehci/ehci.c +++ b/usb/ehci/ehci.c @@ -63,8 +63,20 @@ static inline void ehci_memDmb(void) static int ehci_startAsync(hcd_t *hcd) { ehci_t *ehci = (ehci_t *)hcd->priv; + uint32_t addr = va2pa((void *)ehci->asyncList->hw); - *(ehci->opbase + asynclistaddr) = va2pa((void *)ehci->asyncList->hw); + /* EHCI 4.8.1: async list base address must be 32-byte aligned and non-null. */ + assert(addr != 0 && (addr & 0x1f) == 0); + + /* EHCI 3.6.1: reclamation header QH must have H-bit set. */ + if ((ehci->asyncList->hw->info[0] & QH_HEAD) == 0) { + log_error("async list head missing H-bit (info[0]=%08x addr=%08x), repairing", + ehci->asyncList->hw->info[0], addr); + ehci->asyncList->hw->info[0] |= QH_HEAD; + } + + *(ehci->opbase + asynclistaddr) = addr; + ehci_memDmb(); *(ehci->opbase + usbcmd) |= USBCMD_ASE; ehci_memDmb(); return ehci_handshake(ehci->opbase + usbsts, USBSTS_AS, USBSTS_AS, 20000); @@ -82,6 +94,9 @@ static void ehci_enqueue(hcd_t *hcd, ehci_qh_t *qh, ehci_qtd_t *first, ehci_qtd_ { ehci_t *ehci = (ehci_t *)hcd->priv; + /* EHCI 3.6.1: QH must be 32-byte aligned before HC can traverse it */ + assert(qh->hw != NULL && (va2pa((void *)qh->hw) & 0x1f) == 0); + mutexLock(ehci->asyncLock); last->hw->next = QTD_PTR_INVALID; last->hw->token |= QTD_IOC; @@ -207,6 +222,9 @@ static ehci_qtd_t *ehci_qtdAlloc(ehci_t *ehci, int pid, size_t maxpacksz, char * qtd->paddr = QTD_PTR(qtd); } + /* EHCI 3.5.1: QTD must be 32-byte aligned */ + assert((va2pa((void *)qtd->hw) & 0x1f) == 0); + qtd->hw->token = (datax << 31) | (pid << 8) | (EHCI_TRANS_ERRORS << 10) | QTD_ACTIVE; qtd->hw->next = QTD_PTR_INVALID; @@ -299,6 +317,9 @@ static ehci_qh_t *ehci_qhAlloc(ehci_t *ehci) } } + /* EHCI 3.6.1: QH must be 32-byte aligned */ + assert((va2pa((void *)qh->hw) & 0x1f) == 0); + qh->hw->info[0] = 0; qh->hw->info[1] = 0; qh->hw->token = 0; From 3d64dbcff8ccb9e79695b4d38b2d2e8344ed48b3 Mon Sep 17 00:00:00 2001 From: Adam Greloch Date: Fri, 10 Jul 2026 16:02:23 +0200 Subject: [PATCH 18/18] ehci: link QH and register a transfer atomically in ehci_transferEnqueue 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 --- usb/ehci/ehci.c | 57 +++++++++++++++++++++++++------------------------ 1 file changed, 29 insertions(+), 28 deletions(-) diff --git a/usb/ehci/ehci.c b/usb/ehci/ehci.c index 0f96eb4ba..3bf0dc5e9 100644 --- a/usb/ehci/ehci.c +++ b/usb/ehci/ehci.c @@ -875,35 +875,10 @@ static int ehci_transferEnqueue(hcd_t *hcd, usb_transfer_t *t, usb_pipe_t *pipe) if (usb_isRoothub(pipe->dev)) return ehci_roothubReq(pipe->dev, t); - if (pipe->hcdpriv == NULL) { - if ((qh = ehci_qhAlloc(hcd->priv)) == NULL) - return -ENOMEM; - - ehci_qhConf(qh, pipe); - pipe->hcdpriv = qh; - qh->pipe = pipe; - - if (t->type == usb_transfer_bulk || t->type == usb_transfer_control) - ehci_qhLinkAsync(hcd, qh); - else - ehci_qhLinkPeriodic(hcd, qh); - } - else { - qh = (ehci_qh_t *)pipe->hcdpriv; - - /* Update fields, which might have been changed */ - if (QH_DEVADDR(qh->hw->info[0]) != pipe->dev->address) - qh->hw->info[0] = (qh->hw->info[0] & ~0x7f) | pipe->dev->address; - - if (QH_PACKLEN(qh->hw->info[0]) != pipe->maxPacketLen) - qh->hw->info[0] = (qh->hw->info[0] & ~(0x7ff << 16)) | (pipe->maxPacketLen << 16); - } - /* Setup stage */ if (t->type == usb_transfer_control) { if (ehci_qtdAdd(hcd->priv, &qtds, setup_token, pipe->maxPacketLen, (char *)t->setup, sizeof(usb_setup_packet_t), 0) < 0) { ehci_qtdsPut(hcd->priv, &qtds); - t->hcdpriv = NULL; return -ENOMEM; } } @@ -913,7 +888,6 @@ static int ehci_transferEnqueue(hcd_t *hcd, usb_transfer_t *t, usb_pipe_t *pipe) t->type == usb_transfer_interrupt) { if (ehci_qtdAdd(hcd->priv, &qtds, token, pipe->maxPacketLen, t->buffer, t->size, 1) < 0) { ehci_qtdsPut(hcd->priv, &qtds); - t->hcdpriv = NULL; return -ENOMEM; } } @@ -923,7 +897,6 @@ static int ehci_transferEnqueue(hcd_t *hcd, usb_transfer_t *t, usb_pipe_t *pipe) token = (token == in_token) ? out_token : in_token; if (ehci_qtdAdd(hcd->priv, &qtds, token, pipe->maxPacketLen, NULL, 0, 1) < 0) { ehci_qtdsPut(hcd->priv, &qtds); - t->hcdpriv = NULL; return -ENOMEM; } } @@ -932,6 +905,35 @@ static int ehci_transferEnqueue(hcd_t *hcd, usb_transfer_t *t, usb_pipe_t *pipe) if (qtds == NULL) return -1; + mutexLock(hcd->transLock); + + if (pipe->hcdpriv == NULL) { + if ((qh = ehci_qhAlloc(hcd->priv)) == NULL) { + mutexUnlock(hcd->transLock); + ehci_qtdsPut(hcd->priv, &qtds); + return -ENOMEM; + } + + ehci_qhConf(qh, pipe); + pipe->hcdpriv = qh; + qh->pipe = pipe; + + if (t->type == usb_transfer_bulk || t->type == usb_transfer_control) + ehci_qhLinkAsync(hcd, qh); + else + ehci_qhLinkPeriodic(hcd, qh); + } + else { + qh = (ehci_qh_t *)pipe->hcdpriv; + + /* Update fields, which might have been changed */ + if (QH_DEVADDR(qh->hw->info[0]) != pipe->dev->address) + qh->hw->info[0] = (qh->hw->info[0] & ~0x7f) | pipe->dev->address; + + if (QH_PACKLEN(qh->hw->info[0]) != pipe->maxPacketLen) + qh->hw->info[0] = (qh->hw->info[0] & ~(0x7ff << 16)) | (pipe->maxPacketLen << 16); + } + t->hcdpriv = qtds; do { ehci_qtdLink(qtds, qtds->next); @@ -941,7 +943,6 @@ static int ehci_transferEnqueue(hcd_t *hcd, usb_transfer_t *t, usb_pipe_t *pipe) qtds = (ehci_qtd_t *)t->hcdpriv; - mutexLock(hcd->transLock); LIST_ADD(&hcd->transfers, t); ehci_enqueue(hcd, qh, qtds, qtds->prev); mutexUnlock(hcd->transLock);