From mboxrd@z Thu Jan 1 00:00:00 1970 From: jhugo@codeaurora.org (Jeffrey Hugo) Date: Tue, 7 Feb 2017 11:41:29 -0700 Subject: [PATCH] efi/libstub/arm*: Set default address and size cells values for an empty dtb In-Reply-To: <20170207181554.GD26173@leverpostej> References: <1486490390-25251-1-git-send-email-jhugo@codeaurora.org> <20170207181554.GD26173@leverpostej> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 2/7/2017 11:15 AM, Mark Rutland wrote: > Hi, > > On Tue, Feb 07, 2017 at 10:59:50AM -0700, Jeffrey Hugo wrote: >> From: Sameer Goel >> >> In cases where a device tree is not provided (ie ACPI based system), an >> empty fdt is generated by efistub. Sets the address and size cell values >> in a generated fdt to support 64 bit addressing. >> >> This enables kexec/kdump on Qualcomm Technologies QDF24XX platforms as those >> utilities will read the address/size values from the fdt, and such values >> may exceed the range provided by the 32 bit default. > > The description here doesn't state why this is a problem for ACPI. > > What values are being read by the tools, and for what purpose? > > Are they extracting data from the DTB, or is this part of inserting a > new property? > > Why does this adversely affect ACPI? I think the response I am about to send to Ard will address these questions. > >> Change-Id: Ie7f3637e375bd6631c6bda1f7b3c9003765ff4a5 > > Please remove this kind of tags from upstream patches. It's irrelevant. Yep, sorry about that. I should know better. > >> Signed-off-by: Sameer Goel >> Signed-off-by: Jeffrey Hugo >> --- >> drivers/firmware/efi/libstub/fdt.c | 38 +++++++++++++++++++++++++++++++++++++- >> 1 file changed, 37 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c >> index 921dfa0..def5c9c 100644 >> --- a/drivers/firmware/efi/libstub/fdt.c >> +++ b/drivers/firmware/efi/libstub/fdt.c >> @@ -16,6 +16,34 @@ >> >> #include "efistub.h" >> >> +#define EFI_DT_ADDR_CELLS_DEFAULT 2 >> +#define EFI_DT_SIZE_CELLS_DEFAULT 2 >> + >> +static void fdt_update_cell_size(efi_system_table_t *sys_table, void *fdt) >> +{ >> + int offset; >> + int status; >> + >> + offset = fdt_path_offset(fdt, "/"); >> + /* Set the #address-cells and #size-cells values for an empty tree */ >> + >> + status = fdt_setprop_u32(fdt, offset, "#address-cells", >> + EFI_DT_ADDR_CELLS_DEFAULT); >> + if (status) { >> + pr_efi(sys_table, >> + "Failed to set #address-cells for empty dtb\n"); >> + return; >> + } >> + >> + status = fdt_setprop_u32(fdt, offset, "#size-cells", >> + EFI_DT_SIZE_CELLS_DEFAULT); >> + if (status) { >> + pr_efi(sys_table, >> + "Failed to set #size-cells for empty dtb\n"); >> + return; >> + } >> +} > > We don't seem to log anything for most other failures within > update_fdt() where this is called. > > Are these really much more special? > >> static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt, >> unsigned long orig_fdt_size, >> void *fdt, int new_fdt_size, char *cmdline_ptr, >> @@ -44,8 +72,16 @@ static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt, >> >> if (orig_fdt) >> status = fdt_open_into(orig_fdt, fdt, new_fdt_size); >> - else >> + else { > > Nit: if one side of an if-else has braces, the other should too per the > usual kernel coding style. Will fix. > > Thanks, > Mark. > -- Jeffrey Hugo Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.