[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3] usb: assign unique serial numbers to hid dev
From: |
Daniel P . Berrangé |
Subject: |
Re: [Qemu-devel] [PATCH v3] usb: assign unique serial numbers to hid devices |
Date: |
Thu, 10 Jan 2019 12:17:50 +0000 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
On Thu, Jan 10, 2019 at 01:08:43PM +0100, Gerd Hoffmann 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>
> ---
> hw/usb/dev-hid.c | 26 +++++++++++++++-----------
> 1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/hw/usb/dev-hid.c b/hw/usb/dev-hid.c
> index 62d18290dc..205236e9b1 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);
Don't we need some kind of machine type compat handling to avoid
the serial number being changed during migration ?
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|