[edk2-platforms][PATCH 1/1] RPi4: add descriptive errors in PlatformRegisterOptionsAndKeys


Andrei Warkentin
 

I have reports of debug builds sitting with an ASSERT inside
PlatformRegisterOptionsAndKeys, but have not been able to verify.
Let's log errors to help diagnose this in the future.

Signed-off-by: Andrei Warkentin <andrey.warkentin@...>
---
Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c | 25 ++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c b/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c
index 996ba8f3..f0a2fe1a 100644
--- a/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c
@@ -1,7 +1,7 @@
/** @file
*
* Copyright (c) 2018, Pete Batard <pete@...>
- * Copyright (c) 2017-2018, Andrei Warkentin <andrey.warkentin@...>
+ * Copyright (c) 2017-2020, Andrei Warkentin <andrey.warkentin@...>
* Copyright (c) 2016, Linaro Ltd. All rights reserved.
* Copyright (c) 2015-2016, Red Hat, Inc.
* Copyright (c) 2014, ARM Ltd. All rights reserved.
@@ -468,7 +468,13 @@ PlatformRegisterOptionsAndKeys (
F1.ScanCode = SCAN_F1;
F1.UnicodeChar = CHAR_NULL;
Status = EfiBootManagerAddKeyOptionVariable (NULL, (UINT16)ShellOption, 0, &F1, NULL);
- ASSERT (Status == EFI_SUCCESS || Status == EFI_ALREADY_STARTED);
+ if (Status == EFI_ALREADY_STARTED) {
+ Status = EFI_SUCCESS;
+ }
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Failed to register F1 as UEFI Shell key: %r\n", Status));
+ }
+ ASSERT_EFI_ERROR (Status);
}

//
@@ -477,6 +483,9 @@ PlatformRegisterOptionsAndKeys (
Enter.ScanCode = SCAN_NULL;
Enter.UnicodeChar = CHAR_CARRIAGE_RETURN;
Status = EfiBootManagerRegisterContinueKeyOption (0, &Enter, NULL);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Failed to register ENTER as CONTINUE key: %r\n", Status));
+ }
ASSERT_EFI_ERROR (Status);

//
@@ -485,9 +494,17 @@ PlatformRegisterOptionsAndKeys (
Esc.ScanCode = SCAN_ESC;
Esc.UnicodeChar = CHAR_NULL;
Status = EfiBootManagerGetBootManagerMenu (&BootOption);
+ if (!EFI_ERROR (Status)) {
+ Status = EfiBootManagerAddKeyOptionVariable (NULL, (UINT16)BootOption.OptionNumber, 0, &Esc, NULL);
+ if (Status == EFI_ALREADY_STARTED) {
+ Status = EFI_SUCCESS;
+ }
+ }
+
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Failed to register ESC as Boot Manager key: %r\n", Status));
+ }
ASSERT_EFI_ERROR (Status);
- Status = EfiBootManagerAddKeyOptionVariable (NULL, (UINT16)BootOption.OptionNumber, 0, &Esc, NULL);
- ASSERT (Status == EFI_SUCCESS || Status == EFI_ALREADY_STARTED);
}

STATIC VOID
--
2.17.1


Ard Biesheuvel
 

On 6/1/20 9:32 AM, Andrei Warkentin wrote:
I have reports of debug builds sitting with an ASSERT inside
PlatformRegisterOptionsAndKeys, but have not been able to verify.
Let's log errors to help diagnose this in the future.
Signed-off-by: Andrei Warkentin <andrey.warkentin@...>
---
Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c | 25 ++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c b/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c
index 996ba8f3..f0a2fe1a 100644
--- a/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c
@@ -1,7 +1,7 @@
/** @file
*
* Copyright (c) 2018, Pete Batard <pete@...>
- * Copyright (c) 2017-2018, Andrei Warkentin <andrey.warkentin@...>
+ * Copyright (c) 2017-2020, Andrei Warkentin <andrey.warkentin@...>
* Copyright (c) 2016, Linaro Ltd. All rights reserved.
* Copyright (c) 2015-2016, Red Hat, Inc.
* Copyright (c) 2014, ARM Ltd. All rights reserved.
@@ -468,7 +468,13 @@ PlatformRegisterOptionsAndKeys (
F1.ScanCode = SCAN_F1;
F1.UnicodeChar = CHAR_NULL;
Status = EfiBootManagerAddKeyOptionVariable (NULL, (UINT16)ShellOption, 0, &F1, NULL);
- ASSERT (Status == EFI_SUCCESS || Status == EFI_ALREADY_STARTED);
+ if (Status == EFI_ALREADY_STARTED) {
+ Status = EFI_SUCCESS;
+ }
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Failed to register F1 as UEFI Shell key: %r\n", Status));
+ }
+ ASSERT_EFI_ERROR (Status);
Mind if we invert this 'success handling' pattern?

if (EFI_ERROR (Status) && Status != EFI_ALREADY_STARTED) {
DEBUG ((DEBUG_ERROR, "Failed to register ...
ASSERT_EFI_ERROR (Status);
}

(same below)

}
//
@@ -477,6 +483,9 @@ PlatformRegisterOptionsAndKeys (
Enter.ScanCode = SCAN_NULL;
Enter.UnicodeChar = CHAR_CARRIAGE_RETURN;
Status = EfiBootManagerRegisterContinueKeyOption (0, &Enter, NULL);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Failed to register ENTER as CONTINUE key: %r\n", Status));
+ }
ASSERT_EFI_ERROR (Status);
//
@@ -485,9 +494,17 @@ PlatformRegisterOptionsAndKeys (
Esc.ScanCode = SCAN_ESC;
Esc.UnicodeChar = CHAR_NULL;
Status = EfiBootManagerGetBootManagerMenu (&BootOption);
+ if (!EFI_ERROR (Status)) {
+ Status = EfiBootManagerAddKeyOptionVariable (NULL, (UINT16)BootOption.OptionNumber, 0, &Esc, NULL);
+ if (Status == EFI_ALREADY_STARTED) {
+ Status = EFI_SUCCESS;
+ }
+ }
+
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Failed to register ESC as Boot Manager key: %r\n", Status));
+ }
ASSERT_EFI_ERROR (Status);
- Status = EfiBootManagerAddKeyOptionVariable (NULL, (UINT16)BootOption.OptionNumber, 0, &Esc, NULL);
- ASSERT (Status == EFI_SUCCESS || Status == EFI_ALREADY_STARTED);
}
STATIC VOID


Andrei Warkentin
 

Sure. Let me know if you want me to resend.

A


From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Ard Biesheuvel via groups.io <ard.biesheuvel@...>
Sent: Monday, June 1, 2020 4:05 AM
To: Andrei Warkentin <andrey.warkentin@...>; devel@edk2.groups.io <devel@edk2.groups.io>
Cc: leif@... <leif@...>; pete@... <pete@...>; philmd@... <philmd@...>
Subject: Re: [edk2-devel] [edk2-platforms][PATCH 1/1] RPi4: add descriptive errors in PlatformRegisterOptionsAndKeys
 
On 6/1/20 9:32 AM, Andrei Warkentin wrote:
> I have reports of debug builds sitting with an ASSERT inside
> PlatformRegisterOptionsAndKeys, but have not been able to verify.
> Let's log errors to help diagnose this in the future.
>
> Signed-off-by: Andrei Warkentin <andrey.warkentin@...>
> ---
>   Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c | 25 ++++++++++++++++----
>   1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c b/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c
> index 996ba8f3..f0a2fe1a 100644
> --- a/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -1,7 +1,7 @@
>   /** @file
>    *
>    *  Copyright (c) 2018, Pete Batard <pete@...>
> - *  Copyright (c) 2017-2018, Andrei Warkentin <andrey.warkentin@...>
> + *  Copyright (c) 2017-2020, Andrei Warkentin <andrey.warkentin@...>
>    *  Copyright (c) 2016, Linaro Ltd. All rights reserved.
>    *  Copyright (c) 2015-2016, Red Hat, Inc.
>    *  Copyright (c) 2014, ARM Ltd. All rights reserved.
> @@ -468,7 +468,13 @@ PlatformRegisterOptionsAndKeys (
>       F1.ScanCode = SCAN_F1;
>       F1.UnicodeChar = CHAR_NULL;
>       Status = EfiBootManagerAddKeyOptionVariable (NULL, (UINT16)ShellOption, 0, &F1, NULL);
> -    ASSERT (Status == EFI_SUCCESS || Status == EFI_ALREADY_STARTED);
> +    if (Status == EFI_ALREADY_STARTED) {
> +      Status = EFI_SUCCESS;
> +    }
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "Failed to register F1 as UEFI Shell key: %r\n", Status));
> +    }
> +    ASSERT_EFI_ERROR (Status);

Mind if we invert this 'success handling' pattern?

if (EFI_ERROR (Status) && Status != EFI_ALREADY_STARTED) {
   DEBUG ((DEBUG_ERROR, "Failed to register ...
   ASSERT_EFI_ERROR (Status);
}

(same below)

>     }
>  
>     //
> @@ -477,6 +483,9 @@ PlatformRegisterOptionsAndKeys (
>     Enter.ScanCode = SCAN_NULL;
>     Enter.UnicodeChar = CHAR_CARRIAGE_RETURN;
>     Status = EfiBootManagerRegisterContinueKeyOption (0, &Enter, NULL);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "Failed to register ENTER as CONTINUE key: %r\n", Status));
> +  }
>     ASSERT_EFI_ERROR (Status);
>  
>     //
> @@ -485,9 +494,17 @@ PlatformRegisterOptionsAndKeys (
>     Esc.ScanCode = SCAN_ESC;
>     Esc.UnicodeChar = CHAR_NULL;
>     Status = EfiBootManagerGetBootManagerMenu (&BootOption);
> +  if (!EFI_ERROR (Status)) {
> +    Status = EfiBootManagerAddKeyOptionVariable (NULL, (UINT16)BootOption.OptionNumber, 0, &Esc, NULL);
> +    if (Status == EFI_ALREADY_STARTED) {
> +      Status = EFI_SUCCESS;
> +    }
> +  }
> +
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "Failed to register ESC as Boot Manager key: %r\n", Status));
> +  }
>     ASSERT_EFI_ERROR (Status);
> -  Status = EfiBootManagerAddKeyOptionVariable (NULL, (UINT16)BootOption.OptionNumber, 0, &Esc, NULL);
> -  ASSERT (Status == EFI_SUCCESS || Status == EFI_ALREADY_STARTED);
>   }
>  
>   STATIC VOID
>