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

["Andrei Warkentin" <awarkentin@vmware.com>]

[-- Attachment #1: Type: text/plain, Size: 3783 bytes --]

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=arm.com@groups.io>
Sent: Monday, June 1, 2020 4:05 AM
To: Andrei Warkentin <andrey.warkentin@gmail.com>; devel@edk2.groups.io <devel@edk2.groups.io>
Cc: leif@nuviainc.com <leif@nuviainc.com>; pete@akeo.ie <pete@akeo.ie>; philmd@redhat.com <philmd@redhat.com>
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@gmail.com>
> ---
>   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@akeo.ie>
> - *  Copyright (c) 2017-2018, Andrei Warkentin <andrey.warkentin@gmail.com>
> + *  Copyright (c) 2017-2020, Andrei Warkentin <andrey.warkentin@gmail.com>
>    *  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
>





[-- Attachment #2: Type: text/html, Size: 6341 bytes --]