From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935537AbcLOLOA (ORCPT ); Thu, 15 Dec 2016 06:14:00 -0500 Received: from mx2.suse.de ([195.135.220.15]:42090 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752837AbcLOLN7 (ORCPT ); Thu, 15 Dec 2016 06:13:59 -0500 Date: Thu, 15 Dec 2016 12:13:54 +0100 From: Jean Delvare To: Andy Shevchenko Cc: linux-kernel@vger.kernel.org, Mika Westerberg Subject: Re: [PATCH v1 1/2] firmware: dmi_scan: Split out dmi_get_entry_point() helper Message-ID: <20161215121354.34352ed5@endymion> In-Reply-To: <20161202195416.58953-2-andriy.shevchenko@linux.intel.com> References: <20161202195416.58953-1-andriy.shevchenko@linux.intel.com> <20161202195416.58953-2-andriy.shevchenko@linux.intel.com> Organization: SUSE Linux X-Mailer: Claws Mail 3.12.0 (GTK+ 2.24.31; x86_64-suse-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Andy, On Fri, 2 Dec 2016 21:54:15 +0200, Andy Shevchenko wrote: > This is preparatory patch to pass DMI entry point to kexec'ed kernel. You are doing more than that. You are actually changing the logic. There is this comment in the code: * [...] If we * have the 64-bit entry point, but fail to decode it, fall * back to the legacy one (if available) The original code was doing that, but your code does not. You will select one entry point, try to decode it, and if it fails, there is no fallback. dmi_present(buf) is not realistically going to succeed in decoding an SMBIOS 3 entry point, just like dmi_smbios3_present(buf) has zero chance of success on an SMBIOS 2 entry point. This is merely wasting CPU cycles without actually providing a fallback solution. It can be discussed whether this fallback mechanism is mandatory to keep, but dropping it silently as part of refactoring and leaving the comment in is not OK. > Signed-off-by: Andy Shevchenko > --- > drivers/firmware/dmi_scan.c | 37 +++++++++++++++++-------------------- > 1 file changed, 17 insertions(+), 20 deletions(-) > > diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c > index 88bebe1..b88def6 100644 > --- a/drivers/firmware/dmi_scan.c > +++ b/drivers/firmware/dmi_scan.c > @@ -595,11 +595,8 @@ static int __init dmi_smbios3_present(const u8 *buf) > return 1; > } > > -void __init dmi_scan_machine(void) > +static resource_size_t __init dmi_get_entry_point(void) I would like this function to have "efi" in its name, as it is EFI-specific. > { > - char __iomem *p, *q; > - char buf[32]; > - > if (efi_enabled(EFI_CONFIG_TABLES)) { > /* > * According to the DMTF SMBIOS reference spec v3.0.0, it is > @@ -614,32 +611,32 @@ void __init dmi_scan_machine(void) > * have the 64-bit entry point, but fail to decode it, fall > * back to the legacy one (if available) > */ > - if (efi.smbios3 != EFI_INVALID_TABLE_ADDR) { > - p = dmi_early_remap(efi.smbios3, 32); > - if (p == NULL) > - goto error; > - memcpy_fromio(buf, p, 32); > - dmi_early_unmap(p, 32); > - > - if (!dmi_smbios3_present(buf)) { > - dmi_available = 1; > - goto out; > - } > - } > - if (efi.smbios == EFI_INVALID_TABLE_ADDR) > - goto error; > + if (efi.smbios3 != EFI_INVALID_TABLE_ADDR) > + return efi.smbios3; > + if (efi.smbios != EFI_INVALID_TABLE_ADDR) > + return efi.smbios; > + } > + return 0; > +} > + > +void __init dmi_scan_machine(void) > +{ > + resource_size_t ep = dmi_get_entry_point(); Likewise, this variable should have "efi" in its name. > + char __iomem *p, *q; > + char buf[32]; > > + if (ep) { > /* This is called as a core_initcall() because it isn't > * needed during early boot. This also means we can > * iounmap the space when we're done with it. > */ > - p = dmi_early_remap(efi.smbios, 32); > + p = dmi_early_remap(ep, 32); > if (p == NULL) > goto error; > memcpy_fromio(buf, p, 32); > dmi_early_unmap(p, 32); > > - if (!dmi_present(buf)) { > + if (!dmi_smbios3_present(buf) || !dmi_present(buf)) { > dmi_available = 1; > goto out; > } -- Jean Delvare SUSE L3 Support