[U-Boot] [PATCH v2 3/7] usb: ehci: Support interrupt transfers via periodic list

[Jim Lin <jilin@nvidia.com>]

There is a potential bug. See below.

> +struct int_queue {
> +       struct QH *first;
> +       struct QH *current;
> +       struct QH *last;
> +       struct qTD *tds;
> +};
> +
> +#define NEXT_QH(qh) (struct QH *)((qh)->qh_link & ~0x1f)
> +
> +static int
> +enable_periodic(struct ehci_ctrl *ctrl)
> +{
> +       uint32_t cmd;
> +       struct ehci_hcor *hcor = ctrl->hcor;
> +       int ret;
> +
> +       cmd = ehci_readl(&hcor->or_usbcmd);
> +       cmd |= CMD_PSE;
> +       ehci_writel(&hcor->or_usbcmd, cmd);
> +
> +       ret = handshake((uint32_t *)&hcor->or_usbsts,
> +                       STD_PSS, STD_PSS, 100 * 1000);
Using STS_PSS may be better. STS means status, for USBSTS register.

> +       if (ret < 0) {
> +               printf("EHCI failed: timeout when enabling periodic list\n");
> +               return -ETIMEDOUT;
> +       }
> +       udelay(1000);
> +       return 0;
> +}
> +
> +static int
> +disable_periodic(struct ehci_ctrl *ctrl)
> +{
> +       uint32_t cmd;
> +       struct ehci_hcor *hcor = ctrl->hcor;
> +       int ret;
> +
> +       cmd = ehci_readl(&hcor->or_usbcmd);
> +       cmd &= ~CMD_PSE;
> +       ehci_writel(&hcor->or_usbcmd, cmd);
> +
> +       ret = handshake((uint32_t *)&hcor->or_usbsts,
> +                       STD_PSS, 0, 100 * 1000);
Using STS_PSS may be better. STS means status, for USBSTS register.

> +       if (ret < 0) {
> +               printf("EHCI failed: timeout when enabling periodic list\n");
> +               return -ETIMEDOUT;
> +       }
> +       return 0;
> +}
> +
> +static int periodic_schedules;
> +
> +struct int_queue *
> +create_int_queue(struct usb_device *dev, unsigned long pipe, int queuesize,
> +                int elementsize, void *buffer)
> +{
> +       struct ehci_ctrl *ctrl = dev->controller;
> +       struct int_queue *result = NULL;
> +       int i;
> +
> +       debug("Enter create_int_queue\n");
> +       if (usb_pipetype(pipe) != PIPE_INTERRUPT) {
> +               debug("non-interrupt pipe (type=%lu)", usb_pipetype(pipe));
> +               return NULL;
> +       }
> +
> +       /* limit to 4 full pages worth of data -
> +        * we can safely fit them in a single TD,
> +        * no matter the alignment
> +        */
> +       if (elementsize >= 16384) {
> +               debug("too large elements for interrupt transfers\n");
> +               return NULL;
> +       }
> +
> +       result = malloc(sizeof(*result));
> +       if (!result) {
> +               debug("ehci intr queue: out of memory\n");
> +               goto fail1;
> +       }
> +       result->first = memalign(32, sizeof(struct QH) * queuesize);
> +       if (!result->first) {
> +               debug("ehci intr queue: out of memory\n");
> +               goto fail2;
> +       }
> +       result->current = result->first;
> +       result->last = result->first + elementsize - 1;
If input queuesize is 1 and elementsize is 8 (for a low-speed keyboard),
result->first points to one allocated QH.
result->last points to 8th QH (starting from result->first).
But we only allocate one QH space for result->first.
The location pointed by result->last is beyond the space we allocate.
It seems not right.

> +       result->tds = memalign(32, sizeof(struct qTD) * queuesize);
> +       if (!result->tds) {
> +               debug("ehci intr queue: out of memory\n");
> +               goto fail3;
> +       }
> +       memset(result->first, 0, sizeof(struct QH) * queuesize);
> +       memset(result->tds, 0, sizeof(struct qTD) * queuesize);
> +
> +       for (i = 0; i < queuesize; i++) {
> +               struct QH *qh = result->first + i;
> +               struct qTD *td = result->tds + i;
> +               void **buf = &qh->buffer;
> +
> +               qh->qh_link = (uint32_t)(qh+1) | QH_LINK_TYPE_QH;
> +               if (i == queuesize - 1)
> +                       qh->qh_link = QH_LINK_TERMINATE;
> +
> +               qh->qh_overlay.qt_next = (uint32_t)td;
> +               qh->qh_endpt1 = (0 << 28) | /* No NAK reload (ehci 4.9) */
> +                       (usb_maxpacket(dev, pipe) << 16) | /* MPS */
> +                       (1 << 14) |
> +                       QH_ENDPT1_EPS(ehci_encode_speed(dev->speed)) |
> +                       (usb_pipeendpoint(pipe) << 8) | /* Endpoint Number */
> +                       (usb_pipedevice(pipe) << 0);
> +               qh->qh_endpt2 = (1 << 30); /* 1 Tx per mframe */
> +               if (dev->speed == USB_SPEED_LOW ||
> +                               dev->speed == USB_SPEED_FULL) {
> +                       debug("TT: port: %d, hub address: %d\n",
> +                               dev->portnr, dev->parent->devnum);
> +                       qh->qh_endpt2 |= (dev->portnr << 23) |
> +                               (dev->parent->devnum << 16) |
> +                               (0x1c << 8) | /* C-mask: microframes 2-4 */
> +                               (1 << 0); /* S-mask: microframe 0 */
> +               }
> +
> +               td->qt_next = QT_NEXT_TERMINATE;
> +               td->qt_altnext = QT_NEXT_TERMINATE;
> +               debug("communication direction is '%s'\n",
> +                     usb_pipein(pipe) ? "in" : "out");
> +               td->qt_token = (elementsize << 16) |
> +                       ((usb_pipein(pipe) ? 1 : 0) << 8) | /* IN/OUT token */
> +                       0x80; /* active */
> +               td->qt_buffer[0] = (uint32_t)buffer + i * elementsize;
> +               td->qt_buffer[1] = (td->qt_buffer[0] + 0x1000) & ~0xfff;
> +               td->qt_buffer[2] = (td->qt_buffer[0] + 0x2000) & ~0xfff;
> +               td->qt_buffer[3] = (td->qt_buffer[0] + 0x3000) & ~0xfff;
> +               td->qt_buffer[4] = (td->qt_buffer[0] + 0x4000) & ~0xfff;
> +
> +               *buf = buffer + i * elementsize;
> +       }
> +
> +       if (disable_periodic(ctrl) < 0) {
> +               debug("FATAL: periodic should never fail, but did");
> +               goto fail3;
> +       }
> +
> +       /* hook up to periodic list */
> +       struct QH *list = &ctrl->periodic_queue;
> +       result->last->qh_link = list->qh_link;
> +       list->qh_link = (uint32_t)result->first | QH_LINK_TYPE_QH;
> +
> +       if (enable_periodic(ctrl) < 0) {
> +               debug("FATAL: periodic should never fail, but did");
> +               goto fail3;
> +       }
> +       periodic_schedules++;
> +
> +       debug("Exit create_int_queue\n");
> +       return result;
> +fail3:
> +       if (result->tds)
> +               free(result->tds);
> +fail2:
> +       if (result->first)
> +               free(result->first);
> +       if (result)
> +               free(result);
> +fail1:
> +       return NULL;
> +}

