[Qemu-devel] [PATCH 1/6] usb-redir: Fix printing of device version

[Qemu-devel] [PATCH 1/6] usb-redir: Fix printing of device version

[Hans de Goede]

The device version is in bcd format, which requires some special handling to
print.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 usb-redir.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/usb-redir.c b/usb-redir.c
index 85f40d6..9b804e9 100644
--- a/usb-redir.c
+++ b/usb-redir.c
@@ -1076,8 +1076,10 @@ static void usbredir_device_connect(void *priv,
                                     usb_redir_cap_connect_device_version)) {
         INFO("attaching %s device %04x:%04x version %d.%d class %02x\n",
              speed, device_connect->vendor_id, device_connect->product_id,
-             device_connect->device_version_bcd >> 8,
-             device_connect->device_version_bcd & 0xff,
+             ((device_connect->device_version_bcd & 0xf000) >> 12) * 10 +
+             ((device_connect->device_version_bcd & 0x0f00) >>  8),
+             ((device_connect->device_version_bcd & 0x00f0) >>  4) * 10 +
+             ((device_connect->device_version_bcd & 0x000f) >>  0),
              device_connect->device_class);
     } else {
         INFO("attaching %s device %04x:%04x class %02x\n", speed,
-- 
1.7.7.6

[Qemu-devel] [PATCH 2/6] usb-redir: Always clear device state on filter reject

[Hans de Goede]

Always call usbredir_device_disconnect() when usbredir_check_filter() fails
to clean up all the device state (ie received endpoint info).

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 usb-redir.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/usb-redir.c b/usb-redir.c
index 9b804e9..fe3b0a3 100644
--- a/usb-redir.c
+++ b/usb-redir.c
@@ -985,7 +985,7 @@ static int usbredir_check_filter(USBRedirDevice *dev)
 {
     if (dev->interface_info.interface_count == 0) {
         ERROR("No interface info for device\n");
-        return -1;
+        goto error;
     }
 
     if (dev->filter_rules) {
@@ -993,7 +993,7 @@ static int usbredir_check_filter(USBRedirDevice *dev)
                                     usb_redir_cap_connect_device_version)) {
             ERROR("Device filter specified and peer does not have the "
                   "connect_device_version capability\n");
-            return -1;
+            goto error;
         }
 
         if (usbredirfilter_check(
@@ -1010,11 +1010,15 @@ static int usbredir_check_filter(USBRedirDevice *dev)
                 dev->device_info.product_id,
                 dev->device_info.device_version_bcd,
                 0) != 0) {
-            return -1;
+            goto error;
         }
     }
 
     return 0;
+
+error:
+    usbredir_device_disconnect(dev);
+    return -1;
 }
 
 /*
@@ -1140,7 +1144,6 @@ static void usbredir_interface_info(void *priv,
         if (usbredir_check_filter(dev)) {
             ERROR("Device no longer matches filter after interface info "
                   "change, disconnecting!\n");
-            usbredir_device_disconnect(dev);
         }
     }
 }
-- 
1.7.7.6

[Qemu-devel] [PATCH 3/6] usb-redir: Let the usb-host know about our device filtering

[Hans de Goede]

libusbredirparser-0.3.4 adds 2 new packets which allows us to notify
the usb-host:
-about the usb device filter we have (if any), so that it knows not the even
 try to redirect certain devices
-when we reject a device based on filtering (in case it tries anyways)

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 configure   |    2 +-
 usb-redir.c |   20 ++++++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletions(-)

diff --git a/configure b/configure
index c7e37df..a4848a4 100755
--- a/configure
+++ b/configure
@@ -2541,7 +2541,7 @@ fi
 
 # check for usbredirparser for usb network redirection support
 if test "$usb_redir" != "no" ; then
-    if $pkg_config --atleast-version=0.3.3 libusbredirparser >/dev/null 2>&1 ; then
+    if $pkg_config --atleast-version=0.3.4 libusbredirparser >/dev/null 2>&1 ; then
         usb_redir="yes"
         usb_redir_cflags=$($pkg_config --cflags libusbredirparser 2>/dev/null)
         usb_redir_libs=$($pkg_config --libs libusbredirparser 2>/dev/null)
diff --git a/usb-redir.c b/usb-redir.c
index fe3b0a3..d10d8de 100644
--- a/usb-redir.c
+++ b/usb-redir.c
@@ -106,6 +106,7 @@ struct AsyncURB {
     QTAILQ_ENTRY(AsyncURB)next;
 };
 
+static void usbredir_hello(void *priv, struct usb_redir_hello_header *h);
 static void usbredir_device_connect(void *priv,
     struct usb_redir_device_connect_header *device_connect);
 static void usbredir_device_disconnect(void *priv);
@@ -812,6 +813,7 @@ static void usbredir_open_close_bh(void *opaque)
         dev->parser->log_func = usbredir_log;
         dev->parser->read_func = usbredir_read;
         dev->parser->write_func = usbredir_write;
+        dev->parser->hello_func = usbredir_hello;
         dev->parser->device_connect_func = usbredir_device_connect;
         dev->parser->device_disconnect_func = usbredir_device_disconnect;
         dev->parser->interface_info_func = usbredir_interface_info;
@@ -830,6 +832,7 @@ static void usbredir_open_close_bh(void *opaque)
         dev->read_buf_size = 0;
 
         usbredirparser_caps_set_cap(caps, usb_redir_cap_connect_device_version);
+        usbredirparser_caps_set_cap(caps, usb_redir_cap_filter);
         usbredirparser_init(dev->parser, VERSION, caps, USB_REDIR_CAPS_SIZE, 0);
         usbredirparser_do_write(dev->parser);
     }
@@ -1018,6 +1021,10 @@ static int usbredir_check_filter(USBRedirDevice *dev)
 
 error:
     usbredir_device_disconnect(dev);
+    if (usbredirparser_peer_has_cap(dev->parser, usb_redir_cap_filter)) {
+        usbredirparser_send_filter_reject(dev->parser);
+        usbredirparser_do_write(dev->parser);
+    }
     return -1;
 }
 
@@ -1043,6 +1050,19 @@ static int usbredir_handle_status(USBRedirDevice *dev,
     }
 }
 
+static void usbredir_hello(void *priv, struct usb_redir_hello_header *h)
+{
+    USBRedirDevice *dev = priv;
+
+    /* Try to send the filter info now that we've the usb-host's caps */
+    if (usbredirparser_peer_has_cap(dev->parser, usb_redir_cap_filter) &&
+            dev->filter_rules) {
+        usbredirparser_send_filter_filter(dev->parser, dev->filter_rules,
+                                          dev->filter_rules_count);
+        usbredirparser_do_write(dev->parser);
+    }
+}
+
 static void usbredir_device_connect(void *priv,
     struct usb_redir_device_connect_header *device_connect)
 {
-- 
1.7.7.6

[Qemu-devel] [PATCH 4/6] usb-redir: Limit return values returned by iso packets

[Hans de Goede]

The usbredir protocol uses a status of usb_redir_stall to indicate that
an iso data stream has stopped (ie because the urbs failed on resubmit),
but iso packets should never return a result of USB_RET_STALL, since iso
endpoints cannot stall. So instead simply always return USB_RET_NAK on
iso stream errors.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 usb-redir.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/usb-redir.c b/usb-redir.c
index d10d8de..c76e55d 100644
--- a/usb-redir.c
+++ b/usb-redir.c
@@ -441,7 +441,7 @@ static int usbredir_handle_iso_data(USBRedirDevice *dev, USBPacket *p,
             /* Check iso_error for stream errors, otherwise its an underrun */
             status = dev->endpoint[EP2I(ep)].iso_error;
             dev->endpoint[EP2I(ep)].iso_error = 0;
-            return usbredir_handle_status(dev, status, 0);
+            return status ? USB_RET_NAK : 0;
         }
         DPRINTF2("iso-token-in ep %02X status %d len %d queue-size: %d\n", ep,
                  isop->status, isop->len, dev->endpoint[EP2I(ep)].bufpq_size);
@@ -449,7 +449,7 @@ static int usbredir_handle_iso_data(USBRedirDevice *dev, USBPacket *p,
         status = isop->status;
         if (status != usb_redir_success) {
             bufp_free(dev, isop, ep);
-            return usbredir_handle_status(dev, status, 0);
+            return USB_RET_NAK;
         }
 
         len = isop->len;
-- 
1.7.7.6

[Qemu-devel] [PATCH 5/6] usb-redir: Return USB_RET_NAK when we've no data for an interrupt endpoint

[Hans de Goede]

We should return USB_RET_NAK, rather then a 0 sized packet, when we've no data
for an interrupt IN endpoint.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 usb-redir.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/usb-redir.c b/usb-redir.c
index c76e55d..ea828a8 100644
--- a/usb-redir.c
+++ b/usb-redir.c
@@ -558,7 +558,9 @@ static int usbredir_handle_interrupt_data(USBRedirDevice *dev,
             /* Check interrupt_error for stream errors */
             status = dev->endpoint[EP2I(ep)].interrupt_error;
             dev->endpoint[EP2I(ep)].interrupt_error = 0;
-            return usbredir_handle_status(dev, status, 0);
+            if (status)
+                return usbredir_handle_status(dev, status, 0);
+            return USB_RET_NAK;
         }
         DPRINTF("interrupt-token-in ep %02X status %d len %d\n", ep,
                 intp->status, intp->len);
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 5/6] usb-redir: Return USB_RET_NAK when we've no data for an interrupt endpoint

[Gerd Hoffmann]

On 02/26/12 16:14, Hans de Goede wrote:
> +            if (status)
> +                return usbredir_handle_status(dev, status, 0);

Fails checkpatch.pl, braces needed here.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 5/6] usb-redir: Return USB_RET_NAK when we've no data for an interrupt endpoint

