ACK: [PATCH 10/21] FADT: expand compliance checks for DSDT and X_DSDT fields

[Colin Ian King]

On 09/02/16 01:32, Al Stone wrote:
> Expand the testing of the DSDT address -- and by extension, the
> X_DSDT address field -- to check for full compliance with the
> spec.  Only one or the other may be used at any one time, per 6.1,
> but we also have to acknowledge there are tables that do use both
> the 32- and 64-bit values.  At that point, we re-use parts of the
> existing test to verify that they are at least consistent.
> 
> Signed-off-by: Al Stone <al.stone at linaro.org>
> ---
>  src/acpi/fadt/fadt.c | 99 +++++++++++++++++++++++++++++-----------------------
>  1 file changed, 56 insertions(+), 43 deletions(-)
> 
> diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c
> index 84f4e09..b21653d 100644
> --- a/src/acpi/fadt/fadt.c
> +++ b/src/acpi/fadt/fadt.c
> @@ -295,54 +295,67 @@ static void acpi_table_check_fadt_firmware_ctrl(fwts_framework *fw)
>  	}
>  }
>  
> -static void acpi_table_check_fadt_dsdt(
> -	fwts_framework *fw,
> -	const fwts_acpi_table_fadt *fadt,
> -	bool *passed)
> +static void acpi_table_check_fadt_dsdt(fwts_framework *fw)
>  {
> -
> +	/* check out older FADTs */
>  	if (fadt->header.length < 148) {
> -		if (fadt->dsdt == 0) {
> -			*passed = false;
> +		if (fadt->dsdt == 0)
>  			fwts_failed(fw, LOG_LEVEL_MEDIUM,
>  				"FADTDSDTNull",
>  				"FADT DSDT address is null.");
> -		}
> -	} else {
> -		if (fadt->x_dsdt == 0) {
> -			if (fadt->dsdt == 0) {
> -				*passed = false;
> -				fwts_failed(fw, LOG_LEVEL_MEDIUM,
> -					"FADTXDSDTNull",
> -					"FADT X_DSDT and DSDT address are null.");
> -			} else {
> -				*passed = false;
> -				fwts_failed(fw, LOG_LEVEL_MEDIUM,
> -					"FADTXDSDTNull",
> -					"FADT X_DSDT address is null.");
> -				fwts_advice(fw,
> -					"An ACPI 2.0 FADT is being used however "
> -					"the 64 bit X_DSDT is null."
> -					"The kernel will fall back to using "
> -					"the 32 bit DSDT pointer instead.");
> -			}
> -		} else if ((uint64_t)fadt->dsdt != fadt->x_dsdt && fadt->dsdt != 0) {
> -			*passed = false;
> +	}
> +
> +	/* if one field is being used, the other must be null */
> +	if ((fadt->dsdt != 0 && fadt->x_dsdt == 0) ||
> +	    (fadt->dsdt == 0 && fadt->x_dsdt != 0))
> +		fwts_passed(fw,
> +			    "FADT has only one of X_DSDT or DSDT addresses "
> +			    "being used.");
> +	else {
> +		if (fadt->dsdt == 0 && fadt->x_dsdt == 0)
>  			fwts_failed(fw, LOG_LEVEL_MEDIUM,
> -				"FADT32And64Mismatch",
> -				"FADT 32 bit DSDT (0x%" PRIx32 ") "
> -				"does not point to same "
> -				"physical address as 64 bit X_DSDT "
> -				"(0x%" PRIx64 ").",
> -				fadt->dsdt, fadt->x_dsdt);
> -			fwts_advice(fw,
> -				"One would expect the 32 bit DSDT and "
> -				"64 bit X_DSDT pointers to point to the "
> -				"same DSDT, however they don't which is "
> -				"clearly ambiguous and wrong. "
> -				"The kernel works around this by using the "
> -				"64 bit X_DSDT pointer to the DSDT. ");
> -		}
> +				    "FADTOneDSDTNull",
> +				    "FADT X_DSDT and DSDT addresses cannot "
> +				    "both be null.");
> +		else
> +			fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +				    "FADTBothDSDTSet",
> +				    "FADT X_DSDT and DSDT addresses cannot "
> +				    "both be set to a value.");
> +	}
> +
> +	/* unexpected use of addresses */
> +	if (fadt->dsdt != 0 && fadt->x_dsdt == 0) {
> +		fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +			    "FADTXDSDTNull",
> +			    "FADT X_DSDT address is null.");
> +		fwts_advice(fw,
> +			    "An ACPI 2.0 or newer FADT is being used however "
> +			    "the 64 bit X_DSDT is null."
> +			    "The kernel will fall back to using "
> +			    "the 32 bit DSDT pointer instead.");
> +	}
> +
> +	/*
> +	 * If you are going to insist on using both fields, even though
> +	 * that is incorrect, at least make it unambiguous as to which
> +	 * address is the one to use.
> +	 */
> +	if ((uint64_t)fadt->dsdt != fadt->x_dsdt && fadt->dsdt != 0) {
> +		fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +			    "FADT32And64Mismatch",
> +			    "FADT 32 bit DSDT (0x%" PRIx32 ") "
> +			    "does not point to same "
> +			    "physical address as 64 bit X_DSDT "
> +			    "(0x%" PRIx64 ").",
> +			    fadt->dsdt, fadt->x_dsdt);
> +		fwts_advice(fw,
> +			    "One would expect the 32 bit DSDT and "
> +			    "64 bit X_DSDT pointers to point to the "
> +			    "same DSDT, however they don't which is "
> +			    "clearly ambiguous and wrong. "
> +			    "The kernel works around this by using the "
> +			    "64 bit X_DSDT pointer to the DSDT. ");
>  	}
>  }
>  
> @@ -531,7 +544,7 @@ static int fadt_test1(fwts_framework *fw)
>  	bool passed = true;
>  
>  	acpi_table_check_fadt_firmware_ctrl(fw);
> -	acpi_table_check_fadt_dsdt(fw, fadt, &passed);
> +	acpi_table_check_fadt_dsdt(fw);
>  	acpi_table_check_fadt_smi(fw, fadt, &passed);
>  	acpi_table_check_fadt_pm_tmr(fw, fadt, &passed);
>  	acpi_table_check_fadt_gpe(fw, fadt, &passed);
> 
Acked-by: Colin Ian King <colin.king at canonucal.com>