From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by mx.groups.io with SMTP id smtpd.web12.23510.1633428691905064823 for ; Tue, 05 Oct 2021 03:11:31 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=S0HsqHe2; spf=pass (domain: kernel.org, ip: 198.145.29.99, mailfrom: ardb@kernel.org) Received: by mail.kernel.org (Postfix) with ESMTPSA id D1B6A6159A for ; Tue, 5 Oct 2021 10:11:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1633428689; bh=gpMvo4j3Ktw0zQtymcTwMkzgR4QpzqiHHyPZ7+coq3s=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=S0HsqHe2Ron6P/80UmiI4ja1SLQBTRSQslRwSku5HenaF+Oz9vd7LQ0XcbPsq5tfE uDz4xoTCWMtqS8tbRwCFa0pHulZdJQhbUCpbTB3NqvqxgQriQIjPrGS4YE77WfAzEU NtDi02WotcyWDD8U8xpm26jOGyN7DSll3F4k2YE2WrKj/Liaz/xN44UmPpMV/M2EMG eHQf3uXxPVVe0aa+MWVaVFEl98W642l6oq0fvug+mPz97AqqkYe75NMQrY+9lozYBG VehM7xjqKMWBQxtgRhST+i/TfO5gnDbW4a0g6kdsx5CE8prUyrHim2CubFEyh2Kzfg VvlQRSeu9E/aA== Received: by mail-ot1-f44.google.com with SMTP id c26-20020a056830349a00b0054d96d25c1eso25173915otu.9 for ; Tue, 05 Oct 2021 03:11:29 -0700 (PDT) X-Gm-Message-State: AOAM532N3qBtfCDwd3hdkdMd7YCBwvW9J2dudRzeSF+Tdj9MAPKKAkeb v0V/GFIy+/7PlxGXD+lcyPTVw2w2Qdsvp7D/a6Q= X-Google-Smtp-Source: ABdhPJyCEP8dyC7D3fk1umIwK7OfMGfa4TGOpmNV1kLJq3LSK6Bsywgd0vti3dQGTmIA9TNx7d0pElDzWZORCYMNstc= X-Received: by 2002:a05:6830:3189:: with SMTP id p9mr13457590ots.147.1633428689126; Tue, 05 Oct 2021 03:11:29 -0700 (PDT) MIME-Version: 1.0 References: <20211002005238.40280-1-jeremy.linton@arm.com> <20211002005238.40280-6-jeremy.linton@arm.com> In-Reply-To: <20211002005238.40280-6-jeremy.linton@arm.com> From: "Ard Biesheuvel" Date: Tue, 5 Oct 2021 12:11:18 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 5/5] Platform/RaspberryPi: Disconnect/shutdown all drivers before reboot To: Jeremy Linton Cc: edk2-devel-groups-io , Peter Batard , Ard Biesheuvel , Leif Lindholm , Andrei Warkentin , Sunny Wang , Samer El-Haj-Mahmoud Content-Type: text/plain; charset="UTF-8" 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. 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 >