[Qemu-devel] [PATCH 1/5] usb-linux.c: allow full-size control transfers, do not overrun buffer

[Qemu-devel] [PATCH 1/5] usb-linux.c: allow full-size control transfers, do not overrun buffer

[Ian Jackson]

The buffer in struct ctrl_struct needs to be big enough for any
control transfer which may be initiated by the guest, since we are
perhaps trying to pass a device through.  The biggest possible size is
2^16-1 since the length fields are 16 bits.

Also, assert that the transfer request we are about to make to our
host kernel will not overrun the buffer.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 usb-linux.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index fb1153b..321c1db 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -34,6 +34,8 @@
 #include "qemu-timer.h"
 #include "console.h"
 
+#if defined(__linux__)
+#include <assert.h>
 #include <dirent.h>
 #include <sys/ioctl.h>
 #include <signal.h>
@@ -115,7 +117,7 @@ struct ctrl_struct {
     uint16_t offset;
     uint8_t  state;
     struct   usb_ctrlrequest req;
-    uint8_t  buffer[1024];
+    uint8_t  buffer[65536];
 };
 
 typedef struct USBHostDevice {
@@ -603,6 +605,8 @@ static int usb_host_handle_control(USBHostDevice *s, USBPacket *p)
     urb->type     = USBDEVFS_URB_TYPE_CONTROL;
     urb->endpoint = p->devep;
 
+    assert(s->ctrl.len < sizeof(s->ctrl.buffer));
+
     urb->buffer        = &s->ctrl.req;
     urb->buffer_length = 8 + s->ctrl.len;
 
-- 
1.4.4.4

[Qemu-devel] [PATCH 2/5] usb-linux.c: set urb->type correctly for control and interrupt transfers

[Ian Jackson]

Previously we would always request that the host kernel perform a bulk
transfer if the endpoint was not isochronous.  This is wrong and
results in EINVAL if the endpoint is control or interrupt.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 usb-linux.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index 321c1db..3019a13 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -469,16 +469,13 @@ static int usb_host_handle_data(USBHostDevice *s, USBPacket *p)
 
     urb->buffer        = p->data;
     urb->buffer_length = p->len;
+    urb->type          = s->endp_table[p->devep - 1].type;
 
-    if (is_isoc(s, p->devep)) {
+    if (urb->type == USBDEVFS_URB_TYPE_ISO) {
         /* 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;
-- 
1.4.4.4

[Qemu-devel] [PATCH 3/5] usb-linux.c: somewhat improve some error and debugging messages

[Ian Jackson]

Firstly, always print strerror(errno) (when relevant) rather than
numerical errno.  Numerical errno values are not very useful and the
libc provides us with strerror especially for this purpose.

Secondly, distinguish previously-identical messages relating to
control and data transfers.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 usb-linux.c |   23 ++++++++++++-----------
 1 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index 3019a13..8498b1d 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -258,7 +258,7 @@ static void async_complete(void *opaque)
                 return;
             }
 
-            dprintf("husb: async. reap urb failed errno %d\n", errno);
+            dprintf("husb: async. reap urb failed: %s\n", strerror(errno));
             return;
         }
 
@@ -302,7 +302,7 @@ static void async_cancel(USBPacket *unused, void *opaque)
 
     int r = ioctl(s->fd, USBDEVFS_DISCARDURB, aurb);
     if (r < 0) {
-        dprintf("husb: async. discard urb failed errno %d\n", errno);
+        dprintf("husb: async. discard urb failed: %s\n", strerror(errno));
     }
 }
 
@@ -444,7 +444,7 @@ static int usb_host_handle_data(USBHostDevice *s, USBPacket *p)
 
     aurb = async_alloc();
     if (!aurb) {
-        dprintf("husb: async malloc failed\n");
+        dprintf("husb: async malloc (data) failed\n");
         return USB_RET_NAK;
     }
     aurb->hdev   = s;
@@ -460,8 +460,8 @@ static int usb_host_handle_data(USBHostDevice *s, USBPacket *p)
     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);
+            dprintf("husb: failed to clear halt. ep 0x%x: %s\n",
+                   urb->endpoint, strerror(errno));
             return USB_RET_NAK;
         }
         clear_halt(s, p->devep);
@@ -485,7 +485,7 @@ static int usb_host_handle_data(USBHostDevice *s, USBPacket *p)
     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);
+        dprintf("husb: submit data failed: %s\n", strerror(errno));
         async_free(aurb);
 
         switch(errno) {
@@ -522,7 +522,8 @@ static int usb_host_set_config(USBHostDevice *s, int config)
 
     int ret = ioctl(s->fd, USBDEVFS_SETCONFIGURATION, &config);
  
-    dprintf("husb: ctrl set config %d ret %d errno %d\n", config, ret, errno);
+    dprintf("husb: ctrl set config %d ret %d: %s\n", config, ret,
+	    ret < 0 ? strerror(errno) : "ok");
     
     if (ret < 0)
         return ctrl_error();
@@ -540,8 +541,8 @@ static int usb_host_set_interface(USBHostDevice *s, int iface, int alt)
     si.altsetting = alt;
     ret = ioctl(s->fd, USBDEVFS_SETINTERFACE, &si);
     
-    dprintf("husb: ctrl set iface %d altset %d ret %d errno %d\n", 
-    	iface, alt, ret, errno);
+    dprintf("husb: ctrl set iface %d altset %d ret %d: %s\n",
+    	iface, alt, ret, ret < 0 ? strerror(errno): "ok");
     
     if (ret < 0)
         return ctrl_error();
@@ -585,7 +586,7 @@ static int usb_host_handle_control(USBHostDevice *s, USBPacket *p)
 
     aurb = async_alloc();
     if (!aurb) {
-        dprintf("husb: async malloc failed\n");
+        dprintf("husb: async malloc (ctrl) failed\n");
         return USB_RET_NAK;
     }
     aurb->hdev   = s;
@@ -614,7 +615,7 @@ static int usb_host_handle_control(USBHostDevice *s, USBPacket *p)
     dprintf("husb: submit ctrl. len %u aurb %p\n", urb->buffer_length, aurb);
 
     if (ret < 0) {
-        dprintf("husb: submit failed. errno %d\n", errno);
+        dprintf("husb: submit ctrl failed: %s\n", strerror(errno));
         async_free(aurb);
 
         switch(errno) {
-- 
1.4.4.4

[Qemu-devel] [PATCH 4/5] usb-linux.c: more improved debugging messages (endpoint table)

[Ian Jackson]

Use dprintf to dump some debugging output while we walk the endpoint
table in usb_linux_update_endp_table.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 usb-linux.c |   22 ++++++++++++++++++++--
 1 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index 8498b1d..b592514 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -800,6 +800,8 @@ static int usb_linux_update_endp_table(USBHostDevice *s)
     ct.data = &configuration;
     ct.timeout = 50;
 
+    dprintf("husb: updating endpoint table for %d.%d\n", s->bus_num, s->addr);
+
     ret = ioctl(s->fd, USBDEVFS_CONTROL, &ct);
     if (ret < 0) {
         perror("usb_linux_update_endp_table");
@@ -807,8 +809,10 @@ static int usb_linux_update_endp_table(USBHostDevice *s)
     }
 
     /* in address state */
-    if (configuration == 0)
+    if (configuration == 0) {
+        dprintf("husb: ... in address state\n");
         return 1;
+    }
 
     /* get the desired configuration, interface, and endpoint descriptors
      * from device description */
@@ -824,6 +828,8 @@ static int usb_linux_update_endp_table(USBHostDevice *s)
     i += descriptors[i];
 
     while (i < length) {
+        dprintf("husb:  descriptor offset 0x%x\n", i);
+
         if (descriptors[i + 1] != USB_DT_INTERFACE ||
             (descriptors[i + 1] == USB_DT_INTERFACE &&
              descriptors[i + 4] == 0)) {
@@ -841,6 +847,8 @@ static int usb_linux_update_endp_table(USBHostDevice *s)
         ct.data = &alt_interface;
         ct.timeout = 50;
 
+        dprintf("husb:   interface 0x%x\n", interface);
+
         ret = ioctl(s->fd, USBDEVFS_CONTROL, &ct);
         if (ret < 0) {
             perror("usb_linux_update_endp_table");
@@ -851,6 +859,7 @@ static int usb_linux_update_endp_table(USBHostDevice *s)
          * and has endpoints */
         if (descriptors[i + 3] != alt_interface) {
             i += descriptors[i];
+            dprintf("husb:   ... active, with endpoints\n");
             continue;
         }
 
@@ -862,11 +871,19 @@ static int usb_linux_update_endp_table(USBHostDevice *s)
             break;
 
         while (i < length) {
+            dprintf("husb:    endpoint offset 0x%x\n", i);
             if (descriptors[i + 1] != USB_DT_ENDPOINT)
                 break;
 
             devep = descriptors[i + 2];
-            switch (descriptors[i + 3] & 0x3) {
+            int eptype = descriptors[i + 3] & 0x3;
+            
+            dprintf("husb:     %02x%02x%02x%02x... devep=0x%x type=0x%x\n",
+                    descriptors[i], descriptors[i+1],
+                    descriptors[i+2], descriptors[i+3],
+                    devep, eptype);
+            
+            switch (eptype) {
             case 0x00:
                 type = USBDEVFS_URB_TYPE_CONTROL;
                 break;
@@ -889,6 +906,7 @@ static int usb_linux_update_endp_table(USBHostDevice *s)
             i += descriptors[i];
         }
     }
+    dprintf("husb: endpoint table update complete.\n");
     return 0;
 }
 
-- 
1.4.4.4

[Qemu-devel] [PATCH 5/5] usb-linux.c: fix handling of asynchronous isochronous completion

[Ian Jackson]

When an isochronous URB completes, we need to get the actual transfer
length and status out of the iso_frame_desc[] array rather than the
main urb struct.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 usb-linux.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index b592514..4c0e164 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -264,8 +264,13 @@ static void async_complete(void *opaque)
 
         p = aurb->packet;
 
-	dprintf("husb: async completed. aurb %p status %d alen %d\n", 
-                aurb, aurb->urb.status, aurb->urb.actual_length);
+	if (aurb->urb.type == USBDEVFS_URB_TYPE_ISO) {
+            aurb->urb.status        = aurb->urb.iso_frame_desc[0].status;
+            aurb->urb.actual_length = aurb->urb.iso_frame_desc[0].actual_length;
+        }
+        dprintf("husb: async completed. aurb %p type %d ep 0x%x status %d alen %d\n", 
+                aurb, aurb->urb.type, aurb->urb.endpoint,
+                aurb->urb.status, aurb->urb.actual_length);
 
 	if (p) {
             switch (aurb->urb.status) {
-- 
1.4.4.4

Re: [Qemu-devel] [PATCH 1/5] usb-linux.c: allow full-size control transfers, do not overrun buffer

[malc]

On Tue, 3 Feb 2009, Ian Jackson wrote:

> The buffer in struct ctrl_struct needs to be big enough for any
> control transfer which may be initiated by the guest, since we are
> perhaps trying to pass a device through.  The biggest possible size is
> 2^16-1 since the length fields are 16 bits.
> 
> Also, assert that the transfer request we are about to make to our
> host kernel will not overrun the buffer.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
>  usb-linux.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/usb-linux.c b/usb-linux.c
> index fb1153b..321c1db 100644
> --- a/usb-linux.c
> +++ b/usb-linux.c
> @@ -34,6 +34,8 @@
>  #include "qemu-timer.h"
>  #include "console.h"
>  
> +#if defined(__linux__)

And endif is... (Leaving aside the fact that __linux__ guard in a file
called something-linux.c is a weird looking thing)

> +#include <assert.h>
>  #include <dirent.h>
>  #include <sys/ioctl.h>
>  #include <signal.h>
> @@ -115,7 +117,7 @@ struct ctrl_struct {
>      uint16_t offset;
>      uint8_t  state;
>      struct   usb_ctrlrequest req;
> -    uint8_t  buffer[1024];
> +    uint8_t  buffer[65536];
>  };
>  
>  typedef struct USBHostDevice {
> @@ -603,6 +605,8 @@ static int usb_host_handle_control(USBHostDevice *s, USBPacket *p)
>      urb->type     = USBDEVFS_URB_TYPE_CONTROL;
>      urb->endpoint = p->devep;
>  
> +    assert(s->ctrl.len < sizeof(s->ctrl.buffer));
> +

If something can happen it will, if assert can turn into a nop it will do
so also, `if (cond) abort();' is more apropriate.

>      urb->buffer        = &s->ctrl.req;
>      urb->buffer_length = 8 + s->ctrl.len;
>  
> 

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 1/5] usb-linux.c: allow full-size control transfers, do not overrun buffer

[Ian Jackson]

malc writes ("Re: [Qemu-devel] [PATCH 1/5] usb-linux.c: allow full-size control transfers, do not overrun buffer"):
> On Tue, 3 Feb 2009, Ian Jackson wrote:
> > +#if defined(__linux__)
> 
> And endif is... (Leaving aside the fact that __linux__ guard in a file
> called something-linux.c is a weird looking thing)

I don't know how that got there.  It's a mistake.  Sorry.  I'll redo
the patch ...

> > +    assert(s->ctrl.len < sizeof(s->ctrl.buffer));
> > +
> 
> If something can happen it will, if assert can turn into a nop it will do
> so also, `if (cond) abort();' is more apropriate.

assert is used a lot on the rest of qemu so I assumed it was sensible
to use it here.  Anyone who compiles qemu with -DNDEBUG deserves what
they get, I think.

Ian.