From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.5728.1586425044293936661 for ; Thu, 09 Apr 2020 02:37:24 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 95F5C31B; Thu, 9 Apr 2020 02:37:23 -0700 (PDT) Received: from [10.37.8.121] (unknown [10.37.8.121]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 2132D3F73D; Thu, 9 Apr 2020 02:37:20 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/DeviceManagerUiLib: connect all before creating menu page To: devel@edk2.groups.io, lersek@redhat.com Cc: Zhichao Gao , Ray Ni , Jian J Wang , Hao A Wu , Dandan Bi References: <20200408172807.10108-1-ard.biesheuvel@arm.com> <1e1d4c9d-01ed-2b65-69ec-278f95309d25@redhat.com> From: "Ard Biesheuvel" Message-ID: Date: Thu, 9 Apr 2020 11:37:19 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: <1e1d4c9d-01ed-2b65-69ec-278f95309d25@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >> Cc: Ray Ni >> Cc: Jian J Wang >> Cc: Hao A Wu >> Cc: Dandan Bi >> Signed-off-by: Ard Biesheuvel >> --- >> 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 >> #include >> #include >> +#include >> #include >> >> // >> 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] >> > > > >