ACK + COMMENT : [PATCH 2/2] acpi: sbbr: refactor by fwts_get_fadt_version

[Colin Ian King]

On 13/08/2021 22:35, Alex Hung wrote:
> Signed-off-by: Alex Hung <alex.hung at canonical.com>
> ---
>  src/acpi/acpiinfo/acpiinfo.c | 7 +------
>  src/acpi/fadt/fadt.c         | 5 +----
>  src/acpi/madt/madt.c         | 4 +---
>  src/sbbr/fadt/fadt.c         | 3 +--
>  4 files changed, 4 insertions(+), 15 deletions(-)
> 
> diff --git a/src/acpi/acpiinfo/acpiinfo.c b/src/acpi/acpiinfo/acpiinfo.c
> index 8f521605..b5f32242 100644
> --- a/src/acpi/acpiinfo/acpiinfo.c
> +++ b/src/acpi/acpiinfo/acpiinfo.c
> @@ -111,7 +111,6 @@ static int acpiinfo_test1(fwts_framework *fw)
>  static int acpiinfo_test2(fwts_framework *fw)
>  {
>  	fwts_acpi_table_info *table;
> -	fwts_acpi_table_fadt *fadt;
>  	uint8_t major;
>  	uint8_t minor = 0;
>  
> @@ -121,11 +120,7 @@ static int acpiinfo_test2(fwts_framework *fw)
>  	if (table == NULL || table->data == NULL)
>  		return FWTS_ERROR;
>  
> -	fadt = (fwts_acpi_table_fadt *)table->data;
> -
> -	major = fadt->header.revision;
> -	if (major >= 5 && fadt->header.length >= 268)
> -		minor = fadt->minor_version;
> +	fwts_get_fadt_version(fw, &major, &minor);
>  
>  	fwts_log_info(fw,
>  		"FACP ACPI Version: %" PRIu8 ".%" PRIu8, major, minor);
> diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c
> index fc8b5987..d72345b7 100644
> --- a/src/acpi/fadt/fadt.c
> +++ b/src/acpi/fadt/fadt.c
> @@ -201,10 +201,7 @@ static int fadt_revision(fwts_framework *fw)
>  	uint8_t major;
>  	uint8_t minor;
>  
> -	major = fadt->header.revision;
> -	minor = 0;
> -	if (major >= 5 && fadt->header.length >= 268)
> -		minor = fadt->minor_version & 0xF;   /* field added ACPI 5.1 */
> +	fwts_get_fadt_version(fw, &major, &minor);
>  
>  	fwts_log_info(fw, "FADT revision: %" PRIu8 ".%" PRIu8, major, minor);
>  	fwts_log_info(fw, "FADT table length: %" PRIu32, fadt->header.length);
> diff --git a/src/acpi/madt/madt.c b/src/acpi/madt/madt.c
> index 7173023a..82ec1115 100644
> --- a/src/acpi/madt/madt.c
> +++ b/src/acpi/madt/madt.c
> @@ -393,9 +393,7 @@ static int madt_init(fwts_framework *fw)
>  			return FWTS_ERROR;
>  		}
>  	}
> -
> -	if (fadt_major >= 5 && fadt->header.length >= 268)
> -		fadt_minor = fadt->minor_version;   /* field added ACPI 5.1 */
> +	fwts_get_fadt_version(fw, &fadt_major, &fadt_minor);
>  
>  	/* find the first occurrence for this version of MADT */
>  	while (ms->num_types != 0) {
> diff --git a/src/sbbr/fadt/fadt.c b/src/sbbr/fadt/fadt.c
> index a9b409ae..649a0840 100644
> --- a/src/sbbr/fadt/fadt.c
> +++ b/src/sbbr/fadt/fadt.c
> @@ -68,8 +68,7 @@ static int fadt_sbbr_revision(fwts_framework *fw)
>  	uint8_t major = fadt->header.revision;
>  	uint8_t minor = 0;
>  
> -	if ((major >= 5) && (fadt->header.length >= 268))
> -		minor = fadt->minor_version;   /* field added ACPI 5.1 */
> +	fwts_get_fadt_version(fw, &major, &minor);
>  
>  	fwts_log_info(fw, "FADT revision: %" PRIu8 ".%" PRIu8, major, minor);
>  
> 

Acked-by: Colin Ian King <colin.king at canonical.com>

Removing the logic from these functions is OK by me for this patch.

However, I'm concerned that the minor version field that was added in
ACPI 5.1 is being read by fwts_get_acpi_version when the structure in
pre-5.1 ACPI does not exist.

So.. if I understand things correctly:

fwts_get_acpi_version() should be checking for fadt->header.length >=
268 and only using the minor field if this is true (e.g. ACPI 5.1 onwards).

e.g.

	if ((fadt->header.revision >= 5) && (fadt->header.length >= 268))
		minor = ((fadt->minor_version & 0xF) << 4) + ((fadt->minor_version &
0xF0) >> 4);
	else
		minor = 0;


Colin