Patchwork RFC v2: hcd-ohci: add dma error handling

login
register
mail settings
Submitter Alexey Kardashevskiy
Date July 24, 2013, 8:28 a.m.
Message ID <1374654505-17755-1-git-send-email-aik@ozlabs.ru>
Download mbox | patch
Permalink /patch/261343/
State New
Headers show

Comments

Alexey Kardashevskiy - July 24, 2013, 8:28 a.m.
Current hcd-ohci does not handle DMA errors. However they may happen
so here we introduce simple error handling.

On such errors, a typical OHCI will stop operating, signal the guest
about the error by sending "UnrecoverableError Event", set itself into
error state and set "Detected Parity Error" in its PCI config space
to signal that it got an error and so does the patch.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---

As I am just getting familiar with USB stack, this all may be very wrong.
Comments are very welcome.

The open questions are:

1. if a physical device once reported UnrecoverableError, what is it
expected to be able to do after that? Should ohci_hcd module reloading bring
it back to life? With this patch, OHCI dies till reboot (but at least it does
not break other subsystems).

2. is UnrecoverableError a correct event here?

Thanks!


---
 hw/usb/hcd-ohci.c | 164 ++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 117 insertions(+), 47 deletions(-)
Benjamin Herrenschmidt - July 24, 2013, 9:05 a.m.
On Wed, 2013-07-24 at 18:28 +1000, Alexey Kardashevskiy wrote:
> 1. if a physical device once reported UnrecoverableError, what is it
> expected to be able to do after that? Should ohci_hcd module reloading bring
> it back to life? With this patch, OHCI dies till reboot (but at least it does
> not break other subsystems).