[Gerd Hoffmann]

On 02/27/12 12:45, Gerd Hoffmann wrote:
> On 02/26/12 16:14, Hans de Goede wrote:
>> +            if (status)
>> +                return usbredir_handle_status(dev, status, 0);
> 
> Fails checkpatch.pl, braces needed here.

Fixed & applied (all patches).

cheers,
  Gerd

[Qemu-devel] [PATCH 6/6] usb-ehci: Handle ISO packets failing with an error other then NAK

[Hans de Goede]

Before this patch the ehci code was not checking for any other errors other
then USB_RET_NAK. This causes 2 problems:
1) Other errors are not reported to the guest.
2) When transactions with the ITD_XACT_IOC bit set completing with another
   error would not result in USBSTS_INT getting set.

I hit this problem when unplugging devices while iso data was streaming from
the device to the guest. When this happens it takes a while for the guest to
process the unplugging and remove ISO transactions from the ehci schedule, in
the mean time these transactions would complete with a result of USB_RET_NODEV,
which was not handled. This lead to the Linux guest's usb subsystem "hanging",
that is it would no longer see new usb devices getting plugged in and running
for example lsusb would lead to a stuck (D state) lsusb process. This patch
fixes this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb-ehci.c |   22 +++++++++++++++++++---
 1 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index 69bcc4b..a6b6ae5 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -1512,11 +1512,27 @@ static int ehci_process_itd(EHCIState *ehci,
                     /* IN */
                     set_field(&itd->transact[i], ret, ITD_XACT_LENGTH);
                 }
-
-                if (itd->transact[i] & ITD_XACT_IOC) {
-                    ehci_record_interrupt(ehci, USBSTS_INT);
+            } else {
+                switch (ret) {
+                default:
+                    fprintf(stderr, "Unexpected iso usb result: %d\n", ret);
+                    /* Fall through */
+                case USB_RET_NODEV:
+                    /* 3.3.2: XACTERR is only allowed on IN transactions */
+                    if (dir) {
+                        itd->transact[i] |= ITD_XACT_XACTERR;
+                        ehci_record_interrupt(ehci, USBSTS_ERRINT);
+                    }
+                    break;
+                case USB_RET_BABBLE:
+                    itd->transact[i] |= ITD_XACT_BABBLE;
+                    ehci_record_interrupt(ehci, USBSTS_ERRINT);
+                    break;
                 }
             }
+            if (itd->transact[i] & ITD_XACT_IOC) {
+                ehci_record_interrupt(ehci, USBSTS_INT);
+            }
             itd->transact[i] &= ~ITD_XACT_ACTIVE;
         }
     }
-- 
1.7.7.6