[edk2-devel] [PATCH 13/18] MdeModulePkg/Usb/Keyboard.c: don't request protocol before setting
MrChromebox
matt.devillier at gmail.com
Fri Feb 11 22:45:58 UTC 2022
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 at intel.com> wrote:
>
> Hello,
>
> Inline comments below:
>
>
> > -----Original Message-----
> > From: Sean Rhodes <sean at starlabs.systems>
> > Sent: Friday, February 11, 2022 5:27 AM
> > To: devel at edk2.groups.io
> > Cc: Wu, Hao A <hao.a.wu at intel.com>; Matt DeVillier
> > <matt.devillier at gmail.com>; Patrick Rudolph
> > <patrick.rudolph at 9elements.com>
> > Subject: [PATCH 13/18] MdeModulePkg/Usb/Keyboard.c: don't request
> > protocol before setting
> >
> > From: Matt DeVillier <matt.devillier at 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 at gmail.com>
> > Signed-off-by: Patrick Rudolph <patrick.rudolph at 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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#86627): https://edk2.groups.io/g/devel/message/86627
Mute This Topic: https://groups.io/mt/89056141/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list