On 04/09/20 11:37, Ard Biesheuvel wrote: > 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.
OK. > 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. Yes, this was the connect-all I seemed to recall. > 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. Sigh, thanks. I understand now. I assumed that DeviceManagerUiLib's constructor (linked into UiApp via NULL class resolution) would only register a callback into a list. Then UiApp would run that list of callbacks at the right time. This is a frequently used pattern in edk2 (with various "pluggable" modules). > >> 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. I agree. New APIs would likely be needed. Thanks for the explanation. If the MdeModulePkg folks are happy with this patch, I have nothing against it! In that case, I think it would be helpful to include a short version of the above explanation in the commit message (no need to repost, of course). Thanks! Laszlo > > >>> 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 (#57125): https://edk2.groups.io/g/devel/message/57125 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] -=-=-=-=-=-=-=-=-=-=-=-
