From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by mx.groups.io with SMTP id smtpd.web10.7976.1586679105940271915 for ; Sun, 12 Apr 2020 01:11:46 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.93, mailfrom: hot.tian@intel.com) IronPort-SDR: dk4jYsvbU1u1H0cVZMXVrlnc6crifmI9EQ1FCf8+ue2z+2HCT9HjxJnUVwyCNz6USgid95eMqi Xu8yRKYg6oDg== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Apr 2020 01:11:45 -0700 IronPort-SDR: ofy6odmWaBhTB26SmiWS7SqB2kREl6Vk642/QlTwWdBfu31XhC2oCnyrwrSg/3QX6nZTzHxvIZ DjvoXUJGLs/w== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.72,374,1580803200"; d="scan'208";a="264972692" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by orsmga008.jf.intel.com with ESMTP; 12 Apr 2020 01:11:45 -0700 Received: from fmsmsx121.amr.corp.intel.com (10.18.125.36) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 12 Apr 2020 01:11:45 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx121.amr.corp.intel.com (10.18.125.36) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 12 Apr 2020 01:11:44 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.225]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.129]) with mapi id 14.03.0439.000; Sun, 12 Apr 2020 16:11:42 +0800 From: "Hot Tian" To: "devel@edk2.groups.io" , "Bi, Dandan" , "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: AQHWDcsz2tjJGv2QCUC5ffzzp6Z9vahwAPoAgAACQICABJqdgIAAih4Q Date: Sun, 12 Apr 2020 08:11:42 +0000 Message-ID: <97159AD15C0F454180C255F8DA661355363BD4D7@SHSMSX104.ccr.corp.intel.com> References: <20200408172807.10108-1-ard.biesheuvel@arm.com> <1e1d4c9d-01ed-2b65-69ec-278f95309d25@redhat.com> <3C0D5C461C9E904E8F62152F6274C0BB40DB1A8A@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <3C0D5C461C9E904E8F62152F6274C0BB40DB1A8A@SHSMSX104.ccr.corp.intel.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: hot.tian@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable EfiBootManagerConnectAll is kind of BDS policy. Should it be controlled by = Ui App or Ui Lib? Thanks, Hot -----Original Message----- From: devel@edk2.groups.io On Behalf Of Dandan Bi Sent: Sunday, April 12, 2020 15:56 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 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=20 > Ard Biesheuvel > Sent: Thursday, April 9, 2020 5:37 PM > To: devel@edk2.groups.io; lersek@redhat.com > Cc: Gao, Zhichao ; Ni, Ray ;=20 > Wang, Jian J ; Wu, Hao A ;=20 > 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= =20 > >> a list of network devices in the system. The logic that creates=20 > >> this menu assumes that all handles have been connected to their=20 > >> drivers, but this is not guaranteed in the general case. > >> > >> So work around this by doing an explicit ConnectAll() before=20 > >> 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)=20 > > affected by this issue? > > >=20 > Yes. The SynQuacer platform has a NIC driver that waits up to 5=20 > seconds for the link to come up after it resets the PHY, which never=20 > happens if you boot without a network cable connected. But in the=20 > general case, connecting all network drivers on a typical BootOrder base= d boot should be be necessary. >=20 > Currently, ArmPkg's PlatformBootManagerLib does a=20 > EfiBootManagerConnectAll () in its AfterConsole() callback, and so=20 > this delay is always induced. I intend to 'fix' this platform (and=20 > this library in general) to only do a=20 > EfiBootManagerConnectAllDefaultConsoles > () at this point, which is sufficient to interrupt the boot if=20 > necessary, and otherwise, boot as fast as possible, without connecting= =20 > any devices that are not needed for this. >=20 > This works surprisingly well, with the exception of the 'Network=20 > Device List' in the device manager UiApp menu, which no longer lists=20 > 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=20 > results in the 'network device list' menu to be populated before any=20 > of the non- connected handles are connected. This is obviously against= =20 > the intent of doing a connect-all and enumerating all devices, and so th= is is a bug in UiApp. >=20 > > EfiBootManagerConnectAll() feels quite heavy-weight; I'd *vaguely*=20 > > prefer adding it elsewhere, instead of a "UI lib"'s constructor. I=20 > > don't have a particular suggestion however. > > > > I guess part of the issue must be that the=20 > > EfiBootManagerConnectAll() calls in BootManagerMenuApp / UiApp are=20 > > not reached, somehow. Maybe those are the calls that DeviceManagerUiLi= b takes for granted. > > >=20 > I agree that this approach is not the most elegant, but the way UiApp=20 > is constructed makes it very hard to find a better way without some=20 > 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=20 > >> + network // controllers in the system. This list can only be=20 > >> + 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