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 DA8A7200840D0 for ; Tue, 28 Mar 2017 03:49:05 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4CFEBC611A; Tue, 28 Mar 2017 10:49:05 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 4CFEBC611A Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 4CFEBC611A Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-89.phx2.redhat.com [10.3.116.89]) by smtp.corp.redhat.com (Postfix) with ESMTP id CF0F797A68; Tue, 28 Mar 2017 10:49:03 +0000 (UTC) To: Ard Biesheuvel , edk2-devel@lists.01.org, leif.lindholm@linaro.org, marcin.wojtas@semihalf.com References: <20170327100754.32387-1-ard.biesheuvel@linaro.org> <2420a783-d72d-e9f2-bd92-54e4e4866cc3@redhat.com> Cc: agraf@suse.de From: Laszlo Ersek Message-ID: <383c1aff-9a8a-b8c4-a0c4-c5d272f52c36@redhat.com> Date: Tue, 28 Mar 2017 12:49:02 +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: <2420a783-d72d-e9f2-bd92-54e4e4866cc3@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Tue, 28 Mar 2017 10:49:05 +0000 (UTC) Subject: Re: [PATCH] EmbeddedPkg: add DT platform driver to select between DT and ACPI 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: Tue, 28 Mar 2017 10:49:06 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 03/28/17 12:43, Laszlo Ersek wrote: > Ard, > > On 03/27/17 12:07, Ard Biesheuvel wrote: [snip] >> +/** >> + The entry point for DtPlatformDxe driver. >> + >> + @param[in] ImageHandle The image handle of the driver. >> + @param[in] SystemTable The system table. >> + >> + @retval EFI_ALREADY_STARTED The driver already exists in system. >> + @retval EFI_OUT_OF_RESOURCES Fail to execute entry point due to lack of >> + resources. >> + @retval EFI_SUCCES All the related protocols are installed on >> + the driver. >> + >> +**/ >> +EFI_STATUS >> +EFIAPI >> +DtPlatformDxeEntryPoint ( >> + IN EFI_HANDLE ImageHandle, >> + IN EFI_SYSTEM_TABLE *SystemTable >> + ) >> +{ >> + EFI_STATUS Status; >> + EFI_HANDLE DriverHandle; >> + DT_ACPI_VARSTORE_DATA DtAcpiPref; >> + UINTN BufferSize; >> + VOID *Dtb; >> + UINTN DtbSize; >> + VOID *DtbCopy; >> + >> + // >> + // Check whether a DTB blob is included in the firmware image. >> + // >> + Dtb = NULL; >> + Status = GetSectionFromAnyFv (&gDtPlatformDefaultDtbFileGuid, >> + EFI_SECTION_RAW, 0, &Dtb, &DtbSize); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_WARN, "%a: no DTB blob found, defaulting to ACPI\n", >> + __FUNCTION__)); >> + DtAcpiPref.Pref = DT_ACPI_SELECT_ACPI; >> + } else { >> + // >> + // Get the current ACPI/DT preference from the AcpiDtPref variable. >> + // > > The comment says "AcpiDtPref variable" (which corresponds to the name under which the VFR also refers to the variable), but below the variable services actually get a "DtAcpiPref" argument as variable name. > > Please fix this inconsistency together with the other instances. > >> + BufferSize = sizeof (DtAcpiPref); >> + Status = gRT->GetVariable(L"DtAcpiPref", &gDtPlatformFormSetGuid, NULL, >> + &BufferSize, &DtAcpiPref); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_WARN, "%a: no DT/ACPI preference found, defaulting to DT\n", >> + __FUNCTION__)); >> + DtAcpiPref.Pref = DT_ACPI_SELECT_DT; >> + } >> + } >> + >> + if (!EFI_ERROR (Status) && >> + DtAcpiPref.Pref != DT_ACPI_SELECT_ACPI && >> + DtAcpiPref.Pref != DT_ACPI_SELECT_DT) { >> + DEBUG ((DEBUG_WARN, "%a: illegal value for DtAcpiPref, defaulting to DT\n", >> + __FUNCTION__)); > > Personal request: please replace "illegal" with "invalid". > >> + DtAcpiPref.Pref = DT_ACPI_SELECT_DT; >> + Status = EFI_INVALID_PARAMETER; // trigger setvar below >> + } >> + >> + // >> + // Write the newly selected default value back to the variable store. >> + // >> + if (EFI_ERROR (Status)) { >> + Status = gRT->SetVariable(L"DtAcpiPref", &gDtPlatformFormSetGuid, >> + EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS, >> + sizeof (DtAcpiPref), &DtAcpiPref); > > I suggest to use a macro for the variable name... > >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + } >> + >> + if (DtAcpiPref.Pref == DT_ACPI_SELECT_ACPI) { >> + // >> + // ACPI was selected: install the gEdkiiPlatformHasAcpiGuid GUID as a >> + // NULL protocol to unlock dispatch of ACPI related drivers. >> + // >> + DriverHandle = NULL; >> + Status = gBS->InstallMultipleProtocolInterfaces (&DriverHandle, >> + gEdkiiPlatformHasAcpiGuid, NULL, NULL); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_ERROR, >> + "%a: failed to install gEdkiiPlatformHasAcpiGuid as a protocol\n", >> + __FUNCTION__)); >> + return Status; >> + } > > I think you don't need to create a new controller handle for gEdkiiPlatformHasAcpiGuid; using ImageHandle should be fine. > > (Same for gEfiDevicePathProtocolGuid in InstallHiiPages().) > > Not critical though, just simpler perhaps. > >> + } 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); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_ERROR, "%a: failed to install FDT configuration table\n", >> + __FUNCTION__)); >> + FreePool (DtbCopy); >> + return Status; >> + } > > Looks good. The original DTB (found in the FV section) lives in flash, generally speaking, right? > >> + } else { >> + ASSERT (FALSE); >> + } >> + >> + // >> + // No point in installing the HII pages if ACPI is the only description >> + // we have >> + // >> + if (Dtb == NULL) { >> + return EFI_SUCCESS; >> + } >> + >> + return InstallHiiPages (); >> +} Consider uninstalling the FDT config table / the "has ACPI" protocol if InstallHiiPages() fails and the driver exits with error (it gets unloaded)? My inner pedant wants you to do this, although I do realize that neither the FDT config table nor the NULL "has ACPI" protocol hook back into this driver. Thanks Laszlo