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

Submitted by Ivan Hu on Dec. 6, 2013, 4:26 a.m.

Details

Message ID 1386304013-25029-1-git-send-email-ivan.hu@canonical.com
State Accepted
Headers show

Commit Message

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(-)

Comments

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 hide | download patch | download mbox

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);
 					}
 				}