On Tue, 5 Oct 2021 at 23:25, Jeremy Linton <jeremy.lin...@arm.com> wrote:
>
> Hi,
>
> On 10/5/21 5:11 AM, Ard Biesheuvel wrote:
> > On Sat, 2 Oct 2021 at 02:52, Jeremy Linton <jeremy.lin...@arm.com> wrote:
> >>
> >> In theory we should be properly cleaning up all the device drivers before
> >> pulling the big switch. Particularly the partition mgr will issue
> >> flush commands to attached disks as it goes down. This assures that
> >> devices running in WB mode (that correctly handle flush/sync/etc) commands
> >> are persisted to physical media before we hit reset.
> >>
> >> Without this, there are definitly cases where the relevant specifications
> >> don't guarantee persistence of data in their buffers in the face of
> >> reset conditions. We can't really do anything about the many
> >> devices that don't honor persistance requests but we can start here.
> >>
> >> Signed-off-by: Jeremy Linton <jeremy.lin...@arm.com>
> >> ---
> >> Platform/RaspberryPi/Library/ResetLib/ResetLib.c | 44
> >> ++++++++++++++++++++++++
> >> 1 file changed, 44 insertions(+)
> >>
> >> diff --git a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
> >> b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
> >> index a70eee485d..036f619cb5 100644
> >> --- a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
> >> +++ b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
> >> @@ -19,11 +19,54 @@
> >> #include <Library/TimerLib.h>
> >> #include <Library/EfiResetSystemLib.h>
> >> #include <Library/ArmSmcLib.h>
> >> +#include <Library/UefiBootServicesTableLib.h>
> >> #include <Library/UefiLib.h>
> >> #include <Library/UefiRuntimeLib.h>
> >>
> >> #include <IndustryStandard/ArmStdSmc.h>
> >>
> >> +
> >> +/**
> >> + Disconnect everything.
> >> + Modified from the UEFI 2.3 spec (May 2009 version)
> >> +
> >> + @retval EFI_SUCCESS The operation was successful.
> >> +
> >> +**/
> >
> > STATIC
> >
> >> +EFI_STATUS
> >> +DisconnectAll(
> >
> > Space before (
> >
> >> + VOID
> >> + )
> >> +{
> >> + EFI_STATUS Status;
> >> + UINTN HandleCount;
> >> + EFI_HANDLE *HandleBuffer;
> >> + UINTN HandleIndex;
> >> +
> >> + //
> >> + // Retrieve the list of all handles from the handle database
> >> + //
> >> + Status = gBS->LocateHandleBuffer (
> >> + AllHandles,
> >> + NULL,
> >> + NULL,
> >> + &HandleCount,
> >> + &HandleBuffer
> >> + );
> >> + if (!EFI_ERROR (Status)) {
> >
> > I understand that this code is copy/pasted but I'd still prefer to
> > avoid the 'success handling' anti pattern here.
>
> Sure.
>
> >
> > if (EFI_ERROR (Status)) {
> > return Status;
> > }
> >
> >> + for (HandleIndex = 0; HandleIndex < HandleCount; HandleIndex++) {
> >> + Status = gBS->DisconnectController (
> >> + HandleBuffer[HandleIndex],
> >> + NULL,
> >> + NULL
> >> + );
> >> + }
> >> + gBS->FreePool(HandleBuffer);
> >> + }
> >> + return (EFI_SUCCESS);
> >
> > No need for ()
>
> Yup
>
> >
> >> +}
> >> +
> >> +
> >> /**
> >> Resets the entire platform.
> >>
> >> @@ -57,6 +100,7 @@ LibResetSystem (
> >> if (Delay != 0) {
> >> DEBUG ((DEBUG_INFO, "Platform will be reset in %d.%d seconds...\n",
> >> Delay / 1000000, (Delay % 1000000) / 100000));
> >> + DisconnectAll ();
> >
> > Capture Status here and ASSERT_EFI_ERROR() ??
> >
> > Maybe it is overkill, and maybe DisconnectController() fails
> > spuriously, so I am not entirely sure, but adding a local function
> > that returns a value and then ignore it seems slightly sloppy to me.
>
> Which makes the above bits about failure returns sorta redundant as I
> should probably just make DisconnectAll() void. There isn't really
> anything to do with a failed return other than print a message and
> ignore it.
>
>
Works for me.
> >
> >> MicroSecondDelay (Delay);
> >> }
> >> }
> >> --
> >> 2.13.7
> >>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#81523): https://edk2.groups.io/g/devel/message/81523
Mute This Topic: https://groups.io/mt/86014865/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
