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

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

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 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.

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).

> 
>> 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.

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

> 
> 
>>> 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]
>>>
>>
>>
>> 
>>
>