From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web11.8067.1586858588949660739 for ; Tue, 14 Apr 2020 03:03:09 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=PhZkb91Y; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1586858588; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=EfGnCJ7ipLaCQF9I2CyOOgn0w5rZHafbn0Vk1HKsfa8=; b=PhZkb91Yl8M6S0t6KcTn59qhXDaX55sNtKhSjhYyeBndpRf/lyVW3F1/MJU3a1qH6TRFaB Gc6KrpJQ1T0ob6S3qDOPT7n4l/fB1H2vIQ36SCOgrMGPao8dQREWv/M4PS4O/tmglOZUi3 XbY+ltnLpQb4KWMXYWyFtDZFQk4M93w= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-239-EQ7TZxGWMjycum5U9rD6IQ-1; Tue, 14 Apr 2020 06:03:01 -0400 X-MC-Unique: EQ7TZxGWMjycum5U9rD6IQ-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id E43EB107ACC4; Tue, 14 Apr 2020 10:02:59 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-193.ams2.redhat.com [10.36.113.193]) by smtp.corp.redhat.com (Postfix) with ESMTP id BE87C5C1B0; Tue, 14 Apr 2020 10:02:57 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/DeviceManagerUiLib: connect all before creating menu page To: "Tian, Hot" , "devel@edk2.groups.io" , "Bi, Dandan" , "ard.biesheuvel@arm.com" Cc: "Gao, Zhichao" , "Ni, Ray" , "Wang, Jian J" , "Wu, Hao A" References: <20200408172807.10108-1-ard.biesheuvel@arm.com> <1e1d4c9d-01ed-2b65-69ec-278f95309d25@redhat.com> <3C0D5C461C9E904E8F62152F6274C0BB40DB1A8A@SHSMSX104.ccr.corp.intel.com> <97159AD15C0F454180C255F8DA661355363BD4D7@SHSMSX104.ccr.corp.intel.com> From: "Laszlo Ersek" Message-ID: Date: Tue, 14 Apr 2020 12:02:56 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <97159AD15C0F454180C255F8DA661355363BD4D7@SHSMSX104.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 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 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; 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 >> >> 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] >>>> >>> >>> >>> >>> >> >> >> > > > >