[Qemu-devel] [PATCH 0/3] usb-linux do not send unnecessary GET_CONFIGURATION (v2)

[Qemu-devel] [PATCH 0/3] usb-linux do not send unnecessary GET_CONFIGURATION (v2)

[Hans de Goede]

Hi All,

This is version 2 of this patchset, it changes the first patch in the
series to not use the devpath sysfs attribute as that is not available on
kernels < 2.6.33

The other 2 patches are unchanged.

For those just tuning in here is the summary of the patchset again:
I've begun looking into the current usb redirection support in qemu.

It failed at the first device I threw at it (a usb keychain picture frame),
the problem is that this (el cheapo) device does not seem to grok
GET_CONFIGURATION. This patch sets makes this device work (and stops qemu
from unnecessary sending a GET_CONFIGURATION ctrl msg in general) by
reading this value directly from sysfs.

Regards,

Hans

[Qemu-devel] [PATCH 1/3] usb-linux: Store devpath into USBHostDevice when usb_fs_type == USB_FS_SYS

[Hans de Goede]

This allows us to recreate the sysfspath used during scanning later
(which will be used in a later patch in this series).
---
 usb-linux.c |   26 +++++++++++++++-----------
 1 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index c3c38ec..0b154c2 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -62,8 +62,8 @@ struct usb_ctrlrequest {
     uint16_t wLength;
 };
 
