linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jean Delvare <jdelvare@suse.de>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: linux-kernel@vger.kernel.org,
	Mika Westerberg <mika.westerberg@linux.intel.com>
Subject: Re: [PATCH v1 1/2] firmware: dmi_scan: Split out dmi_get_entry_point() helper
Date: Thu, 15 Dec 2016 12:13:54 +0100	[thread overview]
Message-ID: <20161215121354.34352ed5@endymion> (raw)
In-Reply-To: <20161202195416.58953-2-andriy.shevchenko@linux.intel.com>

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 <andriy.shevchenko@linux.intel.com>
> ---
>  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

  reply	other threads:[~2016-12-15 11:14 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-12-02 19:54 [PATCH v1 0/2] firmware: dmi_scan: Make it work in kexec'ed kernel Andy Shevchenko
2016-12-02 19:54 ` [PATCH v1 1/2] firmware: dmi_scan: Split out dmi_get_entry_point() helper Andy Shevchenko
2016-12-15 11:13   ` Jean Delvare [this message]
2016-12-02 19:54 ` [PATCH v1 2/2] firmware: dmi_scan: Pass dmi_entry_point to kexec'ed kernel Andy Shevchenko
2016-12-15 11:28   ` Jean Delvare
2016-12-16  2:32     ` Dave Young
2016-12-16 12:18       ` Andy Shevchenko
2016-12-16 13:33         ` Jean Delvare
2016-12-17 10:57           ` Dave Young
2019-09-06 19:00             ` Andy Shevchenko
2020-01-20 12:19             ` Andy Shevchenko
2020-01-20 16:04               ` Eric W. Biederman
2020-01-20 21:42                 ` Jean Delvare
2020-01-20 21:55                   ` Andy Shevchenko
2020-01-21  9:03                     ` Jean Delvare
2020-01-21 16:29                       ` Eric W. Biederman
2020-01-21 17:24                         ` Andy Shevchenko
2020-01-20 22:31                 ` Andy Shevchenko
2020-01-20 23:18                   ` Ard Biesheuvel
2020-01-21 15:37                     ` Andy Shevchenko
2020-01-21 17:17                       ` Eric W. Biederman
2020-05-21 17:39                         ` Andy Shevchenko
2021-06-02  8:37                     ` Andy Shevchenko
2021-06-02  8:53                       ` Andy Shevchenko
2016-12-17 10:50         ` Dave Young
2020-01-20 12:16 ` [PATCH v1 0/2] firmware: dmi_scan: Make it work in " Andy Shevchenko
2020-05-21 15:53   ` Andy Shevchenko
2020-05-21 15:59     ` Andy Shevchenko
2021-06-02  8:42 ` Andy Shevchenko
2021-06-02  8:53   ` Andy Shevchenko
2021-06-05  7:51     ` Dave Young
2021-06-07 16:22       ` Andy Shevchenko
2021-06-07 17:18         ` Andy Shevchenko
2021-06-08 12:25           ` Dave Young
2021-06-08 12:38             ` Andy Shevchenko
2021-06-09 11:55               ` Dave Young
2021-06-12  4:40                 ` Dave Young
2021-06-14 15:38                   ` Andy Shevchenko
2021-06-14 17:07                     ` Andy Shevchenko
2021-06-14 17:27                       ` Andy Shevchenko
2021-07-19  7:53                         ` Ard Biesheuvel
2021-07-19  8:25                           ` Andy Shevchenko
2021-10-06 16:28                         ` Andy Shevchenko
2021-10-07  7:20                           ` Ard Biesheuvel
2021-10-07  7:23                             ` Andy Shevchenko
2021-10-17 13:31                               ` Ard Biesheuvel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20161215121354.34352ed5@endymion \
    --to=jdelvare@suse.de \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mika.westerberg@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).