Not sure what the spec says but I assume an HC reset should work. That
and a PCI reset which we can potentially trigger in SW (though we
haven't yet implemented the RTAS interfaces for it).

> 2. is UnrecoverableError a correct event here?

Yes.

Cheers,
Ben.
Alexey Kardashevskiy - July 24, 2013, 9:49 a.m.
On 07/24/2013 07:05 PM, Benjamin Herrenschmidt wrote:
> On Wed, 2013-07-24 at 18:28 +1000, Alexey Kardashevskiy wrote:
>> 1. if a physical device once reported UnrecoverableError, what is it
>> expected to be able to do after that? Should ohci_hcd module reloading bring
>> it back to life? With this patch, OHCI dies till reboot (but at least it does
>> not break other subsystems).
> 
> Not sure what the spec says but I assume an HC reset should work. That
> and a PCI reset which we can potentially trigger in SW (though we
> haven't yet implemented the RTAS interfaces for it).


Double checked. "rmmod ohci_hcd; modprobe ohci_hcd" brings it back so we
are good here :)
Gerd Hoffmann - July 24, 2013, 12:55 p.m.
> +/** A typical O/EHCI will stop operating, set itself into error state
> + * (which can be queried by MMIO) and will set PERR in its config
> + * space to signal that it got an error
> + */
> +static void ohci_stop(OHCIState *ohci)
> +{
> +    OHCIPCIState *dev = container_of(ohci, OHCIPCIState, state);
> +
> +    fprintf(stderr, "%s: DMA error\n", __func__);
> +
> +    ohci_set_interrupt(ohci, OHCI_INTR_UE);
> +    pci_set_word(dev->parent_obj.config + PCI_STATUS,
> +                 PCI_STATUS_DETECTED_PARITY);
> +}

I think you want add a ohci_bus_stop() call here to make sure the ohci
emulation stops processing.

Also while being at it: ohci_bus_start() has a TODO item where you can
fill in a call to the new ohci_stop function.

cheers,
  Gerd

Patch

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index 2bab8ff..3888ca9 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -308,6 +308,8 @@  struct ohci_iso_td {
 
 #define OHCI_HRESET_FSBIR       (1 << 0)
 
+static void ohci_stop(OHCIState *ohci);
+
 /* Update IRQ levels */
 static inline void ohci_intr_update(OHCIState *ohci)
 {
@@ -508,11 +510,13 @@  static inline int get_dwords(OHCIState *ohci,
     addr += ohci->localmem_base;
 
     for (i = 0; i < num; i++, buf++, addr += sizeof(*buf)) {
-        dma_memory_read(ohci->as, addr, buf, sizeof(*buf));
+        if (dma_memory_read(ohci->as, addr, buf, sizeof(*buf))) {
+            return -1;
+        }
         *buf = le32_to_cpu(*buf);
     }
 
-    return 1;
+    return 0;
 }
 
 /* Put an array of dwords in to main memory */
@@ -525,10 +529,12 @@  static inline int put_dwords(OHCIState *ohci,
 
     for (i = 0; i < num; i++, buf++, addr += sizeof(*buf)) {
         uint32_t tmp = cpu_to_le32(*buf);
-        dma_memory_write(ohci->as, addr, &tmp, sizeof(tmp));
+        if (dma_memory_write(ohci->as, addr, &tmp, sizeof(tmp))) {
+            return -1;
+        }
     }
 
-    return 1;
+    return 0;
 }
 
 /* Get an array of words from main memory */
@@ -540,11 +546,13 @@  static inline int get_words(OHCIState *ohci,
     addr += ohci->localmem_base;
 
     for (i = 0; i < num; i++, buf++, addr += sizeof(*buf)) {
-        dma_memory_read(ohci->as, addr, buf, sizeof(*buf));
+        if (dma_memory_read(ohci->as, addr, buf, sizeof(*buf))) {
+            return -1;
+        }
         *buf = le16_to_cpu(*buf);
     }
 
-    return 1;
+    return 0;
 }
 
 /* Put an array of words in to main memory */
@@ -557,10 +565,12 @@  static inline int put_words(OHCIState *ohci,
 
     for (i = 0; i < num; i++, buf++, addr += sizeof(*buf)) {
         uint16_t tmp = cpu_to_le16(*buf);
-        dma_memory_write(ohci->as, addr, &tmp, sizeof(tmp));
+        if (dma_memory_write(ohci->as, addr, &tmp, sizeof(tmp))) {
+            return -1;
+        }
     }
 
-    return 1;
+    return 0;
 }
 
 static inline int ohci_read_ed(OHCIState *ohci,
@@ -578,15 +588,15 @@  static inline int ohci_read_td(OHCIState *ohci,
 static inline int ohci_read_iso_td(OHCIState *ohci,
                                    dma_addr_t addr, struct ohci_iso_td *td)
 {
-    return (get_dwords(ohci, addr, (uint32_t *)td, 4) &&
+    return (get_dwords(ohci, addr, (uint32_t *)td, 4) ||
             get_words(ohci, addr + 16, td->offset, 8));
 }
 
 static inline int ohci_read_hcca(OHCIState *ohci,
                                  dma_addr_t addr, struct ohci_hcca *hcca)
 {
-    dma_memory_read(ohci->as, addr + ohci->localmem_base, hcca, sizeof(*hcca));
-    return 1;
+    return dma_memory_read(ohci->as, addr + ohci->localmem_base,
+                           hcca, sizeof(*hcca));
 }
 
 static inline int ohci_put_ed(OHCIState *ohci,
@@ -610,23 +620,22 @@  static inline int ohci_put_td(OHCIState *ohci,
 static inline int ohci_put_iso_td(OHCIState *ohci,
                                   dma_addr_t addr, struct ohci_iso_td *td)
 {
-    return (put_dwords(ohci, addr, (uint32_t *)td, 4) &&
+    return (put_dwords(ohci, addr, (uint32_t *)td, 4) ||
             put_words(ohci, addr + 16, td->offset, 8));
 }
 
 static inline int ohci_put_hcca(OHCIState *ohci,
                                 dma_addr_t addr, struct ohci_hcca *hcca)
 {
-    dma_memory_write(ohci->as,
-                     addr + ohci->localmem_base + HCCA_WRITEBACK_OFFSET,
-                     (char *)hcca + HCCA_WRITEBACK_OFFSET,
-                     HCCA_WRITEBACK_SIZE);
-    return 1;
+    return dma_memory_write(ohci->as,
+                            addr + ohci->localmem_base + HCCA_WRITEBACK_OFFSET,
+                            (char *)hcca + HCCA_WRITEBACK_OFFSET,
+                            HCCA_WRITEBACK_SIZE);
 }
 
 /* Read/Write the contents of a TD from/to main memory.  */
-static void ohci_copy_td(OHCIState *ohci, struct ohci_td *td,
-                         uint8_t *buf, int len, DMADirection dir)
+static int ohci_copy_td(OHCIState *ohci, struct ohci_td *td,
+                        uint8_t *buf, int len, DMADirection dir)
 {
     dma_addr_t ptr, n;
 
@@ -634,18 +643,26 @@  static void ohci_copy_td(OHCIState *ohci, struct ohci_td *td,
     n = 0x1000 - (ptr & 0xfff);
     if (n > len)
         n = len;
-    dma_memory_rw(ohci->as, ptr + ohci->localmem_base, buf, n, dir);
-    if (n == len)
-        return;
+
+    if (dma_memory_rw(ohci->as, ptr + ohci->localmem_base, buf, n, dir)) {
+        return -1;
+    }
+    if (n == len) {
+        return 0;
+    }
     ptr = td->be & ~0xfffu;
     buf += n;
-    dma_memory_rw(ohci->as, ptr + ohci->localmem_base, buf, len - n, dir);
+    if (dma_memory_rw(ohci->as, ptr + ohci->localmem_base, buf,
+                      len - n, dir)) {
+        return -1;
+    }
+    return 0;
 }
 
 /* Read/Write the contents of an ISO TD from/to main memory.  */
-static void ohci_copy_iso_td(OHCIState *ohci,
-                             uint32_t start_addr, uint32_t end_addr,
-                             uint8_t *buf, int len, DMADirection dir)
+static int ohci_copy_iso_td(OHCIState *ohci,
+                            uint32_t start_addr, uint32_t end_addr,
+                            uint8_t *buf, int len, DMADirection dir)
 {
     dma_addr_t ptr, n;
 
@@ -653,12 +670,20 @@  static void ohci_copy_iso_td(OHCIState *ohci,
     n = 0x1000 - (ptr & 0xfff);
     if (n > len)
         n = len;
-    dma_memory_rw(ohci->as, ptr + ohci->localmem_base, buf, n, dir);
-    if (n == len)
-        return;
+
+    if (dma_memory_rw(ohci->as, ptr + ohci->localmem_base, buf, n, dir)) {
+        return -1;
+    }
+    if (n == len) {
+        return 0;
+    }
     ptr = end_addr & ~0xfffu;
     buf += n;
-    dma_memory_rw(ohci->as, ptr + ohci->localmem_base, buf, len - n, dir);
+    if (dma_memory_rw(ohci->as, ptr + ohci->localmem_base, buf,
+                      len - n, dir)) {
+        return -1;
+    }
+    return 0;
 }
 
 static void ohci_process_lists(OHCIState *ohci, int completion);
@@ -698,8 +723,9 @@  static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
 
     addr = ed->head & OHCI_DPTR_MASK;
 
-    if (!ohci_read_iso_td(ohci, addr, &iso_td)) {
+    if (ohci_read_iso_td(ohci, addr, &iso_td)) {
         printf("usb-ohci: ISO_TD read error at %x\n", addr);
+        ohci_stop(ohci);
         return 0;
     }
 
@@ -740,7 +766,10 @@  static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
         i = OHCI_BM(iso_td.flags, TD_DI);
         if (i < ohci->done_count)
             ohci->done_count = i;
-        ohci_put_iso_td(ohci, addr, &iso_td);
+        if (ohci_put_iso_td(ohci, addr, &iso_td)) {
+            ohci_stop(ohci);
+            return 1;
+        }
         return 0;
     }
 
@@ -821,8 +850,11 @@  static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
     }
 
     if (len && dir != OHCI_TD_DIR_IN) {
-        ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, len,
-                         DMA_DIRECTION_TO_DEVICE);
+        if (ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, len,
+                             DMA_DIRECTION_TO_DEVICE)) {
+            ohci_stop(ohci);
+            return 1;
+        }
     }
 
     if (!completion) {
@@ -852,8 +884,11 @@  static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
     /* Writeback */
     if (dir == OHCI_TD_DIR_IN && ret >= 0 && ret <= len) {
         /* IN transfer succeeded */
-        ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, ret,
-                         DMA_DIRECTION_FROM_DEVICE);
+        if (ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, ret,
+                             DMA_DIRECTION_FROM_DEVICE)) {
+            ohci_stop(ohci);
+            return 1;
+        }
         OHCI_SET_BM(iso_td.offset[relative_frame_number], TD_PSW_CC,
                     OHCI_CC_NOERROR);
         OHCI_SET_BM(iso_td.offset[relative_frame_number], TD_PSW_SIZE, ret);
@@ -910,7 +945,9 @@  static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
         if (i < ohci->done_count)
             ohci->done_count = i;
     }
-    ohci_put_iso_td(ohci, addr, &iso_td);
+    if (ohci_put_iso_td(ohci, addr, &iso_td)) {
+        ohci_stop(ohci);
+    }
     return 1;
 }
 
@@ -943,8 +980,9 @@  static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
 #endif
         return 1;
     }
-    if (!ohci_read_td(ohci, addr, &td)) {
+    if (ohci_read_td(ohci, addr, &td)) {
         fprintf(stderr, "usb-ohci: TD read error at %x\n", addr);
+        ohci_stop(ohci);
         return 0;
     }
 
@@ -997,8 +1035,10 @@  static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
                 pktlen = len;
             }
             if (!completion) {
-                ohci_copy_td(ohci, &td, ohci->usb_buf, pktlen,
-                             DMA_DIRECTION_TO_DEVICE);
+                if (ohci_copy_td(ohci, &td, ohci->usb_buf, pktlen,
+                                 DMA_DIRECTION_TO_DEVICE)) {
+                    ohci_stop(ohci);
+                }
             }
         }
     }
@@ -1055,8 +1095,10 @@  static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
 
     if (ret >= 0) {
         if (dir == OHCI_TD_DIR_IN) {
-            ohci_copy_td(ohci, &td, ohci->usb_buf, ret,
-                         DMA_DIRECTION_FROM_DEVICE);
+            if (ohci_copy_td(ohci, &td, ohci->usb_buf, ret,
+                             DMA_DIRECTION_FROM_DEVICE)) {
+                ohci_stop(ohci);
+            }
 #ifdef DEBUG_PACKET
             DPRINTF("  data:");
             for (i = 0; i < ret; i++)
@@ -1133,7 +1175,10 @@  static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
     if (i < ohci->done_count)
         ohci->done_count = i;
 exit_no_retire:
-    ohci_put_td(ohci, addr, &td);
+    if (ohci_put_td(ohci, addr, &td)) {
+        ohci_stop(ohci);
+        return 1;
+    }
     return OHCI_BM(td.flags, TD_CC) != OHCI_CC_NOERROR;
 }
 
@@ -1151,8 +1196,9 @@  static int ohci_service_ed_list(OHCIState *ohci, uint32_t head, int completion)
         return 0;
 
     for (cur = head; cur; cur = next_ed) {
-        if (!ohci_read_ed(ohci, cur, &ed)) {
+        if (ohci_read_ed(ohci, cur, &ed)) {
             fprintf(stderr, "usb-ohci: ED read error at %x\n", cur);
+            ohci_stop(ohci);
             return 0;
         }
 
@@ -1194,7 +1240,10 @@  static int ohci_service_ed_list(OHCIState *ohci, uint32_t head, int completion)
             }
         }
 
-        ohci_put_ed(ohci, cur, &ed);
+        if (ohci_put_ed(ohci, cur, &ed)) {
+            ohci_stop(ohci);
+            return 0;
+        }
     }
 
     return active;
@@ -1236,7 +1285,11 @@  static void ohci_frame_boundary(void *opaque)
     OHCIState *ohci = opaque;
     struct ohci_hcca hcca;
 
-    ohci_read_hcca(ohci, ohci->hcca, &hcca);
+    if (ohci_read_hcca(ohci, ohci->hcca, &hcca)) {
+        fprintf(stderr, "usb-ohci: HCCA read error at %x\n", ohci->hcca);
+        ohci_stop(ohci);
+        return;
+    }
 
     /* Process all the lists at the end of the frame */
     if (ohci->ctl & OHCI_CTL_PLE) {
@@ -1282,7 +1335,9 @@  static void ohci_frame_boundary(void *opaque)
     ohci_sof(ohci);
 
     /* Writeback HCCA */
-    ohci_put_hcca(ohci, ohci->hcca, &hcca);
+    if (ohci_put_hcca(ohci, ohci->hcca, &hcca)) {
+        ohci_stop(ohci);
+    }
 }
 
 /* Start sending SOF tokens across the USB bus, lists are processed in
@@ -1857,6 +1912,21 @@  typedef struct {
     uint32_t firstport;
 } OHCIPCIState;
 
+/** A typical O/EHCI will stop operating, set itself into error state
+ * (which can be queried by MMIO) and will set PERR in its config
+ * space to signal that it got an error
+ */
+static void ohci_stop(OHCIState *ohci)
+{
+    OHCIPCIState *dev = container_of(ohci, OHCIPCIState, state);
+
+    fprintf(stderr, "%s: DMA error\n", __func__);
+
+    ohci_set_interrupt(ohci, OHCI_INTR_UE);
+    pci_set_word(dev->parent_obj.config + PCI_STATUS,
+                 PCI_STATUS_DETECTED_PARITY);
+}
+
 static int usb_ohci_initfn_pci(PCIDevice *dev)
 {
     OHCIPCIState *ohci = PCI_OHCI(dev);