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

[Alex Hung]

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>


-- 
Cheers,
Alex Hung