From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.100; helo=mga07.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 0BEF8224DCA30 for ; Wed, 7 Mar 2018 18:47:15 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 07 Mar 2018 18:53:31 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.47,438,1515484800"; d="scan'208";a="206413754" Received: from ray-dev.ccr.corp.intel.com (HELO [10.239.9.44]) ([10.239.9.44]) by orsmga005.jf.intel.com with ESMTP; 07 Mar 2018 18:53:29 -0800 To: Guo Heyi Cc: edk2-devel@lists.01.org, Star Zeng , Eric Dong , Laszlo Ersek References: <1519871972-39281-1-git-send-email-heyi.guo@linaro.org> <8863d9e7-9735-1009-1476-a38b31305eaa@Intel.com> <20180307015402.GA91936@SZX1000114654> From: "Ni, Ruiyu" Message-ID: <7bd9dcc5-4345-ee0a-6842-0901427cf568@Intel.com> Date: Thu, 8 Mar 2018 10:53:29 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180307015402.GA91936@SZX1000114654> Subject: Re: [PATCH 1/1] MdeModulePkg/UefiBootManagerLib: limit recursive call depth X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 08 Mar 2018 02:47:16 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >>> Cc: Star Zeng >>> Cc: Eric Dong >>> Cc: Ruiyu Ni >>> Cc: Laszlo Ersek >>> --- >>> >>> 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 >> >> -- >> Thanks, >> Ray Done in 72208a9a90b8c6cd5011ddf174ad01e567b67454. -- Thanks, Ray