diff mbox

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

Message ID 1454981583-31872-11-git-send-email-al.stone@linaro.org
State Rejected
Headers show

Commit Message

Al Stone Feb. 9, 2016, 1:32 a.m. UTC
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@linaro.org>
---
 src/acpi/fadt/fadt.c | 99 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 56 insertions(+), 43 deletions(-)

Comments

Colin Ian King Feb. 9, 2016, 12:02 p.m. UTC | #1
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@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@canonucal.com>
Alex Hung Feb. 17, 2016, 6:06 a.m. UTC | #2
On 2016-02-09 09:32 AM, 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@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.");

I seems to me that somewhere below treats (fadt->dsdt == fadt->x_dsdt) 
&& (fadt->dsdt != 0) as a passing and it is the case for ACPI 6.0 and 
before. However, an error is already generated here.

Will it be better if it is moved here as below:

	/*
	 * 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.
	 */
	else 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. ");


> +		else
> +			fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +				    "FADTBothDSDTSet",
> +				    "FADT X_DSDT and DSDT addresses cannot "
> +				    "both be set to a value.");

Alternatively, if it is really needed to force x_dsdt must be used, how 
about adding an advice saying it may be safe to ignore, such as

		else {
			fwts_failed(fw, LOG_LEVEL_MEDIUM,
			    "FADTBothDSDTSet",
			    "FADT X_DSDT and DSDT addresses cannot "
			    "both be set to a value.");
                        if (fadt->dsdt == fadt->x_dsdt)
                            fwts_advice(fw, "if ACPI 6.0 or before was 
targeted, it is safe to ignore this error")
}

> +	}
> +
> +	/* 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);
>
Al Stone Feb. 19, 2016, 9:52 p.m. UTC | #3
On 02/16/2016 11:06 PM, Alex Hung wrote:
> On 2016-02-09 09:32 AM, 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@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.");
> 
> I seems to me that somewhere below treats (fadt->dsdt == fadt->x_dsdt) &&
> (fadt->dsdt != 0) as a passing and it is the case for ACPI 6.0 and before.
> However, an error is already generated here.
> 
> Will it be better if it is moved here as below:
> 
>     /*
>      * 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.
>      */
>     else 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. ");
> 
> 
>> +        else
>> +            fwts_failed(fw, LOG_LEVEL_MEDIUM,
>> +                    "FADTBothDSDTSet",
>> +                    "FADT X_DSDT and DSDT addresses cannot "
>> +                    "both be set to a value.");
> 
> Alternatively, if it is really needed to force x_dsdt must be used, how about
> adding an advice saying it may be safe to ignore, such as
> 
>         else {
>             fwts_failed(fw, LOG_LEVEL_MEDIUM,
>                 "FADTBothDSDTSet",
>                 "FADT X_DSDT and DSDT addresses cannot "
>                 "both be set to a value.");
>                        if (fadt->dsdt == fadt->x_dsdt)
>                            fwts_advice(fw, "if ACPI 6.0 or before was targeted,
> it is safe to ignore this error")
> }

So, I've done two things that will show up in the next patch series: (1) I've
simplified the checking here and relaxed the test a bit as suggested by adding
in more advice, because -- as you point out -- it is reasonable to ignore this
situation in some cases; (2) a separate new patch later in the series will make
sure that all of the tests on fields work properly on older FADT tables -- this
one and several others do not, now that I look at them with different eyes.

Hopefully that will make more sense once I post v2 of the series...

>> +    }
>> +
>> +    /* 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);
>>
> 
>
diff mbox

Patch

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);