Re: [PATCH v1 1/2] firmware: dmi_scan: Split out dmi_get_entry_point() helper

[Jean Delvare]

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@xxxxxxxxxxxxxxx>
> ---
>  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