From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:400e:c01::22c; helo=mail-pl0-x22c.google.com; envelope-from=heyi.guo@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-pl0-x22c.google.com (mail-pl0-x22c.google.com [IPv6:2607:f8b0:400e:c01::22c]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id A8010224C0F33 for ; Tue, 6 Mar 2018 17:47:52 -0800 (PST) Received: by mail-pl0-x22c.google.com with SMTP id f23-v6so431445plr.10 for ; Tue, 06 Mar 2018 17:54:06 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:date:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=DmfwQKwGlX5eFBxMue3m/otdubguSd3LgK601MinSyA=; b=IFo6MQsuDFO31Q24IQSER1v1IHRkoLbRVPiOVjl/6f5QkLz4qtD0cLuTZfBRyGcyQd FzOsHKM6wZ5kpPM3ykp3u6Lwz0CReg9dXTS8aXPwfe6pZWH3e/FFYmIz55wdQDKRJnH+ dYmd0blRuKZGdjQEVygo5tYNXzDiX612PtIwE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:date:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=DmfwQKwGlX5eFBxMue3m/otdubguSd3LgK601MinSyA=; b=bBnOHEghNvOIcKDGZdGj4UQjb7Zf6yaaaM8BtjomdyCxuRft5S237pZl/suvUr+k8g rlXXLnnAP2tH+EcNHnx4H/igTCYLR6HMsSa9u0jDA8JBd1mUTAs+eSETsc89d7+EewcD wqgGaQYaw5pDcme7cDJH27EfLPA2LyLFDbj6Iq8mYHOqkNsmObRVCCp2P3P4SVfOtDrr HoSsAKTFCRIQlWq2VdDmvN80sEShLXImtSC168y3U43m8vpyeqBDaN6TNL53ENZweNwV AtxHQBJR8b6HuPNssQlK48cLqzHX0rP/4AbdCkhWc0WRMKmIeTgIvHwWCyfSEJc83ESK ToJg== X-Gm-Message-State: APf1xPAR9oxujWlfAP8LrioHlGy2dPWJHI8dS+MGosBZATpHYCFo39uz NZuRs/uEf4m7UF1PeWzV6k0dfQ== X-Google-Smtp-Source: AG47ELvd/FL32zAdIVT62yChCgEECwavyFXdE3UK0peebT6seB7bwh12MsGm9zr4qqVC/ihtaTzhtg== X-Received: by 2002:a17:902:ba95:: with SMTP id k21-v6mr18951688pls.111.1520387646447; Tue, 06 Mar 2018 17:54:06 -0800 (PST) Received: from SZX1000114654 ([45.56.152.76]) by smtp.gmail.com with ESMTPSA id l64sm35723471pfi.142.2018.03.06.17.54.04 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 06 Mar 2018 17:54:05 -0800 (PST) From: Guo Heyi X-Google-Original-From: Guo Heyi Date: Wed, 7 Mar 2018 09:54:02 +0800 To: "Ni, Ruiyu" Cc: Heyi Guo , edk2-devel@lists.01.org, Star Zeng , Eric Dong , Laszlo Ersek Message-ID: <20180307015402.GA91936@SZX1000114654> References: <1519871972-39281-1-git-send-email-heyi.guo@linaro.org> <8863d9e7-9735-1009-1476-a38b31305eaa@Intel.com> MIME-Version: 1.0 In-Reply-To: <8863d9e7-9735-1009-1476-a38b31305eaa@Intel.com> User-Agent: Mutt/1.5.24 (2015-08-30) 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: Wed, 07 Mar 2018 01:47:53 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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