[PATCH 10/21] FADT: expand compliance checks for DSDT and X_DSDT fields
Al Stone
al.stone at linaro.org
Fri Feb 19 21:52:28 UTC 2016
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 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")
> }
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);
>>
>
>
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone at linaro.org
-----------------------------------
More information about the fwts-devel
mailing list