Hi Laszlo, I agree the current patch makes the code ugly, and turning the logic into a normal loop should be the perfect solution. If Ray also agrees on it, I can try to do that.
Thanks and regards, Heyi On Mon, Feb 26, 2018 at 05:23:29PM +0100, Laszlo Ersek wrote: > On 02/26/18 09:29, Heyi Guo wrote: > > 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. > > Not really my place to comment on this, but how about removing the > recursion entirely, and turning the logic into a normal (iterative) loop > instead? > > (1) Rename the current function to: > > STATIC > VOID > BmRepairAllControllersWorker ( > OUT BOOLEAN *ReconnectRequired, > OUT BOOLEAN *RebootRequired > ); > > > (2) The worker function should end right before > > if (ReconnectRequired) { > BmRepairAllControllers (); > } > > > (3) The worker function should not contain > > RebootRequired = FALSE; > ReconnectRequired = FALSE; > > Such initialization should be left to the caller. > > > (4) The worker function should be called in a loop from a new > BmRepairAllControllers() function, like this: > > Reconnect = 0; > RebootRequired = FALSE; > do { > ReconnectRequired = FALSE; > BmRepairAllControllersWorker (&ReconnectRequired, &RebootRequired); > ++Reconnect; > } while (ReconnectRequired && Reconnect < MAX_RECONNECT_REPAIR); > > DEBUG_CODE (...); > > if (RebootRequired) { > ... > } > > > In addition to eliminating the shoddy recursive call (and the shoddier > global counter, ewww :) ), this would fix the following other warts with > the code: > > - When a nested call chain is unwound, we currently dump a series of > "driver health info" lists (assuming no reboot is required), in the > DEBUG_CODE section. I would argue that we should do that only once, at > the end. (Even if we have to do it multiple times, it can be moved into > the worker function, to the end.) > > - It seems to be sufficient to accumulate RebootRequired into one > variable (i.e. not multiple instances of the same local variable on the > stack) and to act upon it at the very end. > > > Feel free to ignore my comments -- I just think we should be moving in > the opposite direction; that is, away from recursion (especially from > recursion combined with global variables -- that's one difficult pattern > to reason about). > > Thanks > Laszlo > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Heyi Guo <heyi....@linaro.org> > > Cc: Star Zeng <star.z...@intel.com> > > Cc: Eric Dong <eric.d...@intel.com> > > Cc: Ruiyu Ni <ruiyu...@intel.com> > > --- > > 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 ( > > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel