From 987b295099267bac3012fd401dc3ea34c3e40071 Mon Sep 17 00:00:00 2001 From: Qi Zhou atmgnd@outlook.com Date: Mon, 14 Nov 2022 20:55:44 +0800 Subject: [PATCH] fix wrong init of keyboard/mouse's if first interface is not boot protocol
There is always some endpoint descriptors after each interface descriptor, We should only decrement num_iface if interface type is USB_DT_INTERFACE, see https://www.beyondlogic.org/usbnutshell/usb5.shtml#ConfigurationDescriptors
Signed-off-by: Qi Zhou atmgnd@outlook.com --- src/hw/usb-hid.c | 13 ++++++++----- src/hw/usb.c | 12 +++++++----- 2 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/src/hw/usb-hid.c b/src/hw/usb-hid.c index 92c6b19..dec198a 100644 --- a/src/hw/usb-hid.c +++ b/src/hw/usb-hid.c @@ -22,13 +22,13 @@ struct usb_pipe *mouse_pipe VARFSEG;
// Send USB HID protocol message. static int -set_protocol(struct usb_pipe *pipe, u16 val) +set_protocol(struct usb_pipe *pipe, u16 val, u16 inferface) { struct usb_ctrlrequest req; req.bRequestType = USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE; req.bRequest = HID_REQ_SET_PROTOCOL; req.wValue = val; - req.wIndex = 0; + req.wIndex = inferface; req.wLength = 0; return usb_send_default_control(pipe, &req, NULL); } @@ -76,9 +76,12 @@ usb_kbd_setup(struct usbdevice_s *usbdev }
// Enable "boot" protocol. - int ret = set_protocol(usbdev->defpipe, 0); - if (ret) + int ret = set_protocol(usbdev->defpipe, 0, usbdev->iface->bInterfaceNumber); + if (ret) { + dprintf(3, "Failed to set boot protocol\n"); return -1; + } + // Periodically send reports to enable key repeat. ret = set_idle(usbdev->defpipe, KEYREPEATMS); if (ret) @@ -118,7 +121,7 @@ usb_mouse_setup(struct usbdevice_s *usbdev }
// Enable "boot" protocol. - int ret = set_protocol(usbdev->defpipe, 0); + int ret = set_protocol(usbdev->defpipe, 0, usbdev->iface->bInterfaceNumber); if (ret) return -1;
diff --git a/src/hw/usb.c b/src/hw/usb.c index 38a866a..409da8c 100644 --- a/src/hw/usb.c +++ b/src/hw/usb.c @@ -372,17 +372,19 @@ configure_usb_device(struct usbdevice_s *usbdev) void *config_end = (void*)config + config->wTotalLength; struct usb_interface_descriptor *iface = (void*)(&config[1]); for (;;) { - if (!num_iface-- || (void*)iface + iface->bLength > config_end) + if (!num_iface || (void*)iface + iface->bLength > config_end) // Not a supported device. goto fail; - if (iface->bDescriptorType == USB_DT_INTERFACE - && (iface->bInterfaceClass == USB_CLASS_HUB + if (iface->bDescriptorType == USB_DT_INTERFACE) { + num_iface--; + if (iface->bInterfaceClass == USB_CLASS_HUB || (iface->bInterfaceClass == USB_CLASS_MASS_STORAGE && (iface->bInterfaceProtocol == US_PR_BULK || iface->bInterfaceProtocol == US_PR_UAS)) || (iface->bInterfaceClass == USB_CLASS_HID - && iface->bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT))) - break; + && iface->bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT)) + break; + } iface = (void*)iface + iface->bLength; }
On Mon, Nov 14, 2022 at 12:59:25PM +0000, qi zhou wrote:
From 987b295099267bac3012fd401dc3ea34c3e40071 Mon Sep 17 00:00:00 2001 From: Qi Zhou atmgnd@outlook.com Date: Mon, 14 Nov 2022 20:55:44 +0800 Subject: [PATCH] fix wrong init of keyboard/mouse's if first interface is not boot protocol
There is always some endpoint descriptors after each interface descriptor, We should only decrement num_iface if interface type is USB_DT_INTERFACE, see https://www.beyondlogic.org/usbnutshell/usb5.shtml#ConfigurationDescriptors
Signed-off-by: Qi Zhou atmgnd@outlook.com
Reviewed-by: Gerd Hoffmann kraxel@redhat.com
On Mon, Nov 14, 2022 at 12:59:25PM +0000, qi zhou wrote:
From 987b295099267bac3012fd401dc3ea34c3e40071 Mon Sep 17 00:00:00 2001 From: Qi Zhou atmgnd@outlook.com Date: Mon, 14 Nov 2022 20:55:44 +0800 Subject: [PATCH] fix wrong init of keyboard/mouse's if first interface is not boot protocol
There is always some endpoint descriptors after each interface descriptor, We should only decrement num_iface if interface type is USB_DT_INTERFACE, see https://www.beyondlogic.org/usbnutshell/usb5.shtml#ConfigurationDescriptors
Thanks. The change looks fine to me. Did this change fix a problem with a particular device, and if so can you provide some details on that device? Was it on qemu or coreboot?
Thanks, -Kevin
I found this problem when I use qemu with usb passthrough.
The original problem is a little more complex, The patch I send only fix part of it
The problem is I have a rapoo keyboard and a rapoo mouse. The rapoo mouse, it report 2 boot protocols on first configuration descriptor, first one is keyboard protocol, second one is mouse protocol.
So when boot, seabios will found two devices with keyboard boot protocol on first config descriptor's first Interface Descriptor. but seabios can use only one keyboard. see definition of global variable keyboard_pipe
In some situation, if seabios initialize the buggy mouse after keyboard, It treat the mouse as keyboard, and ignore the true keyboard. the result is I cannot use the true keyboard
I think this mouse may buggy or less common, so I did not include all modifiecations in the patch
But I do found the code in configure_usb_device is wrong. may be there is some keyboard/mouse's boot protocol interface is not the first interface ?
The rapoo mouse's device descriptor root@h470:~# lsusb -d 24ae:4104 -v
Bus 001 Device 012: ID 24ae:4104 Rapoo Rapoo Gaming Mouse Device Descriptor: bLength 18 bDescriptorType 1 bcdUSB 2.00 bDeviceClass 0 bDeviceSubClass 0 bDeviceProtocol 0 bMaxPacketSize0 8 idVendor 0x24ae idProduct 0x4104 bcdDevice 2.0d iManufacturer 1 Rapoo iProduct 2 Rapoo Gaming Mouse iSerial 0 bNumConfigurations 1 Configuration Descriptor: bLength 9 bDescriptorType 2 wTotalLength 0x005b bNumInterfaces 3 bConfigurationValue 1 iConfiguration 0 bmAttributes 0xa0 (Bus Powered) Remote Wakeup MaxPower 350mA Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 0 bAlternateSetting 0 bNumEndpoints 1 bInterfaceClass 3 Human Interface Device bInterfaceSubClass 1 Boot Interface Subclass bInterfaceProtocol 1 Keyboard iInterface 0 HID Device Descriptor: bLength 9 bDescriptorType 33 bcdHID 1.10 bCountryCode 0 Not supported bNumDescriptors 1 bDescriptorType 34 Report wDescriptorLength 59 Report Descriptors: ** UNAVAILABLE ** Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x81 EP 1 IN bmAttributes 3 Transfer Type Interrupt Synch Type None Usage Type Data wMaxPacketSize 0x0008 1x 8 bytes bInterval 4 Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 1 bAlternateSetting 0 bNumEndpoints 1 bInterfaceClass 3 Human Interface Device bInterfaceSubClass 1 Boot Interface Subclass bInterfaceProtocol 2 Mouse iInterface 0 HID Device Descriptor: bLength 9 bDescriptorType 33 bcdHID 1.10 bCountryCode 0 Not supported bNumDescriptors 1 bDescriptorType 34 Report wDescriptorLength 148 Report Descriptors: ** UNAVAILABLE ** Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x82 EP 2 IN bmAttributes 3 Transfer Type Interrupt Synch Type None Usage Type Data wMaxPacketSize 0x0008 1x 8 bytes bInterval 1 Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 2 bAlternateSetting 0 bNumEndpoints 2 bInterfaceClass 3 Human Interface Device bInterfaceSubClass 0 bInterfaceProtocol 0 iInterface 0 HID Device Descriptor: bLength 9 bDescriptorType 33 bcdHID 1.10 bCountryCode 0 Not supported bNumDescriptors 1 bDescriptorType 34 Report wDescriptorLength 33 Report Descriptors: ** UNAVAILABLE ** Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x83 EP 3 IN bmAttributes 3 Transfer Type Interrupt Synch Type None Usage Type Data wMaxPacketSize 0x0020 1x 32 bytes bInterval 4 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x04 EP 4 OUT bmAttributes 3 Transfer Type Interrupt Synch Type None Usage Type Data wMaxPacketSize 0x0020 1x 32 bytes bInterval 4 can't get device qualifier: Resource temporarily unavailable can't get debug descriptor: Resource temporarily unavailable Device Status: 0x0000 (Bus Powered) root@h470:~#
From: Kevin O'Connor kevin@koconnor.net Sent: Wednesday, November 23, 2022 2:22 To: qi zhou atmgnd@outlook.com Cc: seabios@seabios.org seabios@seabios.org Subject: Re: [SeaBIOS] [PATCH] fix wrong init of keyboard/mouse's if first interface is not boot protocol On Mon, Nov 14, 2022 at 12:59:25PM +0000, qi zhou wrote:
From 987b295099267bac3012fd401dc3ea34c3e40071 Mon Sep 17 00:00:00 2001 From: Qi Zhou atmgnd@outlook.com Date: Mon, 14 Nov 2022 20:55:44 +0800 Subject: [PATCH] fix wrong init of keyboard/mouse's if first interface is not boot protocol
There is always some endpoint descriptors after each interface descriptor, We should only decrement num_iface if interface type is USB_DT_INTERFACE, see https://www.beyondlogic.org/usbnutshell/usb5.shtml#ConfigurationDescriptors
Thanks. The change looks fine to me. Did this change fix a problem with a particular device, and if so can you provide some details on that device? Was it on qemu or coreboot?
Thanks, -Kevin
Below is the full modification I have made, maybe someone can review it. the logic is traverse all Interface Descriptors, and if found any mouse boot protocol, then use mouse protocol.
Or just use the orginal patch I sent before
From 6d7c25a249e31b3e5c4b30ce23cae89d70548df6 Mon Sep 17 00:00:00 2001 From: Qi Zhou atmgnd@outlook.com Date: Mon, 14 Nov 2022 20:55:44 +0800 Subject: [PATCH] fix wrong init of keyboard/mouse if first interface is not boot protocol
There is always some endpoint descriptors after each interface descriptor, We should only decrement num_iface if interface type is USB_DT_INTERFACE, see https://www.beyondlogic.org/usbnutshell/usb5.shtml#ConfigurationDescriptors
Signed-off-by: Qi Zhou atmgnd@outlook.com --- src/hw/usb-hid.c | 13 ++++++++----- src/hw/usb.c | 33 +++++++++++++++++++++------------ 2 files changed, 29 insertions(+), 17 deletions(-)
diff --git a/src/hw/usb-hid.c b/src/hw/usb-hid.c index 92c6b19..dec198a 100644 --- a/src/hw/usb-hid.c +++ b/src/hw/usb-hid.c @@ -22,13 +22,13 @@ struct usb_pipe *mouse_pipe VARFSEG; // Send USB HID protocol message. static int -set_protocol(struct usb_pipe *pipe, u16 val) +set_protocol(struct usb_pipe *pipe, u16 val, u16 inferface) { struct usb_ctrlrequest req; req.bRequestType = USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE; req.bRequest = HID_REQ_SET_PROTOCOL; req.wValue = val; - req.wIndex = 0; + req.wIndex = inferface; req.wLength = 0; return usb_send_default_control(pipe, &req, NULL); } @@ -76,9 +76,12 @@ usb_kbd_setup(struct usbdevice_s *usbdev } // Enable "boot" protocol. - int ret = set_protocol(usbdev->defpipe, 0); - if (ret) + int ret = set_protocol(usbdev->defpipe, 0, usbdev->iface->bInterfaceNumber); + if (ret) { + dprintf(3, "Failed to set boot protocol\n"); return -1; + } + // Periodically send reports to enable key repeat. ret = set_idle(usbdev->defpipe, KEYREPEATMS); if (ret) @@ -118,7 +121,7 @@ usb_mouse_setup(struct usbdevice_s *usbdev } // Enable "boot" protocol. - int ret = set_protocol(usbdev->defpipe, 0); + int ret = set_protocol(usbdev->defpipe, 0, usbdev->iface->bInterfaceNumber); if (ret) return -1; diff --git a/src/hw/usb.c b/src/hw/usb.c index 38a866a..00fe586 100644 --- a/src/hw/usb.c +++ b/src/hw/usb.c @@ -370,22 +370,31 @@ configure_usb_device(struct usbdevice_s *usbdev) // interfaces of the first configuration. int num_iface = config->bNumInterfaces; void *config_end = (void*)config + config->wTotalLength; - struct usb_interface_descriptor *iface = (void*)(&config[1]); + struct usb_interface_descriptor *iface_walker = (void*)(&config[1]), *iface = NULL; for (;;) { - if (!num_iface-- || (void*)iface + iface->bLength > config_end) - // Not a supported device. - goto fail; - if (iface->bDescriptorType == USB_DT_INTERFACE - && (iface->bInterfaceClass == USB_CLASS_HUB - || (iface->bInterfaceClass == USB_CLASS_MASS_STORAGE - && (iface->bInterfaceProtocol == US_PR_BULK - || iface->bInterfaceProtocol == US_PR_UAS)) - || (iface->bInterfaceClass == USB_CLASS_HID - && iface->bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT))) + if (!num_iface || (void*)iface_walker + iface_walker->bLength > config_end) break; - iface = (void*)iface + iface->bLength; + if (iface_walker->bDescriptorType == USB_DT_INTERFACE) { + num_iface--; + if(iface_walker->bInterfaceClass == USB_CLASS_HUB + || (iface_walker->bInterfaceClass == USB_CLASS_MASS_STORAGE + && (iface_walker->bInterfaceProtocol == US_PR_BULK + || iface_walker->bInterfaceProtocol == US_PR_UAS))){ + iface = iface_walker; + break; + } else if(iface_walker->bInterfaceClass == USB_CLASS_HID + && iface_walker->bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT) { + if (iface == NULL || iface_walker->bInterfaceProtocol == USB_INTERFACE_PROTOCOL_MOUSE) + iface = iface_walker; + } + } + iface_walker = (void*)iface_walker + iface_walker->bLength; } + if(iface == NULL) + // Not a supported device. + goto fail; + // Set the configuration. ret = set_configuration(usbdev->defpipe, config->bConfigurationValue); if (ret)
On Wed, Nov 23, 2022 at 03:31:19PM +0000, qi zhou wrote:
I found this problem when I use qemu with usb passthrough.
The original problem is a little more complex, The patch I send only fix part of it
The problem is I have a rapoo keyboard and a rapoo mouse. The rapoo mouse, it report 2 boot protocols on first configuration descriptor, first one is keyboard protocol, second one is mouse protocol.
So when boot, seabios will found two devices with keyboard boot protocol on first config descriptor's first Interface Descriptor. but seabios can use only one keyboard. see definition of global variable keyboard_pipe
In some situation, if seabios initialize the buggy mouse after keyboard, It treat the mouse as keyboard, and ignore the true keyboard. the result is I cannot use the true keyboard
I think this mouse may buggy or less common, so I did not include all modifiecations in the patch
But I do found the code in configure_usb_device is wrong. may be there is some keyboard/mouse's boot protocol interface is not the first interface ?
The rapoo mouse's device descriptor root@h470:~# lsusb -d 24ae:4104 -v
Bus 001 Device 012: ID 24ae:4104 Rapoo Rapoo Gaming Mouse Device Descriptor: bLength 18 bDescriptorType 1 bcdUSB 2.00 bDeviceClass 0 bDeviceSubClass 0 bDeviceProtocol 0 bMaxPacketSize0 8 idVendor 0x24ae idProduct 0x4104 bcdDevice 2.0d iManufacturer 1 Rapoo iProduct 2 Rapoo Gaming Mouse iSerial 0 bNumConfigurations 1 Configuration Descriptor: bLength 9 bDescriptorType 2 wTotalLength 0x005b bNumInterfaces 3 bConfigurationValue 1 iConfiguration 0 bmAttributes 0xa0 (Bus Powered) Remote Wakeup MaxPower 350mA Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 0 bAlternateSetting 0 bNumEndpoints 1 bInterfaceClass 3 Human Interface Device bInterfaceSubClass 1 Boot Interface Subclass bInterfaceProtocol 1 Keyboard iInterface 0 HID Device Descriptor: bLength 9 bDescriptorType 33 bcdHID 1.10 bCountryCode 0 Not supported bNumDescriptors 1 bDescriptorType 34 Report wDescriptorLength 59 Report Descriptors: ** UNAVAILABLE ** Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x81 EP 1 IN bmAttributes 3 Transfer Type Interrupt Synch Type None Usage Type Data wMaxPacketSize 0x0008 1x 8 bytes bInterval 4 Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 1 bAlternateSetting 0 bNumEndpoints 1 bInterfaceClass 3 Human Interface Device bInterfaceSubClass 1 Boot Interface Subclass bInterfaceProtocol 2 Mouse iInterface 0 HID Device Descriptor: bLength 9 bDescriptorType 33 bcdHID 1.10 bCountryCode 0 Not supported bNumDescriptors 1 bDescriptorType 34 Report wDescriptorLength 148 Report Descriptors: ** UNAVAILABLE ** Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x82 EP 2 IN bmAttributes 3 Transfer Type Interrupt Synch Type None Usage Type Data wMaxPacketSize 0x0008 1x 8 bytes bInterval 1 Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 2 bAlternateSetting 0 bNumEndpoints 2 bInterfaceClass 3 Human Interface Device bInterfaceSubClass 0 bInterfaceProtocol 0 iInterface 0 HID Device Descriptor: bLength 9 bDescriptorType 33 bcdHID 1.10 bCountryCode 0 Not supported bNumDescriptors 1 bDescriptorType 34 Report wDescriptorLength 33 Report Descriptors: ** UNAVAILABLE ** Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x83 EP 3 IN bmAttributes 3 Transfer Type Interrupt Synch Type None Usage Type Data wMaxPacketSize 0x0020 1x 32 bytes bInterval 4 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x04 EP 4 OUT bmAttributes 3 Transfer Type Interrupt Synch Type None Usage Type Data wMaxPacketSize 0x0020 1x 32 bytes bInterval 4 can't get device qualifier: Resource temporarily unavailable can't get debug descriptor: Resource temporarily unavailable Device Status: 0x0000 (Bus Powered) root@h470:~#
Thanks. I committed this change.
-Kevin
-
Gerd Hoffmann -
Kevin O'Connor -
qi zhou