Hi Heyi, Thanks for looking into my suggestion. You already mentioned the value I thought. :) The value is to get better boot performance by setting the MAX_RECONNECT_REPAIR to a smaller number. 10 times reconnect may be suitable for consumer products like laptop. However, it may be not suitable for server. For example, user installs a problematic NIC 4-port card and its OPROM produced 4 driver handles to manage 4 ports. For this case, 10 times reconnect may take much longer time. If you think it's still not worth adding a PCD for this or have other concern about adding a PCD, I'm fine with dropping this suggestion.
Regards, Sunny Wang -----Original Message----- From: Guo Heyi [mailto:[email protected]] Sent: Monday, February 26, 2018 7:34 PM To: Wang, Sunny (HPS SW) <[email protected]> Cc: Heyi Guo <[email protected]>; [email protected]; Ruiyu Ni <[email protected]>; Eric Dong <[email protected]>; Star Zeng <[email protected]> Subject: Re: [edk2] [PATCH 1/1] MdeModulePkg/UefiBootManagerLib: limit recursive call depth Importance: High Hi Sunny, I didn't consider it as a value necessary for platform override, for the retry count should only have some impact on boot performance and it only happens when there is something wrong. May I know what value you will use for your platform and why? Thanks and regards, Gary (Heyi Guo) On Mon, Feb 26, 2018 at 08:56:50AM +0000, Wang, Sunny (HPS SW) wrote: > Hi Heyi, > Just a suggestion. > Is it better to use a PCD instead of a define for MAX_RECONNECT_REPAIR? So > that we can easily override it in our platform dsc file. > > Regards, > Sunny Wang > > -----Original Message----- > From: edk2-devel [mailto:[email protected]] On Behalf Of > Heyi Guo > Sent: Monday, February 26, 2018 4:30 PM > To: [email protected] > Cc: Ruiyu Ni <[email protected]>; Heyi Guo <[email protected]>; > Eric Dong <[email protected]>; Star Zeng <[email protected]> > Subject: [edk2] [PATCH 1/1] MdeModulePkg/UefiBootManagerLib: limit > recursive call depth > > Function BmRepairAllControllers may recursively call itself if some driver > health protocol returns EfiDriverHealthStatusReconnectRequired. > However, driver health protocol of some buggy third party driver may always > return such status even after one and another reconnect. The endless > iteration will cause stack overflow and then system exception, and it may be > not easy to find that the exception is actually caused by stack overflow. > > So we limit the number of reconnect retry to 10 to improve code robustness. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Heyi Guo <[email protected]> > Cc: Star Zeng <[email protected]> > Cc: Eric Dong <[email protected]> > Cc: Ruiyu Ni <[email protected]> > --- > MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h | 6 ++++++ > MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c | 17 > ++++++++++++++++- > 2 files changed, 22 insertions(+), 1 deletion(-) > > diff --git a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h > b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h > index 25a1d522fe84..9aa86b096525 100644 > --- a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h > +++ b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h > @@ -108,6 +108,12 @@ CHAR16 * > #define BM_OPTION_NAME_LEN sizeof > ("PlatformRecovery####") > extern CHAR16 *mBmLoadOptionName[]; > > +// > +// Maximum number of reconnect retry to repair controller; it is to > +limit the // number of recursive call of BmRepairAllControllers. > +// > +#define MAX_RECONNECT_REPAIR 10 > + > /** > Visitor function to be called by BmForEachVariable for each variable > in variable storage. > diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c > b/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c > index ddcee8b0676f..30d70f32af84 100644 > --- a/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c > +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c > @@ -26,6 +26,12 @@ GLOBAL_REMOVE_IF_UNREFERENCED > L"Reboot Required" > }; > > +// > +// Counter of reconnect retry to repair controller; it is to limit > +the // number of recursive call of BmRepairAllControllers. > +// > +STATIC UINTN mReconnectRepairCount; > + > /** > Return the controller name. > > @@ -549,7 +555,16 @@ BmRepairAllControllers ( > > > if (ReconnectRequired) { > - BmRepairAllControllers (); > + if (mReconnectRepairCount < MAX_RECONNECT_REPAIR) { > + mReconnectRepairCount++; > + BmRepairAllControllers (); > + } else { > + DEBUG ((DEBUG_ERROR, "[%a:%d] Repair failed after %d retries.\n", > + __FUNCTION__, __LINE__, mReconnectRepairCount)); > + // Reset counter so that it will not affect calling > + // BmRepairAllControllers() somewhere else > + mReconnectRepairCount = 0; > + } > } > > DEBUG_CODE ( > -- > 2.7.4 > > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
