[SeaBIOS] Re: [PATCH] fix wrong init of keyboard/mouse's if first interface is not boot protocol

qi zhou posted 1 patch 5 days, 12 hours ago
Failed in applying to current master (apply log)
src/hw/usb-hid.c | 13 ++++++++-----
src/hw/usb.c     | 33 +++++++++++++++++++++------------
2 files changed, 29 insertions(+), 17 deletions(-)
[SeaBIOS] Re: [PATCH] fix wrong init of keyboard/mouse's if first interface is not boot protocol
Posted by qi zhou 5 days, 12 hours ago
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)
-- 
2.25.1



From: qi zhou <atmgnd@outlook.com>
Sent: Wednesday, November 23, 2022 23:31
To: Kevin O'Connor <kevin@koconnor.net>
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 
 
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
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org