Improve USB mass storage reliability on legacy EHCI/UHCI systems

EHCI: don't treat the qTD Ping State bit as an error, disable SMIs at BIOS
handoff, fail fast and recover when the controller halts, and stop DMA before
freeing controller memory (also on UHCI).
 MSD: retry failed BOT commands.
FAT32: cache the current sector.
Quirk: fix VIA VT823x EHCI bus starvation.

system/ehci.c

@@ -31,6 +31,12 @@
 
 #define EHCI_EXT_CAP_OS_HANDOFF 0x01
 
+// USB Legacy Support extended capability registers (byte offsets from the capability pointer)
+
+#define EHCI_USBLEGSUP_BIOS     0x02            // HC BIOS Owned Semaphore
+#define EHCI_USBLEGSUP_OS       0x03            // HC OS Owned Semaphore
+#define EHCI_USBLEGCTLSTS       0x04            // Legacy Support Control/Status register
+
 // Host Controller Structural Parameters
 
 #define EHCI_HCS_PPC            0x00000010      // Port Power Control
@@ -384,28 +390,57 @@ static void build_ehci_qhd(ehci_qhd_t *qhd, const ehci_qtd_t *qtd, const usb_ep_
     qhd->next_qtd_ptr = (uintptr_t)qtd;
 }
 
