[PATCH 10/21] FADT: expand compliance checks for DSDT and X_DSDT fields
Alex Hung
alex.hung at canonical.com
Wed Feb 17 06:06:25 UTC 2016
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 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.");
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);
>
--
Cheers,
Alex Hung
More information about the fwts-devel
mailing list