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

["Jeremy Linton" <jeremy.linton@arm.com>]

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.


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