Re: [Qemu-devel] [PULL 1/8] usb: assign unique serial numbers to hid devices

[Dr. David Alan Gilbert]

* Gerd Hoffmann (address@hidden) wrote:
> Windows guests have trouble dealing with usb devices having identical
> serial numbers.  So, assign unique serial numbers to usb hid devices.
> All other usb devices have this already.
> 
> In the past the fixed serial number has been used to indicate working
> remote setup to linux guests.  Here is a bit of history:
> 
>  * First there was nothing.
>  * Then I added a rule to udev checking for serial == 42.
>    (this is in rhel-6).
>  * Then systemd + udev merged.
>  * Then I changed the rule to check for serial != 1 instead, so we can
>    use any serial but "1" which is the one the old broken devices had
>    (this is in rhel-7).  March 2014 in upstream systemd.
>  * Then all usb power management rules where dropped from systemd (June
>    2015).  Which I figured today (Sept 2018), after wondering that the
>    rules are gone in fedora 28.
> 
> So, three years ago the serial number check was dropped upstream, yet I
> hav't seen a single report about autosuspend issues (or cpu usage for
> usb emulation going up, which is the typical symtom).
> 
> So I figured I can stop worring that changing the serial number will
> break things and just do it.
> 
> And even if it turns out autosuspend is still an issue:  I think
> meanwhile we can really stop worrying about guests running in old qemu
> versions with broken usb suspend (fixed in 0.13 !).  If needed we can
> enable autosuspend unconditionally in guests.
> 
> Signed-off-by: Gerd Hoffmann <address@hidden>
> Message-id: address@hidden
> ---
>  hw/core/machine.c |  3 +++
>  hw/usb/dev-hid.c  | 26 +++++++++++++++-----------
>  2 files changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 2629515363..077fbd182a 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -30,6 +30,9 @@ GlobalProperty hw_compat_3_1[] = {
>      { "memory-backend-memfd", "x-use-canonical-path-for-ramblock-id", "true" 
> },
>      { "tpm-crb", "ppi", "false" },
>      { "tpm-tis", "ppi", "false" },
> +    { "usb-kbd", "serial", "42" },
> +    { "usb-mouse", "serial", "42" },
> +    { "usb-kbd", "serial", "42" },

Hi Gerd,
  There's a copy-pasteism there that happened when you squashed
it down to the new format; you've got the usb-kbd twice
as opposed to having the usb-tablet in there.

Hmm, now how do we fix that? That means if we fix that
now then the usb 3-1 machine type in 4.1 would be the same as
3.1 but different from 4.0; which I suspect is the right fix
at this time.

Dave

>  };
>  const size_t hw_compat_3_1_len = G_N_ELEMENTS(hw_compat_3_1);
>  
> diff --git a/hw/usb/dev-hid.c b/hw/usb/dev-hid.c
> index 90cd745f06..f9ea3033a1 100644
> --- a/hw/usb/dev-hid.c
> +++ b/hw/usb/dev-hid.c
> @@ -61,10 +61,13 @@ enum {
>      STR_PRODUCT_MOUSE,
>      STR_PRODUCT_TABLET,
>      STR_PRODUCT_KEYBOARD,
> -    STR_SERIALNUMBER,
> +    STR_SERIAL_COMPAT,
>      STR_CONFIG_MOUSE,
>      STR_CONFIG_TABLET,
>      STR_CONFIG_KEYBOARD,
> +    STR_SERIAL_MOUSE,
> +    STR_SERIAL_TABLET,
> +    STR_SERIAL_KEYBOARD,
>  };
>  
>  static const USBDescStrings desc_strings = {
> @@ -72,10 +75,13 @@ static const USBDescStrings desc_strings = {
>      [STR_PRODUCT_MOUSE]    = "QEMU USB Mouse",
>      [STR_PRODUCT_TABLET]   = "QEMU USB Tablet",
>      [STR_PRODUCT_KEYBOARD] = "QEMU USB Keyboard",
> -    [STR_SERIALNUMBER]     = "42", /* == remote wakeup works */
> +    [STR_SERIAL_COMPAT]    = "42",
>      [STR_CONFIG_MOUSE]     = "HID Mouse",
>      [STR_CONFIG_TABLET]    = "HID Tablet",
>      [STR_CONFIG_KEYBOARD]  = "HID Keyboard",
> +    [STR_SERIAL_MOUSE]     = "89126",
> +    [STR_SERIAL_TABLET]    = "28754",
> +    [STR_SERIAL_KEYBOARD]  = "68284",
>  };
>  
>  static const USBDescIface desc_iface_mouse = {
> @@ -375,7 +381,7 @@ static const USBDesc desc_mouse = {
>          .bcdDevice         = 0,
>          .iManufacturer     = STR_MANUFACTURER,
>          .iProduct          = STR_PRODUCT_MOUSE,
> -        .iSerialNumber     = STR_SERIALNUMBER,
> +        .iSerialNumber     = STR_SERIAL_MOUSE,
>      },
>      .full = &desc_device_mouse,
>      .str  = desc_strings,
> @@ -389,7 +395,7 @@ static const USBDesc desc_mouse2 = {
>          .bcdDevice         = 0,
>          .iManufacturer     = STR_MANUFACTURER,
>          .iProduct          = STR_PRODUCT_MOUSE,
> -        .iSerialNumber     = STR_SERIALNUMBER,
> +        .iSerialNumber     = STR_SERIAL_MOUSE,
>      },
>      .full = &desc_device_mouse,
>      .high = &desc_device_mouse2,
> @@ -404,7 +410,7 @@ static const USBDesc desc_tablet = {
>          .bcdDevice         = 0,
>          .iManufacturer     = STR_MANUFACTURER,
>          .iProduct          = STR_PRODUCT_TABLET,
> -        .iSerialNumber     = STR_SERIALNUMBER,
> +        .iSerialNumber     = STR_SERIAL_TABLET,
>      },
>      .full = &desc_device_tablet,
>      .str  = desc_strings,
> @@ -418,7 +424,7 @@ static const USBDesc desc_tablet2 = {
>          .bcdDevice         = 0,
>          .iManufacturer     = STR_MANUFACTURER,
>          .iProduct          = STR_PRODUCT_TABLET,
> -        .iSerialNumber     = STR_SERIALNUMBER,
> +        .iSerialNumber     = STR_SERIAL_TABLET,
>      },
>      .full = &desc_device_tablet,
>      .high = &desc_device_tablet2,
> @@ -433,7 +439,7 @@ static const USBDesc desc_keyboard = {
>          .bcdDevice         = 0,
>          .iManufacturer     = STR_MANUFACTURER,
>          .iProduct          = STR_PRODUCT_KEYBOARD,
> -        .iSerialNumber     = STR_SERIALNUMBER,
> +        .iSerialNumber     = STR_SERIAL_KEYBOARD,
>      },
>      .full = &desc_device_keyboard,
>      .str  = desc_strings,
> @@ -447,7 +453,7 @@ static const USBDesc desc_keyboard2 = {
>          .bcdDevice         = 0,
>          .iManufacturer     = STR_MANUFACTURER,
>          .iProduct          = STR_PRODUCT_KEYBOARD,
> -        .iSerialNumber     = STR_SERIALNUMBER,
> +        .iSerialNumber     = STR_SERIAL_KEYBOARD,
>      },
>      .full = &desc_device_keyboard,
>      .high = &desc_device_keyboard2,
> @@ -718,9 +724,7 @@ static void usb_hid_initfn(USBDevice *dev, int kind,
>          return;
>      }
>  
> -    if (dev->serial) {
> -        usb_desc_set_string(dev, STR_SERIALNUMBER, dev->serial);
> -    }
> +    usb_desc_create_serial(dev);
>      usb_desc_init(dev);
>      us->intr = usb_ep_get(dev, USB_TOKEN_IN, 1);
>      hid_init(&us->hid, kind, usb_hid_changed);
> -- 
> 2.9.3
> 
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK