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

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

On 12/25/20 7:10 PM, Samer El-Haj-Mahmoud wrote:
> I tested the patch using the procedure documented in https://github.com/pftf/RPi4/issues/95 (which is relatively easy to reproduce), and did not se the async exception/hang with SD card. The SCT Runtime Variables test complete without any hangs. It looks like this may be a sufficient workaround for that issue.
> 
> Tested-By: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> 
> 

Pushed as a34e27a1fa79..94e9fba43d7e

Thanks all!


>> -----Original Message-----
>> From: Pete Batard <pete@akeo.ie>
>> Sent: Tuesday, December 22, 2020 8:45 AM
>> To: devel@edk2.groups.io
>> Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>; leif@nuviainc.com; Samer
>> El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; Andrei Warkentin
>> (awarkentin@vmware.com) <awarkentin@vmware.com>
>> Subject: [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: Add
>> system/user defined reset delay
>>
>> 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);
>>
>> +  }
>>
>> +  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"|gConfigDxeFor
>> mSetGuid|0x0|0
>>
>>
>> gRaspberryPiTokenSpaceGuid.PcdFanTemp|L"FanTemp"|gConfigDxeFormS
>> etGuid|0x0|0
>>
>>
>>
>> +  #
>>
>> +  # Reset-related.
>>
>> +  #
>>
>> +
>>
>> +
>> gRaspberryPiTokenSpaceGuid.PcdPlatformResetDelay|L"ResetDelay"|gRasp
>> berryPiTokenSpaceGuid|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"|gConfigDxeFor
>> mSetGuid|0x0|0
>>
>>
>> gRaspberryPiTokenSpaceGuid.PcdFanTemp|L"FanTemp"|gConfigDxeFormS
>> etGuid|0x0|60
>>
>>
>>
>> +  #
>>
>> +  # Reset-related.
>>
>> +  #
>>
>> +
>>
>> +
>> gRaspberryPiTokenSpaceGuid.PcdPlatformResetDelay|L"ResetDelay"|gRasp
>> berryPiTokenSpaceGuid|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|0x0000001
>> E
>>
>> --
>> 2.29.2.windows.2
>