[V2][PATCH 2/2] acpi: acpitables: FADT: Ignore fields at offset 46 through 108 for HW_REDUCED_ACPI

[Hanjun Guo]

On 2015年04月07日 18:20, Colin Ian King wrote:
> On 07/04/15 11:12, Hanjun Guo wrote:
>> On 2015年04月07日 13:44, Heyi Guo wrote:
>>> According to ACPI spec 5.1, section 5.2.9, "If the HW_REDUCED_ACPI
>>> flag in the table is set, OSPM will ignore fields related to the ACPI
>>> HW register interface: Fields at offsets 46 through 108 and 148
>>> through 232, as well as FADT Flag bits 1, 2, 3,7,8,12,13, 14, 16 and
>>> 17).", add precondition of checking SMI_CMD, PM_TMR_LEN, etc. As the
>>> code becomes a little complex, split it into small functions for each
>>> important field check.
>>>
>>> Signed-off-by: Heyi Guo <heyi.guo at linaro.org>
>>> ---
>>>    src/acpi/acpitables/acpitables.c | 90
>>> +++++++++++++++++++++++++++++++---------
>>>    1 file changed, 70 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/src/acpi/acpitables/acpitables.c
>>> b/src/acpi/acpitables/acpitables.c
>>> index e5fffff..52b6659 100644
>>> --- a/src/acpi/acpitables/acpitables.c
>>> +++ b/src/acpi/acpitables/acpitables.c
>>> @@ -69,12 +69,10 @@ static void acpi_table_check_hpet(fwts_framework
>>> *fw, fwts_acpi_table_info *tabl
>>>
>>>    }
>>>
>>> -static void acpi_table_check_fadt(fwts_framework *fw,
>>> fwts_acpi_table_info *table)
>>> +static void acpi_table_check_fadt_firmware_control(fwts_framework
>>> *fw, fwts_acpi_table_fadt *fadt)
>>>    {
>>> -    fwts_acpi_table_fadt *fadt = (fwts_acpi_table_fadt*)table->data;
>>> -
>>>        if (fadt->firmware_control == 0) {
>>> -        if (table->length >= 140) {
>>> +        if (fadt->header.length >= 140) {
>>>                if ((fadt->x_firmware_ctrl == 0) &&
>>> !(fwts_acpi_is_reduced_hardware(fadt))) {
>>>                    fwts_failed(fw, LOG_LEVEL_CRITICAL, "FADTFACSZero",
>>> "FADT 32 bit FIRMWARE_CONTROL and 64 bit X_FIRMWARE_CONTROL (FACS
>>> address) are null.");
>>>                    fwts_advice(fw, "The 32 bit FIRMWARE_CTRL or 64 bit
>>> X_FIRMWARE_CTRL should point to a valid "
>>> @@ -88,7 +86,7 @@ static void acpi_table_check_fadt(fwts_framework
>>> *fw, fwts_acpi_table_info *tabl
>>>                        "bug and needs to be fixed.");
>>>            }
>>>        } else {
>>> -        if (table->length >= 140) {
>>> +        if (fadt->header.length >= 140) {
>>>                if (fadt->x_firmware_ctrl != 0) {
>>>                    if (((uint64_t)fadt->firmware_control !=
>>> fadt->x_firmware_ctrl)) {
>>>                        fwts_failed(fw, LOG_LEVEL_MEDIUM,
>>> "FwCtrl32and64Differ",
>>> @@ -103,10 +101,14 @@ static void acpi_table_check_fadt(fwts_framework
>>> *fw, fwts_acpi_table_info *tabl
>>>                }
>>>            }
>>>        }
>>> +}
>>>
>>> +static void acpi_table_check_fadt_dsdt(fwts_framework *fw,
>>> fwts_acpi_table_fadt *fadt)
>>> +{
>>>        if (fadt->dsdt == 0)
>>>            fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTDSTNull", "FADT DSDT
>>> address is null.");
>>> -    if (table->length >= 148) {
>>> +
>>> +    if (fadt->header.length >= 148) {
>>>            if (fadt->x_dsdt == 0) {
>>>                fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTXDSTDNull", "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."
>>> @@ -121,6 +123,16 @@ static void acpi_table_check_fadt(fwts_framework
>>> *fw, fwts_acpi_table_info *tabl
>>>                        "The kernel works around this by using the 64
>>> bit X_DSDT pointer to the DSDT. ");
>>>            }
>>>        }
>>> +}
>>> +
>>> +
>>> +static void acpi_table_check_fadt_smi(fwts_framework *fw,
>>> fwts_acpi_table_fadt *fadt)
>>> +{
>>> +    if (fwts_acpi_is_reduced_hardware(fadt)) {
>>> +        if (fadt->smi_cmd != 0)
>>> +            fwts_warning(fw, "FADT SMI_CMD is not zero but should be
>>> in reduced hardware mode.");
>>> +        return;
>>> +    }
>>>
>>>        /*
>>>         *  Section 5.2.9 (Fixed ACPI Description Table) of the ACPI 5.0
>>> @@ -153,6 +165,15 @@ static void acpi_table_check_fadt(fwts_framework
>>> *fw, fwts_acpi_table_info *tabl
>>>                        "specific functions will not work.");
>>>            }
>>>        }
>>> +}
>>> +
>>> +static void acpi_table_check_fadt_pm_tmr(fwts_framework *fw,
>>> fwts_acpi_table_fadt *fadt)
>>> +{
>>> +    if (fwts_acpi_is_reduced_hardware(fadt)) {
>>> +        if (fadt->pm_tmr_len != 0)
>>> +            fwts_warning(fw, "FADT PM_TMR_LEN is not zero but should
>>> be in reduced hardware mode.");
>>> +        return;
>>> +    }
>>>
>>>        if (fadt->pm_tmr_len != 4) {
>>>            fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTBadPMTMRLEN",
>>> @@ -161,6 +182,18 @@ static void acpi_table_check_fadt(fwts_framework
>>> *fw, fwts_acpi_table_info *tabl
>>>                    "This fields value must be 4. If it is not the
>>> correct size then the kernel "
>>>                    "will not request a region for the pm timer block. ");
>>>        }
>>> +}
>>> +
>>> +static void acpi_table_check_fadt_gpe(fwts_framework *fw,
>>> fwts_acpi_table_fadt *fadt)
>>> +{
>>> +    if (fwts_acpi_is_reduced_hardware(fadt)) {
>>> +        if (fadt->gpe0_blk_len != 0)
>>> +            fwts_warning(fw, "FADT GPE0_BLK_LEN is not zero but
>>> should be in reduced hardware mode.");
>>> +        if (fadt->gpe1_blk_len != 0)
>>> +            fwts_warning(fw, "FADT GPE1_BLK_LEN is not zero but
>>> should be in reduced hardware mode.");
>>
>> I prefer to print a single message:
>>
>> if ((fadt->gpe0_blk_len != 0) || if (fadt->gpe1_blk_len != 0))
>>      fwts_warningfw, "FADT GPE0_BLK_LEN should be zero in reduced
>> hardware mode.");
>>
> I'd prefer two distinct warning messages because the more exact
> information we have the less work an engineer needs to do to check
> exactly what is wrong when they get the message. I prefer clarity over
> bloat.

ah, yes, this is the firmware test suit, it should be specific,
I agree with you now :)

Thanks
Hanjun