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

[Kevin Wolf <kevin@xxxxxxxxxx>]

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?

> +    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.

> +}

Kevin