Re: [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: Add system/user defined reset delay

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

On 12/22/20 2:45 PM, Pete Batard wrote:
> Due to the method in which NV variables are stored on removable media
> for the Raspberry Pi platform, and the manner in which we dump updated
> variables right before reset, it is possible, and has been repeatedly
> demonstrated with SSD-based USB 3.0 devices, that the updated file does
> not actually end up being written to permanent storage, due to the
> device write-cache not having enough time to be flushed before reset.
> 
> To compensate for this, since we don't know of a generic method that
> would allow turning off USB mass storage devices write cache (and also
> because we are seeing an issue that seems related for SD-based media),
> we add a new reset delay PCD, which can be set by the user, and which
> we also set as required when NV variables are being dumped.
> 
> Our testing show that, with more than 3 seconds of extra delay, the
> storage media has enough time to finalize its internal write, thus
> solving the issue of configuration changes not being persisted.
> 
> Signed-off-by: Pete Batard <pete@akeo.ie>
> ---
>  Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.c   | 24 ++++++++++++++++++++
>  Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.inf |  1 +
>  Platform/RaspberryPi/Library/ResetLib/ResetLib.c                       | 11 +++++++++
>  Platform/RaspberryPi/Library/ResetLib/ResetLib.inf                     |  5 ++++
>  Platform/RaspberryPi/RPi3/RPi3.dsc                                     |  6 +++++
>  Platform/RaspberryPi/RPi4/RPi4.dsc                                     |  6 +++++
>  Platform/RaspberryPi/RaspberryPi.dec                                   |  1 +
>  7 files changed, 54 insertions(+)
> 
> diff --git a/Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.c b/Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.c
> index 07f3f1c24295..4071a3fca468 100644
> --- a/Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.c
> +++ b/Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.c
> @@ -10,6 +10,18 @@
>  
>  #include "VarBlockService.h"
>  
> +//
> +// Minimum delay to enact before reset, when variables are dirty (in μs).
> +// Needed to ensure that SSD-based USB 3.0 devices have time to flush their
> +// write cache after updating the NV vars. A much smaller delay is applied
> +// on Pi 3 compared to Pi 4, as we haven't had reports of issues there yet.
> +//
> +#if (RPI_MODEL == 3)
> +#define PLATFORM_RESET_DELAY     500000
> +#else
> +#define PLATFORM_RESET_DELAY    3500000
> +#endif
> +
>  VOID *mSFSRegistration;
>  
>  
> @@ -154,6 +166,7 @@ DumpVars (
>    )
>  {
>    EFI_STATUS Status;
> +  RETURN_STATUS PcdStatus;
>  
>    if (mFvInstance->Device == NULL) {
>      DEBUG ((DEBUG_INFO, "Variable store not found?\n"));
> @@ -173,6 +186,17 @@ DumpVars (
>    }
>  
>    DEBUG ((DEBUG_INFO, "Variables dumped!\n"));
> +
> +  //
> +  // Add a reset delay to give time for slow/cached devices
> +  // to flush the NV variables write to permanent storage.
> +  // But only do so if this won't reduce an existing user-set delay.
> +  //
> +  if (PcdGet32 (PcdPlatformResetDelay) < PLATFORM_RESET_DELAY) {
> +    PcdStatus = PcdSet32S (PcdPlatformResetDelay, PLATFORM_RESET_DELAY);
> +    ASSERT_RETURN_ERROR (PcdStatus);
> +  }
> +
>    mFvInstance->Dirty = FALSE;
>  }
>  
> diff --git a/Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.inf b/Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.inf
> index ecfb8f85c9c1..c2edb25bd41d 100644
> --- a/Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.inf
> +++ b/Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.inf
> @@ -79,6 +79,7 @@ [Pcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase
>    gRaspberryPiTokenSpaceGuid.PcdNvStorageEventLogBase
> +  gRaspberryPiTokenSpaceGuid.PcdPlatformResetDelay
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64
>  
>  [FeaturePcd]
> diff --git a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
> index c62a92321ecb..4a50166dd63b 100644
> --- a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
> +++ b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
> @@ -16,6 +16,7 @@
>  
>  #include <Library/BaseLib.h>
>  #include <Library/DebugLib.h>
> +#include <Library/TimerLib.h>
>  #include <Library/EfiResetSystemLib.h>
>  #include <Library/ArmSmcLib.h>
>  #include <Library/UefiLib.h>
> @@ -44,6 +45,7 @@ LibResetSystem (
>    )
>  {
>    ARM_SMC_ARGS ArmSmcArgs;
> +  UINT32 Delay;
>  
>    if (!EfiAtRuntime ()) {
>      /*
> @@ -52,6 +54,15 @@ LibResetSystem (
>      EfiEventGroupSignal (&gRaspberryPiEventResetGuid);
>    }
>  
> +  Delay = PcdGet32 (PcdPlatformResetDelay);
> +  if (Delay != 0) {
> +    DEBUG ((DEBUG_INFO, "Platform will be reset in %d.%d seconds...\n",
> +            Delay / 1000000, (Delay % 1000000) / 100000));
> +    MicroSecondDelay (Delay);
> +  }

Doesn't this cause a crash at runtime on the DEBUG build due to the fact
that the UART is no longer 1:1 mapped?


> +  DEBUG ((DEBUG_INFO, "Platform %a.\n",
> +          (ResetType == EfiResetShutdown) ? "shutdown" : "reset"));
> +
>    switch (ResetType) {
>    case EfiResetPlatformSpecific:
>      // Map the platform specific reset as reboot
> diff --git a/Platform/RaspberryPi/Library/ResetLib/ResetLib.inf b/Platform/RaspberryPi/Library/ResetLib/ResetLib.inf
> index b02a06d9d0bf..9bdb94a52ebf 100644
> --- a/Platform/RaspberryPi/Library/ResetLib/ResetLib.inf
> +++ b/Platform/RaspberryPi/Library/ResetLib/ResetLib.inf
> @@ -33,8 +33,13 @@ [LibraryClasses]
>    DebugLib
>    BaseLib
>    ArmSmcLib
> +  PcdLib
> +  TimerLib
>    UefiLib
>    UefiRuntimeLib
>  
>  [Guids]
>    gRaspberryPiEventResetGuid
> +
> +[Pcd]
> +  gRaspberryPiTokenSpaceGuid.PcdPlatformResetDelay      ## CONSUMES
> diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc
> index 9408138d0a09..530b42796a0d 100644
> --- a/Platform/RaspberryPi/RPi3/RPi3.dsc
> +++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
> @@ -508,6 +508,12 @@ [PcdsDynamicHii.common.DEFAULT]
>    gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|L"FanOnGpio"|gConfigDxeFormSetGuid|0x0|0
>    gRaspberryPiTokenSpaceGuid.PcdFanTemp|L"FanTemp"|gConfigDxeFormSetGuid|0x0|0
>  
> +  #
> +  # Reset-related.
> +  #
> +
> +  gRaspberryPiTokenSpaceGuid.PcdPlatformResetDelay|L"ResetDelay"|gRaspberryPiTokenSpaceGuid|0x0|0
> +
>    #
>    # Common UEFI ones.
>    #
> diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
> index ddf4dd6a416e..5f8452aa0b76 100644
> --- a/Platform/RaspberryPi/RPi4/RPi4.dsc
> +++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
> @@ -522,6 +522,12 @@ [PcdsDynamicHii.common.DEFAULT]
>    gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|L"FanOnGpio"|gConfigDxeFormSetGuid|0x0|0
>    gRaspberryPiTokenSpaceGuid.PcdFanTemp|L"FanTemp"|gConfigDxeFormSetGuid|0x0|60
>  
> +  #
> +  # Reset-related.
> +  #
> +
> +  gRaspberryPiTokenSpaceGuid.PcdPlatformResetDelay|L"ResetDelay"|gRaspberryPiTokenSpaceGuid|0x0|0
> +
>    #
>    # Common UEFI ones.
>    #
> diff --git a/Platform/RaspberryPi/RaspberryPi.dec b/Platform/RaspberryPi/RaspberryPi.dec
> index c64c61930ea8..10723036aa31 100644
> --- a/Platform/RaspberryPi/RaspberryPi.dec
> +++ b/Platform/RaspberryPi/RaspberryPi.dec
> @@ -68,3 +68,4 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>    gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB|0|UINT32|0x0000001A
>    gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|0|UINT32|0x0000001C
>    gRaspberryPiTokenSpaceGuid.PcdFanTemp|0|UINT32|0x0000001D
> +  gRaspberryPiTokenSpaceGuid.PcdPlatformResetDelay|0|UINT32|0x0000001E
>