[PATCH v2 08/23] FADT: minor cleanup and initial compliance tests

[Al Stone]

On 02/23/2016 12:32 AM, ivanhu wrote:
> 
> 
> On 2016年02月20日 07:39, Al Stone wrote:
>> The primary purpose of this patch is to catch some very minor white
>> space edits while adding in two very simple compliance tests -- is the
>> table checksum correct, and is the revision number current?
>>
>> Signed-off-by: Al Stone <al.stone at linaro.org>
>> ---
>>   src/acpi/fadt/fadt.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++----
>>   1 file changed, 56 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c
>> index 7ca351d..afe52f3 100644
>> --- a/src/acpi/fadt/fadt.c
>> +++ b/src/acpi/fadt/fadt.c
>> @@ -20,8 +20,8 @@
>>    */
>>   #include "fwts.h"
>>   -#include <stdlib.h>
>>   #include <stdio.h>
>> +#include <stdlib.h>
>>   #include <sys/types.h>
>>   #include <sys/stat.h>
>>   #ifdef FWTS_ARCH_INTEL
>> @@ -53,7 +53,7 @@ static int fadt_init(fwts_framework *fw)
>>           fwts_log_error(fw, "ACPI table FACP does not exist!");
>>           return FWTS_ERROR;
>>       }
>> -    fadt = (const fwts_acpi_table_fadt*)table->data;
>> +    fadt = (const fwts_acpi_table_fadt *)table->data;
>>       fadt_size = table->length;
>>         /*  Not having a FADT is not a failure on x86 */
>> @@ -154,6 +154,56 @@ static int fadt_info(fwts_framework *fw)
>>       return FWTS_OK;
>>   }
>>   +static int fadt_checksum(fwts_framework *fw)
>> +{
>> +    const uint8_t *data = (const uint8_t *)fadt;
>> +    ssize_t length = fadt->header.length;
>> +    uint8_t checksum = 0;
>> +
>> +    /* verify the table checksum */
>> +    checksum = fwts_checksum(data, length);
>> +    if (checksum == 0)
>> +        fwts_passed(fw, "FADT checksum is correct");
>> +    else
>> +        fwts_failed(fw, LOG_LEVEL_MEDIUM,
>> +                "SPECMADTChecksum",
>> +                "FADT checksum is incorrect: 0x%x", checksum);
>> +
>> +    return FWTS_OK;
>> +}
>> +
>> +static int fadt_revision(fwts_framework *fw)
>> +{
>> +    const uint8_t LATEST_MAJOR = 6;
>> +    const uint8_t LATEST_MINOR = 1;
>> +    uint8_t major;
>> +    uint8_t minor;
>> +
>> +    major = fadt->header.revision;
>> +    minor = 0;
>> +    if (major >= 5 && fadt->header.length >= 268)
>> +        minor = fadt->minor_version;   /* field added ACPI 5.1 */
>> +
>> +    fwts_log_info(fw, "FADT revision: %d.%d", major, minor);
>> +    fwts_log_info(fw, "FADT table length: %d", fadt->header.length);
>> +
>> +    if (major == LATEST_MAJOR && minor == LATEST_MINOR)
>> +        fwts_passed(fw, "FADT revision is up to date.");
>> +    else {
>> +        fwts_warning(fw, "FADT revision is outdated: %d.%d",
>> +                 major, minor);
>> +        fwts_advice(fw, "The most recent revision of the FADT "
>> +                "defined in the ACPI specification is %d.%d.  "
>> +                "While older revisions of the FADT can be used, "
>> +                "newer ones may enable additional functionality "
>> +                "that cannot be used until the FADT is updated.",
>> +                LATEST_MAJOR, LATEST_MINOR);
>> +    }
>> +
>> +    return FWTS_OK;
>> +}
>> +
>> +
>>   static void acpi_table_check_fadt_firmware_control(
>>       fwts_framework *fw,
>>       const fwts_acpi_table_fadt *fadt,
>> @@ -637,8 +687,10 @@ static int fadt_test3(fwts_framework *fw)
>>   }
>>     static fwts_framework_minor_test fadt_tests[] = {
>> -    { fadt_info, "FADT ACPI Description Table flag info." },
>> -    { fadt_test1, "Test FADT ACPI Description Table tests." },
>> +    { fadt_info, "ACPI FADT Description Table flag info." },
>> +    { fadt_checksum, "FADT checksum test." },
>> +    { fadt_revision, "FADT revision test." },
>> +    { fadt_test1, "ACPI FADT Description Table tests." },
>>       { fadt_test2, "Test FADT SCI_EN bit is enabled." },
>>       { fadt_test3, "Test FADT reset register." },
>>       { NULL, NULL }
> Acked-by: Ivan Hu <ivan.hu at canonical.com>
> 
> I give this an ack, but I think this cause hard to the maintain the code, we
> need to modify it every time we got a new ACPI spec released.
> For future, I think we might need to collect all the version info to a single 
> header file and just sync up the single header file.
> 
> 
> 

Yeah, I agree, Ivan.  I'll think about doing this in a more general
manner, but we will still have changes when the spec changes -- the
goal would be to minimize it as much as possible.

Thanks for the ACK!

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone at linaro.org
-----------------------------------