>  int
>  submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
>                int length, int interval)
>  {
> +       void *backbuffer;
> +       struct int_queue *queue;
> +       unsigned long timeout;
> +       int result = 0, ret;
> +
>         debug("dev=%p, pipe=%lu, buffer=%p, length=%d, interval=%d",
>               dev, pipe, buffer, length, interval);
> 
> @@ -975,9 +1260,31 @@ submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
>          * not require more than a single qTD.
>          */
>         if (length > usb_maxpacket(dev, pipe)) {
> -               printf("%s: Interrupt transfers requiring several transactions "
> -                       "are not supported.\n", __func__);
> +               printf("%s: Interrupt transfers requiring several "
> +                       "transactions are not supported.\n", __func__);
>                 return -1;
>         }
> -       return ehci_submit_async(dev, pipe, buffer, length, NULL);
>  +
> +       queue = create_int_queue(dev, pipe, 1, length, buffer);


> diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
> index 1e3cd79..8bc2ba1 100644
> --- a/drivers/usb/host/ehci.h
> +++ b/drivers/usb/host/ehci.h
> @@ -69,6 +69,7 @@ struct ehci_hcor {
>  #define CMD_RUN                (1 << 0)                /* start/stop HC */
>         uint32_t or_usbsts;
>  #define STS_ASS                (1 << 15)
> +#define        STD_PSS         (1 << 14)
Using STS_PSS may be better. STS means status, for USBSTS register.




-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------