* [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: Add system/user defined reset delay
@ 2020-12-22 13:45 Pete Batard
2020-12-24 4:09 ` Andrei Warkentin
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Pete Batard @ 2020-12-22 13:45 UTC (permalink / raw)
To: devel; +Cc: ard.biesheuvel, leif, samer.el-haj-mahmoud, awarkentin
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"|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
--
2.29.2.windows.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: Add system/user defined reset delay
2020-12-22 13:45 [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: Add system/user defined reset delay Pete Batard
@ 2020-12-24 4:09 ` Andrei Warkentin
2020-12-25 18:10 ` Samer El-Haj-Mahmoud
2021-01-04 9:44 ` Ard Biesheuvel
2 siblings, 0 replies; 7+ messages in thread
From: Andrei Warkentin @ 2020-12-24 4:09 UTC (permalink / raw)
To: Pete Batard, devel@edk2.groups.io
Cc: ard.biesheuvel@arm.com, leif@nuviainc.com,
samer.el-haj-mahmoud@arm.com
[-- Attachment #1: Type: text/plain, Size: 8375 bytes --]
Sure. I wish there was something more precise, I'm guessing that would at least be very different between USB and SD/MMC...
Reviewed-by: Andrei Warkentin <awarkentin@vmware.com>
________________________________
From: Pete Batard <pete@akeo.ie>
Sent: Tuesday, December 22, 2020 7:45 AM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: ard.biesheuvel@arm.com <ard.biesheuvel@arm.com>; leif@nuviainc.com <leif@nuviainc.com>; samer.el-haj-mahmoud@arm.com <samer.el-haj-mahmoud@arm.com>; Andrei Warkentin <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"|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
--
2.29.2.windows.2
[-- Attachment #2: Type: text/html, Size: 12482 bytes --]
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: Add system/user defined reset delay
2020-12-22 13:45 [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: Add system/user defined reset delay Pete Batard
2020-12-24 4:09 ` Andrei Warkentin
@ 2020-12-25 18:10 ` Samer El-Haj-Mahmoud
2021-01-04 17:10 ` Ard Biesheuvel
2021-01-04 9:44 ` Ard Biesheuvel
2 siblings, 1 reply; 7+ messages in thread
From: Samer El-Haj-Mahmoud @ 2020-12-25 18:10 UTC (permalink / raw)
To: Pete Batard, devel@edk2.groups.io
Cc: Ard Biesheuvel, leif@nuviainc.com,
Andrei Warkentin (awarkentin@vmware.com), Samer El-Haj-Mahmoud
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>
> -----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
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: Add system/user defined reset delay
2020-12-22 13:45 [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: Add system/user defined reset delay Pete Batard
2020-12-24 4:09 ` Andrei Warkentin
2020-12-25 18:10 ` Samer El-Haj-Mahmoud
@ 2021-01-04 9:44 ` Ard Biesheuvel
2021-01-04 18:24 ` Pete Batard
2 siblings, 1 reply; 7+ messages in thread
From: Ard Biesheuvel @ 2021-01-04 9:44 UTC (permalink / raw)
To: Pete Batard, devel; +Cc: leif, samer.el-haj-mahmoud, awarkentin
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: Add system/user defined reset delay
2020-12-25 18:10 ` Samer El-Haj-Mahmoud
@ 2021-01-04 17:10 ` Ard Biesheuvel
0 siblings, 0 replies; 7+ messages in thread
From: Ard Biesheuvel @ 2021-01-04 17:10 UTC (permalink / raw)
To: Samer El-Haj-Mahmoud, Pete Batard, devel@edk2.groups.io
Cc: leif@nuviainc.com, Andrei Warkentin (awarkentin@vmware.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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: Add system/user defined reset delay
2021-01-04 9:44 ` Ard Biesheuvel
@ 2021-01-04 18:24 ` Pete Batard
2021-01-04 18:39 ` Ard Biesheuvel
0 siblings, 1 reply; 7+ messages in thread
From: Pete Batard @ 2021-01-04 18:24 UTC (permalink / raw)
To: Ard Biesheuvel, devel; +Cc: leif, samer.el-haj-mahmoud, awarkentin
Hi Ard,
Thanks for pushing the patch. But I think it will need some amending
indeed, as per comments below:
On 2021.01.04 09:44, Ard Biesheuvel wrote:
> 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?
You are bringing a good point that made me test this patch a bit more
comprehensively.
Whereas I saw no ill outcome from leaving the DEBUG statements in the
DEBUG firmware, I am seeing a kernel panic in Linux (I mostly tested
with Debian 11) on reset/poweroff, with either the RELEASE or DEBUG UEFI
firmware, which I have isolated to the PcdGet32 () line.
My understanding is that this occurs because we shouldn't use that call
after ExitBootServices (), and therefore the 6 new lines above should
have been inside the "if (!EfiAtRuntime ())" block, rather than as an
unconditional section of code. It also makes sense to only have that
code there anyway, since, even if I tried to introduce a generic PCD,
this patch is mostly a workaround for variables not being stored after
the user changes them though the firmware UI, and therefore where
platform reset happens before we exit boot services.
Thus, unless someone sees it differently, I'll be sending an addendum
patch, that moves the 6 lines above into the "if (!EfiAtRuntime ())" block.
Regards,
/Pete
>
>
>> + 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
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: Add system/user defined reset delay
2021-01-04 18:24 ` Pete Batard
@ 2021-01-04 18:39 ` Ard Biesheuvel
0 siblings, 0 replies; 7+ messages in thread
From: Ard Biesheuvel @ 2021-01-04 18:39 UTC (permalink / raw)
To: Pete Batard, devel; +Cc: leif, samer.el-haj-mahmoud, awarkentin
On 1/4/21 7:24 PM, Pete Batard wrote:
> Hi Ard,
>
> Thanks for pushing the patch. But I think it will need some amending
> indeed, as per comments below:
>
Hmm, I did not intend to merge it, but apparently I did ...
Please follow up with a fix that addresses the crash, thanks.
--
Ard.
> On 2021.01.04 09:44, Ard Biesheuvel wrote:
>> 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?
>
> You are bringing a good point that made me test this patch a bit more
> comprehensively.
>
> Whereas I saw no ill outcome from leaving the DEBUG statements in the
> DEBUG firmware, I am seeing a kernel panic in Linux (I mostly tested
> with Debian 11) on reset/poweroff, with either the RELEASE or DEBUG UEFI
> firmware, which I have isolated to the PcdGet32 () line.
>
> My understanding is that this occurs because we shouldn't use that call
> after ExitBootServices (), and therefore the 6 new lines above should
> have been inside the "if (!EfiAtRuntime ())" block, rather than as an
> unconditional section of code. It also makes sense to only have that
> code there anyway, since, even if I tried to introduce a generic PCD,
> this patch is mostly a workaround for variables not being stored after
> the user changes them though the firmware UI, and therefore where
> platform reset happens before we exit boot services.
>
> Thus, unless someone sees it differently, I'll be sending an addendum
> patch, that moves the 6 lines above into the "if (!EfiAtRuntime ())" block.
>
> Regards,
>
> /Pete
>
>>
>>
>>> + 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
>>>
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-01-04 18:39 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-12-22 13:45 [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: Add system/user defined reset delay Pete Batard
2020-12-24 4:09 ` Andrei Warkentin
2020-12-25 18:10 ` Samer El-Haj-Mahmoud
2021-01-04 17:10 ` Ard Biesheuvel
2021-01-04 9:44 ` Ard Biesheuvel
2021-01-04 18:24 ` Pete Batard
2021-01-04 18:39 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox