From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.3113.1633469108988991409 for ; Tue, 05 Oct 2021 14:25:09 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: jeremy.linton@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 80FEF1FB; Tue, 5 Oct 2021 14:25:07 -0700 (PDT) Received: from [192.168.122.166] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id EF2863F70D; Tue, 5 Oct 2021 14:25:06 -0700 (PDT) Subject: Re: [PATCH 5/5] Platform/RaspberryPi: Disconnect/shutdown all drivers before reboot To: Ard Biesheuvel Cc: edk2-devel-groups-io , Peter Batard , Ard Biesheuvel , Leif Lindholm , Andrei Warkentin , Sunny Wang , Samer El-Haj-Mahmoud References: <20211002005238.40280-1-jeremy.linton@arm.com> <20211002005238.40280-6-jeremy.linton@arm.com> From: "Jeremy Linton" Message-ID: <452e1938-3d6d-97f0-9922-35a7d573c621@arm.com> Date: Tue, 5 Oct 2021 16:24:35 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Hi, On 10/5/21 5:11 AM, Ard Biesheuvel wrote: > On Sat, 2 Oct 2021 at 02:52, Jeremy Linton 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 >> --- >> 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 >> #include >> #include >> +#include >> #include >> #include >> >> #include >> >> + >> +/** >> + 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 >>