From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f43.google.com (mail-wr1-f43.google.com [209.85.221.43]) by mx.groups.io with SMTP id smtpd.web09.2803.1644619571219783770 for ; Fri, 11 Feb 2022 14:46:11 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20210112 header.b=RR0MqANO; spf=pass (domain: gmail.com, ip: 209.85.221.43, mailfrom: matt.devillier@gmail.com) Received: by mail-wr1-f43.google.com with SMTP id d27so17514935wrc.6 for ; Fri, 11 Feb 2022 14:46:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=qwXjvxSdzH7iLUYpHU2uI/TVcj+ef6dm2n2IsLz43R8=; b=RR0MqANOFivu02pSaFAXzGQcU//k7NH6roBv45zuY0gP9WkV6v83eNPPtWXlarxP8Z O1cnsYRJrsltg0bjlBn9CVWWoKZxZhPIirerR2ZFaTRt6FNmiDbaQL8tk5HBxdT8pcvh taEBACdx+IXJRtGWIgXHSssKm0v9bL9na9n9E67G/HWPYlUlr/AjeSlX2BuhGGLonYfX seHsr4aJ0ttugenBSK/CAXyBzD2nMjjENlWXPTBAmwZ2jnE2K214gtBj7sDducFv93V2 r9ydH1puRzf4Ibv+neqeQ45j2vxCvaI7ur7P68sZto3Tjnv7J6TBM2LNIfSlzSdhKjd8 P/Kg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=qwXjvxSdzH7iLUYpHU2uI/TVcj+ef6dm2n2IsLz43R8=; b=IW2Le8HOzHk3j4OB/HnGLDXC5jNTqmlnZa++dr1rzTRkQLDSLGJCHnrVbt8n3OyYzK z3wWSUcob35AdB8FrxdZnu9Xb5tgTO3KFpH+eSIn8bBuavSQkF+Pk6pj3MMnkrZkxF0P P6+9rXb4SY/Vc2ovhzi9s+WTJkG3hcltawvatnVxO+K3L0NZYBiVvkellX7hWXf5PDa4 mwISNwDBXPAjZ9S1JZMkr8qBU4Rzr27ZPMitpbE8Q1+0m95r27fxp2aqxUBat7a9IvDW 5Pj5gQ49Xdk4sgvenC99dHA+2XlKUFvK/FRiVGBuz+LoU6TX+n9rurd+sUk1UzglvV2B YMug== X-Gm-Message-State: AOAM5328pwRSrooKKeS2Sx48ouzVpgOLEgOuK1aNTYz6nc850+CinqWg f7hS6eTE4aF4oYmHpaSZBPwf3VP7gBZoXY2xFCE= X-Google-Smtp-Source: ABdhPJwWGnqnnqlXX/zuHqsc7P+/qOXd8u0+A+ML4HlSZg+Qd1aOYRXRTC2bm1oTN3UN0uc3iefTeUxvsA2BhlZk/gQ= X-Received: by 2002:a5d:4c90:: with SMTP id z16mr2820255wrs.72.1644619569649; Fri, 11 Feb 2022 14:46:09 -0800 (PST) MIME-Version: 1.0 References: <5527aab4cdea820aa58de36fd8d58ef88f24da82.1644527848.git.sean@starlabs.systems> In-Reply-To: From: "MrChromebox" Date: Fri, 11 Feb 2022 16:45:58 -0600 Message-ID: Subject: Re: [PATCH 13/18] MdeModulePkg/Usb/Keyboard.c: don't request protocol before setting To: "Wu, Hao A" Cc: "Rhodes, Sean" , Patrick Rudolph , "devel@edk2.groups.io" , "Wang, Jian J" , "Gao, Liming" , "Ni, Ray" Content-Type: text/plain; charset="UTF-8" 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 wrote: > > Hello, > > Inline comments below: > > > > -----Original Message----- > > From: Sean Rhodes > > Sent: Friday, February 11, 2022 5:27 AM > > To: devel@edk2.groups.io > > Cc: Wu, Hao A ; Matt DeVillier > > ; Patrick Rudolph > > > > Subject: [PATCH 13/18] MdeModulePkg/Usb/Keyboard.c: don't request > > protocol before setting > > > > From: Matt DeVillier > > > > 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 > > Signed-off-by: Patrick Rudolph > > --- > > 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 >