[edk2-devel] [PATCH] MdeModulePkg/DeviceManagerUiLib: connect all before creating menu page
Laszlo Ersek
lersek at redhat.com
Tue Apr 14 10:02:56 UTC 2020
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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Dandan Bi
> Sent: Sunday, April 12, 2020 15:56
> To: devel at edk2.groups.io; ard.biesheuvel at arm.com; lersek at redhat.com
> Cc: Gao, Zhichao <zhichao.gao at intel.com>; Ni, Ray <ray.ni at intel.com>; Wang, Jian J <jian.j.wang at intel.com>; Wu, Hao A <hao.a.wu at intel.com>
> 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 at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf Of
>> Ard Biesheuvel
>> Sent: Thursday, April 9, 2020 5:37 PM
>> To: devel at edk2.groups.io; lersek at redhat.com
>> Cc: Gao, Zhichao <zhichao.gao at intel.com>; Ni, Ray <ray.ni at intel.com>;
>> Wang, Jian J <jian.j.wang at intel.com>; Wu, Hao A <hao.a.wu at intel.com>;
>> Bi, Dandan <dandan.bi at intel.com>
>> 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 <zhichao.gao at intel.com>
>>>> Cc: Ray Ni <ray.ni at intel.com>
>>>> Cc: Jian J Wang <jian.j.wang at intel.com>
>>>> Cc: Hao A Wu <hao.a.wu at intel.com>
>>>> Cc: Dandan Bi <dandan.bi at intel.com>
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel at arm.com>
>>>> ---
>>>> 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 <Library/BaseLib.h>
>>>> #include <Library/HiiLib.h>
>>>> #include <Library/DevicePathLib.h>
>>>> +#include <Library/UefiBootManagerLib.h>
>>>> #include <Library/UefiHiiServicesLib.h>
>>>>
>>>> //
>>>> 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]
>>>>
>>>
>>>
>>>
>>>
>>
>>
>>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#57305): https://edk2.groups.io/g/devel/message/57305
Mute This Topic: https://groups.io/mt/72879609/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list