[Qemu-devel] Re: [PATCH] Move 16k limitation in request size for host devices from ehci to usb-linux

[Jan Kiszka]

David Ahern wrote:
> Update usb-linux to handle maximum of 16k transfers. The 16k limit
> is imposed by USBFS. EHCI allows up to 20k per request.
> 
> USBFS cannot be increased to 20k due to constraints on memory use (wants
> contiguous memory in allocations that are powers of 2). This change
> breaks large requests from a host controller into 2 URB submissions
> to the host devices.
> 
> Signed-off-by: David Ahern <address@hidden>

Thanks, applied.

Jan

PS: This is what I get when attaching an external USB disk:

husb: open device 1.12
husb: config #1 need -1
husb: 1 interfaces claimed for configuration 1
husb: grabbed usb device 1.12
husb: config #1 need 1
husb: 1 interfaces claimed for configuration 1
husb: config #1 need 1
husb: 1 interfaces claimed for configuration 1
husb: config #1 need 1
husb: 1 interfaces claimed for configuration 1
USB stall
USB stall

That "stall" is irritating - but it seems to work fine otherwise.

> ---
>  hw/usb-ehci.c |   48 +----------------------
>  usb-linux.c   |  121 ++++++++++++++++++++++++++++++++++----------------------
>  2 files changed, 75 insertions(+), 94 deletions(-)
> 
> diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
> index c91a6b5..29baf74 100644
> --- a/hw/usb-ehci.c
> +++ b/hw/usb-ehci.c
> @@ -380,8 +380,6 @@ typedef struct {
>      USBPort ports[NB_PORTS];
>      uint8_t buffer[BUFF_SIZE];
>  
> -    int more;
> -
>      /* cached data from guest - needs to be flushed 
>       * when guest removes an entry (doorbell, handshake sequence)
>       */
> @@ -1005,43 +1003,6 @@ err:
>          // DPRINTF("Short packet condition\n");
>          // TODO check 4.12 for splits
>  
> -        /* see if a follow-up rquest is needed - request for
> -         * 20k yet OS only allows 16k requests at a time
> -         */
> -        if ((ret > 0) && (ehci->tbytes > 16384) && !ehci->more) {
> -        /* TO-DO: put this in a function for use here and by execute */
> -        USBPort *port = &ehci->ports[i];
> -        USBDevice *dev = port->dev;
> -
> -        ehci->more = ret;
> -
> -        ehci->usb_packet.pid = ehci->pid;
> -        ehci->usb_packet.devaddr = get_field(qh->epchar, QH_EPCHAR_DEVADDR);
> -        ehci->usb_packet.devep = get_field(qh->epchar, QH_EPCHAR_EP);
> -        ehci->usb_packet.data = ehci->buffer + ret;
> -        /* linux devio.c limits max to 16k */
> -        ehci->usb_packet.len = ehci->tbytes - ret;
> -        ehci->usb_packet.complete_cb = ehci_async_complete_packet;
> -        ehci->usb_packet.complete_opaque = ehci;
> -
> -        ret = dev->info->handle_packet(dev, &ehci->usb_packet);
> -
> -        DPRINTF("submit followup: qh %x qtd %x pid %x len %d ret %d\n",
> -                ehci->qhaddr, ehci->qtdaddr, ehci->pid,
> -                ehci->usb_packet.len, ret);
> -
> -        if (ret == USB_RET_ASYNC) {
> -            ehci->async_port_in_progress = i;
> -            ehci->async_complete = 0;
> -            return ret;
> -        }
> -        if (ret < 0) {
> -            goto err;
> -        }
> -        }
> -
> -        ret += ehci->more;
> -
>          if ((ret > ehci->tbytes) && (ehci->pid == USB_TOKEN_IN)) {
>              ret = USB_RET_BABBLE;
>              goto err;
> @@ -1128,12 +1089,7 @@ static int ehci_execute(EHCIState *ehci, EHCIqh *qh)
>          ehci->usb_packet.devaddr = devadr;
>          ehci->usb_packet.devep = endp;
>          ehci->usb_packet.data = ehci->buffer;
> -        /* linux devio.c limits max to 16k */
> -        if (ehci->tbytes > 16384) {
> -            ehci->usb_packet.len = 16384;
> -        } else {
> -            ehci->usb_packet.len = ehci->tbytes;
> -        }
> +        ehci->usb_packet.len = ehci->tbytes;
>          ehci->usb_packet.complete_cb = ehci_async_complete_packet;
>          ehci->usb_packet.complete_opaque = ehci;
>  
> @@ -1611,7 +1567,7 @@ static int ehci_state_execute(EHCIState *ehci, int 
> async, int *state)
>  
>      if (async)
>          ehci->usbsts |= USBSTS_REC;
> -    ehci->more = 0;
> +
>      ehci->exec_status = ehci_execute(ehci, qh);
>      if (ehci->exec_status == USB_RET_PROCERR) {
>          again = -1;
> diff --git a/usb-linux.c b/usb-linux.c
> index b3d6b28..7181e2f 100644
> --- a/usb-linux.c
> +++ b/usb-linux.c
> @@ -182,6 +182,8 @@ typedef struct AsyncURB
>  
>      USBPacket     *packet;
>      USBHostDevice *hdev;
> +
> +    int more;     /* packet required multiple URBs */
>  } AsyncURB;
>  
>  static AsyncURB *async_alloc(void)
> @@ -247,7 +249,7 @@ static void async_complete(void *opaque)
>          if (p) {
>              switch (aurb->urb.status) {
>              case 0:
> -                p->len = aurb->urb.actual_length;
> +                p->len += aurb->urb.actual_length;
>                  if (aurb->urb.type == USBDEVFS_URB_TYPE_CONTROL) {
>                      async_complete_ctrl(s, p);
>                  }
> @@ -263,7 +265,10 @@ static void async_complete(void *opaque)
>                  break;
>              }
>  
> -            usb_packet_complete(p);
> +            if (!aurb->more) {
> +                DPRINTF("invoking packet_complete. plen = %d\n", p->len);
> +                usb_packet_complete(p);
> +            }
>          }
>  
>          async_free(aurb);
> @@ -408,69 +413,89 @@ static void usb_host_handle_destroy(USBDevice *dev)
>  
>  static int usb_linux_update_endp_table(USBHostDevice *s);
>  
> +/* devio.c limits single requests to 16k */
> +#define MAX_USBFS_BUFFER_SIZE 16384
> +
>  static int usb_host_handle_data(USBHostDevice *s, USBPacket *p)
>  {
>      struct usbdevfs_urb *urb;
>      AsyncURB *aurb;
> -    int ret;
> +    int ret, len;
> +    int rem = p->len;
> +    uint8_t *pbuf = p->data;
>  
> -    aurb = async_alloc();
> -    aurb->hdev   = s;
> -    aurb->packet = p;
> +    p->len = 0;
> +    while (rem) {
> +        aurb = async_alloc();
> +        aurb->hdev   = s;
> +        aurb->packet = p;
>  
> -    urb = &aurb->urb;
> +        urb = &aurb->urb;
>  
> -    if (p->pid == USB_TOKEN_IN) {
> -        urb->endpoint = p->devep | 0x80;
> -    } else {
> -        urb->endpoint = p->devep;
> -    }
> +        if (p->pid == USB_TOKEN_IN) {
> +            urb->endpoint = p->devep | 0x80;
> +        } else {
> +            urb->endpoint = p->devep;
> +        }
>  
> -    if (is_halted(s, p->devep)) {
> -        ret = ioctl(s->fd, USBDEVFS_CLEAR_HALT, &urb->endpoint);
> -        if (ret < 0) {
> -            DPRINTF("husb: failed to clear halt. ep 0x%x errno %d\n",
> -                   urb->endpoint, errno);
> -            return USB_RET_NAK;
> +        if (is_halted(s, p->devep)) {
> +            ret = ioctl(s->fd, USBDEVFS_CLEAR_HALT, &urb->endpoint);
> +            if (ret < 0) {
> +                DPRINTF("husb: failed to clear halt. ep 0x%x errno %d\n",
> +                        urb->endpoint, errno);
> +                return USB_RET_NAK;
> +            }
> +            clear_halt(s, p->devep);
>          }
> -        clear_halt(s, p->devep);
> -    }
>  
> -    urb->buffer        = p->data;
> -    urb->buffer_length = p->len;
> +        if (is_isoc(s, p->devep)) {
> +            /* Setup ISOC transfer */
> +            urb->type     = USBDEVFS_URB_TYPE_ISO;
> +            urb->flags    = USBDEVFS_URB_ISO_ASAP;
> +            urb->number_of_packets = 1;
> +            urb->iso_frame_desc[0].length = p->len;
> +        } else {
> +            /* Setup bulk transfer */
> +            urb->type     = USBDEVFS_URB_TYPE_BULK;
> +        }
>  
> -    if (is_isoc(s, p->devep)) {
> -        /* Setup ISOC transfer */
> -        urb->type     = USBDEVFS_URB_TYPE_ISO;
> -        urb->flags    = USBDEVFS_URB_ISO_ASAP;
> -        urb->number_of_packets = 1;
> -        urb->iso_frame_desc[0].length = p->len;
> -    } else {
> -        /* Setup bulk transfer */
> -        urb->type     = USBDEVFS_URB_TYPE_BULK;
> -    }
> +        urb->usercontext = s;
>  
> -    urb->usercontext = s;
> +        /* USBFS limits max request size to 16k */
> +        if (rem > MAX_USBFS_BUFFER_SIZE) {
> +            len = MAX_USBFS_BUFFER_SIZE;
> +            aurb->more = 1;
> +        } else {
> +            len = rem;
> +            aurb->more = 0;
> +        }
> +        urb->buffer_length = len;
> +        urb->buffer = pbuf;
>  
> -    ret = ioctl(s->fd, USBDEVFS_SUBMITURB, urb);
> +        ret = ioctl(s->fd, USBDEVFS_SUBMITURB, urb);
>  
> -    DPRINTF("husb: data submit. ep 0x%x len %u aurb %p\n",
> -            urb->endpoint, p->len, aurb);
> +        DPRINTF("husb: data submit. ep 0x%x len %u aurb %p\n",
> +                urb->endpoint, len, aurb);
>  
> -    if (ret < 0) {
> -        DPRINTF("husb: submit failed. errno %d\n", errno);
> -        async_free(aurb);
> +        if (ret < 0) {
> +            DPRINTF("husb: submit failed. errno %d\n", errno);
> +                async_free(aurb);
>  
> -        switch(errno) {
> -        case ETIMEDOUT:
> -            return USB_RET_NAK;
> -        case EPIPE:
> -        default:
> -            return USB_RET_STALL;
> +            switch(errno) {
> +            case ETIMEDOUT:
> +                return USB_RET_NAK;
> +            case EPIPE:
> +            default:
> +                return USB_RET_STALL;
> +            }
>          }
> +
> +        usb_defer_packet(p, async_cancel, aurb);
> +
> +        pbuf += len;
> +        rem -= len;
>      }
>  
> -    usb_defer_packet(p, async_cancel, aurb);
>      return USB_RET_ASYNC;
>  }
>

signature.asc
Description: OpenPGP digital signature