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

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

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.

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 ()

> +}
> +
> +
>  /**
>    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.

>        MicroSecondDelay (Delay);
>      }
>    }
> --
> 2.13.7
>