* [Qemu-devel] [PATCH v2] usb: assign unique serial numbers to hid devices
@ 2018-09-14 10:48 Gerd Hoffmann
2018-09-14 11:34 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 4+ messages in thread
From: Gerd Hoffmann @ 2018-09-14 10:48 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann, dgilbert
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v2] usb: assign unique serial numbers to hid devices
2018-09-14 10:48 [Qemu-devel] [PATCH v2] usb: assign unique serial numbers to hid devices Gerd Hoffmann
@ 2018-09-14 11:34 ` Dr. David Alan Gilbert
2018-09-14 12:54 ` Gerd Hoffmann
0 siblings, 1 reply; 4+ messages in thread
From: Dr. David Alan Gilbert @ 2018-09-14 11:34 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: qemu-devel
* 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v2] usb: assign unique serial numbers to hid devices
2018-09-14 11:34 ` Dr. David Alan Gilbert
@ 2018-09-14 12:54 ` Gerd Hoffmann
2018-09-14 14:46 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 4+ messages in thread
From: Gerd Hoffmann @ 2018-09-14 12:54 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v2] usb: assign unique serial numbers to hid devices
2018-09-14 12:54 ` Gerd Hoffmann
@ 2018-09-14 14:46 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 4+ messages in thread
From: Dr. David Alan Gilbert @ 2018-09-14 14:46 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: qemu-devel
* 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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-09-14 14:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-09-14 10:48 [Qemu-devel] [PATCH v2] usb: assign unique serial numbers to hid devices Gerd Hoffmann
2018-09-14 11:34 ` Dr. David Alan Gilbert
2018-09-14 12:54 ` Gerd Hoffmann
2018-09-14 14:46 ` Dr. David Alan Gilbert
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).