[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [cdi-devel] [PATCH v3 08/10] ehci: Add EHCI driver



On 11.10.2015 at 23:03, Kevin Wolf wrote:
On Mon, May 11, 2015 at 09:59:28PM +0200, Max Reitz wrote:
+ This adds an EHCI USB host controller driver. It is missing support
   for periodic schedule (interrupt and isochronous transfers),
   hotplugging, suspend/resume, etc. pp..

Signed-off-by: Max Reitz <max@xxxxxxxxxx>
---
  ehci/ehci.c | 643 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
  ehci/ehci.h | 290 +++++++++++++++++++++++++++
  ehci/main.c |  68 +++++++
  3 files changed, 1001 insertions(+)
  create mode 100644 ehci/ehci.c
  create mode 100644 ehci/ehci.h
  create mode 100644 ehci/main.c
+static void retire_qtd(ehci_fat_qtd_t *qtd)
+{
+    ehci_fat_qh_t *qh = qtd->qh;
+
+    uint32_t err_mask = (qh->qh.ep_state[0] & EHCI_QHES0_EPS_MASK)
+                         == EHCI_QHES0_EPS_HS
+                        ? EHCI_QTDS_HS_ERR_MASK
+                        : EHCI_QTDS_LFS_ERR_MASK;
+
+    if (qtd->qtd.status & err_mask) {
+        if (qtd->qtd.status & EHCI_QTDS_BABBLE) {
+            *qtd->result = CDI_USB_BABBLE;
+        } else if (qtd->qtd.status & (EHCI_QTDS_XACTERR | EHCI_QTDS_P_ERR)) {
+            *qtd->result = CDI_USB_BAD_RESPONSE;
+        } else if (qtd->qtd.status & (EHCI_QTDS_BUFERR | EHCI_QTDS_MuF)) {
+            *qtd->result = CDI_USB_HC_ERROR;
+        } else if (qtd->qtd.status & EHCI_QTDS_HALTED) {
+            *qtd->result = CDI_USB_STALL;
+        } else {
+            *qtd->result = CDI_USB_HC_ERROR;
+        }
+    }
+
+    if (qtd->write_back) {
+        size_t rem = qtd->write_back_size;
+        for (int i = 0; i < 5 && rem; i++) {
+            size_t sz = rem > 4096 ? 4096 : rem;
+            memcpy((void *)((uintptr_t)qtd->write_back + i * 4096),
+                   qtd->buffers[i]->vaddr, sz);
+            rem -= sz;
+        }
+        assert(!rem);
+    }
+
+    qh->first_qtd = qtd->next;
+    if (qh->last_qtd == qtd) {
+        qh->last_qtd = NULL;
+    }
Worth an assert(qh->first_qtd == qtd) before doing this?

It would probably be better to rename this function to retire_first_qtd() with the only parameter being the QH.

+    for (int i = 0; i < 5; i++) {
+        if (qtd->buffers[i]) {
+            cdi_mem_free(qtd->buffers[i]);
+        }
+    }
+
+    cdi_mem_free(qtd->cdi_mem_area);
+}
+
+
+static void enter_qtd(ehci_fat_qh_t *qh, ehci_fat_qtd_t *qtd)
+{
+    bool first;
+
+    qtd->qh = qh;
+    qtd->qtd.next_qtd = EHCI_T;
+    qtd->qtd.alt_next_qtd = EHCI_T;
+
+    while (qh->first_qtd) {
+        // Break if an active qTD is encountered
+        if (qh->first_qtd->qtd.status & EHCI_QTDS_ACTIVE) {
+            break;
+        }
+
+        retire_qtd(qh->first_qtd);
+    }
+
+    first = !qh->first_qtd;
+
+    // Note: Race condition (a qTD might retire here and we are thus appending
+    // the one to be queued to an inactive qTD); it's fine as long as we are
+    // constantly rescanning the queue (here and in ehci_wait_transaction())
+
+    if (first) {
+        qh->first_qtd = qtd;
+    } else {
+        qh->last_qtd->next = qtd;
+    }
+    qh->last_qtd = qtd;
+
+    if (!first) {
+        qh->last_qtd->qtd.next_qtd = qtd->paddr;
+    }
+
+    // If the QH is not active, it's done with its queue of qTDs and we should
+    // continue from this qTD
+    if (!(qh->qh.overlay.status & EHCI_QTDS_ACTIVE)) {
+        qh->qh.overlay.next_qtd = qh->first_qtd->paddr;
+    }
Is this racy? If you enter two qTDs and the hardware hasn't copied the
active bit of the first entered qTD into the overlay yet before the
second one is entered, it looks to me as if the QH pointed to the second
one now and the first one got ignored.

Good thing I am not entering qtd->paddr but qh->first_qtd->paddr (independently of whether qh->first_qtd == qtd). ;-)

The comment is wrong, though. It should read something like "If the QH has stopped operation we need to point it to the first qTD".

Hm... Now that I think about it, maybe there is a race in the opposite direction. The active bit was not set, but then the qTD is copied into the overlay and we overwrite next_qtd by a pointer to itself. It probably would not be horrible, because that means that the HC will stop operation once the qTD is processed (because the active bit is cleared then), and we would then fix this condition in ehci_wait_transaction().

But it still is bad. Should be fixable by checking whether qh->qh.current_qtd == qh->qh.overlay.next_qtd after having set qh->qh.overlay.next_qtd, and while that is the case, replace qh->qh.overlay.next_qtd by qh->first_qtd->qtd.next_qtd (“while” so we can handle multiple races; the “if (!active)” then maybe needs to be some kind of while loop, too).

Max