Re: [PATCH 5/5] Platform/RaspberryPi: Disconnect/shutdown all drivers before reboot

["Ard Biesheuvel" <ardb@kernel.org>]

On Tue, 5 Oct 2021 at 23:25, Jeremy Linton <jeremy.linton@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.linton@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.linton@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
> >>
>