From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by mx.groups.io with SMTP id smtpd.web11.8116.1586678150335546923 for ; Sun, 12 Apr 2020 00:55:50 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.31, mailfrom: dandan.bi@intel.com) IronPort-SDR: VSTZwUF+JlhIr/vFdUuTIVxo/+GqP6fYnQbvdrPDr0XbkhEif47iLYuEgJNDfIOZcAsEU/8aZJ 9AijTsvSuj6Q== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Apr 2020 00:55:49 -0700 IronPort-SDR: IsrQoQRlA6iVMvm84wGM8Uh1BXlSVqjwbFHEth/pW7H7q6Mk2LHhmKRMIR8hvYeaZVznQxO0yJ gkb4fxJU+mKw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.72,374,1580803200"; d="scan'208";a="298160456" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by FMSMGA003.fm.intel.com with ESMTP; 12 Apr 2020 00:55:48 -0700 Received: from fmsmsx112.amr.corp.intel.com (10.18.116.6) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 12 Apr 2020 00:55:48 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by FMSMSX112.amr.corp.intel.com (10.18.116.6) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 12 Apr 2020 00:55:47 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.225]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.209]) with mapi id 14.03.0439.000; Sun, 12 Apr 2020 15:55:45 +0800 From: "Dandan Bi" To: "devel@edk2.groups.io" , "ard.biesheuvel@arm.com" , "lersek@redhat.com" CC: "Gao, Zhichao" , "Ni, Ray" , "Wang, Jian J" , "Wu, Hao A" Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/DeviceManagerUiLib: connect all before creating menu page Thread-Topic: [edk2-devel] [PATCH] MdeModulePkg/DeviceManagerUiLib: connect all before creating menu page Thread-Index: AQHWDcsz9TXzQBDqTEOYsaCFZ8oQ2KhwAPoAgAACQICABR3ekA== Date: Sun, 12 Apr 2020 07:55:45 +0000 Message-ID: <3C0D5C461C9E904E8F62152F6274C0BB40DB1A8A@SHSMSX104.ccr.corp.intel.com> References: <20200408172807.10108-1-ard.biesheuvel@arm.com> <1e1d4c9d-01ed-2b65-69ec-278f95309d25@redhat.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: dandan.bi@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Ard, It seems that the root cause is the 'Network Device List' in the device ma= nager menu is crated before EfiBootManagerConnectAll () is called in UiEntr= y function. If we choose to add the EfiBootManagerConnectAll() in DeviceManagerUiLib w= ith this patch, could we don't call the EfiBootManagerConnectAll() in UiEnt= ry 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; lersek@redhat.com > Cc: Gao, Zhichao ; Ni, Ray ; > Wang, Jian J ; Wu, Hao A ; Bi= , > Dandan > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/DeviceManagerUiLib: > connect all before creating menu page >=20 > 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? > > >=20 > 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 a= ll > network drivers on a typical BootOrder based boot should be be necessary= . >=20 > 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. >=20 > This works surprisingly well, with the exception of the 'Network Device = List' in > the device manager UiApp menu, which no longer lists the network interfa= ce, > even though UiApp does a connect-all as it starts up. > As you guessed, the constructor ordering versus the connect-all call res= ults 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. >=20 > > 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. > > >=20 > 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. >=20 >=20 > >> 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 !=3D 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] > >> > > > > > > > > >=20 >=20 >=20