From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2173C21A04830 for ; Fri, 31 Mar 2017 04:39:55 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 72422437F7E; Fri, 31 Mar 2017 11:39:54 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 72422437F7E Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 72422437F7E Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-31.phx2.redhat.com [10.3.116.31]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5C6DD1712C; Fri, 31 Mar 2017 11:39:53 +0000 (UTC) To: Ard Biesheuvel , edk2-devel@lists.01.org References: <20170331105607.3477-1-ard.biesheuvel@linaro.org> <20170331105607.3477-4-ard.biesheuvel@linaro.org> Cc: leif.lindholm@linaro.org From: Laszlo Ersek Message-ID: <3bf58c8c-3582-c668-541f-651dea8bdbb1@redhat.com> Date: Fri, 31 Mar 2017 13:39:52 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170331105607.3477-4-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Fri, 31 Mar 2017 11:39:54 +0000 (UTC) Subject: Re: [PATCH v2 3/3] EmbeddedPkg/DtPlatformDxe: load platform DTB via new library X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 31 Mar 2017 11:39:55 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 03/31/17 12:56, Ard Biesheuvel wrote: > To give platforms some room to decide which DTB is suitable and where > to load it from, indirect loading of the DTB image via the new > DtPlatformDtbLoaderLib library class. I think you accidentally the verb in the above sentence :) > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel > --- > EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c | 26 ++++++++++---------- > EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf | 2 +- > EmbeddedPkg/EmbeddedPkg.dsc | 2 ++ > 3 files changed, 16 insertions(+), 14 deletions(-) > > diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c > index 5778633b4985..c75f088a34e5 100644 > --- a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c > +++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c > @@ -15,7 +15,7 @@ > #include > #include > #include > -#include > +#include > #include > #include > #include > @@ -114,14 +114,12 @@ DtPlatformDxeEntryPoint ( > UINTN BufferSize; > VOID *Dtb; > UINTN DtbSize; > - VOID *DtbCopy; > > // > // Check whether a DTB blob is included in the firmware image. > // Please drop this comment; this detail is now abstracted out. > Dtb = NULL; > - Status = GetSectionFromAnyFv (&gDtPlatformDefaultDtbFileGuid, > - EFI_SECTION_RAW, 0, &Dtb, &DtbSize); > + Status = DtPlatformLoadDtb (&Dtb, &DtbSize); > if (EFI_ERROR (Status)) { > DEBUG ((DEBUG_WARN, "%a: no DTB blob found, defaulting to ACPI\n", > __FUNCTION__)); This can now fail due to out-of-memory in DtPlatformLoadDtb(), so I suggest printing Status with %r in the debug message. > @@ -157,7 +155,7 @@ DtPlatformDxeEntryPoint ( > EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS, > sizeof (DtAcpiPref), &DtAcpiPref); > if (EFI_ERROR (Status)) { > - return Status; > + goto FreeDtb; > } > } > > @@ -172,23 +170,18 @@ DtPlatformDxeEntryPoint ( > DEBUG ((DEBUG_ERROR, > "%a: failed to install gEdkiiPlatformHasAcpiGuid as a protocol\n", > __FUNCTION__)); > - return Status; > + goto FreeDtb; > } > } else if (DtAcpiPref.Pref == DT_ACPI_SELECT_DT) { > // > // DT was selected: copy the blob into newly allocated memory and install > // a reference to it as the FDT configuration table. > // > - DtbCopy = AllocateCopyPool (DtbSize, Dtb); > - if (DtbCopy == NULL) { > - return EFI_OUT_OF_RESOURCES; > - } > - Status = gBS->InstallConfigurationTable (&gFdtTableGuid, DtbCopy); > + Status = gBS->InstallConfigurationTable (&gFdtTableGuid, Dtb); > if (EFI_ERROR (Status)) { > DEBUG ((DEBUG_ERROR, "%a: failed to install FDT configuration table\n", > __FUNCTION__)); > - FreePool (DtbCopy); > - return Status; > + goto FreeDtb; > } > } else { > ASSERT (FALSE); > @@ -210,4 +203,11 @@ DtPlatformDxeEntryPoint ( > // installed in that case. > // > return InstallHiiPages (); > + > +FreeDtb: > + if (Dtb != NULL) { > + FreePool (Dtb); > + } > + > + return Status; > } > diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf > index b73877a6086b..45dfd9088bf0 100644 > --- a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf > +++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf > @@ -40,7 +40,7 @@ [Packages] > [LibraryClasses] > BaseLib > DebugLib > - DxeServicesLib > + DtPlatformDtbLoaderLib > HiiLib > MemoryAllocationLib > UefiBootServicesTableLib > diff --git a/EmbeddedPkg/EmbeddedPkg.dsc b/EmbeddedPkg/EmbeddedPkg.dsc > index 0d5db68631bb..b75678276e8d 100644 > --- a/EmbeddedPkg/EmbeddedPkg.dsc > +++ b/EmbeddedPkg/EmbeddedPkg.dsc > @@ -294,5 +294,7 @@ [Components.common] > EmbeddedPkg/Library/PrePiHobLib/PrePiHobLib.inf > EmbeddedPkg/Library/PrePiMemoryAllocationLib/PrePiMemoryAllocationLib.inf > > + EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf > + > [Components.IA32, Components.X64, Components.IPF, Components.ARM] > EmbeddedPkg/GdbStub/GdbStub.inf > Please split off the last hunk (for "EmbeddedPkg/EmbeddedPkg.dsc") to a separate patch. (You might want to make that the first patch in the series, saying "we forgot this before, but it's needed to build the driver at all", or some such.) With these changes, for both patches: Reviewed-by: Laszlo Ersek Thanks Laszlo