ACK: [PATCH] acpi: fadt: sanity check reset register settings (LP: #1230394)

[Alex Hung]

On 10/02/2013 10:54 AM, Alex Hung wrote:
> On 09/26/2013 12:30 AM, Colin King wrote:
>> From: Colin Ian King <colin.king at canonical.com>
>>
>> Probably a good idea to test for ACPI compliance on the reset register.
>> This extra test checks that the reset register width is 8 bits and the
>> offset is zero, as per the requirement of section 5.2.9 of the ACPI 5.0
>> specification.
>>
>> The patch also shuffles up the fadt_size check to the initialization
>> section.
>>
>> Signed-off-by: Colin Ian King <colin.king at canonical.com>
>> ---
>>   src/acpi/fadt/fadt.c | 55
>> ++++++++++++++++++++++++++++++++++++++++++++++------
>>   1 file changed, 49 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c
>> index d18ca4e..1c15f46 100644
>> --- a/src/acpi/fadt/fadt.c
>> +++ b/src/acpi/fadt/fadt.c
>> @@ -31,6 +31,8 @@
>>   #include <inttypes.h>
>>   #include <string.h>
>>
>> +#define FADT_RESET_SUPPORTED        (1 << 10)
>> +
>>   static const fwts_acpi_table_fadt *fadt;
>>   static int fadt_size;
>>
>> @@ -49,6 +51,12 @@ static int fadt_init(fwts_framework *fw)
>>       fadt = (const fwts_acpi_table_fadt*)table->data;
>>       fadt_size = table->length;
>>
>> +    /*  Not having a FADT is not a failure */
>> +    if (fadt_size == 0) {
>> +        fwts_log_info(fw, "FADT does not exist, this is not
>> necessarily a failure, skipping tests.");
>> +        return FWTS_SKIP;
>> +    }
>> +
>>       return FWTS_OK;
>>   }
>>
>> @@ -57,12 +65,6 @@ static int fadt_test1(fwts_framework *fw)
>>       uint32_t port, width, val32;
>>       int ret = FWTS_OK;
>>
>> -    /*  Not having a FADT is not a failure */
>> -    if (fadt_size == 0) {
>> -        fwts_log_info(fw, "FADT does not exist, this is not
>> necessarily a failure.");
>> -        return FWTS_OK;
>> -    }
>> -
>>       fwts_log_info(fw, "FADT Preferred PM Profile: %hhu (%s)\n",
>>           fadt->preferred_pm_profile,
>>
>> FWTS_ACPI_FADT_PREFERRED_PM_PROFILE(fadt->preferred_pm_profile));
>> @@ -148,8 +150,49 @@ static int fadt_test1(fwts_framework *fw)
>>       return FWTS_OK;
>>   }
>>
>> +static int fadt_test2(fwts_framework *fw)
>> +{
>> +    if (fadt->header.length != 244) {
>> +        fwts_skipped(fw, "Header size indicates an ACPI 1.0 FADT,
>> skipping test.");
>> +        return FWTS_SKIP;
>> +    }
>> +    /*
>> +      * Sanity check Reset reg, c.f. ACPICA
>> +     * commit 57bdfbee675cd2c0618c66882d68a6bdf9f8efc4
>> +     */
>> +    if (fadt->flags & FADT_RESET_SUPPORTED) {
>> +        if (fadt->reset_reg.register_bit_width != 8) {
>> +            fwts_failed(fw, LOG_LEVEL_HIGH, "FadtResetRegWidth",
>> +                "FADT reset register is not 8 bits wide, got instead:
>> %" PRIi8 ".",
>> +                fadt->reset_reg.register_bit_width);
>> +            fwts_advice(fw,
>> +                "Section 5.2.9 Fixed ACPI Description Table (Table
>> 5-34) of the ACPI "
>> +                "specification states that the FADT register width
>> should "
>> +                "be 8 bits wide. However, recent versions of ACPICA
>> will ignore this "
>> +                "and default to 8 bits.");
>> +        } else
>> +            fwts_passed(fw, "FADT reset register width is 8 bits wide
>> as expected.");
>> +
>> +        if (fadt->reset_reg.register_bit_offset != 0) {
>> +            fwts_failed(fw, LOG_LEVEL_HIGH, "FadtResetRegBitOffset",
>> +                "FADT reset register bit offset is not 0, got
>> instead: %" PRIi8 ".",
>> +                fadt->reset_reg.register_bit_offset);
>> +            fwts_advice(fw,
>> +                "Section 5.2.9 Fixed ACPI Description Table (Table
>> 5-34) of the ACPI "
>> +                "specification states that the FADT register bit
>> offset should be 0.");
>> +        } else
>> +            fwts_passed(fw, "FADT register bit offset is 0 as
>> expected.");
>> +    } else {
>> +        fwts_skipped(fw, "FADT flags indicates reset register not
>> supported, skipping test.");
>> +        return FWTS_SKIP;
>> +    }
>> +
>> +    return FWTS_OK;
>> +}
>> +
>>   static fwts_framework_minor_test fadt_tests[] = {
>>       { fadt_test1, "Check FADT SCI_EN bit is enabled." },
>> +    { fadt_test2, "Check FADT reset register." },
>>       { NULL, NULL }
>>   };
>>
>>
>
> New acpica release (20130927) has a change to remove the use of width
> value as below:
> =======================================================
> Hardcoded the access width for the FADT-defined reset register. The ACPI
> specification requires the reset register width to be 8 bits. ACPICA now
> hardcodes the width to 8 and ignores the FADT width value. This provides
> compatibility with other ACPI implementations that have allowed BIOS
> code with bad register width values to go unnoticed.
> =======================================================
>
> but it is always a good idea to check the firmware's santiy. We may be
> able to lower the severity after acpica is updated to upstream kernel
> and most distros include it.
>
> Acked-by: Alex Hung <alex.hung at canonical.com>
>
>

Oops, I intended to ack v2 but accidently acked this one. Please ignore.


-- 
Cheers,
Alex Hung