| Message ID | 1519871972-39281-1-git-send-email-heyi.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [edk2,1/1] MdeModulePkg/UefiBootManagerLib: limit recursive call depth | expand |
Sorry, forgot to add a "v2" mark in the subjuect prefix... Regards, Heyi On Thu, Mar 01, 2018 at 10:39:32AM +0800, 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, and DEBUG_CODE is moved ahead before recursive repair to > track the repair result. > > We also remove a duplicated declaration of BmRepairAllControllers() in > InternalBm.h in this patch, for it is only a trivial change. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Heyi Guo <heyi.guo@linaro.org> > Cc: Star Zeng <star.zeng@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > --- > > Notes: > v2 > - Use argument instead of global variable to record the recursive > count [Ray] > - Move DEBUG_CODE before recursively calling BmRepairAllControllers() > to track the change of each reconnect [Ray] > - Remove a duplicated declaration of BmRepairAllControllers() in > InternalBm.h. > > MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h | 19 ++++++++++--------- > MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 2 +- > MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c | 18 +++++++++++++----- > 3 files changed, 24 insertions(+), 15 deletions(-) > > diff --git a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h > index 25a1d522fe84..21ecd8584d24 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. > @@ -145,10 +151,13 @@ typedef struct { > > /** > Repair all the controllers according to the Driver Health status queried. > + > + @param ReconnectRepairCount To record the number of recursive call of > + this function itself. > **/ > VOID > BmRepairAllControllers ( > - VOID > + UINTN ReconnectRepairCount > ); > > #define BM_HOTKEY_SIGNATURE SIGNATURE_32 ('b', 'm', 'h', 'k') > @@ -328,14 +337,6 @@ BmDelPartMatchInstance ( > ); > > /** > - Repair all the controllers according to the Driver Health status queried. > -**/ > -VOID > -BmRepairAllControllers ( > - VOID > - ); > - > -/** > Print the device path info. > > @param DevicePath The device path need to print. > diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c > index ce19ae400660..b842d5824aed 100644 > --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c > +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c > @@ -1767,7 +1767,7 @@ EfiBootManagerBoot ( > // > // 4. Repair system through DriverHealth protocol > // > - BmRepairAllControllers (); > + BmRepairAllControllers (0); > } > > PERF_START_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32) OptionNumber); > diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c b/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c > index ddcee8b0676f..db2f859ae73d 100644 > --- a/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c > +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c > @@ -423,10 +423,13 @@ EfiBootManagerFreeDriverHealthInfo ( > > /** > Repair all the controllers according to the Driver Health status queried. > + > + @param ReconnectRepairCount To record the number of recursive call of > + this function itself. > **/ > VOID > BmRepairAllControllers ( > - VOID > + UINTN ReconnectRepairCount > ) > { > EFI_STATUS Status; > @@ -548,10 +551,6 @@ BmRepairAllControllers ( > EfiBootManagerFreeDriverHealthInfo (DriverHealthInfo, Count); > > > - if (ReconnectRequired) { > - BmRepairAllControllers (); > - } > - > DEBUG_CODE ( > CHAR16 *ControllerName; > > @@ -576,6 +575,15 @@ BmRepairAllControllers ( > EfiBootManagerFreeDriverHealthInfo (DriverHealthInfo, Count); > ); > > + if (ReconnectRequired) { > + if (ReconnectRepairCount < MAX_RECONNECT_REPAIR) { > + BmRepairAllControllers (ReconnectRepairCount + 1); > + } else { > + DEBUG ((DEBUG_ERROR, "[%a:%d] Repair failed after %d retries.\n", > + __FUNCTION__, __LINE__, ReconnectRepairCount)); > + } > + } > + > if (RebootRequired) { > DEBUG ((EFI_D_INFO, "[BDS] One of the Driver Health instances requires rebooting.\n")); > gRT->ResetSystem (EfiResetWarm, EFI_SUCCESS, 0, NULL); > -- > 2.7.4 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 3/1/2018 10:39 AM, 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, and DEBUG_CODE is moved ahead before recursive repair to > track the repair result. > > We also remove a duplicated declaration of BmRepairAllControllers() in > InternalBm.h in this patch, for it is only a trivial change. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Heyi Guo <heyi.guo@linaro.org> > Cc: Star Zeng <star.zeng@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > --- > > Notes: > v2 > - Use argument instead of global variable to record the recursive > count [Ray] > - Move DEBUG_CODE before recursively calling BmRepairAllControllers() > to track the change of each reconnect [Ray] > - Remove a duplicated declaration of BmRepairAllControllers() in > InternalBm.h. > > MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h | 19 ++++++++++--------- > MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 2 +- > MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c | 18 +++++++++++++----- > 3 files changed, 24 insertions(+), 15 deletions(-) > > diff --git a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h > index 25a1d522fe84..21ecd8584d24 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. > @@ -145,10 +151,13 @@ typedef struct { > > /** > Repair all the controllers according to the Driver Health status queried. > + > + @param ReconnectRepairCount To record the number of recursive call of > + this function itself. > **/ > VOID > BmRepairAllControllers ( > - VOID > + UINTN ReconnectRepairCount > ); > > #define BM_HOTKEY_SIGNATURE SIGNATURE_32 ('b', 'm', 'h', 'k') > @@ -328,14 +337,6 @@ BmDelPartMatchInstance ( > ); > > /** > - Repair all the controllers according to the Driver Health status queried. > -**/ > -VOID > -BmRepairAllControllers ( > - VOID > - ); > - > -/** > Print the device path info. > > @param DevicePath The device path need to print. > diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c > index ce19ae400660..b842d5824aed 100644 > --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c > +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c > @@ -1767,7 +1767,7 @@ EfiBootManagerBoot ( > // > // 4. Repair system through DriverHealth protocol > // > - BmRepairAllControllers (); > + BmRepairAllControllers (0); > } > > PERF_START_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32) OptionNumber); > diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c b/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c > index ddcee8b0676f..db2f859ae73d 100644 > --- a/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c > +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c > @@ -423,10 +423,13 @@ EfiBootManagerFreeDriverHealthInfo ( > > /** > Repair all the controllers according to the Driver Health status queried. > + > + @param ReconnectRepairCount To record the number of recursive call of > + this function itself. > **/ > VOID > BmRepairAllControllers ( > - VOID > + UINTN ReconnectRepairCount > ) > { > EFI_STATUS Status; > @@ -548,10 +551,6 @@ BmRepairAllControllers ( > EfiBootManagerFreeDriverHealthInfo (DriverHealthInfo, Count); > > > - if (ReconnectRequired) { > - BmRepairAllControllers (); > - } > - > DEBUG_CODE ( > CHAR16 *ControllerName; > > @@ -576,6 +575,15 @@ BmRepairAllControllers ( > EfiBootManagerFreeDriverHealthInfo (DriverHealthInfo, Count); > ); > > + if (ReconnectRequired) { > + if (ReconnectRepairCount < MAX_RECONNECT_REPAIR) { > + BmRepairAllControllers (ReconnectRepairCount + 1); > + } else { > + DEBUG ((DEBUG_ERROR, "[%a:%d] Repair failed after %d retries.\n", > + __FUNCTION__, __LINE__, ReconnectRepairCount)); > + } > + } > + > if (RebootRequired) { > DEBUG ((EFI_D_INFO, "[BDS] One of the Driver Health instances requires rebooting.\n")); > gRT->ResetSystem (EfiResetWarm, EFI_SUCCESS, 0, NULL); > Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com> -- Thanks, Ray _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Hi Ray, Sorry to disturb, but I didn't find the patch committed. Could you help to do that? Thanks, Heyi On Thu, Mar 01, 2018 at 12:46:32PM +0800, Ni, Ruiyu wrote: > On 3/1/2018 10:39 AM, 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, and DEBUG_CODE is moved ahead before recursive repair to > >track the repair result. > > > >We also remove a duplicated declaration of BmRepairAllControllers() in > >InternalBm.h in this patch, for it is only a trivial change. > > > >Contributed-under: TianoCore Contribution Agreement 1.1 > >Signed-off-by: Heyi Guo <heyi.guo@linaro.org> > >Cc: Star Zeng <star.zeng@intel.com> > >Cc: Eric Dong <eric.dong@intel.com> > >Cc: Ruiyu Ni <ruiyu.ni@intel.com> > >Cc: Laszlo Ersek <lersek@redhat.com> > >--- > > > >Notes: > > v2 > > - Use argument instead of global variable to record the recursive > > count [Ray] > > - Move DEBUG_CODE before recursively calling BmRepairAllControllers() > > to track the change of each reconnect [Ray] > > - Remove a duplicated declaration of BmRepairAllControllers() in > > InternalBm.h. > > > > MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h | 19 ++++++++++--------- > > MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 2 +- > > MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c | 18 +++++++++++++----- > > 3 files changed, 24 insertions(+), 15 deletions(-) > > > >diff --git a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h > >index 25a1d522fe84..21ecd8584d24 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. > >@@ -145,10 +151,13 @@ typedef struct { > > /** > > Repair all the controllers according to the Driver Health status queried. > >+ > >+ @param ReconnectRepairCount To record the number of recursive call of > >+ this function itself. > > **/ > > VOID > > BmRepairAllControllers ( > >- VOID > >+ UINTN ReconnectRepairCount > > ); > > #define BM_HOTKEY_SIGNATURE SIGNATURE_32 ('b', 'm', 'h', 'k') > >@@ -328,14 +337,6 @@ BmDelPartMatchInstance ( > > ); > > /** > >- Repair all the controllers according to the Driver Health status queried. > >-**/ > >-VOID > >-BmRepairAllControllers ( > >- VOID > >- ); > >- > >-/** > > Print the device path info. > > @param DevicePath The device path need to print. > >diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c > >index ce19ae400660..b842d5824aed 100644 > >--- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c > >+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c > >@@ -1767,7 +1767,7 @@ EfiBootManagerBoot ( > > // > > // 4. Repair system through DriverHealth protocol > > // > >- BmRepairAllControllers (); > >+ BmRepairAllControllers (0); > > } > > PERF_START_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32) OptionNumber); > >diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c b/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c > >index ddcee8b0676f..db2f859ae73d 100644 > >--- a/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c > >+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c > >@@ -423,10 +423,13 @@ EfiBootManagerFreeDriverHealthInfo ( > > /** > > Repair all the controllers according to the Driver Health status queried. > >+ > >+ @param ReconnectRepairCount To record the number of recursive call of > >+ this function itself. > > **/ > > VOID > > BmRepairAllControllers ( > >- VOID > >+ UINTN ReconnectRepairCount > > ) > > { > > EFI_STATUS Status; > >@@ -548,10 +551,6 @@ BmRepairAllControllers ( > > EfiBootManagerFreeDriverHealthInfo (DriverHealthInfo, Count); > >- if (ReconnectRequired) { > >- BmRepairAllControllers (); > >- } > >- > > DEBUG_CODE ( > > CHAR16 *ControllerName; > >@@ -576,6 +575,15 @@ BmRepairAllControllers ( > > EfiBootManagerFreeDriverHealthInfo (DriverHealthInfo, Count); > > ); > >+ if (ReconnectRequired) { > >+ if (ReconnectRepairCount < MAX_RECONNECT_REPAIR) { > >+ BmRepairAllControllers (ReconnectRepairCount + 1); > >+ } else { > >+ DEBUG ((DEBUG_ERROR, "[%a:%d] Repair failed after %d retries.\n", > >+ __FUNCTION__, __LINE__, ReconnectRepairCount)); > >+ } > >+ } > >+ > > if (RebootRequired) { > > DEBUG ((EFI_D_INFO, "[BDS] One of the Driver Health instances requires rebooting.\n")); > > gRT->ResetSystem (EfiResetWarm, EFI_SUCCESS, 0, NULL); > > > Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com> > > -- > Thanks, > Ray _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 3/7/2018 9:54 AM, Guo Heyi wrote: > Hi Ray, > > Sorry to disturb, but I didn't find the patch committed. Could you help to do > that? > > Thanks, > Heyi > > > On Thu, Mar 01, 2018 at 12:46:32PM +0800, Ni, Ruiyu wrote: >> On 3/1/2018 10:39 AM, 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, and DEBUG_CODE is moved ahead before recursive repair to >>> track the repair result. >>> >>> We also remove a duplicated declaration of BmRepairAllControllers() in >>> InternalBm.h in this patch, for it is only a trivial change. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org> >>> Cc: Star Zeng <star.zeng@intel.com> >>> Cc: Eric Dong <eric.dong@intel.com> >>> Cc: Ruiyu Ni <ruiyu.ni@intel.com> >>> Cc: Laszlo Ersek <lersek@redhat.com> >>> --- >>> >>> Notes: >>> v2 >>> - Use argument instead of global variable to record the recursive >>> count [Ray] >>> - Move DEBUG_CODE before recursively calling BmRepairAllControllers() >>> to track the change of each reconnect [Ray] >>> - Remove a duplicated declaration of BmRepairAllControllers() in >>> InternalBm.h. >>> >>> MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h | 19 ++++++++++--------- >>> MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 2 +- >>> MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c | 18 +++++++++++++----- >>> 3 files changed, 24 insertions(+), 15 deletions(-) >>> >>> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h >>> index 25a1d522fe84..21ecd8584d24 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. >>> @@ -145,10 +151,13 @@ typedef struct { >>> /** >>> Repair all the controllers according to the Driver Health status queried. >>> + >>> + @param ReconnectRepairCount To record the number of recursive call of >>> + this function itself. >>> **/ >>> VOID >>> BmRepairAllControllers ( >>> - VOID >>> + UINTN ReconnectRepairCount >>> ); >>> #define BM_HOTKEY_SIGNATURE SIGNATURE_32 ('b', 'm', 'h', 'k') >>> @@ -328,14 +337,6 @@ BmDelPartMatchInstance ( >>> ); >>> /** >>> - Repair all the controllers according to the Driver Health status queried. >>> -**/ >>> -VOID >>> -BmRepairAllControllers ( >>> - VOID >>> - ); >>> - >>> -/** >>> Print the device path info. >>> @param DevicePath The device path need to print. >>> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c >>> index ce19ae400660..b842d5824aed 100644 >>> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c >>> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c >>> @@ -1767,7 +1767,7 @@ EfiBootManagerBoot ( >>> // >>> // 4. Repair system through DriverHealth protocol >>> // >>> - BmRepairAllControllers (); >>> + BmRepairAllControllers (0); >>> } >>> PERF_START_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32) OptionNumber); >>> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c b/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c >>> index ddcee8b0676f..db2f859ae73d 100644 >>> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c >>> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c >>> @@ -423,10 +423,13 @@ EfiBootManagerFreeDriverHealthInfo ( >>> /** >>> Repair all the controllers according to the Driver Health status queried. >>> + >>> + @param ReconnectRepairCount To record the number of recursive call of >>> + this function itself. >>> **/ >>> VOID >>> BmRepairAllControllers ( >>> - VOID >>> + UINTN ReconnectRepairCount >>> ) >>> { >>> EFI_STATUS Status; >>> @@ -548,10 +551,6 @@ BmRepairAllControllers ( >>> EfiBootManagerFreeDriverHealthInfo (DriverHealthInfo, Count); >>> - if (ReconnectRequired) { >>> - BmRepairAllControllers (); >>> - } >>> - >>> DEBUG_CODE ( >>> CHAR16 *ControllerName; >>> @@ -576,6 +575,15 @@ BmRepairAllControllers ( >>> EfiBootManagerFreeDriverHealthInfo (DriverHealthInfo, Count); >>> ); >>> + if (ReconnectRequired) { >>> + if (ReconnectRepairCount < MAX_RECONNECT_REPAIR) { >>> + BmRepairAllControllers (ReconnectRepairCount + 1); >>> + } else { >>> + DEBUG ((DEBUG_ERROR, "[%a:%d] Repair failed after %d retries.\n", >>> + __FUNCTION__, __LINE__, ReconnectRepairCount)); >>> + } >>> + } >>> + >>> if (RebootRequired) { >>> DEBUG ((EFI_D_INFO, "[BDS] One of the Driver Health instances requires rebooting.\n")); >>> gRT->ResetSystem (EfiResetWarm, EFI_SUCCESS, 0, NULL); >>> >> Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com> >> >> -- >> Thanks, >> Ray Done in 72208a9a90b8c6cd5011ddf174ad01e567b67454. -- Thanks, Ray _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h index 25a1d522fe84..21ecd8584d24 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. @@ -145,10 +151,13 @@ typedef struct { /** Repair all the controllers according to the Driver Health status queried. + + @param ReconnectRepairCount To record the number of recursive call of + this function itself. **/ VOID BmRepairAllControllers ( - VOID + UINTN ReconnectRepairCount ); #define BM_HOTKEY_SIGNATURE SIGNATURE_32 ('b', 'm', 'h', 'k') @@ -328,14 +337,6 @@ BmDelPartMatchInstance ( ); /** - Repair all the controllers according to the Driver Health status queried. -**/ -VOID -BmRepairAllControllers ( - VOID - ); - -/** Print the device path info. @param DevicePath The device path need to print. diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c index ce19ae400660..b842d5824aed 100644 --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c @@ -1767,7 +1767,7 @@ EfiBootManagerBoot ( // // 4. Repair system through DriverHealth protocol // - BmRepairAllControllers (); + BmRepairAllControllers (0); } PERF_START_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32) OptionNumber); diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c b/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c index ddcee8b0676f..db2f859ae73d 100644 --- a/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c @@ -423,10 +423,13 @@ EfiBootManagerFreeDriverHealthInfo ( /** Repair all the controllers according to the Driver Health status queried. + + @param ReconnectRepairCount To record the number of recursive call of + this function itself. **/ VOID BmRepairAllControllers ( - VOID + UINTN ReconnectRepairCount ) { EFI_STATUS Status; @@ -548,10 +551,6 @@ BmRepairAllControllers ( EfiBootManagerFreeDriverHealthInfo (DriverHealthInfo, Count); - if (ReconnectRequired) { - BmRepairAllControllers (); - } - DEBUG_CODE ( CHAR16 *ControllerName; @@ -576,6 +575,15 @@ BmRepairAllControllers ( EfiBootManagerFreeDriverHealthInfo (DriverHealthInfo, Count); ); + if (ReconnectRequired) { + if (ReconnectRepairCount < MAX_RECONNECT_REPAIR) { + BmRepairAllControllers (ReconnectRepairCount + 1); + } else { + DEBUG ((DEBUG_ERROR, "[%a:%d] Repair failed after %d retries.\n", + __FUNCTION__, __LINE__, ReconnectRepairCount)); + } + } + if (RebootRequired) { DEBUG ((EFI_D_INFO, "[BDS] One of the Driver Health instances requires rebooting.\n")); gRT->ResetSystem (EfiResetWarm, EFI_SUCCESS, 0, NULL);
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, and DEBUG_CODE is moved ahead before recursive repair to track the repair result. We also remove a duplicated declaration of BmRepairAllControllers() in InternalBm.h in this patch, for it is only a trivial change. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Heyi Guo <heyi.guo@linaro.org> Cc: Star Zeng <star.zeng@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Ruiyu Ni <ruiyu.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> --- Notes: v2 - Use argument instead of global variable to record the recursive count [Ray] - Move DEBUG_CODE before recursively calling BmRepairAllControllers() to track the change of each reconnect [Ray] - Remove a duplicated declaration of BmRepairAllControllers() in InternalBm.h. MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h | 19 ++++++++++--------- MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 2 +- MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c | 18 +++++++++++++----- 3 files changed, 24 insertions(+), 15 deletions(-) -- 2.7.4 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel