[edk2-devel] [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: Add system/user defined reset delay
Ard Biesheuvel
ard.biesheuvel at arm.com
Mon Jan 4 09:44:13 UTC 2021
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 at 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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69549): https://edk2.groups.io/g/devel/message/69549
Mute This Topic: https://groups.io/mt/79148769/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list