+// Clears a latched host system error and restarts the halted controller for recovery.
+static void restart_host_controller(const workspace_t *ws)
+{
+    ehci_op_regs_t *op_regs = ws->op_regs;
+
+    // start_host_controller rewrites USBCMD wholesale, so preserve the periodic schedule.
+    bool periodic_on = read32(&op_regs->usb_command) & EHCI_USBCMD_PSE;
+
+    write32(&op_regs->usb_status, EHCI_USBSTS_HSE | EHCI_USBSTS_ERR | EHCI_USBSTS_INT);
+    write32(&op_regs->async_list_addr, (uintptr_t)(ws->qhd));
+    (void)start_host_controller(op_regs);
+    if (periodic_on) {
+        enable_periodic_schedule(op_regs);
+    }
+    flush32(&op_regs->config_flag, 1);
+}
+
 static bool do_async_transfer(const workspace_t *ws, int num_tds)
 {
+    ehci_op_regs_t *op_regs = ws->op_regs;
+
     // The controller only detects device errors; a device that NAKs forever would
     // hang us, so also enforce a software timeout.
     bool ok = true;
-    enable_async_schedule(ws->op_regs);
+    bool hc_died = false;
+    enable_async_schedule(op_regs);
     for (int td_idx = 0; td_idx < num_tds && ok; td_idx++) {
         const ehci_qtd_t *qtd = &ws->qtd[td_idx];
         int timer = 5000 * MILLISEC / 10;
         while (qtd->status & EHCI_QTD_ACTIVE) {
+            // A halted controller will never complete this qTD, so fail fast.
+            if (read32(&op_regs->usb_status) & (EHCI_USBSTS_HSE | EHCI_USBSTS_HCH)) {
+                ok = false;
+                hc_died = true;
+                break;
+            }
             if (timer-- == 0) {
                 ok = false;
                 break;
             }
             usleep(10);
         }
-        if (qtd->status & (EHCI_QTD_HALTED | EHCI_QTD_DB_ERR | EHCI_QTD_BABBLE | EHCI_QTD_TR_ERR | EHCI_QTD_MMF | EHCI_QTD_PS)) {
+        if (qtd->status & (EHCI_QTD_HALTED | EHCI_QTD_DB_ERR | EHCI_QTD_BABBLE | EHCI_QTD_TR_ERR | EHCI_QTD_MMF)) {
             ok = false;
         }
     }
     // This waits for the schedule to go idle, so it also stops a timed-out transfer.
-    disable_async_schedule(ws->op_regs);
+    disable_async_schedule(op_regs);
+    if (hc_died) {
+        restart_host_controller(ws);
+    }
     return ok;
 }
 
@@ -468,7 +503,7 @@ static void poll_keyboards(const usb_hcd_t *hcd)
 
         hid_kbd_rpt_t *kbd_rpt = &ws->kbd_rpt[kbd_idx];
 
-        uint8_t error_mask = EHCI_QTD_HALTED | EHCI_QTD_DB_ERR | EHCI_QTD_BABBLE | EHCI_QTD_TR_ERR | EHCI_QTD_MMF | EHCI_QTD_PS;
+        uint8_t error_mask = EHCI_QTD_HALTED | EHCI_QTD_DB_ERR | EHCI_QTD_BABBLE | EHCI_QTD_TR_ERR | EHCI_QTD_MMF;
         if (~status & error_mask) {
             hid_kbd_rpt_t *prev_kbd_rpt = &ws->prev_kbd_rpt[kbd_idx];
             if (process_usb_keyboard_report(hcd, kbd_rpt, prev_kbd_rpt)) {
@@ -678,13 +713,22 @@ bool ehci_reset(int bus, int dev, int func, uintptr_t base_addr)
         uint8_t ext_cap_id = pci_config_read8(bus, dev, func, ext_cap_ptr + 0);
         if (ext_cap_id == EHCI_EXT_CAP_OS_HANDOFF) {
             // Take ownership from the SMM if necessary.
+            bool acquired = true;
             int timer = 1000;
-            pci_config_write8(bus, dev, func, ext_cap_ptr + 3, 1);
-            while (pci_config_read8(bus, dev, func, ext_cap_ptr + 2) & 1) {
-                if (timer == 0) return false;
+            pci_config_write8(bus, dev, func, ext_cap_ptr + EHCI_USBLEGSUP_OS, 1);
+            while (pci_config_read8(bus, dev, func, ext_cap_ptr + EHCI_USBLEGSUP_BIOS) & 1) {
+                if (timer == 0) {
+                    acquired = false;
+                    break;
+                }
                 usleep(1*MILLISEC);
                 timer--;
             }
+
+            // Disable all SMI sources either way: they survive HCRESET and can wedge the CPU in SMM.
+            pci_config_write32(bus, dev, func, ext_cap_ptr + EHCI_USBLEGCTLSTS, 0);
+
+            if (!acquired) return false;
         }
         ext_cap_ptr = pci_config_read8(bus, dev, func, ext_cap_ptr + 1);
     }
@@ -883,6 +927,9 @@ bool ehci_probe(uintptr_t base_addr, usb_hcd_t *hcd)
     return true;
 
 no_keyboards_found:
+    // The frame list and workspace are freed and reused below, so stop all DMA first.
+    (void)halt_host_controller(op_regs);
+    (void)reset_host_controller(op_regs);
     heap_rewind(HEAP_TYPE_LM_1, initial_heap_mark);
     return false;
 }

system/fat32.c

@@ -73,12 +73,30 @@ static uint32_t cluster_to_lba(const fat32_fs_t *fs, uint32_t cluster)
 
 static bool read_sector(fat32_fs_t *fs, uint32_t lba)
 {
-    return msd_read_sectors(fs->msd, fs->partition_lba + lba, 1, fs->sector_buf);
+    // The FAT is walked one cluster at a time, and a FAT32 sector holds 128 of them, so
+    // without this the same sector is fetched over the bus 128 times in a row.
+    if (fs->buf_valid && fs->buf_lba == lba) {
+        return true;
+    }
+    fs->buf_valid = false;
+    if (!msd_read_sectors(fs->msd, fs->partition_lba + lba, 1, fs->sector_buf)) {
+        return false;
+    }
+    fs->buf_lba   = lba;
+    fs->buf_valid = true;
+    return true;
 }
 
 static bool write_sector(fat32_fs_t *fs, uint32_t lba)
 {
-    return msd_write_sectors(fs->msd, fs->partition_lba + lba, 1, fs->sector_buf);
+    // The buffer holds this sector's contents once written, so it stays a valid entry.
+    fs->buf_valid = false;
+    if (!msd_write_sectors(fs->msd, fs->partition_lba + lba, 1, fs->sector_buf)) {
+        return false;
+    }
+    fs->buf_lba   = lba;
+    fs->buf_valid = true;
+    return true;
 }
 
 static uint32_t fat_read_entry(fat32_fs_t *fs, uint32_t cluster)
@@ -308,6 +326,9 @@ bool fat32_mount(fat32_fs_t *fs, usb_msd_t *msd, uint8_t *buf)
     fs->sector_buf = buf;
     fs->partition_lba = 0;
 
+    // Mounting reads through msd_read_sectors directly, bypassing the sector cache.
+    fs->buf_valid = false;
+
     // Read sector 0.
     if (!msd_read_sectors(msd, 0, 1, buf)) return false;
 

system/fat32.h

@@ -36,6 +36,8 @@ typedef struct {
     uint32_t    fat_start_lba;
     uint32_t    data_start_lba;
     uint8_t     *sector_buf;        // one sector buffer
+    uint32_t    buf_lba;            // LBA currently held in sector_buf
+    bool        buf_valid;          // true if buf_lba is meaningful
 } fat32_fs_t;
 
 /**

system/hwquirks.c

@@ -253,6 +253,19 @@ static void loongson_7a00_ehci_workaround(void)
     write8((uint8_t *)(reg_addr + 0x3830), 0x0);
 }
 
+static void via_vt823x_ehci_workaround(void)
+{
+    // Stretch the EHCI MMIO sleep timer from 1us to 10us (bit 5 of config reg 0x4B)
+    // so bulk DMA can't saturate the PCI bus until devices stop answering.
+    for (int func = 3; func <= 4; func++) {         // VT8235 is 00:10.3, VT8237 00:10.4
+        if (pci_config_read16(0, 0x10, func, PCI_VID_REG) == PCI_VID_VIA
+        &&  pci_config_read16(0, 0x10, func, PCI_DID_REG) == 0x3104) {   // VIA EHCI
+            pci_config_write8(0, 0x10, func, 0x4b,
+                              pci_config_read8(0, 0x10, func, 0x4b) | 0x20);
+        }
+    }
+}
+
 
 static void unhide_ich_0_5(void)
 {
@@ -440,6 +453,18 @@ void quirks_init(void)
         }
     }
 
+    //  ------------------------------------------------------
+    //  -- VIA VT8235/37 EHCI PCI bus starvation workaround --
+    //  ------------------------------------------------------
+    if (quirk.root_vid == PCI_VID_VIA) {
+        if (pci_config_read16(0, 0x10, 3, PCI_DID_REG) == 0x3104
+        ||  pci_config_read16(0, 0x10, 4, PCI_DID_REG) == 0x3104) {
+            quirk.id    = QUIRK_VIA_VT823X_EHCI;
+            quirk.type |= QUIRK_TYPE_USB;
+            quirk.process = via_vt823x_ehci_workaround;
+        }
+    }
+
     //  -----------------------------------------------------
     //  -- Unhide SMBus on early Intel Southbridges (ICHx) --
     //  -----------------------------------------------------

system/hwquirks.h

@@ -35,6 +35,7 @@ typedef enum {
     QUIRK_VIA_VP,
     QUIRK_SIS_530,
     QUIRK_LOONGSON7A00_EHCI_WORKARD,
+    QUIRK_VIA_VT823X_EHCI,
     QUIRK_UNHIDE_ICH05,
     QUIRK_UNHIDE_ASUS_SMBUS,
 } quirk_id_t;

system/uhci.c

@@ -536,7 +536,6 @@ bool uhci_probe(uint16_t io_base, usb_hcd_t *hcd)
                    num_keyboards, num_keyboards != 1 ? "s" : "");
 
     if (num_keyboards == 0) {
-        (void)halt_host_controller(io_base);
         goto no_keyboards_found;
     }
 
@@ -577,6 +576,10 @@ bool uhci_probe(uint16_t io_base, usb_hcd_t *hcd)
     return true;
 
 no_keyboards_found:
+    // The frame list is freed and reused below, so the controller must not reach it.
+    (void)halt_host_controller(io_base);
+    (void)reset_host_controller(io_base);
+    outl(0, UHCI_FLBASE);
     heap_rewind(HEAP_TYPE_LM_1, initial_heap_mark);
     return false;
 }

system/usbmsd.c

@@ -77,15 +77,14 @@ static bool msd_clear_stall(usb_msd_t *msd, const usb_ep_t *ep, bool is_in)
     usb_setup_pkt_t setup_pkt;
     build_setup_packet(&setup_pkt, USB_REQ_TO_ENDPOINT, USB_CLR_FEATURE,
                        USB_ENDPOINT_HALT, ep->endpoint_num | (is_in ? 0x80 : 0), 0);
-    if (!hcd->methods->setup_request(hcd, &msd->ep0, &setup_pkt)) {
-        return false;
-    }
+    bool ok = hcd->methods->setup_request(hcd, &msd->ep0, &setup_pkt);
 
+    // Resync the host toggle even on failure: a mismatched toggle wedges the endpoint.
     if (hcd->methods->reset_bulk_ep != NULL) {
         int ep_id = 2 * ep->endpoint_num + (is_in ? 1 : 0);
-        return hcd->methods->reset_bulk_ep(hcd, ep, ep_id);
+        ok = hcd->methods->reset_bulk_ep(hcd, ep, ep_id) && ok;
     }
-    return true;
+    return ok;
 }
 
 // BOT Reset Recovery (BOT spec 5.3.4): class reset, then clear both bulk endpoints.
@@ -157,6 +156,19 @@ static bool msd_bot_command(usb_msd_t *msd, const uint8_t *cdb, int cdb_len,
     return data_ok && csw.status == CSW_STATUS_PASSED;
 }
 
+// Retry transient bus errors. The command is reissued whole, so a partial op just repeats.
+static bool msd_bot_command_retry(usb_msd_t *msd, const uint8_t *cdb, int cdb_len,
+                                  void *data, uint32_t data_len, bool data_in)
+{
+    for (int attempt = 0; attempt < 3; attempt++) {
+        if (msd_bot_command(msd, cdb, cdb_len, data, data_len, data_in)) {
+            return true;
+        }
+        usleep(20 * MILLISEC);
+    }
+    return false;
+}
+
 //------------------------------------------------------------------------------
 // Public Functions
 //------------------------------------------------------------------------------
@@ -177,7 +189,7 @@ static bool read_capacity_16(usb_msd_t *msd)
         0, 0
     };
     uint8_t cap_data[32];
-    if (!msd_bot_command(msd, cdb, 16, cap_data, sizeof(cap_data), true)) {
+    if (!msd_bot_command_retry(msd, cdb, 16, cap_data, sizeof(cap_data), true)) {
         return false;
     }
 
@@ -211,7 +223,7 @@ bool msd_init(usb_msd_t *msd)
     uint8_t cdb_cap[10] = { SCSI_READ_CAPACITY_10 };
 
     uint8_t cap_data[8];
-    if (!msd_bot_command(msd, cdb_cap, 10, cap_data, 8, true)) {
+    if (!msd_bot_command_retry(msd, cdb_cap, 10, cap_data, 8, true)) {
         // Some larger drives reject 10-byte commands; try the 16-byte variant.
         if (!read_capacity_16(msd)) return false;
         msd->use_16 = true;
@@ -247,7 +259,7 @@ bool msd_read_sectors(usb_msd_t *msd, uint64_t lba, uint32_t count, void *buffer
             (uint8_t)(count >> 24), (uint8_t)(count >> 16), (uint8_t)(count >> 8), (uint8_t)count,
             0, 0
         };
-        return msd_bot_command(msd, cdb, 16, buffer, count * msd->block_size, true);
+        return msd_bot_command_retry(msd, cdb, 16, buffer, count * msd->block_size, true);
     }
 
     uint8_t cdb[10] = {
@@ -256,7 +268,7 @@ bool msd_read_sectors(usb_msd_t *msd, uint64_t lba, uint32_t count, void *buffer
         0,
         (uint8_t)(count >> 8), (uint8_t)count, 0
     };
-    return msd_bot_command(msd, cdb, 10, buffer, count * msd->block_size, true);
+    return msd_bot_command_retry(msd, cdb, 10, buffer, count * msd->block_size, true);
 }
 
 bool msd_write_sectors(usb_msd_t *msd, uint64_t lba, uint32_t count, const void *buffer)
@@ -269,7 +281,7 @@ bool msd_write_sectors(usb_msd_t *msd, uint64_t lba, uint32_t count, const void
             (uint8_t)(count >> 24), (uint8_t)(count >> 16), (uint8_t)(count >> 8), (uint8_t)count,
             0, 0
         };
-        return msd_bot_command(msd, cdb, 16, (void *)buffer, count * msd->block_size, false);
+        return msd_bot_command_retry(msd, cdb, 16, (void *)buffer, count * msd->block_size, false);
     }
 
     uint8_t cdb[10] = {
@@ -278,5 +290,5 @@ bool msd_write_sectors(usb_msd_t *msd, uint64_t lba, uint32_t count, const void
         0,
         (uint8_t)(count >> 8), (uint8_t)count, 0
     };
-    return msd_bot_command(msd, cdb, 10, (void *)buffer, count * msd->block_size, false);
+    return msd_bot_command_retry(msd, cdb, 10, (void *)buffer, count * msd->block_size, false);
 }