[PATCH 0/2] Remove 'remote wakeup' flag from USB config descriptor

[PATCH 0/2] Remove 'remote wakeup' flag from USB config descriptor

[Yuri Benditovich]

This series of patches addresses possible functional problem of USB
devices with 'remote wakeup' capability, redirected to Windows VM
(local redirection using libusb or spice redirection using usbredir).

Yuri Benditovich (2):
  usb-host: remove 'remote wakeup' flag from configuration descriptor
  usb-redir: remove 'remote wakeup' flag from configuration descriptor

 hw/core/machine.c    |  2 ++
 hw/usb/host-libusb.c | 18 ++++++++++++++++++
 hw/usb/redirect.c    | 19 +++++++++++++++++++
 hw/usb/trace-events  |  1 +
 4 files changed, 40 insertions(+)

-- 
2.17.1

[PATCH 1/2] usb-host: remove 'remote wakeup' flag from configuration descriptor

[Yuri Benditovich]

If the redirected device has this capability, Windows guest may
place the device into D2 and expect it to wake when the device
becomes active, but this will never happen. For example, when
internal Bluetooth adapter is redirected, keyboards and mice
connected to it do not work. Current commit removes this
capability (starting from machine 4.2)
Set 'usb-host.suppress-remote-wake' property to 'off' to keep
'remote wake' as is or to 'on' to remove 'remote wake' on
4.1 or earlier.

Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
---
 hw/core/machine.c    |  1 +
 hw/usb/host-libusb.c | 18 ++++++++++++++++++
 hw/usb/trace-events  |  1 +
 3 files changed, 20 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 1689ad3bf8..8c0eaad091 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -29,6 +29,7 @@
 
 GlobalProperty hw_compat_4_1[] = {
     { "virtio-pci", "x-pcie-flr-init", "off" },
+    { "usb-host", "suppress-remote-wake", "off" },
 };
 const size_t hw_compat_4_1_len = G_N_ELEMENTS(hw_compat_4_1);
 
diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index fcf48c0193..77c2720ec6 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -88,6 +88,7 @@ struct USBHostDevice {
     bool                             needs_autoscan;
     bool                             allow_one_guest_reset;
     bool                             allow_all_guest_resets;
+    bool                             suppress_remote_wake;
 
     /* state */
     QTAILQ_ENTRY(USBHostDevice)      next;
@@ -386,6 +387,8 @@ static void LIBUSB_CALL usb_host_req_complete_ctrl(struct libusb_transfer *xfer)
     r->p->status = status_map[xfer->status];
     r->p->actual_length = xfer->actual_length;
     if (r->in && xfer->actual_length) {
+        USBDevice *udev = USB_DEVICE(s);
+        struct libusb_config_descriptor *conf = (void *)r->cbuf;
         memcpy(r->cbuf, r->buffer + 8, xfer->actual_length);
 
         /* Fix up USB-3 ep0 maxpacket size to allow superspeed connected devices
@@ -394,6 +397,20 @@ static void LIBUSB_CALL usb_host_req_complete_ctrl(struct libusb_transfer *xfer)
             r->cbuf[7] == 9) {
             r->cbuf[7] = 64;
         }
+        /* If this is GET_DESCRIPTOR request for configuration descriptor,
+         * remove 'remote wakeup' flag from it to prevent idle power down
+         * in Windows guest */
+        if (s->suppress_remote_wake &&
+            udev->setup_buf[0] == USB_DIR_IN &&
+            udev->setup_buf[1] == USB_REQ_GET_DESCRIPTOR &&
+            udev->setup_buf[3] == USB_DT_CONFIG && udev->setup_buf[2] == 0 &&
+            xfer->actual_length > offsetof(struct libusb_config_descriptor, bmAttributes) &&
+            (conf->bmAttributes & USB_CFG_ATT_WAKEUP)) {
+                struct libusb_device_descriptor desc;
+                libusb_get_device_descriptor(s->dev, &desc);
+                trace_usb_host_remote_wakeup_removed(desc.idVendor, desc.idProduct);
+                conf->bmAttributes &= ~USB_CFG_ATT_WAKEUP;
+        }
     }
     trace_usb_host_req_complete(s->bus_num, s->addr, r->p,
                                 r->p->status, r->p->actual_length);
@@ -1596,6 +1613,7 @@ static Property usb_host_dev_properties[] = {
                        LIBUSB_LOG_LEVEL_WARNING),
     DEFINE_PROP_BIT("pipeline",    USBHostDevice, options,
                     USB_HOST_OPT_PIPELINE, true),
+    DEFINE_PROP_BOOL("suppress-remote-wake", USBHostDevice, suppress_remote_wake, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/usb/trace-events b/hw/usb/trace-events
index 2d3713351c..6f87b5262a 100644
--- a/hw/usb/trace-events
+++ b/hw/usb/trace-events
@@ -266,3 +266,4 @@ usb_host_parse_config(int bus, int addr, int value, int active) "dev %d:%d, valu
 usb_host_parse_interface(int bus, int addr, int num, int alt, int active) "dev %d:%d, num %d, alt %d, active %d"
 usb_host_parse_endpoint(int bus, int addr, int ep, const char *dir, const char *type, int active) "dev %d:%d, ep %d, %s, %s, active %d"
 usb_host_parse_error(int bus, int addr, const char *errmsg) "dev %d:%d, msg %s"
+usb_host_remote_wakeup_removed(uint16_t vid, uint16_t pid) "dev %04x:%04x"
-- 
2.17.1

[PATCH 2/2] usb-redir: remove 'remote wakeup' flag from configuration descriptor

[Yuri Benditovich]

If the redirected device has this capability, Windows guest may
place the device into D2 and expect it to wake when the device
becomes active, but this will never happen. For example, when
internal Bluetooth adapter is redirected, keyboards and mice
connected to it do not work. Current commit removes this
capability (starting from machine 4.2)
Set 'usb-redir.suppress-remote-wake' property to 'off' to keep
'remote wake' as is or to 'on' to remove 'remote wake' on
4.1 or earlier.

Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
---
 hw/core/machine.c |  1 +
 hw/usb/redirect.c | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 8c0eaad091..44408ff87c 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -30,6 +30,7 @@
 GlobalProperty hw_compat_4_1[] = {
     { "virtio-pci", "x-pcie-flr-init", "off" },
     { "usb-host", "suppress-remote-wake", "off" },
+    { "usb-redir", "suppress-remote-wake", "off" },
 };
 const size_t hw_compat_4_1_len = G_N_ELEMENTS(hw_compat_4_1);
 
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index e0f5ca6f81..7cc9c2aa00 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -113,6 +113,7 @@ struct USBRedirDevice {
     /* Properties */
     CharBackend cs;
     bool enable_streams;
+    bool suppress_remote_wake;
     uint8_t debug;
     int32_t bootindex;
     char *filter_str;
@@ -1989,6 +1990,23 @@ static void usbredir_control_packet(void *priv, uint64_t id,
             memcpy(dev->dev.data_buf, data, data_len);
         }
         p->actual_length = len;
+        /*
+         * If this is GET_DESCRIPTOR request for configuration descriptor,
+         * remove 'remote wakeup' flag from it to prevent idle power down
+         * in Windows guest
+         */
+        if (dev->suppress_remote_wake &&
+            control_packet->requesttype == USB_DIR_IN &&
+            control_packet->request == USB_REQ_GET_DESCRIPTOR &&
+            control_packet->value == (USB_DT_CONFIG << 8) &&
+            control_packet->index == 0 &&
+            /* bmAttributes field of config descriptor */
+            len > 7 && (dev->dev.data_buf[7] & USB_CFG_ATT_WAKEUP)) {
+                DPRINTF("Removed remote wake %04X:%04X\n",
+                    dev->device_info.vendor_id,
+                    dev->device_info.product_id);
+                dev->dev.data_buf[7] &= ~USB_CFG_ATT_WAKEUP;
+            }
         usb_generic_async_ctrl_complete(&dev->dev, p);
     }
     free(data);
@@ -2530,6 +2548,7 @@ static Property usbredir_properties[] = {
     DEFINE_PROP_UINT8("debug", USBRedirDevice, debug, usbredirparser_warning),
     DEFINE_PROP_STRING("filter", USBRedirDevice, filter_str),
     DEFINE_PROP_BOOL("streams", USBRedirDevice, enable_streams, true),
+    DEFINE_PROP_BOOL("suppress-remote-wake", USBRedirDevice, suppress_remote_wake, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.17.1

Re: [PATCH 0/2] Remove 'remote wakeup' flag from USB config descriptor

[no-reply]

Patchew URL: https://patchew.org/QEMU/20191202123430.7125-1-yuri.benditovich@daynix.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH 0/2] Remove 'remote wakeup' flag from USB config descriptor
Type: series
Message-id: 20191202123430.7125-1-yuri.benditovich@daynix.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20191202060806.77968-1-david@gibson.dropbear.id.au -> patchew/20191202060806.77968-1-david@gibson.dropbear.id.au
Switched to a new branch 'test'
91bc743 usb-redir: remove 'remote wakeup' flag from configuration descriptor
e3a79d7 usb-host: remove 'remote wakeup' flag from configuration descriptor

=== OUTPUT BEGIN ===
1/2 Checking commit e3a79d76824a (usb-host: remove 'remote wakeup' flag from configuration descriptor)
WARNING: Block comments use a leading /* on a separate line
#57: FILE: hw/usb/host-libusb.c:400:
+        /* If this is GET_DESCRIPTOR request for configuration descriptor,

WARNING: Block comments use a trailing */ on a separate line
#59: FILE: hw/usb/host-libusb.c:402:
+         * in Windows guest */

ERROR: line over 90 characters
#64: FILE: hw/usb/host-libusb.c:407:
+            xfer->actual_length > offsetof(struct libusb_config_descriptor, bmAttributes) &&

WARNING: line over 80 characters
#68: FILE: hw/usb/host-libusb.c:411:
+                trace_usb_host_remote_wakeup_removed(desc.idVendor, desc.idProduct);

WARNING: line over 80 characters
#78: FILE: hw/usb/host-libusb.c:1616:
+    DEFINE_PROP_BOOL("suppress-remote-wake", USBHostDevice, suppress_remote_wake, true),

total: 1 errors, 4 warnings, 53 lines checked

Patch 1/2 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/2 Checking commit 91bc743fae4b (usb-redir: remove 'remote wakeup' flag from configuration descriptor)
WARNING: line over 80 characters
#72: FILE: hw/usb/redirect.c:2551:
+    DEFINE_PROP_BOOL("suppress-remote-wake", USBRedirDevice, suppress_remote_wake, true),

total: 0 errors, 1 warnings, 44 lines checked

Patch 2/2 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20191202123430.7125-1-yuri.benditovich@daynix.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 1/2] usb-host: remove 'remote wakeup' flag from configuration descriptor

[Gerd Hoffmann]

  Hi,

> +        /* If this is GET_DESCRIPTOR request for configuration descriptor,
> +         * remove 'remote wakeup' flag from it to prevent idle power down
> +         * in Windows guest */

scripts/checkpatch.pl complains about that, please fix (and also the
other checkpatch warnings).

> +        if (s->suppress_remote_wake &&
> +            udev->setup_buf[0] == USB_DIR_IN &&
> +            udev->setup_buf[1] == USB_REQ_GET_DESCRIPTOR &&
> +            udev->setup_buf[3] == USB_DT_CONFIG && udev->setup_buf[2] == 0 &&
> +            xfer->actual_length > offsetof(struct libusb_config_descriptor, bmAttributes) &&
> +            (conf->bmAttributes & USB_CFG_ATT_WAKEUP)) {
> +                struct libusb_device_descriptor desc;
> +                libusb_get_device_descriptor(s->dev, &desc);
> +                trace_usb_host_remote_wakeup_removed(desc.idVendor, desc.idProduct);

Please use s->bus_num and s->addr to identify the device, like all the
other trace points do.  I don't think there is a need to log
desc.idVendor and desc.idProduct here.


Otherwise the patch looks fine.

cheers,
  Gerd

Re: [PATCH 2/2] usb-redir: remove 'remote wakeup' flag from configuration descriptor

[Gerd Hoffmann]

  Hi,

> +        /*
> +         * If this is GET_DESCRIPTOR request for configuration descriptor,
> +         * remove 'remote wakeup' flag from it to prevent idle power down
> +         * in Windows guest
> +         */
> +        if (dev->suppress_remote_wake &&
> +            control_packet->requesttype == USB_DIR_IN &&
> +            control_packet->request == USB_REQ_GET_DESCRIPTOR &&
> +            control_packet->value == (USB_DT_CONFIG << 8) &&
> +            control_packet->index == 0 &&
> +            /* bmAttributes field of config descriptor */
> +            len > 7 && (dev->dev.data_buf[7] & USB_CFG_ATT_WAKEUP)) {
> +                DPRINTF("Removed remote wake %04X:%04X\n",
> +                    dev->device_info.vendor_id,
> +                    dev->device_info.product_id);
> +                dev->dev.data_buf[7] &= ~USB_CFG_ATT_WAKEUP;
> +            }

Hmm, not much opportunity to factor out stuff to share with usb-host.
Ok then.

I think checkpatch has complains for this too, otherwise it looks fine.

cheers,
  Gerd