Inline comments below:
> -----Original Message----- > From: [email protected] <[email protected]> On Behalf Of > MrChromebox > Sent: Saturday, February 12, 2022 6:46 AM > To: Wu, Hao A <[email protected]> > Cc: Rhodes, Sean <[email protected]>; Patrick Rudolph > <[email protected]>; [email protected]; Wang, Jian J > <[email protected]>; Gao, Liming <[email protected]>; Ni, Ray > <[email protected]> > Subject: Re: [edk2-devel] [PATCH 13/18] MdeModulePkg/Usb/Keyboard.c: > don't request protocol before setting > > 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; > } I am okay with unconditionally sending the Set_Protocol command. Could you help to: 1. Just keep the previous behavior for calling UsbSetProtocolRequest() with no error handling logic? 2. Check if similar changes need to be applied in: * MdeModulePkg\Bus\Usb\UsbMouseAbsolutePointerDxe * MdeModulePkg\Bus\Usb\UsbMouseDxe If you do not have device for verifying the change, I suggest to make the change only in UsbKbDxe. 3. Send out a new V2 patch rather than replying the previous patch mail. Thanks in advance. Best Regards, Hao Wu > > UsbKeyboardDevice->CtrlOn = FALSE; > -- > 2.32.0 > > On Thu, Feb 10, 2022 at 9:32 PM Wu, Hao A <[email protected]> wrote: > > > > Hello, > > > > Inline comments below: > > > > > > > -----Original Message----- > > > From: Sean Rhodes <[email protected]> > > > Sent: Friday, February 11, 2022 5:27 AM > > > To: [email protected] > > > Cc: Wu, Hao A <[email protected]>; Matt DeVillier > > > <[email protected]>; Patrick Rudolph > > > <[email protected]> > > > Subject: [PATCH 13/18] MdeModulePkg/Usb/Keyboard.c: don't request > > > protocol before setting > > > > > > From: Matt DeVillier <[email protected]> > > > > > > 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 <[email protected]> > > > Signed-off-by: Patrick Rudolph <[email protected]> > > > --- > > > 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 (#86745): https://edk2.groups.io/g/devel/message/86745 Mute This Topic: https://groups.io/mt/89056141/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
