From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from foss.arm.com (foss.arm.com [217.140.110.172])
 by mx.groups.io with SMTP id smtpd.web12.32870.1591002308940257965
 for <devel@edk2.groups.io>;
 Mon, 01 Jun 2020 02:05:09 -0700
Authentication-Results: mx.groups.io;
 dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com)
Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14])
	by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 695481FB;
	Mon,  1 Jun 2020 02:05:07 -0700 (PDT)
Received: from [192.168.1.69] (unknown [172.31.20.19])
	by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 81B1F3F305;
	Mon,  1 Jun 2020 02:05:06 -0700 (PDT)
Subject: Re: [edk2-platforms][PATCH 1/1] RPi4: add descriptive errors in PlatformRegisterOptionsAndKeys
To: Andrei Warkentin <andrey.warkentin@gmail.com>, devel@edk2.groups.io
Cc: leif@nuviainc.com, pete@akeo.ie, philmd@redhat.com
References: <20200601073238.101651-1-andrey.warkentin@gmail.com>
From: "Ard Biesheuvel" <ard.biesheuvel@arm.com>
Message-ID: <48a133a5-6d57-9469-562e-73f9731693fa@arm.com>
Date: Mon, 1 Jun 2020 11:05:04 +0200
User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:68.0) Gecko/20100101
 Thunderbird/68.8.1
MIME-Version: 1.0
In-Reply-To: <20200601073238.101651-1-andrey.warkentin@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit

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
>