On 2/27/2018 8:48 AM, Guo Heyi wrote:
Hi Laszlo,
I agree the current patch makes the code ugly, and turning the logic into a
normal loop should be the perfect solution. If Ray also agrees on it, I can try
to do that.
Thanks and regards,
Heyi
On Mon, Feb 26, 2018 at 05:23:29PM +0100, Laszlo Ersek wrote:
On 02/26/18 09:29, 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.
Not really my place to comment on this, but how about removing the
recursion entirely, and turning the logic into a normal (iterative) loop
instead?
(1) Rename the current function to:
STATIC
VOID
BmRepairAllControllersWorker (
OUT BOOLEAN *ReconnectRequired,
OUT BOOLEAN *RebootRequired
);
(2) The worker function should end right before
if (ReconnectRequired) {
BmRepairAllControllers ();
}
(3) The worker function should not contain
RebootRequired = FALSE;
ReconnectRequired = FALSE;
Such initialization should be left to the caller.
(4) The worker function should be called in a loop from a new
BmRepairAllControllers() function, like this:
Reconnect = 0;
RebootRequired = FALSE;
do {
ReconnectRequired = FALSE;
BmRepairAllControllersWorker (&ReconnectRequired, &RebootRequired);
++Reconnect;
} while (ReconnectRequired && Reconnect < MAX_RECONNECT_REPAIR);
DEBUG_CODE (...);
if (RebootRequired) {
...
}
In addition to eliminating the shoddy recursive call (and the shoddier
global counter, ewww :) ), this would fix the following other warts with
the code:
- When a nested call chain is unwound, we currently dump a series of
"driver health info" lists (assuming no reboot is required), in the
DEBUG_CODE section. I would argue that we should do that only once, at
the end. (Even if we have to do it multiple times, it can be moved into
the worker function, to the end.)
- It seems to be sufficient to accumulate RebootRequired into one
variable (i.e. not multiple instances of the same local variable on the
stack) and to act upon it at the very end.
Feel free to ignore my comments -- I just think we should be moving in
the opposite direction; that is, away from recursion (especially from
recursion combined with global variables -- that's one difficult pattern
to reason about).
How about to just remove the global variable?
I prefer to change BmRepairAllControllers in the following prototype:
VOID
BmRepairAllControllers (
UINTN ReconnectRepairCount
);
And start to call this like BmRepairAllControllers (0).
I am neutral between recursive call and while loop.
But I am afraid such a big change may introduce some bugs.
And I also like to move the DEBUG_CODE to before:
if (ReconnectRequired) {
BmRepairAllControllers (ReconnectRepairCount + 1);
}
So that we can dump the health info for every reconnect repair.
Thanks
Laszlo
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo <heyi....@linaro.org>
Cc: Star Zeng <star.z...@intel.com>
Cc: Eric Dong <eric.d...@intel.com>
Cc: Ruiyu Ni <ruiyu...@intel.com>
---
MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h | 6 ++++++
MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c | 17
++++++++++++++++-
2 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
index 25a1d522fe84..9aa86b096525 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.
diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c
b/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c
index ddcee8b0676f..30d70f32af84 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c
+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c
@@ -26,6 +26,12 @@ GLOBAL_REMOVE_IF_UNREFERENCED
L"Reboot Required"
};
+//
+// Counter of reconnect retry to repair controller; it is to limit the
+// number of recursive call of BmRepairAllControllers.
+//
+STATIC UINTN mReconnectRepairCount;
+
/**
Return the controller name.
@@ -549,7 +555,16 @@ BmRepairAllControllers (
if (ReconnectRequired) {
- BmRepairAllControllers ();
+ if (mReconnectRepairCount < MAX_RECONNECT_REPAIR) {
+ mReconnectRepairCount++;
+ BmRepairAllControllers ();
+ } else {
+ DEBUG ((DEBUG_ERROR, "[%a:%d] Repair failed after %d retries.\n",
+ __FUNCTION__, __LINE__, mReconnectRepairCount));
+ // Reset counter so that it will not affect calling
+ // BmRepairAllControllers() somewhere else
+ mReconnectRepairCount = 0;
+ }
}
DEBUG_CODE (
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
--
Thanks,
Ray
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
