In addition to my previous letter I have to mention a couple more newly discovered details.
1. UEFI Shell (ShellPkg) actually has 3 functions dedicated to connecting controllers and essentially doing the same thing: - ConnectAllEfi ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c This one locates all handles and runs connect on them. It is the one we mentioned in the bug. - LoadPciRomConnectAllDriversToAllControllers ShellPkg/Library/UefiShellDebug1CommandsLib/LoadPciRom.c This one is similar to ConnectAllEfi. The only difference is that it additionally checks for Ctrl+C via ShellGetExecutionBreakFlag. - ConnectControllers ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c This one is more complex, as it supports explicitly connecting specified controllers, however, for connecting all controllers it locates handles with gEfiDevicePathProtocolGuid. I.e. exactly what we ask. I believe that all these functions should behave the same way in correspondence to gBS->ConnectController at the very least, and most likely there should be a library responsible for connection and disconnection. 2. We also checked EFI 1.1 Shell, and confirmed that it consistently has checks for device path presence before running connect on the handle[1][2]. This makes us believe that our proposal is not really a firmware bug, but rather a limitation of the legacy specification. Considering all these discoveries, we believe our suggested change is legit depending on the minimum supported UEFI version. Therefore we propose submitting a ControllerConnection library with 5 functions: - ConnectController - DisconnectController - GetAllControllers - ConnectAllControllers - DisconnectAllControllers Adopting it throughout the code will let the distribitor to be responsible for choosing what is right for his applications (or firmwares). Best wishes, Vitaly [1] https://sourceforge.net/p/efi-shell/code/64/tree/trunk/Shell/LoadPciRom/LoadPciRom.c#l528 [2] https://sourceforge.net/p/efi-shell/code/64/tree/trunk/Shell/load/load.c#l312 > 14 янв. 2020 г., в 11:36, vit9696 <vit9...@protonmail.com> написал(а): > > Ray, > > We are quite reluctant to have patches in EDK II for a large amount of widely > adopted firmwares. Patches eventually break and require maintenance cost, and > currently we are trying to get rid of them all. We believe that EDK II Shell > is supposed to work on real world platforms and not only the ones that > theoretically support the specification. It is always hard to adopt changes > based on third-party bugs, and we very well understand your concern, yet it > is something we have to do to stay beneficial to the end user. > > Best wishes, > Vitaly > > On Tue, Jan 14, 2020 at 05:53, Ni, Ray <ray...@intel.com > <mailto:ray...@intel.com>> wrote: >> >> Vitaly, >> >> I still have concern to modify the EDKII code to workaround a firmware bug. >> >> Can you just change in your local version? >> >> >> Thanks, >> >> Ray >> >> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vitaly >> Cheptsov via Groups.Io >> Sent: Tuesday, January 14, 2020 4:47 AM >> To: Laszlo Ersek <ler...@redhat.com>; devel@edk2.groups.io; Ni, Ray >> <ray...@intel.com>; Gao, Zhichao <zhichao....@intel.com> >> Subject: Re: [edk2-devel] [PATCH 1/1] ShellPkg: Do not connect handles >> without device paths >> >> >> Thanks all for your input, >> >> These explanations seem sufficient to us that it is not a good idea to >> change the behaviour for everyone. Even so, we still need this to be >> configurable in some way, as having to patch EDK II is impracticable. >> >> We believe there are three possible routes to approach this problem: >> >> Introduce a separate ControllerConnectionLib library for this function. >> While it is small, we found several places in our code that need to call it >> beyond UEFI Shell. This way different implementations could be used >> depending on the chosen library. >> Introduce a ConnectRequiresDevicePath PCD, which will choose the preferred >> logic. >> Introduce a -dp Shell argument for affected commands the way Lazslo >> suggested. >> >> We believe either route or a combination of multiple routes have their own >> benefits, and would suggest either going with 1+2 or with 3. Any approach is >> fine for us. >> >> We will submit V2 of the patch after hearing the opinions. >> >> Best wishes, >> Vitaly >> >> On Mon, Jan 13, 2020 at 20:55, Laszlo Ersek <ler...@redhat.com >> <mailto:ler...@redhat.com>> wrote: >> >> On 01/13/20 12:56, Ni, Ray wrote: >> > We shouldn't assume that a DriverBindingStart() can only start on a handle >> > with device path installed. DevicePath protocol is just a special protocol. >> > It's possible that a bus driver starts on a host controller handle and >> > creates multiple children, each with only a Specific_IO protocol installed. >> > Certain device driver can start on the children handle and open the >> > Specific_IO protocol BY_DRIVER. >> > I am not sure if certain today's network drivers may work like this. It's >> > allowed per UEFI spec. >> >> I agree. >> >> Under "10.2 EFI Device Path Protocol", the spec says, "If the handle >> does not logically map to a physical device, the handle may not >> necessarily support the device path protocol." >> >> I think gBS->ConnectController() and >> EFI_DRIVER_BINDING_PROTOCOL.Supported() should work on such handles. >> >> If we'd like to work around related issues in drivers, then I'd suggest >> new command line options for the "load", "connect", "reconnect" shell >> commands (maybe more), for filtering out handles that do not carry >> device paths. Such command line options could be added as an extension, >> IIUC, such as "-_option". I.e., I believe it's not necessary to start >> with UEFI Shell Spec updates. >> >> Thanks >> Laszlo >> >> >> >> >> > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#53214): https://edk2.groups.io/g/devel/message/53214 Mute This Topic: https://groups.io/mt/69653841/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
signature.asc
Description: OpenPGP digital signature
