ACK: [PATCH 1/2] Remove length test for ACPI 1.0 RSDPs, fix checks against revision field

[ivanhu]

On 11/14/2017 12:47 PM, Alex Hung wrote:
> From: Chris Goldsworthy <goldswo at amazon.com>
> 
> Remove RSDP length test for ACPI 1.0 RSDPs.  Fix RSDP revision field checks to
> check against revision values 0 and 2 only.
> 
> The RSDP length field test for ACPI 1.0 RSDPs is invalid, since a ACPI 1.0 RSDP
> has no length field.  FWTS also checks the RSDP revision number incorrectly -
> the revision field of a ACPI 1.0 RSDP has a value of 0, not 1 (as per ACPI
> specification version 6.2, page 122).
> 
> Add descriptions for specific RSDP tests.
> 
> Fix a typo in a RSDT error message.
> 
> Signed-off-by: Christopher Goldsworthy <goldswo at amazon.de>
> ---
>   src/acpi/rsdp/rsdp.c | 27 ++++++++++++---------------
>   src/acpi/rsdt/rsdt.c |  2 +-
>   2 files changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/src/acpi/rsdp/rsdp.c b/src/acpi/rsdp/rsdp.c
> index becdbb2..2f537be 100644
> --- a/src/acpi/rsdp/rsdp.c
> +++ b/src/acpi/rsdp/rsdp.c
> @@ -80,6 +80,7 @@ static int rsdp_test1(fwts_framework *fw)
>   			    "RSDPBadFirstChecksum",
>   			    "RSDP first checksum is incorrect: 0x%x", checksum);
>   
> +	/* ensure oem_id only contains printable characters */
>   	for (i = 0; i < 6; i++) {
>   		if (!isprint(rsdp->oem_id[i])) {
>   			passed = false;
> @@ -102,27 +103,23 @@ static int rsdp_test1(fwts_framework *fw)
>   			"firmware ACPI complaint.");
>   	}
>   
> -	/* ACPI 6.1 errata clarifies revision 1 must have length 20 */
> -	if (rsdp->revision == 1) {
> -		if (rsdp->length == 20)
> -			fwts_passed(fw, "RSDP: the table is the correct length.");
> -		else
> -			fwts_failed(fw, LOG_LEVEL_MEDIUM,
> -				"RSDPBadLength",
> -				"RSDP: length of Revision 1 is %d bytes but should be 20.",
> -				rsdp->length);
> -
> -		return FWTS_OK;
> -	}
> -
> -	if (rsdp->revision <= 2)
> +	/* check if revision number is valid. note: revision field for
> +	 * ACPI 1.0 RSDP is 0, not 1, as per ACPI specification version
> +	 * 6.2, p.122. */
> +	if (rsdp->revision == 0 || rsdp->revision == 2)
>   		fwts_passed(fw,
>   			    "RSDP: revision is %" PRIu8 ".", rsdp->revision);
>   	else
>   		fwts_failed(fw, LOG_LEVEL_MEDIUM,
>   			"RSDPBadRevisionId",
>   			"RSDP: revision is %" PRIu8 ", expected "
> -			"value less than 2.", rsdp->revision);
> +			"value to be 0 or 2.", rsdp->revision);
> +
> +	/* all proceeding tests involve fields which are only
> +	 * defined in ACPI specifications 2.0 and greater, skip
> +	 * if ACPI version is 1.0 */
> +	if (rsdp->revision == 0)
> +		return FWTS_OK;
>   
>   	/* whether RSDT or XSDT depends arch */
>   	if (rsdp->rsdt_address == 0 && rsdp->xsdt_address == 0)
> diff --git a/src/acpi/rsdt/rsdt.c b/src/acpi/rsdt/rsdt.c
> index bb28e20..3374b8b 100644
> --- a/src/acpi/rsdt/rsdt.c
> +++ b/src/acpi/rsdt/rsdt.c
> @@ -60,7 +60,7 @@ static int rsdt_test1(fwts_framework *fw)
>   				"RSDTEntryNull",
>   				"RSDT Entry %zd is null, should not be non-zero.", i);
>   			fwts_advice(fw,
> -				"A XSDT pointer is null and therefore erroneously "
> +				"A RSDT pointer is null and therefore erroneously "
>   				"points to an invalid 32 bit ACPI table header. "
>   				"At worse this will cause the kernel to oops, at "
>   				"best the kernel may ignore this.  However, it "
> 

Acked-by: Ivan Hu <ivan.hu at canonical.com>