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

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

[Hans de Goede]

Hi All,

First a short self-into. I've been a FOSS developer for 10+ years,
working on a large variety of projects. Most relevant for my appearing
here on the qemu list is my experience in reverse engineering and writing
usb webcam drivers for the Linux kernel and libgphoto2 camlibs for various
small usb keychain picture frames (with proprietary protocols).

I've been a Red Hat employee for 2 years now and in August this year I've
joined Red Hat's SPICE team. I'm currently tasked with looking into /
developing an usb redirection solution for SPICE. So for starters 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

p.s.

While looking at the code I also noticed that the ep numeration in usb-linux
seems completely wrong for multifunction devices. I'll go test that and fix
it (assuming I'm right about it being wrong) tomorrow.

[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 |   29 +++++++++++++++++++----------
 1 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index c3c38ec..61e8ec6 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;
@@ -1300,6 +1302,13 @@ static int usb_host_scan_sys(void *opaque, USBScanFunc *func)
             if (sscanf(line, "%d", &addr) != 1) {
                 goto the_end;
             }
+            if (!usb_host_read_file(line, sizeof(line), "devpath",
+                                    de->d_name)) {
+                goto the_end;
+            }
+            if (sscanf(line, "%d", &devpath) != 1) {
+                goto the_end;
+            }
             if (!usb_host_read_file(line, sizeof(line), "bDeviceClass",
                                     de->d_name)) {
                 goto the_end;
@@ -1343,7 +1352,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 +1443,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 +1479,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 +1639,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 61e8ec6..0a46692 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 0a46692..25ba5b7 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

[Qemu-devel] Re: [PATCH 0/3] usb-linux do not send unnecessary GET_CONFIGURATION

[Gerd Hoffmann]

   Hi,

> 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.

Patches look good to me.

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

cheers,
   Gerd

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

[Yuriy Kaminskiy]

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).
> ---
>  usb-linux.c |   29 +++++++++++++++++++----------
>  1 files changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/usb-linux.c b/usb-linux.c
> index c3c38ec..61e8ec6 100644
> --- a/usb-linux.c
> +++ b/usb-linux.c
[...]
> @@ -1300,6 +1302,13 @@ static int usb_host_scan_sys(void *opaque, USBScanFunc *func)
>              if (sscanf(line, "%d", &addr) != 1) {
>                  goto the_end;
>              }
> +            if (!usb_host_read_file(line, sizeof(line), "devpath",
> +                                    de->d_name)) {
> +                goto the_end;
> +            }

FYI: "devpath" attribute was introduced in 2.6.33 kernel.

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

[Hans de Goede]

Hi,

On 11/09/2010 09:43 PM, Yuriy Kaminskiy wrote:
> 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).
>> ---
>>   usb-linux.c |   29 +++++++++++++++++++----------
>>   1 files changed, 19 insertions(+), 10 deletions(-)
>>
>> diff --git a/usb-linux.c b/usb-linux.c
>> index c3c38ec..61e8ec6 100644
>> --- a/usb-linux.c
>> +++ b/usb-linux.c
> [...]
>> @@ -1300,6 +1302,13 @@ static int usb_host_scan_sys(void *opaque, USBScanFunc *func)
>>               if (sscanf(line, "%d",&addr) != 1) {
>>                   goto the_end;
>>               }
>> +            if (!usb_host_read_file(line, sizeof(line), "devpath",
>> +                                    de->d_name)) {
>> +                goto the_end;
>> +            }
>
> FYI: "devpath" attribute was introduced in 2.6.33 kernel.
>

Ah I did not know that, well the same information is available in de->d_name, and has
been so for much longer. So I'll do a new version getting it from de->d_name instead.

Thanks,

Hans