Re: [edk2-devel] [PATCH] MdeModulePkg/DeviceManagerUiLib: connect all before creating menu page

["Laszlo Ersek" <lersek@redhat.com>]

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 <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 <zhichao.gao@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@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@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 <zhichao.gao@intel.com>; Ni, Ray <ray.ni@intel.com>; 
>> Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; 
>> Bi, Dandan <dandan.bi@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@intel.com>
>>>> Cc: Ray Ni <ray.ni@intel.com>
>>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>>> Cc: Hao A Wu <hao.a.wu@intel.com>
>>>> Cc: Dandan Bi <dandan.bi@intel.com>
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@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]
>>>>
>>>
>>>
>>>
>>>
>>
>>
>>
> 
> 
> 
>