Re: [PATCH] usbredir: remove 'remote wake' capability from configuration descriptor

[Markus Armbruster]

Yuri Benditovich <address@hidden> writes:

> 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. Setting global property
> 'usb-redir.nowake=off' keeps 'remote wake' as is.

"usb-redir.nowake=off" is a double negation.  Gets weirder when dusted
with syntactic sugar: "usb-redir.nonowake".  Can we think of a better
name?  Naming is hard...  What about "usb-redir.wakeup=on"?

> Signed-off-by: Yuri Benditovich <address@hidden>
> ---
>  hw/usb/redirect.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index e0f5ca6f81..e95898fe80 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("nowake", USBRedirDevice, suppress_remote_wake, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };

The default is .nowake=on.  Is that a guest-visible change?  Do we need
compat properties to keep it off for existing machine types?