Patchwork lib: fwts_acpi_table: multi-FADT table support fimware make sure the table from XSDT be checked first. (LP: #1258378)

login
register
mail settings
Submitter Ivan Hu
Date Dec. 6, 2013, 4:26 a.m.
Message ID <1386304013-25029-1-git-send-email-ivan.hu@canonical.com>
Download mbox | patch
Permalink /patch/297569/
State Accepted
Headers show

Comments

Ivan Hu - Dec. 6, 2013, 4:26 a.m.
Some fimwares that contain two FADT tables, one comes from the RSDT and
the other comes from the XSDT. The FWTS will add the FADT tables followed
the order RSDT and XSDT. And the fadt test will load the fadt table first
added to the table what was from RSDT.

Unfortunately, some firmware provide the multi-fadt tables, one(from
XSDT) is correct and the other is uncompleted/wrong (from RSDT). See the
bug(LP: #1253871)

From the ACPI spec: it said "An ACPI-compatible OS must use the XSDT if
present.". So change table adding order to XSDT, RSDT to make sure fwts
check the fadt from XSDT first.

Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
---
 src/lib/src/fwts_acpi_tables.c |   36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)
Colin King - Dec. 6, 2013, 8:51 a.m.
On 06/12/13 04:26, Ivan Hu wrote:
> Some fimwares that contain two FADT tables, one comes from the RSDT and
> the other comes from the XSDT. The FWTS will add the FADT tables followed
> the order RSDT and XSDT. And the fadt test will load the fadt table first
> added to the table what was from RSDT.
> 
> Unfortunately, some firmware provide the multi-fadt tables, one(from
> XSDT) is correct and the other is uncompleted/wrong (from RSDT). See the
> bug(LP: #1253871)
> 
> From the ACPI spec: it said "An ACPI-compatible OS must use the XSDT if
> present.". So change table adding order to XSDT, RSDT to make sure fwts
> check the fadt from XSDT first.
> 
> Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
> ---
>  src/lib/src/fwts_acpi_tables.c |   36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_tables.c
> index b23efa7..6f1e964 100644
> --- a/src/lib/src/fwts_acpi_tables.c
> +++ b/src/lib/src/fwts_acpi_tables.c
> @@ -260,19 +260,19 @@ static int fwts_acpi_load_tables_from_firmware(void)
>  		return FWTS_ERROR;
>  	fwts_acpi_add_table("RSDP", rsdp, (uint64_t)(off_t)rsdp_addr, rsdp_len, FWTS_ACPI_TABLE_FROM_FIRMWARE);
>  
> -	/* Load any tables from RSDT if it's valid */
> -	if (rsdp->rsdt_address) {
> -		if ((rsdt = fwts_acpi_load_table((off_t)rsdp->rsdt_address)) != NULL) {
> -			fwts_acpi_add_table("RSDT", rsdt, (uint64_t)rsdp->rsdt_address,
> -				rsdt->header.length, FWTS_ACPI_TABLE_FROM_FIRMWARE);
> -			num_entries = (rsdt->header.length - sizeof(fwts_acpi_table_header)) / 4;
> +	/* Load any tables from XSDT if it's valid */
> +	if (rsdp->xsdt_address) {
> +		if ((xsdt = fwts_acpi_load_table((off_t)rsdp->xsdt_address)) != NULL) {
> +			fwts_acpi_add_table("XSDT", xsdt, (uint64_t)rsdp->xsdt_address,
> +				xsdt->header.length, FWTS_ACPI_TABLE_FROM_FIRMWARE);
> +			num_entries = (xsdt->header.length - sizeof(fwts_acpi_table_header)) / 8;
>  			for (i=0; i<num_entries; i++) {
> -				if (rsdt->entries[i]) {
> -					if ((header = fwts_acpi_load_table((off_t)rsdt->entries[i])) != NULL) {
> +				if (xsdt->entries[i]) {
> +					if ((header = fwts_acpi_load_table((off_t)xsdt->entries[i])) != NULL) {
>  						if (strncmp("FACP", header->signature, 4) == 0)
>  							fwts_acpi_handle_fadt((fwts_acpi_table_fadt*)header,
>  								FWTS_ACPI_TABLE_FROM_FIRMWARE);
> -						fwts_acpi_add_table(header->signature, header, (uint64_t)rsdt->entries[i],
> +						fwts_acpi_add_table(header->signature, header, xsdt->entries[i],
>  							header->length, FWTS_ACPI_TABLE_FROM_FIRMWARE);
>  					}
>  				}
> @@ -280,19 +280,19 @@ static int fwts_acpi_load_tables_from_firmware(void)
>  		}
>  	}
>  
> -	/* Load any tables from XSDT if it's valid */
> -	if (rsdp->xsdt_address) {
> -		if ((xsdt = fwts_acpi_load_table((off_t)rsdp->xsdt_address)) != NULL) {
> -			fwts_acpi_add_table("XSDT", xsdt, (uint64_t)rsdp->xsdt_address,
> -				xsdt->header.length, FWTS_ACPI_TABLE_FROM_FIRMWARE);
> -			num_entries = (xsdt->header.length - sizeof(fwts_acpi_table_header)) / 8;
> +	/* Load any tables from RSDT if it's valid */
> +	if (rsdp->rsdt_address) {
> +		if ((rsdt = fwts_acpi_load_table((off_t)rsdp->rsdt_address)) != NULL) {
> +			fwts_acpi_add_table("RSDT", rsdt, (uint64_t)rsdp->rsdt_address,
> +				rsdt->header.length, FWTS_ACPI_TABLE_FROM_FIRMWARE);
> +			num_entries = (rsdt->header.length - sizeof(fwts_acpi_table_header)) / 4;
>  			for (i=0; i<num_entries; i++) {
> -				if (xsdt->entries[i]) {
> -					if ((header = fwts_acpi_load_table((off_t)xsdt->entries[i])) != NULL) {
> +				if (rsdt->entries[i]) {
> +					if ((header = fwts_acpi_load_table((off_t)rsdt->entries[i])) != NULL) {
>  						if (strncmp("FACP", header->signature, 4) == 0)
>  							fwts_acpi_handle_fadt((fwts_acpi_table_fadt*)header,
>  								FWTS_ACPI_TABLE_FROM_FIRMWARE);
> -						fwts_acpi_add_table(header->signature, header, xsdt->entries[i],
> +						fwts_acpi_add_table(header->signature, header, (uint64_t)rsdt->entries[i],
>  							header->length, FWTS_ACPI_TABLE_FROM_FIRMWARE);
>  					}
>  				}
> 

Thanks for spotting this, good catch.

Acked-by: Colin Ian King <colin.king@canonical.com>
Alex Hung - Dec. 9, 2013, 4:05 a.m.
On 12/06/2013 12:26 PM, Ivan Hu wrote:
> Some fimwares that contain two FADT tables, one comes from the RSDT and
> the other comes from the XSDT. The FWTS will add the FADT tables followed
> the order RSDT and XSDT. And the fadt test will load the fadt table first
> added to the table what was from RSDT.
> 
> Unfortunately, some firmware provide the multi-fadt tables, one(from
> XSDT) is correct and the other is uncompleted/wrong (from RSDT). See the
> bug(LP: #1253871)
> 
> From the ACPI spec: it said "An ACPI-compatible OS must use the XSDT if
> present.". So change table adding order to XSDT, RSDT to make sure fwts
> check the fadt from XSDT first.
> 
> Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
> ---
>  src/lib/src/fwts_acpi_tables.c |   36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_tables.c
> index b23efa7..6f1e964 100644
> --- a/src/lib/src/fwts_acpi_tables.c
> +++ b/src/lib/src/fwts_acpi_tables.c
> @@ -260,19 +260,19 @@ static int fwts_acpi_load_tables_from_firmware(void)
>  		return FWTS_ERROR;
>  	fwts_acpi_add_table("RSDP", rsdp, (uint64_t)(off_t)rsdp_addr, rsdp_len, FWTS_ACPI_TABLE_FROM_FIRMWARE);
>  
> -	/* Load any tables from RSDT if it's valid */
> -	if (rsdp->rsdt_address) {
> -		if ((rsdt = fwts_acpi_load_table((off_t)rsdp->rsdt_address)) != NULL) {
> -			fwts_acpi_add_table("RSDT", rsdt, (uint64_t)rsdp->rsdt_address,
> -				rsdt->header.length, FWTS_ACPI_TABLE_FROM_FIRMWARE);
> -			num_entries = (rsdt->header.length - sizeof(fwts_acpi_table_header)) / 4;
> +	/* Load any tables from XSDT if it's valid */
> +	if (rsdp->xsdt_address) {
> +		if ((xsdt = fwts_acpi_load_table((off_t)rsdp->xsdt_address)) != NULL) {
> +			fwts_acpi_add_table("XSDT", xsdt, (uint64_t)rsdp->xsdt_address,
> +				xsdt->header.length, FWTS_ACPI_TABLE_FROM_FIRMWARE);
> +			num_entries = (xsdt->header.length - sizeof(fwts_acpi_table_header)) / 8;
>  			for (i=0; i<num_entries; i++) {
> -				if (rsdt->entries[i]) {
> -					if ((header = fwts_acpi_load_table((off_t)rsdt->entries[i])) != NULL) {
> +				if (xsdt->entries[i]) {
> +					if ((header = fwts_acpi_load_table((off_t)xsdt->entries[i])) != NULL) {
>  						if (strncmp("FACP", header->signature, 4) == 0)
>  							fwts_acpi_handle_fadt((fwts_acpi_table_fadt*)header,
>  								FWTS_ACPI_TABLE_FROM_FIRMWARE);
> -						fwts_acpi_add_table(header->signature, header, (uint64_t)rsdt->entries[i],
> +						fwts_acpi_add_table(header->signature, header, xsdt->entries[i],
>  							header->length, FWTS_ACPI_TABLE_FROM_FIRMWARE);
>  					}
>  				}
> @@ -280,19 +280,19 @@ static int fwts_acpi_load_tables_from_firmware(void)
>  		}
>  	}
>  
> -	/* Load any tables from XSDT if it's valid */
> -	if (rsdp->xsdt_address) {
> -		if ((xsdt = fwts_acpi_load_table((off_t)rsdp->xsdt_address)) != NULL) {
> -			fwts_acpi_add_table("XSDT", xsdt, (uint64_t)rsdp->xsdt_address,
> -				xsdt->header.length, FWTS_ACPI_TABLE_FROM_FIRMWARE);
> -			num_entries = (xsdt->header.length - sizeof(fwts_acpi_table_header)) / 8;
> +	/* Load any tables from RSDT if it's valid */
> +	if (rsdp->rsdt_address) {
> +		if ((rsdt = fwts_acpi_load_table((off_t)rsdp->rsdt_address)) != NULL) {
> +			fwts_acpi_add_table("RSDT", rsdt, (uint64_t)rsdp->rsdt_address,
> +				rsdt->header.length, FWTS_ACPI_TABLE_FROM_FIRMWARE);
> +			num_entries = (rsdt->header.length - sizeof(fwts_acpi_table_header)) / 4;
>  			for (i=0; i<num_entries; i++) {
> -				if (xsdt->entries[i]) {
> -					if ((header = fwts_acpi_load_table((off_t)xsdt->entries[i])) != NULL) {
> +				if (rsdt->entries[i]) {
> +					if ((header = fwts_acpi_load_table((off_t)rsdt->entries[i])) != NULL) {
>  						if (strncmp("FACP", header->signature, 4) == 0)
>  							fwts_acpi_handle_fadt((fwts_acpi_table_fadt*)header,
>  								FWTS_ACPI_TABLE_FROM_FIRMWARE);
> -						fwts_acpi_add_table(header->signature, header, xsdt->entries[i],
> +						fwts_acpi_add_table(header->signature, header, (uint64_t)rsdt->entries[i],
>  							header->length, FWTS_ACPI_TABLE_FROM_FIRMWARE);
>  					}
>  				}
> 

Acked-by: Alex Hung <alex.hung@canonical.com>

Patch

diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_tables.c
index b23efa7..6f1e964 100644
--- a/src/lib/src/fwts_acpi_tables.c
+++ b/src/lib/src/fwts_acpi_tables.c
@@ -260,19 +260,19 @@  static int fwts_acpi_load_tables_from_firmware(void)
 		return FWTS_ERROR;
 	fwts_acpi_add_table("RSDP", rsdp, (uint64_t)(off_t)rsdp_addr, rsdp_len, FWTS_ACPI_TABLE_FROM_FIRMWARE);
 
-	/* Load any tables from RSDT if it's valid */
-	if (rsdp->rsdt_address) {
-		if ((rsdt = fwts_acpi_load_table((off_t)rsdp->rsdt_address)) != NULL) {
-			fwts_acpi_add_table("RSDT", rsdt, (uint64_t)rsdp->rsdt_address,
-				rsdt->header.length, FWTS_ACPI_TABLE_FROM_FIRMWARE);
-			num_entries = (rsdt->header.length - sizeof(fwts_acpi_table_header)) / 4;
+	/* Load any tables from XSDT if it's valid */
+	if (rsdp->xsdt_address) {
+		if ((xsdt = fwts_acpi_load_table((off_t)rsdp->xsdt_address)) != NULL) {
+			fwts_acpi_add_table("XSDT", xsdt, (uint64_t)rsdp->xsdt_address,
+				xsdt->header.length, FWTS_ACPI_TABLE_FROM_FIRMWARE);
+			num_entries = (xsdt->header.length - sizeof(fwts_acpi_table_header)) / 8;
 			for (i=0; i<num_entries; i++) {
-				if (rsdt->entries[i]) {
-					if ((header = fwts_acpi_load_table((off_t)rsdt->entries[i])) != NULL) {
+				if (xsdt->entries[i]) {
+					if ((header = fwts_acpi_load_table((off_t)xsdt->entries[i])) != NULL) {
 						if (strncmp("FACP", header->signature, 4) == 0)
 							fwts_acpi_handle_fadt((fwts_acpi_table_fadt*)header,
 								FWTS_ACPI_TABLE_FROM_FIRMWARE);
-						fwts_acpi_add_table(header->signature, header, (uint64_t)rsdt->entries[i],
+						fwts_acpi_add_table(header->signature, header, xsdt->entries[i],
 							header->length, FWTS_ACPI_TABLE_FROM_FIRMWARE);
 					}
 				}
@@ -280,19 +280,19 @@  static int fwts_acpi_load_tables_from_firmware(void)
 		}
 	}
 
-	/* Load any tables from XSDT if it's valid */
-	if (rsdp->xsdt_address) {
-		if ((xsdt = fwts_acpi_load_table((off_t)rsdp->xsdt_address)) != NULL) {
-			fwts_acpi_add_table("XSDT", xsdt, (uint64_t)rsdp->xsdt_address,
-				xsdt->header.length, FWTS_ACPI_TABLE_FROM_FIRMWARE);
-			num_entries = (xsdt->header.length - sizeof(fwts_acpi_table_header)) / 8;
+	/* Load any tables from RSDT if it's valid */
+	if (rsdp->rsdt_address) {
+		if ((rsdt = fwts_acpi_load_table((off_t)rsdp->rsdt_address)) != NULL) {
+			fwts_acpi_add_table("RSDT", rsdt, (uint64_t)rsdp->rsdt_address,
+				rsdt->header.length, FWTS_ACPI_TABLE_FROM_FIRMWARE);
+			num_entries = (rsdt->header.length - sizeof(fwts_acpi_table_header)) / 4;
 			for (i=0; i<num_entries; i++) {
-				if (xsdt->entries[i]) {
-					if ((header = fwts_acpi_load_table((off_t)xsdt->entries[i])) != NULL) {
+				if (rsdt->entries[i]) {
+					if ((header = fwts_acpi_load_table((off_t)rsdt->entries[i])) != NULL) {
 						if (strncmp("FACP", header->signature, 4) == 0)
 							fwts_acpi_handle_fadt((fwts_acpi_table_fadt*)header,
 								FWTS_ACPI_TABLE_FROM_FIRMWARE);
-						fwts_acpi_add_table(header->signature, header, xsdt->entries[i],
+						fwts_acpi_add_table(header->signature, header, (uint64_t)rsdt->entries[i],
 							header->length, FWTS_ACPI_TABLE_FROM_FIRMWARE);
 					}
 				}