-typedef int USBScanFunc(void *opaque, int bus_num, int addr, int class_id,
-                        int vendor_id, int product_id,
+typedef int USBScanFunc(void *opaque, int bus_num, int addr, int devpath,
+                        int class_id, int vendor_id, int product_id,
                         const char *product_name, int speed);
 
 //#define DEBUG
@@ -141,6 +141,7 @@ typedef struct USBHostDevice {
     /* Host side address */
     int bus_num;
     int addr;
+    int devpath;
     struct USBAutoFilter match;
 
     QTAILQ_ENTRY(USBHostDevice) next;
@@ -885,7 +886,7 @@ static int usb_linux_update_endp_table(USBHostDevice *s)
 }
 
 static int usb_host_open(USBHostDevice *dev, int bus_num,
-                         int addr, const char *prod_name)
+                         int addr, int devpath, const char *prod_name)
 {
     int fd = -1, ret;
     struct usbdevfs_connectinfo ci;
@@ -911,6 +912,7 @@ static int usb_host_open(USBHostDevice *dev, int bus_num,
 
     dev->bus_num = bus_num;
     dev->addr = addr;
+    dev->devpath = devpath;
     dev->fd = fd;
 
     /* read the device description */
@@ -1173,7 +1175,7 @@ static int usb_host_scan_dev(void *opaque, USBScanFunc *func)
         if (line[0] == 'T' && line[1] == ':') {
             if (device_count && (vendor_id || product_id)) {
                 /* New device.  Add the previously discovered device.  */
-                ret = func(opaque, bus_num, addr, class_id, vendor_id,
+                ret = func(opaque, bus_num, addr, 0, class_id, vendor_id,
                            product_id, product_name, speed);
                 if (ret) {
                     goto the_end;
@@ -1226,7 +1228,7 @@ static int usb_host_scan_dev(void *opaque, USBScanFunc *func)
     }
     if (device_count && (vendor_id || product_id)) {
         /* Add the last device.  */
-        ret = func(opaque, bus_num, addr, class_id, vendor_id,
+        ret = func(opaque, bus_num, addr, 0, class_id, vendor_id,
                    product_id, product_name, speed);
     }
  the_end:
@@ -1275,7 +1277,7 @@ static int usb_host_scan_sys(void *opaque, USBScanFunc *func)
 {
     DIR *dir = NULL;
     char line[1024];
-    int bus_num, addr, speed, class_id, product_id, vendor_id;
+    int bus_num, addr, devpath, speed, class_id, product_id, vendor_id;
     int ret = 0;
     char product_name[512];
     struct dirent *de;
@@ -1292,7 +1294,9 @@ static int usb_host_scan_sys(void *opaque, USBScanFunc *func)
             if (!strncmp(de->d_name, "usb", 3)) {
                 tmpstr += 3;
             }
-            bus_num = atoi(tmpstr);
+            if (sscanf(tmpstr, "%d-%d", &bus_num, &devpath) < 1) {
+                goto the_end;
+            }
 
             if (!usb_host_read_file(line, sizeof(line), "devnum", de->d_name)) {
                 goto the_end;
@@ -1343,7 +1347,7 @@ static int usb_host_scan_sys(void *opaque, USBScanFunc *func)
                 speed = USB_SPEED_FULL;
             }
 
-            ret = func(opaque, bus_num, addr, class_id, vendor_id,
+            ret = func(opaque, bus_num, addr, devpath, class_id, vendor_id,
                        product_id, product_name, speed);
             if (ret) {
                 goto the_end;
@@ -1434,7 +1438,7 @@ static int usb_host_scan(void *opaque, USBScanFunc *func)
 
 static QEMUTimer *usb_auto_timer;
 
-static int usb_host_auto_scan(void *opaque, int bus_num, int addr,
+static int usb_host_auto_scan(void *opaque, int bus_num, int addr, int devpath,
                               int class_id, int vendor_id, int product_id,
                               const char *product_name, int speed)
 {
@@ -1470,7 +1474,7 @@ static int usb_host_auto_scan(void *opaque, int bus_num, int addr,
         }
         DPRINTF("husb: auto open: bus_num %d addr %d\n", bus_num, addr);
 
-        usb_host_open(s, bus_num, addr, product_name);
+        usb_host_open(s, bus_num, addr, devpath, product_name);
     }
 
     return 0;
@@ -1630,7 +1634,7 @@ static void usb_info_device(Monitor *mon, int bus_num, int addr, int class_id,
 }
 
 static int usb_host_info_device(void *opaque, int bus_num, int addr,
-                                int class_id,
+                                int devpath, int class_id,
                                 int vendor_id, int product_id,
                                 const char *product_name,
                                 int speed)
-- 
1.7.3.2

[Qemu-devel] [PATCH 2/3] usb-linux: introduce a usb_linux_get_configuration function

[Hans de Goede]

The next patch in this series introduces multiple ways to get the
configuration dependent upon usb_fs_type, it is cleaner to put this
into its own function.
---
 usb-linux.c |   30 ++++++++++++++++++++++--------
 1 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index 0b154c2..111fe1c 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -775,13 +775,11 @@ static int usb_host_handle_packet(USBDevice *s, USBPacket *p)
     }
 }
 
-/* returns 1 on problem encountered or 0 for success */
-static int usb_linux_update_endp_table(USBHostDevice *s)
+static int usb_linux_get_configuration(USBHostDevice *s)
 {
-    uint8_t *descriptors;
-    uint8_t devep, type, configuration, alt_interface;
+    uint8_t configuration;
     struct usb_ctrltransfer ct;
-    int interface, ret, length, i;
+    int ret;
 
     ct.bRequestType = USB_DIR_IN;
     ct.bRequest = USB_REQ_GET_CONFIGURATION;
@@ -793,15 +791,31 @@ static int usb_linux_update_endp_table(USBHostDevice *s)
 
     ret = ioctl(s->fd, USBDEVFS_CONTROL, &ct);
     if (ret < 0) {
-        perror("usb_linux_update_endp_table");
-        return 1;
+        perror("usb_linux_get_configuration");
+        return -1;
     }
 
     /* in address state */
     if (configuration == 0) {
-        return 1;
+        return -1;
     }
 
+    return configuration;
+}
+
+/* returns 1 on problem encountered or 0 for success */
+static int usb_linux_update_endp_table(USBHostDevice *s)
+{
+    uint8_t *descriptors;
+    uint8_t devep, type, configuration, alt_interface;
+    struct usb_ctrltransfer ct;
+    int interface, ret, length, i;
+
+    i = usb_linux_get_configuration(s);
+    if (i < 0)
+        return 1;
+    configuration = i;
+
     /* get the desired configuration, interface, and endpoint descriptors
      * from device description */
     descriptors = &s->descr[18];
-- 
1.7.3.2

[Qemu-devel] [PATCH 3/3] usb-linux: Get the active configuration from sysfs rather then asking the dev

[Hans de Goede]

Some devices seem to choke on receiving a USB_REQ_GET_CONFIGURATION ctrl msg
(witnessed with a digital picture frame usb id 1908:1320).
When usb_fs_type == USB_FS_SYS, the active configuration can be read directly
from sysfs, which allows using this device through qemu's usb redirection.
More in general it seems a good idea to not send needless control msg's to
devices, esp. as the code in question is called every time a set_interface
is done. Which happens multiple times during virtual machine startup, and
when device drivers are activating the usb device.
---
 usb-linux.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index 111fe1c..ccf7073 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -152,6 +152,8 @@ static QTAILQ_HEAD(, USBHostDevice) hostdevs = QTAILQ_HEAD_INITIALIZER(hostdevs)
 static int usb_host_close(USBHostDevice *dev);
 static int parse_filter(const char *spec, struct USBAutoFilter *f);
 static void usb_host_auto_check(void *unused);
+static int usb_host_read_file(char *line, size_t line_size,
+                            const char *device_file, const char *device_name);
 
 static int is_isoc(USBHostDevice *s, int ep)
 {
@@ -781,6 +783,23 @@ static int usb_linux_get_configuration(USBHostDevice *s)
     struct usb_ctrltransfer ct;
     int ret;
 
+    if (usb_fs_type == USB_FS_SYS) {
+        char device_name[32], line[1024];
+        int configuration;
+
+        sprintf(device_name, "%d-%d", s->bus_num, s->devpath);
+
+        if (!usb_host_read_file(line, sizeof(line), "bConfigurationValue",
+                                device_name)) {
+            goto usbdevfs;
+        }
+        if (sscanf(line, "%d", &configuration) != 1) {
+            goto usbdevfs;
+        }
+        return configuration;
+    }
+
+usbdevfs:
     ct.bRequestType = USB_DIR_IN;
     ct.bRequest = USB_REQ_GET_CONFIGURATION;
     ct.wValue = 0;
-- 
1.7.3.2

Re: [Qemu-devel] [PATCH 1/3] usb-linux: Store devpath into USBHostDevice when usb_fs_type == USB_FS_SYS

[Anthony Liguori]

On 11/10/2010 03:06 AM, Hans de Goede wrote:
> This allows us to recreate the sysfspath used during scanning later
> (which will be used in a later patch in this series).
>    

Applied all three.  Thanks.

Regards,

Anthony Liguori
> ---
>   usb-linux.c |   26 +++++++++++++++-----------
>   1 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/usb-linux.c b/usb-linux.c
> index c3c38ec..0b154c2 100644
> --- a/usb-linux.c
> +++ b/usb-linux.c
> @@ -62,8 +62,8 @@ struct usb_ctrlrequest {
>       uint16_t wLength;
>   };
>
> -typedef int USBScanFunc(void *opaque, int bus_num, int addr, int class_id,
> -                        int vendor_id, int product_id,
> +typedef int USBScanFunc(void *opaque, int bus_num, int addr, int devpath,
> +                        int class_id, int vendor_id, int product_id,
>                           const char *product_name, int speed);
>
>   //#define DEBUG
> @@ -141,6 +141,7 @@ typedef struct USBHostDevice {
>       /* Host side address */
>       int bus_num;
>       int addr;
> +    int devpath;
>       struct USBAutoFilter match;
>
>       QTAILQ_ENTRY(USBHostDevice) next;
> @@ -885,7 +886,7 @@ static int usb_linux_update_endp_table(USBHostDevice *s)
>   }
>
>   static int usb_host_open(USBHostDevice *dev, int bus_num,
> -                         int addr, const char *prod_name)
> +                         int addr, int devpath, const char *prod_name)
>   {
>       int fd = -1, ret;
>       struct usbdevfs_connectinfo ci;
> @@ -911,6 +912,7 @@ static int usb_host_open(USBHostDevice *dev, int bus_num,
>
>       dev->bus_num = bus_num;
>       dev->addr = addr;
> +    dev->devpath = devpath;
>       dev->fd = fd;
>
>       /* read the device description */
> @@ -1173,7 +1175,7 @@ static int usb_host_scan_dev(void *opaque, USBScanFunc *func)
>           if (line[0] == 'T'&&  line[1] == ':') {
>               if (device_count&&  (vendor_id || product_id)) {
>                   /* New device.  Add the previously discovered device.  */
> -                ret = func(opaque, bus_num, addr, class_id, vendor_id,
> +                ret = func(opaque, bus_num, addr, 0, class_id, vendor_id,
>                              product_id, product_name, speed);
>                   if (ret) {
>                       goto the_end;
> @@ -1226,7 +1228,7 @@ static int usb_host_scan_dev(void *opaque, USBScanFunc *func)
>       }
>       if (device_count&&  (vendor_id || product_id)) {
>           /* Add the last device.  */
> -        ret = func(opaque, bus_num, addr, class_id, vendor_id,
> +        ret = func(opaque, bus_num, addr, 0, class_id, vendor_id,
>                      product_id, product_name, speed);
>       }
>    the_end:
> @@ -1275,7 +1277,7 @@ static int usb_host_scan_sys(void *opaque, USBScanFunc *func)
>   {
>       DIR *dir = NULL;
>       char line[1024];
> -    int bus_num, addr, speed, class_id, product_id, vendor_id;
> +    int bus_num, addr, devpath, speed, class_id, product_id, vendor_id;
>       int ret = 0;
>       char product_name[512];
>       struct dirent *de;
> @@ -1292,7 +1294,9 @@ static int usb_host_scan_sys(void *opaque, USBScanFunc *func)
>               if (!strncmp(de->d_name, "usb", 3)) {
>                   tmpstr += 3;
>               }
> -            bus_num = atoi(tmpstr);
> +            if (sscanf(tmpstr, "%d-%d",&bus_num,&devpath)<  1) {
> +                goto the_end;
> +            }
>
>               if (!usb_host_read_file(line, sizeof(line), "devnum", de->d_name)) {
>                   goto the_end;
> @@ -1343,7 +1347,7 @@ static int usb_host_scan_sys(void *opaque, USBScanFunc *func)
>                   speed = USB_SPEED_FULL;
>               }
>
> -            ret = func(opaque, bus_num, addr, class_id, vendor_id,
> +            ret = func(opaque, bus_num, addr, devpath, class_id, vendor_id,
>                          product_id, product_name, speed);
>               if (ret) {
>                   goto the_end;
> @@ -1434,7 +1438,7 @@ static int usb_host_scan(void *opaque, USBScanFunc *func)
>
>   static QEMUTimer *usb_auto_timer;
>
> -static int usb_host_auto_scan(void *opaque, int bus_num, int addr,
> +static int usb_host_auto_scan(void *opaque, int bus_num, int addr, int devpath,
>                                 int class_id, int vendor_id, int product_id,
>                                 const char *product_name, int speed)
>   {
> @@ -1470,7 +1474,7 @@ static int usb_host_auto_scan(void *opaque, int bus_num, int addr,
>           }
>           DPRINTF("husb: auto open: bus_num %d addr %d\n", bus_num, addr);
>
> -        usb_host_open(s, bus_num, addr, product_name);
> +        usb_host_open(s, bus_num, addr, devpath, product_name);
>       }
>
>       return 0;
> @@ -1630,7 +1634,7 @@ static void usb_info_device(Monitor *mon, int bus_num, int addr, int class_id,
>   }
>
>   static int usb_host_info_device(void *opaque, int bus_num, int addr,
> -                                int class_id,
> +                                int devpath, int class_id,
>                                   int vendor_id, int product_id,
>                                   const char *product_name,
>                                   int speed)
>