Thanks/Ray
> -----Original Message----- > From: Zeng, Star > Sent: Thursday, April 26, 2018 3:05 PM > To: Ni, Ruiyu <[email protected]>; [email protected] > Cc: Wu, Hao A <[email protected]>; Kinney, Michael D > <[email protected]>; Zeng, Star <[email protected]> > Subject: RE: [PATCH] MdeModulePkg/ConPlatform: Support short-form USB > device path > > Ray, > > Some minor comments below. > > 1. How MSG_USB_DP and HW_PCCARD_DP be handled? We see they are > checked in original IsHotPlugDevice()? Original implementation to always accept USB/PCCARD device as active console is wrong. So these code is just removed. > > 2. gEfiUsbIoProtocolGuid needs be stated in ConPlatformDxe.inf? Thanks. I will fix it. > > 3. The comment " If it is not a hot-plug device, append the device path to " > in > ConPlatformTextInDriverBindingStart()/ConPlatformTextOutDriverBindingSt > art() needs be updated accordingly since IsHotPlugDevice() will be removed? Thanks. I will fix it in v2 patch. > > > Thanks, > Star > -----Original Message----- > From: Ni, Ruiyu > Sent: Wednesday, April 25, 2018 1:44 PM > To: [email protected] > Cc: Wu, Hao A <[email protected]>; Kinney, Michael D > <[email protected]>; Zeng, Star <[email protected]> > Subject: [PATCH] MdeModulePkg/ConPlatform: Support short-form USB > device path > > Today's implementation does an exact device path match to check > whether the device path of a console is in ConIn/ConOut/ErrOut. > But that doesn't work for the USB keyboard. > Because when a platform have multiple USB port, ConIn needs to > carry all device paths corresponding to each port. > Even worse, today's BDS core logic removes the device path from > ConIn/ConOut/ErrOut when the connection to that device path fails. > So if user switches the USB keyboard from one port to another across > boot, the USB keyboard doesn't work in the second boot. > > ConPlatform driver solved this problem by adding the > IsHotPlugDevice() function. So that for USB keyboard, ConPlatform > doesn't care whether its device path is in ConIn or not. > But the rule is too loose, and now causes platform BDS cannot control > whether to enable USB keyboard as an active console. > > The patch changes ConPlatform to support USB short-form device path > when checking whether the device path of a console is in > ConIn/ConOut/ErrOut. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ruiyu Ni <[email protected]> > Cc: Hao A Wu <[email protected]> > Cc: Michael D Kinney <[email protected]> > Cc: Star Zeng <[email protected]> > --- > .../Universal/Console/ConPlatformDxe/ConPlatform.c | 526 > ++++++++++++++------- > .../Universal/Console/ConPlatformDxe/ConPlatform.h | 20 +- > 2 files changed, 353 insertions(+), 193 deletions(-) > > diff --git > a/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c > b/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c > index 6b53e8ac74..a509d0e3a5 100644 > --- a/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c > +++ b/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c > @@ -2,7 +2,7 @@ > Console Platform DXE Driver, install Console Device Guids and update > Console > Environment Variables. > > -Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR> > +Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR> > This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD > License > which accompanies this distribution. The full text of the license may be > found at > @@ -270,58 +270,33 @@ ConPlatformTextInDriverBindingStart ( > } > > // > - // Check the device path, if it is a hot plug device, > - // do not put the device path into ConInDev, and install > - // gEfiConsoleInDeviceGuid to the device handle directly. > - // The policy is, make hot plug device plug in and play immediately. > + // If it is not a hot-plug device, append the device path to the > + // ConInDev environment variable > // > - if (IsHotPlugDevice (DevicePath)) { > + ConPlatformUpdateDeviceVariable ( > + L"ConInDev", > + DevicePath, > + Append > + ); > + > + // > + // If the device path is an instance in the ConIn environment variable, > + // then install EfiConsoleInDeviceGuid onto ControllerHandle > + // > + if (IsInConInVariable) { > gBS->InstallMultipleProtocolInterfaces ( > &ControllerHandle, > &gEfiConsoleInDeviceGuid, > NULL, > NULL > ); > - // > - // Append the device path to ConInDev only if it is in ConIn variable. > - // > - if (IsInConInVariable) { > - ConPlatformUpdateDeviceVariable ( > - L"ConInDev", > - DevicePath, > - Append > - ); > - } > } else { > - // > - // If it is not a hot-plug device, append the device path to the > - // ConInDev environment variable > - // > - ConPlatformUpdateDeviceVariable ( > - L"ConInDev", > - DevicePath, > - Append > - ); > - > - // > - // If the device path is an instance in the ConIn environment variable, > - // then install EfiConsoleInDeviceGuid onto ControllerHandle > - // > - if (IsInConInVariable) { > - gBS->InstallMultipleProtocolInterfaces ( > - &ControllerHandle, > - &gEfiConsoleInDeviceGuid, > - NULL, > - NULL > - ); > - } else { > - gBS->CloseProtocol ( > - ControllerHandle, > - &gEfiSimpleTextInProtocolGuid, > - This->DriverBindingHandle, > - ControllerHandle > - ); > - } > + gBS->CloseProtocol ( > + ControllerHandle, > + &gEfiSimpleTextInProtocolGuid, > + This->DriverBindingHandle, > + ControllerHandle > + ); > } > > return EFI_SUCCESS; > @@ -416,95 +391,60 @@ ConPlatformTextOutDriverBindingStart ( > } > > // > - // Check the device path, if it is a hot plug device, > - // do not put the device path into ConOutDev and ErrOutDev, > - // and install gEfiConsoleOutDeviceGuid to the device handle directly. > - // The policy is, make hot plug device plug in and play immediately. > + // If it is not a hot-plug device, append the device path to > + // the ConOutDev and ErrOutDev environment variable. > + // For GOP device path, append the sibling device path as well. > + // > + if (!ConPlatformUpdateGopCandidate (DevicePath)) { > + ConPlatformUpdateDeviceVariable ( > + L"ConOutDev", > + DevicePath, > + Append > + ); > + // > + // Then append the device path to the ErrOutDev environment variable > + // > + ConPlatformUpdateDeviceVariable ( > + L"ErrOutDev", > + DevicePath, > + Append > + ); > + } > + > + // > + // If the device path is an instance in the ConOut environment variable, > + // then install EfiConsoleOutDeviceGuid onto ControllerHandle > + // > + if (IsInConOutVariable) { > + NeedClose = FALSE; > + Status = gBS->InstallMultipleProtocolInterfaces ( > + &ControllerHandle, > + &gEfiConsoleOutDeviceGuid, > + NULL, > + NULL > + ); > + } > + // > + // If the device path is an instance in the ErrOut environment variable, > + // then install EfiStandardErrorDeviceGuid onto ControllerHandle > // > - if (IsHotPlugDevice (DevicePath)) { > + if (IsInErrOutVariable) { > + NeedClose = FALSE; > gBS->InstallMultipleProtocolInterfaces ( > &ControllerHandle, > - &gEfiConsoleOutDeviceGuid, > + &gEfiStandardErrorDeviceGuid, > NULL, > NULL > ); > - // > - // Append the device path to ConOutDev only if it is in ConOut variable. > - // > - if (IsInConOutVariable) { > - ConPlatformUpdateDeviceVariable ( > - L"ConOutDev", > - DevicePath, > - Append > - ); > - } > - // > - // Append the device path to ErrOutDev only if it is in ErrOut variable. > - // > - if (IsInErrOutVariable) { > - ConPlatformUpdateDeviceVariable ( > - L"ErrOutDev", > - DevicePath, > - Append > - ); > - } > - } else { > - // > - // If it is not a hot-plug device, append the device path to > - // the ConOutDev and ErrOutDev environment variable. > - // For GOP device path, append the sibling device path as well. > - // > - if (!ConPlatformUpdateGopCandidate (DevicePath)) { > - ConPlatformUpdateDeviceVariable ( > - L"ConOutDev", > - DevicePath, > - Append > - ); > - // > - // Then append the device path to the ErrOutDev environment variable > - // > - ConPlatformUpdateDeviceVariable ( > - L"ErrOutDev", > - DevicePath, > - Append > - ); > - } > - > - // > - // If the device path is an instance in the ConOut environment variable, > - // then install EfiConsoleOutDeviceGuid onto ControllerHandle > - // > - if (IsInConOutVariable) { > - NeedClose = FALSE; > - Status = gBS->InstallMultipleProtocolInterfaces ( > - &ControllerHandle, > - &gEfiConsoleOutDeviceGuid, > - NULL, > - NULL > - ); > - } > - // > - // If the device path is an instance in the ErrOut environment variable, > - // then install EfiStandardErrorDeviceGuid onto ControllerHandle > - // > - if (IsInErrOutVariable) { > - NeedClose = FALSE; > - gBS->InstallMultipleProtocolInterfaces ( > - &ControllerHandle, > - &gEfiStandardErrorDeviceGuid, > - NULL, > - NULL > - ); > - } > + } > > - if (NeedClose) { > - gBS->CloseProtocol ( > - ControllerHandle, > - &gEfiSimpleTextOutProtocolGuid, > - This->DriverBindingHandle, > - ControllerHandle > - ); > - } > + if (NeedClose) { > + gBS->CloseProtocol ( > + ControllerHandle, > + &gEfiSimpleTextOutProtocolGuid, > + This->DriverBindingHandle, > + ControllerHandle > + ); > } > > return EFI_SUCCESS; > @@ -822,6 +762,288 @@ IsGopSibling ( > return (BOOLEAN) (CompareMem (Left, Right, (UINTN) NodeLeft - (UINTN) > Left) == 0); > } > > +/** > + Check whether a USB device match the specified USB Class device path. > This > + function follows "Load Option Processing" behavior in UEFI specification. > + > + @param UsbIo USB I/O protocol associated with the USB device. > + @param UsbClass The USB Class device path to match. > + > + @retval TRUE The USB device match the USB Class device path. > + @retval FALSE The USB device does not match the USB Class device path. > + > +**/ > +BOOLEAN > +MatchUsbClass ( > + IN EFI_USB_IO_PROTOCOL *UsbIo, > + IN USB_CLASS_DEVICE_PATH *UsbClass > + ) > +{ > + EFI_STATUS Status; > + EFI_USB_DEVICE_DESCRIPTOR DevDesc; > + EFI_USB_INTERFACE_DESCRIPTOR IfDesc; > + UINT8 DeviceClass; > + UINT8 DeviceSubClass; > + UINT8 DeviceProtocol; > + > + if ((DevicePathType (UsbClass) != MESSAGING_DEVICE_PATH) || > + (DevicePathSubType (UsbClass) != MSG_USB_CLASS_DP)){ > + return FALSE; > + } > + > + // > + // Check Vendor Id and Product Id. > + // > + Status = UsbIo->UsbGetDeviceDescriptor (UsbIo, &DevDesc); > + if (EFI_ERROR (Status)) { > + return FALSE; > + } > + > + if ((UsbClass->VendorId != 0xffff) && > + (UsbClass->VendorId != DevDesc.IdVendor)) { > + return FALSE; > + } > + > + if ((UsbClass->ProductId != 0xffff) && > + (UsbClass->ProductId != DevDesc.IdProduct)) { > + return FALSE; > + } > + > + DeviceClass = DevDesc.DeviceClass; > + DeviceSubClass = DevDesc.DeviceSubClass; > + DeviceProtocol = DevDesc.DeviceProtocol; > + if (DeviceClass == 0) { > + // > + // If Class in Device Descriptor is set to 0, use the Class, SubClass and > + // Protocol in Interface Descriptor instead. > + // > + Status = UsbIo->UsbGetInterfaceDescriptor (UsbIo, &IfDesc); > + if (EFI_ERROR (Status)) { > + return FALSE; > + } > + > + DeviceClass = IfDesc.InterfaceClass; > + DeviceSubClass = IfDesc.InterfaceSubClass; > + DeviceProtocol = IfDesc.InterfaceProtocol; > + } > + > + // > + // Check Class, SubClass and Protocol. > + // > + if ((UsbClass->DeviceClass != 0xff) && > + (UsbClass->DeviceClass != DeviceClass)) { > + return FALSE; > + } > + > + if ((UsbClass->DeviceSubClass != 0xff) && > + (UsbClass->DeviceSubClass != DeviceSubClass)) { > + return FALSE; > + } > + > + if ((UsbClass->DeviceProtocol != 0xff) && > + (UsbClass->DeviceProtocol != DeviceProtocol)) { > + return FALSE; > + } > + > + return TRUE; > +} > + > +/** > + Check whether a USB device match the specified USB WWID device path. > This > + function follows "Load Option Processing" behavior in UEFI specification. > + > + @param UsbIo USB I/O protocol associated with the USB device. > + @param UsbWwid The USB WWID device path to match. > + > + @retval TRUE The USB device match the USB WWID device path. > + @retval FALSE The USB device does not match the USB WWID device > path. > + > +**/ > +BOOLEAN > +MatchUsbWwid ( > + IN EFI_USB_IO_PROTOCOL *UsbIo, > + IN USB_WWID_DEVICE_PATH *UsbWwid > + ) > +{ > + EFI_STATUS Status; > + EFI_USB_DEVICE_DESCRIPTOR DevDesc; > + EFI_USB_INTERFACE_DESCRIPTOR IfDesc; > + UINT16 *LangIdTable; > + UINT16 TableSize; > + UINT16 Index; > + CHAR16 *CompareStr; > + UINTN CompareLen; > + CHAR16 *SerialNumberStr; > + UINTN Length; > + > + if ((DevicePathType (UsbWwid) != MESSAGING_DEVICE_PATH) || > + (DevicePathSubType (UsbWwid) != MSG_USB_WWID_DP)) { > + return FALSE; > + } > + > + // > + // Check Vendor Id and Product Id. > + // > + Status = UsbIo->UsbGetDeviceDescriptor (UsbIo, &DevDesc); > + if (EFI_ERROR (Status)) { > + return FALSE; > + } > + if ((DevDesc.IdVendor != UsbWwid->VendorId) || > + (DevDesc.IdProduct != UsbWwid->ProductId)) { > + return FALSE; > + } > + > + // > + // Check Interface Number. > + // > + Status = UsbIo->UsbGetInterfaceDescriptor (UsbIo, &IfDesc); > + if (EFI_ERROR (Status)) { > + return FALSE; > + } > + if (IfDesc.InterfaceNumber != UsbWwid->InterfaceNumber) { > + return FALSE; > + } > + > + // > + // Check Serial Number. > + // > + if (DevDesc.StrSerialNumber == 0) { > + return FALSE; > + } > + > + // > + // Get all supported languages. > + // > + TableSize = 0; > + LangIdTable = NULL; > + Status = UsbIo->UsbGetSupportedLanguages (UsbIo, &LangIdTable, > &TableSize); > + if (EFI_ERROR (Status) || (TableSize == 0) || (LangIdTable == NULL)) { > + return FALSE; > + } > + > + // > + // Serial number in USB WWID device path is the last 64-or-less UTF-16 > characters. > + // > + CompareStr = (CHAR16 *) (UINTN) (UsbWwid + 1); > + CompareLen = (DevicePathNodeLength (UsbWwid) - sizeof > (USB_WWID_DEVICE_PATH)) / sizeof (CHAR16); > + if (CompareStr[CompareLen - 1] == L'\0') { > + CompareLen--; > + } > + > + // > + // Compare serial number in each supported language. > + // > + for (Index = 0; Index < TableSize / sizeof (UINT16); Index++) { > + SerialNumberStr = NULL; > + Status = UsbIo->UsbGetStringDescriptor ( > + UsbIo, > + LangIdTable[Index], > + DevDesc.StrSerialNumber, > + &SerialNumberStr > + ); > + if (EFI_ERROR (Status) || (SerialNumberStr == NULL)) { > + continue; > + } > + > + Length = StrLen (SerialNumberStr); > + if ((Length >= CompareLen) && > + (CompareMem (SerialNumberStr + Length - CompareLen, CompareStr, > CompareLen * sizeof (CHAR16)) == 0)) { > + FreePool (SerialNumberStr); > + return TRUE; > + } > + > + FreePool (SerialNumberStr); > + } > + > + return FALSE; > +} > + > +/** > + Compare whether a full console device path matches a USB shortform > device path. > + > + @param[in] FullPath Full console device path. > + @param[in] ShortformPath Short-form device path. Short-form device > node may in the beginning or in the middle. > + > + @retval TRUE The full console device path matches the short-form device > path. > + @retval FALSE The full console device path doesn't match the short-form > device path. > +**/ > +BOOLEAN > +MatchUsbShortformDevicePath ( > + IN EFI_DEVICE_PATH_PROTOCOL *FullPath, > + IN EFI_DEVICE_PATH_PROTOCOL *ShortformPath > + ) > +{ > + EFI_STATUS Status; > + EFI_DEVICE_PATH_PROTOCOL *ShortformNode; > + EFI_DEVICE_PATH_PROTOCOL *ShortformRemainingDevicePath; > + UINTN ParentDevicePathSize; > + EFI_DEVICE_PATH_PROTOCOL *RemainingDevicePath; > + EFI_USB_IO_PROTOCOL *UsbIo; > + EFI_HANDLE Handle; > + > + for ( ShortformNode = ShortformPath > + ; !IsDevicePathEnd (ShortformNode) > + ; ShortformNode = NextDevicePathNode (ShortformNode) > + ) { > + if ((DevicePathType (ShortformNode) == MESSAGING_DEVICE_PATH) && > + ((DevicePathSubType (ShortformNode) == MSG_USB_CLASS_DP) || > + (DevicePathSubType (ShortformNode) == MSG_USB_WWID_DP)) > + ) { > + break; > + } > + } > + > + // > + // Skip further compare when it's not a shortform device path. > + // > + if (IsDevicePathEnd (ShortformNode)) { > + return FALSE; > + } > + > + // > + // Match the parent device path below USB layer when the ShortformPath > doesn't start with short-form node. > + // > + ParentDevicePathSize = (UINTN) ShortformNode - (UINTN) > ShortformPath; > + ShortformRemainingDevicePath = NextDevicePathNode (ShortformNode); > + > + RemainingDevicePath = FullPath; > + Status = gBS->LocateDevicePath (&gEfiUsbIoProtocolGuid, > &RemainingDevicePath, &Handle); > + if (EFI_ERROR (Status)) { > + return FALSE; > + } > + > + if (ParentDevicePathSize != 0) { > + // > + // Skip comparing the parent device path when ShortformPath begins > with short-form node. > + // > + if ((ParentDevicePathSize != (UINTN) RemainingDevicePath - (UINTN) > FullPath) || > + (CompareMem (FullPath, ShortformPath, ParentDevicePathSize) != 0)) > { > + return FALSE; > + } > + } > + > + // > + // Match the remaining device path above USB layer. > + // For USB keyboard, the remaining device path above USB layer is END > node. > + // > + if (CompareMem (RemainingDevicePath, ShortformRemainingDevicePath, > GetDevicePathSize (RemainingDevicePath)) != 0) { > + return FALSE; > + } > + > + // > + // Match the USB layer. > + // > + Status = gBS->HandleProtocol( > + Handle, > + &gEfiUsbIoProtocolGuid, > + (VOID **) &UsbIo > + ); > + ASSERT_EFI_ERROR (Status); > + > + return MatchUsbClass (UsbIo, (USB_CLASS_DEVICE_PATH > *)ShortformNode) || > + MatchUsbWwid (UsbIo, (USB_WWID_DEVICE_PATH > *)ShortformNode); > +} > + > /** > Function compares a device path data structure to that of all the nodes of > a > second device path instance. > @@ -881,7 +1103,8 @@ ConPlatformMatchDevicePaths ( > // Search for the match of 'Single' in 'Multi' > // > while (DevicePathInst != NULL) { > - if ((CompareMem (Single, DevicePathInst, Size) == 0) || IsGopSibling > (Single, DevicePathInst)) { > + if ((CompareMem (Single, DevicePathInst, Size) == 0) || > + IsGopSibling (Single, DevicePathInst) || MatchUsbShortformDevicePath > (Single, DevicePathInst)) { > if (!Delete) { > // > // If Delete is FALSE, return EFI_SUCCESS if Single is found in > Multi. > @@ -1029,53 +1252,6 @@ ConPlatformUpdateDeviceVariable ( > return Status; > } > > -/** > - Check if the device supports hot-plug through its device path. > - > - This function could be updated to check more types of Hot Plug devices. > - Currently, it checks USB and PCCard device. > - > - @param DevicePath Pointer to device's device path. > - > - @retval TRUE The devcie is a hot-plug device > - @retval FALSE The devcie is not a hot-plug device. > - > -**/ > -BOOLEAN > -IsHotPlugDevice ( > - IN EFI_DEVICE_PATH_PROTOCOL *DevicePath > - ) > -{ > - EFI_DEVICE_PATH_PROTOCOL *CheckDevicePath; > - > - CheckDevicePath = DevicePath; > - while (!IsDevicePathEnd (CheckDevicePath)) { > - // > - // Check device whether is hot plug device or not throught Device Path > - // > - if ((DevicePathType (CheckDevicePath) == MESSAGING_DEVICE_PATH) > && > - (DevicePathSubType (CheckDevicePath) == MSG_USB_DP || > - DevicePathSubType (CheckDevicePath) == MSG_USB_CLASS_DP || > - DevicePathSubType (CheckDevicePath) == MSG_USB_WWID_DP)) { > - // > - // If Device is USB device > - // > - return TRUE; > - } > - if ((DevicePathType (CheckDevicePath) == HARDWARE_DEVICE_PATH) > && > - (DevicePathSubType (CheckDevicePath) == HW_PCCARD_DP)) { > - // > - // If Device is PCCard > - // > - return TRUE; > - } > - > - CheckDevicePath = NextDevicePathNode (CheckDevicePath); > - } > - > - return FALSE; > -} > - > /** > Update ConOutDev and ErrOutDev variables to add the device path of > GOP controller itself and the sibling controllers. > diff --git > a/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.h > b/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.h > index 6d853c1360..28f882afeb 100644 > --- a/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.h > +++ b/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.h > @@ -1,7 +1,7 @@ > /** @file > Header file for Console Platfrom DXE Driver. > > -Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR> > +Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR> > This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD > License > which accompanies this distribution. The full text of the license may be > found at > @@ -21,6 +21,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY > KIND, EITHER EXPRESS OR IMPLIED. > #include <Protocol/DevicePath.h> > #include <Protocol/SimpleTextIn.h> > #include <Protocol/PciIo.h> > +#include <Protocol/UsbIo.h> > #include <Protocol/GraphicsOutput.h> > > #include <Guid/GlobalVariable.h> > @@ -294,23 +295,6 @@ ConPlatformUpdateDeviceVariable ( > IN CONPLATFORM_VAR_OPERATION Operation > ); > > -/** > - Check if the device supports hot-plug through its device path. > - > - This function could be updated to check more types of Hot Plug devices. > - Currently, it checks USB and PCCard device. > - > - @param DevicePath Pointer to device's device path. > - > - @retval TRUE The devcie is a hot-plug device > - @retval FALSE The devcie is not a hot-plug device. > - > -**/ > -BOOLEAN > -IsHotPlugDevice ( > - IN EFI_DEVICE_PATH_PROTOCOL *DevicePath > - ); > - > // > // EFI Component Name Functions > // > -- > 2.16.1.windows.1 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
