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

[PATCH v2 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).

Changes from v1: minor fixes per v1 review and checkpatch

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 | 20 ++++++++++++++++++++
 hw/usb/redirect.c    | 20 ++++++++++++++++++++
 hw/usb/trace-events  |  1 +
 4 files changed, 43 insertions(+)

-- 
2.17.1

[PATCH v2 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 | 20 ++++++++++++++++++++
 hw/usb/trace-events  |  1 +
 3 files changed, 22 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..00e0e36369 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,21 @@ 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)) {
+                trace_usb_host_remote_wakeup_removed(s->bus_num, s->addr);
+                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 +1614,8 @@ 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..1c24d82c09 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(int bus, int addr) "dev %d:%d"
-- 
2.17.1

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

[Eduardo Habkost]

On Tue, Dec 03, 2019 at 09:07:15PM +0200, Yuri Benditovich wrote:
> 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 | 20 ++++++++++++++++++++
>  hw/usb/trace-events  |  1 +
>  3 files changed, 22 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);

In case this doesn't get merged in QEMU 4.2.0, the patch needs to
be updated to change hw_compat_4_2 instead (after applying the
5.0 machine type patch[1]).

[1] https://patchew.org/QEMU/20191112104811.30323-1-cohuck@redhat.com/

-- 
Eduardo

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

[Yuri Benditovich]

On Tue, Dec 10, 2019 at 12:32 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> On Tue, Dec 03, 2019 at 09:07:15PM +0200, Yuri Benditovich wrote:
> > 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 | 20 ++++++++++++++++++++
> >  hw/usb/trace-events  |  1 +
> >  3 files changed, 22 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);
>
> In case this doesn't get merged in QEMU 4.2.0, the patch needs to
> be updated to change hw_compat_4_2 instead (after applying the
> 5.0 machine type patch[1]).

Of course I will resubmit if needed.

>
> [1] https://patchew.org/QEMU/20191112104811.30323-1-cohuck@redhat.com/
>
> --
> Eduardo
>

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

[Gerd Hoffmann]

> > >  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);
> >
> > In case this doesn't get merged in QEMU 4.2.0, the patch needs to
> > be updated to change hw_compat_4_2 instead (after applying the
> > 5.0 machine type patch[1]).
> 
> Of course I will resubmit if needed.

Ping.  Submission was too close to the release, so it'll go into 5.0.
Please resend with compat properties updated accordingly (otherwise the
patches are fine).

thanks,
  Gerd

[PATCH v2 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 | 20 ++++++++++++++++++++
 2 files changed, 21 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..b5c1558687 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,8 @@ 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