public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Guo Heyi <heyi.guo@linaro.org>
To: Heyi Guo <heyi.guo@linaro.org>
Cc: edk2-devel@lists.01.org, Star Zeng <star.zeng@intel.com>,
	Eric Dong <eric.dong@intel.com>, Ruiyu Ni <ruiyu.ni@intel.com>,
	Laszlo Ersek <lersek@redhat.com>
Subject: Re: [PATCH 1/1] MdeModulePkg/UefiBootManagerLib: limit recursive call depth
Date: Thu, 1 Mar 2018 10:43:19 +0800	[thread overview]
Message-ID: <20180301024319.GA39361@SZX1000114654> (raw)
In-Reply-To: <1519871972-39281-1-git-send-email-heyi.guo@linaro.org>

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
> 


  reply	other threads:[~2018-03-01  2:37 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-01  2:39 [PATCH 1/1] MdeModulePkg/UefiBootManagerLib: limit recursive call depth Heyi Guo
2018-03-01  2:43 ` Guo Heyi [this message]
2018-03-01  4:46 ` Ni, Ruiyu
2018-03-07  1:54   ` Guo Heyi
2018-03-08  2:53     ` Ni, Ruiyu
  -- strict thread matches above, loose matches on Subject: below --
2018-02-26  8:29 Heyi Guo
2018-02-26  8:56 ` Wang, Sunny (HPS SW)
2018-02-26 11:34   ` Guo Heyi
2018-02-27  2:47     ` Wang, Sunny (HPS SW)
2018-02-26 16:23 ` Laszlo Ersek
2018-02-27  0:48   ` Guo Heyi
2018-02-27  5:48     ` Ni, Ruiyu
2018-02-27 10:29       ` Laszlo Ersek
2018-02-27 10:39         ` Guo Heyi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180301024319.GA39361@SZX1000114654 \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox