On 04/12/20 10:11, Tian, Hot wrote: > EfiBootManagerConnectAll is kind of BDS policy. Should it be controlled by Ui > App or Ui Lib?
I think platform BDS policy applies to normal (non-interactive, non-interrupted) boot. I agree that connect-all should not be forced into that. But, if the user intentionally enters the setup TUI, or else we fall back to the setup TUI because there's nothing to boot -- see commit range cef7ecf6cdb4..1010873becc5 --, then compatibility (i.e., connect everything we can) is arguably more important than "speed". Because, we're not (immediately) booting an OS anyway, at that point. IOW, I think connect-all should stay in UiApp. Thanks Laszlo > > Thanks, > Hot > > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dandan Bi > Sent: Sunday, April 12, 2020 15:56 > To: devel@edk2.groups.io; ard.biesheu...@arm.com; ler...@redhat.com > Cc: Gao, Zhichao <zhichao....@intel.com>; Ni, Ray <ray...@intel.com>; Wang, > Jian J <jian.j.w...@intel.com>; Wu, Hao A <hao.a...@intel.com> > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/DeviceManagerUiLib: connect > all before creating menu page > > Hi Ard, > > It seems that the root cause is the 'Network Device List' in the device > manager menu is crated before EfiBootManagerConnectAll () is called in > UiEntry function. > If we choose to add the EfiBootManagerConnectAll() in DeviceManagerUiLib with > this patch, could we don't call the EfiBootManagerConnectAll() in UiEntry to > avoid it's called twice when enter UiApp? > > > Thanks, > Dandan >> -----Original Message----- >> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of >> Ard Biesheuvel >> Sent: Thursday, April 9, 2020 5:37 PM >> To: devel@edk2.groups.io; ler...@redhat.com >> Cc: Gao, Zhichao <zhichao....@intel.com>; Ni, Ray <ray...@intel.com>; >> Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A <hao.a...@intel.com>; >> Bi, Dandan <dandan...@intel.com> >> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/DeviceManagerUiLib: >> connect all before creating menu page >> >> On 4/9/20 11:29 AM, Laszlo Ersek via groups.io wrote: >>> On 04/08/20 19:28, Ard Biesheuvel wrote: >>>> The device manager UI library creates a UiApp submenu that contains >>>> a list of network devices in the system. The logic that creates >>>> this menu assumes that all handles have been connected to their >>>> drivers, but this is not guaranteed in the general case. >>>> >>>> So work around this by doing an explicit ConnectAll() before >>>> populating the pages. >>>> >>>> Cc: Zhichao Gao <zhichao....@intel.com> >>>> Cc: Ray Ni <ray...@intel.com> >>>> Cc: Jian J Wang <jian.j.w...@intel.com> >>>> Cc: Hao A Wu <hao.a...@intel.com> >>>> Cc: Dandan Bi <dandan...@intel.com> >>>> Signed-off-by: Ard Biesheuvel <ard.biesheu...@arm.com> >>>> --- >>>> MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c | 7 >> +++++++ >>>> MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.h | 1 + >>>> MdeModulePkg/Library/DeviceManagerUiLib/DeviceManagerUiLib.inf | >> 1 + >>>> 3 files changed, 9 insertions(+) >>> >>> Can you describe one example that's (a) relevant to you and (b) >>> affected by this issue? >>> >> >> Yes. The SynQuacer platform has a NIC driver that waits up to 5 >> seconds for the link to come up after it resets the PHY, which never >> happens if you boot without a network cable connected. But in the >> general case, connecting all network drivers on a typical BootOrder based >> boot should be be necessary. >> >> Currently, ArmPkg's PlatformBootManagerLib does a >> EfiBootManagerConnectAll () in its AfterConsole() callback, and so >> this delay is always induced. I intend to 'fix' this platform (and >> this library in general) to only do a >> EfiBootManagerConnectAllDefaultConsoles >> () at this point, which is sufficient to interrupt the boot if >> necessary, and otherwise, boot as fast as possible, without connecting >> any devices that are not needed for this. >> >> This works surprisingly well, with the exception of the 'Network >> Device List' in the device manager UiApp menu, which no longer lists >> the network interface, even though UiApp does a connect-all as it starts up. >> As you guessed, the constructor ordering versus the connect-all call >> results in the 'network device list' menu to be populated before any >> of the non- connected handles are connected. This is obviously against >> the intent of doing a connect-all and enumerating all devices, and so this >> is a bug in UiApp. >> >>> EfiBootManagerConnectAll() feels quite heavy-weight; I'd *vaguely* >>> prefer adding it elsewhere, instead of a "UI lib"'s constructor. I >>> don't have a particular suggestion however. >>> >>> I guess part of the issue must be that the >>> EfiBootManagerConnectAll() calls in BootManagerMenuApp / UiApp are >>> not reached, somehow. Maybe those are the calls that DeviceManagerUiLib >>> takes for granted. >>> >> >> I agree that this approach is not the most elegant, but the way UiApp >> is constructed makes it very hard to find a better way without some >> major refactoring. >> >> >>>> diff --git >> a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c >>>> b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c >>>> index 0540e6fa8a44..3bc13d340775 100644 >>>> --- a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c >>>> +++ b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c >>>> @@ -892,6 +892,13 @@ DeviceManagerUiLibConstructor ( >>>> ); >>>> ASSERT (gDeviceManagerPrivate.HiiHandle != NULL); >>>> >>>> + // >>>> + // The device manager form contains a page listing all the >>>> + network // controllers in the system. This list can only be >>>> + populated if all // handles have been connected, so do it here. >>>> + // >>>> + EfiBootManagerConnectAll (); >>>> + >>>> // >>>> // Update boot manager page >>>> // >>>> diff --git >> a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.h >>>> b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.h >>>> index 22fe12d2a5e8..c53c2a1a0e1a 100644 >>>> --- a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.h >>>> +++ b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.h >>>> @@ -23,6 +23,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent >>>> #include <Library/BaseLib.h> >>>> #include <Library/HiiLib.h> >>>> #include <Library/DevicePathLib.h> >>>> +#include <Library/UefiBootManagerLib.h> >>>> #include <Library/UefiHiiServicesLib.h> >>>> >>>> // >>>> diff --git >>>> a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManagerUiLib.inf >>>> b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManagerUiLib.inf >>>> index cb01b3b85180..d7f833d8b23a 100644 >>>> --- >> a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManagerUiLib.inf >>>> +++ >> b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManagerUiLib.inf >>>> @@ -40,6 +40,7 @@ [LibraryClasses] >>>> DebugLib >>>> PrintLib >>>> HiiLib >>>> + UefiBootManagerLib >>>> UefiHiiServicesLib >>>> >>>> [Guids] >>>> >>> >>> >>> >>> >> >> >> > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#57305): https://edk2.groups.io/g/devel/message/57305 Mute This Topic: https://groups.io/mt/72879609/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
