ACK: [PATCH] acpi: fadt: sanity check reset register settings (LP: #1230394)
Alex Hung
alex.hung at canonical.com
Wed Oct 2 02:57:08 UTC 2013
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
More information about the fwts-devel
mailing list