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.web10.3192.1633469717713236709 for ; Tue, 05 Oct 2021 14:35:17 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=PRtrp/qL; spf=pass (domain: kernel.org, ip: 198.145.29.99, mailfrom: ardb@kernel.org) Received: by mail.kernel.org (Postfix) with ESMTPSA id C81BA615E4 for ; Tue, 5 Oct 2021 21:35:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1633469716; bh=D0on/5q5kRLxwFc5bgSKXsbkUQ42ZfS0ibOIW/Qf6sk=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=PRtrp/qLlsRAkZm0rmYKtDiDE9knm21DhGXwYUPtoIWkosB8U16ftKainaKrtyO8W HtTU/bLa4mywA1sKWN1QCaBzb/nFPVUvndBe505jRXRC+TRhZ64Jda0dSPJL5IZ8EN /0RypSWowRVqbU0pv4z4GAuHezOOmI+YC87DDO4f+QtG+s+EhGVEdLERqRphlOkmeq uz21ETfi227TR4uyUkNVx4gDd48yVWl4JZch/bbnNIejXDMdN1BNggixegeTgYXria vLTysJe1ES+DJHVZnAvoGff9VOr0JZcA8v1l7+xfw8Ud28AXXrg+KrfGAmMLVngteI P/6VskfAeI6jQ== Received: by mail-oi1-f182.google.com with SMTP id a3so1164383oid.6 for ; Tue, 05 Oct 2021 14:35:16 -0700 (PDT) X-Gm-Message-State: AOAM5302maryhkIudoEVqf8+RL4IfwGeCdtasD15cr73qSJ8WEF0VgSQ XY2UL3T1dU7Mk2j7I2jqC0e8kyUbytAfJW2mRL0= X-Google-Smtp-Source: ABdhPJxSkQgSiiTay59jzPESmBsZ2q24KkkPOImg3Wl08QOQ2dWTvOX9Go8XzfVA4+Kg71KfZa/uc/CqJQoNbFwZVw8= X-Received: by 2002:aca:32c2:: with SMTP id y185mr4457819oiy.47.1633469716119; Tue, 05 Oct 2021 14:35:16 -0700 (PDT) MIME-Version: 1.0 References: <20211002005238.40280-1-jeremy.linton@arm.com> <20211002005238.40280-6-jeremy.linton@arm.com> <452e1938-3d6d-97f0-9922-35a7d573c621@arm.com> In-Reply-To: <452e1938-3d6d-97f0-9922-35a7d573c621@arm.com> From: "Ard Biesheuvel" Date: Tue, 5 Oct 2021 23:35:04 +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 Tue, 5 Oct 2021 at 23:25, Jeremy Linton wrote: > > 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. > > Works for me. > > > >> MicroSecondDelay (Delay); > >> } > >> } > >> -- > >> 2.13.7 > >> >