Re: [PATCH 13/18] MdeModulePkg/Usb/Keyboard.c: don't request protocol before setting

["MrChromebox" <matt.devillier@gmail.com>]

seems this patch got truncated during a rebase, here is the correct version.

For device that does not respond to GetProtocolRequest(), the original flow is:
a.1 Execute GetProtocolRequest()
a.2 Timeout occurs for GetProtocolRequest()
a.3 Conditionally execute UsbSetProtocolRequest()

The proposed new flow is:
b.1 Always execute UsbSetProtocolRequest()

The timeout delay for keyboards which do not support
GetProtocolRequest() that we've seen is significant (>5s, IIRC)

 MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c | 28 ++++++++++++++----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
b/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
index 5a94a4dda7..ad55fa3330 100644
--- a/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
+++ b/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
@@ -805,7 +805,6 @@ InitUSBKeyboard (
   )
 {
   UINT16      ConfigValue;
-  UINT8       Protocol;
   EFI_STATUS  Status;
   UINT32      TransferResult;

@@ -854,21 +853,28 @@ InitUSBKeyboard (
     }
   }

-  UsbGetProtocolRequest (
-    UsbKeyboardDevice->UsbIo,
-    UsbKeyboardDevice->InterfaceDescriptor.InterfaceNumber,
-    &Protocol
-    );
   //
   // Set boot protocol for the USB Keyboard.
   // This driver only supports boot protocol.
   //
-  if (Protocol != BOOT_PROTOCOL) {
-    UsbSetProtocolRequest (
-      UsbKeyboardDevice->UsbIo,
-      UsbKeyboardDevice->InterfaceDescriptor.InterfaceNumber,
-      BOOT_PROTOCOL
+  Status = UsbSetProtocolRequest (
+             UsbKeyboardDevice->UsbIo,
+             UsbKeyboardDevice->InterfaceDescriptor.InterfaceNumber,
+             BOOT_PROTOCOL
+             );
+  if (EFI_ERROR (Status)) {
+    //
+    // If protocol could not be set here, it means
+    // the keyboard interface has some errors and could
+    // not be initialized
+    //
+    REPORT_STATUS_CODE_WITH_DEVICE_PATH (
+      EFI_ERROR_CODE | EFI_ERROR_MINOR,
+      (EFI_PERIPHERAL_KEYBOARD | EFI_P_EC_INTERFACE_ERROR),
+      UsbKeyboardDevice->DevicePath
       );
+
+    return EFI_DEVICE_ERROR;
   }

   UsbKeyboardDevice->CtrlOn    = FALSE;
-- 
2.32.0

On Thu, Feb 10, 2022 at 9:32 PM Wu, Hao A <hao.a.wu@intel.com> wrote:
>
> Hello,
>
> Inline comments below:
>
>
> > -----Original Message-----
> > From: Sean Rhodes <sean@starlabs.systems>
> > Sent: Friday, February 11, 2022 5:27 AM
> > To: devel@edk2.groups.io
> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Matt DeVillier
> > <matt.devillier@gmail.com>; Patrick Rudolph
> > <patrick.rudolph@9elements.com>
> > Subject: [PATCH 13/18] MdeModulePkg/Usb/Keyboard.c: don't request
> > protocol before setting
> >
> > From: Matt DeVillier <matt.devillier@gmail.com>
> >
> > No need to check the interface protocol then conditionally setting,
> > just set it to BOOT_PROTOCOL and check for error.
> >
> > This is what Linux does for HID devices as some don't follow the USB spec.
> > One example is the Aspeed BMC HID keyboard device, which adds a massive
> > boot delay without this patch as it doesn't respond to 'GetProtocolRequest'.
> >
> > Signed-off-by: Matt DeVillier <matt.devillier@gmail.com>
> > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> > ---
> >  MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c | 22
> > +++++++++++++++++-----
> >  1 file changed, 17 insertions(+), 5 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
> > b/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
> > index 5a94a4dda7..56d249f937 100644
> > --- a/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
> > +++ b/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
> > @@ -863,12 +863,24 @@ InitUSBKeyboard (
> >    // Set boot protocol for the USB Keyboard.
> >
> >    // This driver only supports boot protocol.
> >
> >    //
> >
> > -  if (Protocol != BOOT_PROTOCOL) {
> >
> > -    UsbSetProtocolRequest (
> >
> > -      UsbKeyboardDevice->UsbIo,
> >
> > -      UsbKeyboardDevice->InterfaceDescriptor.InterfaceNumber,
> >
> > -      BOOT_PROTOCOL
> >
> > +  Status = UsbSetProtocolRequest (
> >
> > +             UsbKeyboardDevice->UsbIo,
> >
> > +             UsbKeyboardDevice->InterfaceDescriptor.InterfaceNumber,
> >
> > +             BOOT_PROTOCOL
> >
> > +             );
> >
> > +  if (EFI_ERROR (Status)) {
> >
> > +    //
> >
> > +    // If protocol could not be set here, it means
> >
> > +    // the keyboard interface has some errors and could
> >
> > +    // not be initialized
> >
> > +    //
> >
> > +    REPORT_STATUS_CODE_WITH_DEVICE_PATH (
> >
> > +      EFI_ERROR_CODE | EFI_ERROR_MINOR,
> >
> > +      (EFI_PERIPHERAL_KEYBOARD | EFI_P_EC_INTERFACE_ERROR),
> >
> > +      UsbKeyboardDevice->DevicePath
> >
> >        );
> >
> > +
> >
> > +    return EFI_DEVICE_ERROR;
> >
> >    }
>
>
> Sorry for a question, I do not understand why the proposed change will eliminate the boot delay.
> For device that does not respond to GetProtocolRequest(), the origin flow is like:
> a.1 Timeout occurs for GetProtocolRequest()
> a.2 Conditionally execute UsbSetProtocolRequest()
>
> The proposed new flow is like:
> b.1 Timeout occurs for GetProtocolRequest()
> b.2 Always execute UsbSetProtocolRequest()
>
> Could you help to elaborate on the change? Thanks in advance.
>
> Best Regards,
> Hao Wu
>
>
> >
> >
> >
> >    UsbKeyboardDevice->CtrlOn    = FALSE;
> >
> > --
> > 2.32.0
>