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

[Yuri Benditovich]

On Wed, Nov 27, 2019 at 8:36 AM Markus Armbruster <address@hidden> wrote:
>
> 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"?
'"wakeup" is good but "wakeup=on" makes an impression that we add the
capability to the device even if it does not have one.
disable_wake? suppress_wake? clear_wake? wake_allowed?

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

Guest will see the device as one without 'remote wake' capability.
IMO, in the worst case this does not change anything, in the best case
this will suppress device power transition to D2 and the device will
work.
Including existing machine types.
Probably I did not understand the idea of 'compat property', can you
please provide an example of some existing compat property?
And, of course, we can keep existing behavior by default and advise to
turn this property on to make these devices work.
>