[Qemu-devel] Re: [PATCH] Update usb-linux to handle maximum of 16k transfers.

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 7856 bytes --]

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 <daahern@cisco.com>
> ---
>  usb-linux.c |  139 +++++++++++++++++++++++++++++++++++------------------------
>  1 files changed, 83 insertions(+), 56 deletions(-)
> 
> diff --git a/usb-linux.c b/usb-linux.c
> index d0d7cff..688f45e 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)
> @@ -220,9 +222,9 @@ static void async_complete(void *opaque)
>      AsyncURB *aurb;
>  
>      while (1) {
> -    	USBPacket *p;
> +        USBPacket *p;
>  
> -	int r = ioctl(s->fd, USBDEVFS_REAPURBNDELAY, &aurb);
> +        int r = ioctl(s->fd, USBDEVFS_REAPURBNDELAY, &aurb);
>          if (r < 0) {
>              if (errno == EAGAIN)
>                  return;
> @@ -238,31 +240,33 @@ static void async_complete(void *opaque)
>              return;
>          }
>  
> -        p = aurb->packet;
> -
> -	DPRINTF("husb: async completed. aurb %p status %d alen %d\n", 
> +        DPRINTF("husb: async completed. aurb %p status %d alen %d\n", 
>                  aurb, aurb->urb.status, aurb->urb.actual_length);
>  
> -	if (p) {
> +        p = aurb->packet;
> +	    if (p) {

There is still a tab in the line above.

Some hunks below is another inherited style issue. But I would recommend
to avoid style fixes in this patch. Focus on the functional change of
the split-up and do those "cosmetic" fixes separately. This is not the
orthogonal ehci module we need to clean up any, but preexisting code.

>              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);
>                  break;
>  
>              case -EPIPE:
>                  set_halt(s, p->devep);
> -		p->len = USB_RET_STALL;
> -		break;
> +                p->len = USB_RET_STALL;
> +                break;
>  
>              default:
>                  p->len = USB_RET_NAK;
>                  break;
>              }
>  
> -            usb_packet_complete(p);
> -	}
> +            if (!aurb->more) {
> +                DPRINTF("invoking packet_complete. plen = %d\n", p->len);
> +                usb_packet_complete(p);
> +            }
> +        }
>  
>          async_free(aurb);
>      }
> @@ -404,67 +408,90 @@ 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, rem;
> +    uint8_t *pbuf;
>  
> -    aurb = async_alloc();
> -    aurb->hdev   = s;
> -    aurb->packet = p;
> +    rem = p->len;
> +    pbuf = p->data;
>  
> -    urb = &aurb->urb;
> +    p->len = 0;
> +    while (rem) {
> +        aurb = async_alloc();
> +        aurb->hdev   = s;
> +        aurb->packet = p;
> + 
> +        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;

Missing braces and improper indention.

> +
> +        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);
> +        }
>  
> -    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_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;
>          }
> -        clear_halt(s, p->devep);
> -    }
>  
> -    urb->buffer        = p->data;
> -    urb->buffer_length = p->len;
> +        urb->usercontext = s;
>  
> -    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;
> -    }
> +        /* 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;
>  
> -    urb->usercontext = s;
> +        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, len, aurb);
>  
> -    DPRINTF("husb: data submit. ep 0x%x len %u aurb %p\n", urb->endpoint, p->len, aurb);
> +        if (ret < 0) {
> +            DPRINTF("husb: submit failed. errno %d\n", errno);
>  
> -    if (ret < 0) {
> -        DPRINTF("husb: submit failed. errno %d\n", errno);
> -        async_free(aurb);
> +            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;
>  }
>  
> @@ -577,7 +604,7 @@ static int usb_host_handle_control(USBHostDevice *s, USBPacket *p)
>      urb->buffer_length = buffer_len;
>  
>      urb->usercontext = s;
> -
> +    p->len = 0;
>      ret = ioctl(s->fd, USBDEVFS_SUBMITURB, urb);
>  
>      DPRINTF("husb: submit ctrl. len %u aurb %p\n", urb->buffer_length, aurb);

Again, please submit as separate functional change + style cleanup
patches, maybe the latter directly for merge in upstream, and the former
on top of it for the ehci branch.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]