include/hw/compat.h | 14 +++++++++++++- hw/usb/dev-hid.c | 24 +++++++++++++----------- 2 files changed, 26 insertions(+), 12 deletions(-)
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.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
include/hw/compat.h | 14 +++++++++++++-
hw/usb/dev-hid.c | 24 +++++++++++++-----------
2 files changed, 26 insertions(+), 12 deletions(-)
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 6f4d5fc647..c059459394 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -2,7 +2,19 @@
#define HW_COMPAT_H
#define HW_COMPAT_3_0 \
- /* empty */
+ {\
+ .driver = "usb-kbd",\
+ .property = "serial",\
+ .value = "42",\
+ },{\
+ .driver = "usb-mouse",\
+ .property = "serial",\
+ .value = "42",\
+ },{\
+ .driver = "usb-tablet",\
+ .property = "serial",\
+ .value = "42",\
+ },
#define HW_COMPAT_2_12 \
{\
diff --git a/hw/usb/dev-hid.c b/hw/usb/dev-hid.c
index 62d18290dc..7ad6fb33a9 100644
--- a/hw/usb/dev-hid.c
+++ b/hw/usb/dev-hid.c
@@ -61,7 +61,9 @@ enum {
STR_PRODUCT_MOUSE,
STR_PRODUCT_TABLET,
STR_PRODUCT_KEYBOARD,
- STR_SERIALNUMBER,
+ STR_SERIAL_MOUSE,
+ STR_SERIAL_TABLET,
+ STR_SERIAL_KEYBOARD,
STR_CONFIG_MOUSE,
STR_CONFIG_TABLET,
STR_CONFIG_KEYBOARD,
@@ -72,7 +74,9 @@ 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_MOUSE] = "89126",
+ [STR_SERIAL_TABLET] = "28754",
+ [STR_SERIAL_KEYBOARD] = "68284",
[STR_CONFIG_MOUSE] = "HID Mouse",
[STR_CONFIG_TABLET] = "HID Tablet",
[STR_CONFIG_KEYBOARD] = "HID Keyboard",
@@ -375,7 +379,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 +393,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 +408,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 +422,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 +437,7 @@ static const USBDesc desc_keyboard = {
.bcdDevice = 0,
.iManufacturer = STR_MANUFACTURER,
.iProduct = STR_PRODUCT_KEYBOARD,
- .iSerialNumber = STR_SERIALNUMBER,
+ .iSerialNumber = STR_SERIAL_TABLET,
},
.full = &desc_device_keyboard,
.str = desc_strings,
@@ -447,7 +451,7 @@ static const USBDesc desc_keyboard2 = {
.bcdDevice = 0,
.iManufacturer = STR_MANUFACTURER,
.iProduct = STR_PRODUCT_KEYBOARD,
- .iSerialNumber = STR_SERIALNUMBER,
+ .iSerialNumber = STR_SERIAL_TABLET,
},
.full = &desc_device_keyboard,
.high = &desc_device_keyboard2,
@@ -718,9 +722,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
* Gerd Hoffmann (kraxel@redhat.com) 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.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> include/hw/compat.h | 14 +++++++++++++-
> hw/usb/dev-hid.c | 24 +++++++++++++-----------
> 2 files changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 6f4d5fc647..c059459394 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -2,7 +2,19 @@
> #define HW_COMPAT_H
>
> #define HW_COMPAT_3_0 \
> - /* empty */
> + {\
> + .driver = "usb-kbd",\
> + .property = "serial",\
> + .value = "42",\
> + },{\
> + .driver = "usb-mouse",\
> + .property = "serial",\
> + .value = "42",\
> + },{\
> + .driver = "usb-tablet",\
> + .property = "serial",\
> + .value = "42",\
> + },
> #define HW_COMPAT_2_12 \
> {\
> diff --git a/hw/usb/dev-hid.c b/hw/usb/dev-hid.c
> index 62d18290dc..7ad6fb33a9 100644
> --- a/hw/usb/dev-hid.c
> +++ b/hw/usb/dev-hid.c
> @@ -61,7 +61,9 @@ enum {
> STR_PRODUCT_MOUSE,
> STR_PRODUCT_TABLET,
> STR_PRODUCT_KEYBOARD,
> - STR_SERIALNUMBER,
> + STR_SERIAL_MOUSE,
> + STR_SERIAL_TABLET,
> + STR_SERIAL_KEYBOARD,
> STR_CONFIG_MOUSE,
> STR_CONFIG_TABLET,
> STR_CONFIG_KEYBOARD,
> @@ -72,7 +74,9 @@ 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_MOUSE] = "89126",
> + [STR_SERIAL_TABLET] = "28754",
> + [STR_SERIAL_KEYBOARD] = "68284",
OK, so now I've found the bit about the magic 42; commit 7b074a22 of
yours; recommended checking for 42 for knowing we had autosuspend;
what's actually in the current fedora hid rules is serial!=1 - I wonder
what others have.
It's a shame these are random numbers rather than 2,3,4; but still.
> [STR_CONFIG_MOUSE] = "HID Mouse",
> [STR_CONFIG_TABLET] = "HID Tablet",
> [STR_CONFIG_KEYBOARD] = "HID Keyboard",
> @@ -375,7 +379,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 +393,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 +408,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 +422,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 +437,7 @@ static const USBDesc desc_keyboard = {
> .bcdDevice = 0,
> .iManufacturer = STR_MANUFACTURER,
> .iProduct = STR_PRODUCT_KEYBOARD,
> - .iSerialNumber = STR_SERIALNUMBER,
> + .iSerialNumber = STR_SERIAL_TABLET,
> },
> .full = &desc_device_keyboard,
> .str = desc_strings,
> @@ -447,7 +451,7 @@ static const USBDesc desc_keyboard2 = {
> .bcdDevice = 0,
> .iManufacturer = STR_MANUFACTURER,
> .iProduct = STR_PRODUCT_KEYBOARD,
> - .iSerialNumber = STR_SERIALNUMBER,
> + .iSerialNumber = STR_SERIAL_TABLET,
> },
> .full = &desc_device_keyboard,
> .high = &desc_device_keyboard2,
> @@ -718,9 +722,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);
In the cases where I've not passed serial= on the command line, who
wins, does this mean create_serial uses the path or does it use the
magic constants you set above?
Dave
> 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 / dgilbert@redhat.com / Manchester, UK
Hi, > OK, so now I've found the bit about the magic 42; commit 7b074a22 of > yours; recommended checking for 42 for knowing we had autosuspend; > what's actually in the current fedora hid rules is serial!=1 - I wonder > what others have. Whatever upstream systemd/udev has I guess ... 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, 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. cheers, Gerd
Gerd Hoffmann <kraxel@redhat.com> writes: > Hi, > >> OK, so now I've found the bit about the magic 42; commit 7b074a22 of >> yours; recommended checking for 42 for knowing we had autosuspend; >> what's actually in the current fedora hid rules is serial!=1 - I wonder >> what others have. > > Whatever upstream systemd/udev has I guess ... > > 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, 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. Care to work this information into the commit message?
* Gerd Hoffmann (kraxel@redhat.com) wrote: > Hi, > > > OK, so now I've found the bit about the magic 42; commit 7b074a22 of > > yours; recommended checking for 42 for knowing we had autosuspend; > > what's actually in the current fedora hid rules is serial!=1 - I wonder > > what others have. > > Whatever upstream systemd/udev has I guess ... > > 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, 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. OK, so what about the other question - which serial number do we end up with after this patch; do we get the 89126/28754/68284 or do we get the path based one from your change that calls usb_desc_create_serial? Dave > cheers, > Gerd > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 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. > > OK, so what about the other question - which serial number do we end up > with after this patch; do we get the 89126/28754/68284 or do we get the > path based one from your change that calls usb_desc_create_serial? The path based one. and the numbers are the first part, i.e. this: root@fedora ~# lsusb -vs1:2 [ ... ] iManufacturer 1 QEMU iProduct 3 QEMU USB Tablet iSerial 6 28754-0000:00:02.1:00.0-1 bNumConfigurations 1 [ ... ] cheers, Gerd
* Gerd Hoffmann (kraxel@redhat.com) wrote: > > > 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. > > > > OK, so what about the other question - which serial number do we end up > > with after this patch; do we get the 89126/28754/68284 or do we get the > > path based one from your change that calls usb_desc_create_serial? > > The path based one. and the numbers are the first part, i.e. this: > > root@fedora ~# lsusb -vs1:2 > [ ... ] > iManufacturer 1 QEMU > iProduct 3 QEMU USB Tablet > iSerial 6 28754-0000:00:02.1:00.0-1 > bNumConfigurations 1 > [ ... ] Ah OK, that's fine; so : Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > cheers, > Gerd > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
© 2016 - 2025 Red Hat, Inc.