[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 18:39:49 UTC 2021
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 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?
>
> 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
>>>
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69624): https://edk2.groups.io/g/devel/message/69624
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