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:c05::235; helo=mail-pg0-x235.google.com; envelope-from=heyi.guo@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-pg0-x235.google.com (mail-pg0-x235.google.com [IPv6:2607:f8b0:400e:c05::235]) (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 9D84E2235229C for ; Wed, 28 Feb 2018 18:37:15 -0800 (PST) Received: by mail-pg0-x235.google.com with SMTP id i133so1745605pgc.12 for ; Wed, 28 Feb 2018 18:43:23 -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=Uxc9W2ysdBRC8SSlt/SihwJeFDJ0OI6jj9iY4AiTM/g=; b=KI9Mi/b99TOzVmq+YKoaBOjPsPUwSdtacyqDhSkFpLSbfAM6iBiGo82y9g51Yy+Wm2 +ul+SpTjXozgdVHdPlDxji7go/wv3VTau2G7xdYyEVegbsqRNnXxxyQm4nyPPkMgwRv6 MaOpJeBmDT/EPAp6NxeOsQ3fk5JK4+goWoLiI= 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=Uxc9W2ysdBRC8SSlt/SihwJeFDJ0OI6jj9iY4AiTM/g=; b=Xf9Z3RqZhRn0uDdWKQz9z5UjWXwWqd65Hx2+qA1QfuUwEBd+Trdv9yMEPQZYgN/TBK 37C4bAkcASn43bSoX7WYx3BwqFYTER9d2/zbp6/oLHMEGFjWfW3IvkMOIxORUz8yUjN4 kvt0a+Ttj4FDYGLA/h9mfpcGBRkUriSV34MA6sBoLEZwQiwD6msK/kG/RxvYKjTG/gXh OB8pAtSsB2v8mpjvlZgW29PXJzoEtBUj6ymPs2bKOBiY/xQN28BcMLfedGtrdmXzx9cS Tq3V0RF5BLCLXjnNezMF0kkm2cmLs6DZuMFQuzYX6+FEwsykvo4j79yQix55MOJXozr4 r7yQ== X-Gm-Message-State: APf1xPBPMMxKYuxGZb4Xf2g37IFc/Kb2n/VziZvqolZlfhmU++v20ymu Erht7HajtGq8ihoVjrAPMgPdwQ== X-Google-Smtp-Source: AG47ELsjDxNLxhyTg05QiJrTqqYUrC4oNJsBDdf1/cCPmUYaQBIDhrfYjAvstlzVeV9dHvIomFr/IQ== X-Received: by 10.101.78.5 with SMTP id r5mr255973pgt.33.1519872203161; Wed, 28 Feb 2018 18:43:23 -0800 (PST) Received: from SZX1000114654 ([45.56.152.115]) by smtp.gmail.com with ESMTPSA id n1sm4160676pge.19.2018.02.28.18.43.21 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 28 Feb 2018 18:43:22 -0800 (PST) From: Guo Heyi X-Google-Original-From: Guo Heyi Date: Thu, 1 Mar 2018 10:43:19 +0800 To: Heyi Guo Cc: edk2-devel@lists.01.org, Star Zeng , Eric Dong , Ruiyu Ni , Laszlo Ersek Message-ID: <20180301024319.GA39361@SZX1000114654> References: <1519871972-39281-1-git-send-email-heyi.guo@linaro.org> MIME-Version: 1.0 In-Reply-To: <1519871972-39281-1-git-send-email-heyi.guo@linaro.org> 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: Thu, 01 Mar 2018 02:37:16 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > 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); > -- > 2.7.4 >