From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web12.8799.1586439054695014387 for ; Thu, 09 Apr 2020 06:30:55 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=SPvEmHwn; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1586439053; 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=aIxwdM0yuuUDzJZBkcmhSgAh0aRKGWeTs6CxDv2vRiI=; b=SPvEmHwnrl2/CLdY0DncweOaBgrprgsKVo5I+0GQwd7L0NB/QcVpeQ4q1xUeRnswsWP8uU Nuk/+1nE8L57pYTsfKKUt7tBDbBp4zJUvueWM+Na8zvICqPJZInujt8oXuYUBOm48dTw/3 7U3Q8HQEiLqa1zXAucInm9qq79yoWC4= 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-124-XdPzR7iPPmq6b_2EkO51TQ-1; Thu, 09 Apr 2020 09:30:49 -0400 X-MC-Unique: XdPzR7iPPmq6b_2EkO51TQ-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id D244E8018A3; Thu, 9 Apr 2020 13:30:47 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-213.ams2.redhat.com [10.36.112.213]) by smtp.corp.redhat.com (Postfix) with ESMTP id 07C8910027AC; Thu, 9 Apr 2020 13:30:45 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/DeviceManagerUiLib: connect all before creating menu page To: Ard Biesheuvel , devel@edk2.groups.io 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: "Laszlo Ersek" Message-ID: <70f80b7b-ce04-93cf-827d-8de9526aaa47@redhat.com> Date: Thu, 9 Apr 2020 15:30:45 +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: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable 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 men= u >>> assumes that all handles have been connected to their drivers, but thi= s >>> is not guaranteed in the general case. >>> >>> So work around this by doing an explicit ConnectAll() before populatin= g >>> the pages. >>> >>> Cc: Zhichao Gao >>> Cc: Ray Ni >>> Cc: Jian J Wang >>> Cc: Hao A Wu >>> Cc: Dandan Bi >>> Signed-off-by: Ard Biesheuvel >>> --- >>> =A0 MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c=A0=A0=A0= =A0=A0=A0=A0 | 7 >>> +++++++ >>> =A0 MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.h=A0=A0=A0= =A0=A0=A0=A0 | 1 + >>> =A0 MdeModulePkg/Library/DeviceManagerUiLib/DeviceManagerUiLib.inf | 1= + >>> =A0 3 files changed, 9 insertions(+) >> >> Can you describe one example that's (a) relevant to you and (b) affecte= d >> 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 all 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. 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). >=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. 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 >=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 ( >>> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 ); >>> =A0=A0=A0 ASSERT (gDeviceManagerPrivate.HiiHandle !=3D NULL); >>> =A0 +=A0 // >>> +=A0 // The device manager form contains a page listing all the networ= k >>> +=A0 // controllers in the system. This list can only be populated if = all >>> +=A0 // handles have been connected, so do it here. >>> +=A0 // >>> +=A0 EfiBootManagerConnectAll (); >>> + >>> =A0=A0=A0 // >>> =A0=A0=A0 // Update boot manager page >>> =A0=A0=A0 // >>> 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 >>> =A0 #include >>> =A0 #include >>> =A0 #include >>> +#include >>> =A0 #include >>> =A0 =A0 // >>> 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] >>> =A0=A0=A0 DebugLib >>> =A0=A0=A0 PrintLib >>> =A0=A0=A0 HiiLib >>> +=A0 UefiBootManagerLib >>> =A0=A0=A0 UefiHiiServicesLib >>> =A0 =A0 [Guids] >>> >> >> >>=20 >> >=20