Message-ID: <1AE640813FDE7649BE1B193DEA596E880255B3F0@SHSMSX101.ccr.corp.intel.com>
Date:	Mon, 12 May 2014 01:10:00 +0000
From:	"Zheng, Lv" <lv.zheng@...el.com>
To:	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	ACPI Devel Maling List <linux-acpi@...r.kernel.org>
CC:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"Oswald Buddenhagen" <ossi@....org>
Subject: RE: [PATCH] ACPICA: Revert "ACPICA: Add option to favor 32-bit FADT
 addresses."

> From: Rafael J. Wysocki [mailto:rjw@...ysocki.net]
> Sent: Monday, May 12, 2014 8:12 AM
> 
> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> 
> Revert commit 0249ed2444d6 (ACPICA: Add option to favor 32-bit FADT
> addresses.) that breaks resume from system suspend on the Intel DP45SG
> board.

This commit doesn't look like the root cause.
[    0.000000] ACPI BIOS Warning (bug): Incorrect checksum in table [XSDT] - 0xA0, should be 0xC9 (20131218/tbprint-214)
[    0.000000] ACPI BIOS Warning (bug): 32/64X FACS address mismatch in FADT: 0xCF661F40/0x00000000CF667E40, using 64-bit address (20131218/tbfadt-271)
It seems before using the FADT, ACPICA have already detected that the XSDT is not correct.

Please revert it for now.  I'll take a look at this bug.

Thanks and best regards
-Lv

> 
> Fixes: 0249ed2444d6 (ACPICA: Add option to favor 32-bit FADT addresses.)
> References: https://bugzilla.kernel.org/show_bug.cgi?id=74021
> Reported-and-tested-by: Oswald Buddenhagen <ossi@....org>
> Cc: 3.14+ <stable@...r.kernel.org> # 3.14+
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> ---
>  drivers/acpi/acpica/acglobal.h |  10 --
>  drivers/acpi/acpica/tbfadt.c   | 335 +++++++++++++++++++----------------------
>  include/acpi/acpixf.h          |   1 -
>  3 files changed, 152 insertions(+), 194 deletions(-)
> 
> diff --git a/drivers/acpi/acpica/acglobal.h b/drivers/acpi/acpica/acglobal.h
> index 49bbc71fad54..72578a4b8212 100644
> --- a/drivers/acpi/acpica/acglobal.h
> +++ b/drivers/acpi/acpica/acglobal.h
> @@ -136,16 +136,6 @@ ACPI_INIT_GLOBAL(u8, acpi_gbl_copy_dsdt_locally, FALSE);
>  ACPI_INIT_GLOBAL(u8, acpi_gbl_do_not_use_xsdt, FALSE);
> 
>  /*
> - * Optionally use 32-bit FADT addresses if and when there is a conflict
> - * (address mismatch) between the 32-bit and 64-bit versions of the
> - * address. Although ACPICA adheres to the ACPI specification which
> - * requires the use of the corresponding 64-bit address if it is non-zero,
> - * some machines have been found to have a corrupted non-zero 64-bit
> - * address. Default is FALSE, do not favor the 32-bit addresses.
> - */
> -ACPI_INIT_GLOBAL(u8, acpi_gbl_use32_bit_fadt_addresses, FALSE);
> -
> -/*
>   * Optionally truncate I/O addresses to 16 bits. Provides compatibility
>   * with other ACPI implementations. NOTE: During ACPICA initialization,
>   * this value is set to TRUE if any Windows OSI strings have been
> diff --git a/drivers/acpi/acpica/tbfadt.c b/drivers/acpi/acpica/tbfadt.c
> index ec14588254d4..6dd5c590e0bb 100644
> --- a/drivers/acpi/acpica/tbfadt.c
> +++ b/drivers/acpi/acpica/tbfadt.c
> @@ -56,10 +56,9 @@ acpi_tb_init_generic_address(struct acpi_generic_address *generic_address,
> 
>  static void acpi_tb_convert_fadt(void);
> 
> -static void acpi_tb_setup_fadt_registers(void);
> +static void acpi_tb_validate_fadt(void);
> 
> -static u64
> -acpi_tb_select_address(char *register_name, u32 address32, u64 address64);
> +static void acpi_tb_setup_fadt_registers(void);
> 
>  /* Table for conversion of FADT to common internal format and FADT validation */
> 
> @@ -176,7 +175,6 @@ static struct acpi_fadt_pm_info fadt_pm_info_table[] = {
>   *              space_id            - ACPI Space ID for this register
>   *              byte_width          - Width of this register
>   *              address             - Address of the register
> - *              register_name       - ASCII name of the ACPI register
>   *
>   * RETURN:      None
>   *
> @@ -222,68 +220,6 @@ acpi_tb_init_generic_address(struct acpi_generic_address *generic_address,
> 
>  /*******************************************************************************
>   *
> - * FUNCTION:    acpi_tb_select_address
> - *
> - * PARAMETERS:  register_name       - ASCII name of the ACPI register
> - *              address32           - 32-bit address of the register
> - *              address64           - 64-bit address of the register
> - *
> - * RETURN:      The resolved 64-bit address
> - *
> - * DESCRIPTION: Select between 32-bit and 64-bit versions of addresses within
> - *              the FADT. Used for the FACS and DSDT addresses.
> - *
> - * NOTES:
> - *
> - * Check for FACS and DSDT address mismatches. An address mismatch between
> - * the 32-bit and 64-bit address fields (FIRMWARE_CTRL/X_FIRMWARE_CTRL and
> - * DSDT/X_DSDT) could be a corrupted address field or it might indicate
> - * the presence of two FACS or two DSDT tables.
> - *
> - * November 2013:
> - * By default, as per the ACPICA specification, a valid 64-bit address is
> - * used regardless of the value of the 32-bit address. However, this
> - * behavior can be overridden via the acpi_gbl_use32_bit_fadt_addresses flag.
> - *
> - ******************************************************************************/
> -
> -static u64
> -acpi_tb_select_address(char *register_name, u32 address32, u64 address64)
> -{
> -
> -	if (!address64) {
> -
> -		/* 64-bit address is zero, use 32-bit address */
> -
> -		return ((u64)address32);
> -	}
> -
> -	if (address32 && (address64 != (u64)address32)) {
> -
> -		/* Address mismatch between 32-bit and 64-bit versions */
> -
> -		ACPI_BIOS_WARNING((AE_INFO,
> -				   "32/64X %s address mismatch in FADT: "
> -				   "0x%8.8X/0x%8.8X%8.8X, using %u-bit address",
> -				   register_name, address32,
> -				   ACPI_FORMAT_UINT64(address64),
> -				   acpi_gbl_use32_bit_fadt_addresses ? 32 :
> -				   64));
> -
> -		/* 32-bit address override */
> -
> -		if (acpi_gbl_use32_bit_fadt_addresses) {
> -			return ((u64)address32);
> -		}
> -	}
> -
> -	/* Default is to use the 64-bit address */
> -
> -	return (address64);
> -}
> -
> -/*******************************************************************************
> - *
>   * FUNCTION:    acpi_tb_parse_fadt
>   *
>   * PARAMETERS:  table_index         - Index for the FADT
> @@ -395,6 +331,10 @@ void acpi_tb_create_local_fadt(struct acpi_table_header *table, u32 length)
> 
>  	acpi_tb_convert_fadt();
> 
> +	/* Validate FADT values now, before we make any changes */
> +
> +	acpi_tb_validate_fadt();
> +
>  	/* Initialize the global ACPI register structures */
> 
>  	acpi_tb_setup_fadt_registers();
> @@ -404,55 +344,66 @@ void acpi_tb_create_local_fadt(struct acpi_table_header *table, u32 length)
>   *
>   * FUNCTION:    acpi_tb_convert_fadt
>   *
> - * PARAMETERS:  none - acpi_gbl_FADT is used.
> + * PARAMETERS:  None, uses acpi_gbl_FADT
>   *
>   * RETURN:      None
>   *
>   * DESCRIPTION: Converts all versions of the FADT to a common internal format.
> - *              Expand 32-bit addresses to 64-bit as necessary. Also validate
> - *              important fields within the FADT.
> + *              Expand 32-bit addresses to 64-bit as necessary.
>   *
> - * NOTE:        acpi_gbl_FADT must be of size (struct acpi_table_fadt), and must
> - *              contain a copy of the actual BIOS-provided FADT.
> + * NOTE:        acpi_gbl_FADT must be of size (struct acpi_table_fadt),
> + *              and must contain a copy of the actual FADT.
>   *
>   * Notes on 64-bit register addresses:
>   *
>   * After this FADT conversion, later ACPICA code will only use the 64-bit "X"
>   * fields of the FADT for all ACPI register addresses.
>   *
> - * The 64-bit X fields are optional extensions to the original 32-bit FADT
> + * The 64-bit "X" fields are optional extensions to the original 32-bit FADT
>   * V1.0 fields. Even if they are present in the FADT, they are optional and
>   * are unused if the BIOS sets them to zero. Therefore, we must copy/expand
> - * 32-bit V1.0 fields to the 64-bit X fields if the the 64-bit X field is
> - * originally zero.
> + * 32-bit V1.0 fields if the corresponding X field is zero.
>   *
> - * For ACPI 1.0 FADTs (that contain no 64-bit addresses), all 32-bit address
> - * fields are expanded to the corresponding 64-bit X fields in the internal
> - * common FADT.
> + * For ACPI 1.0 FADTs, all 32-bit address fields are expanded to the
> + * corresponding "X" fields in the internal FADT.
>   *
>   * For ACPI 2.0+ FADTs, all valid (non-zero) 32-bit address fields are expanded
> - * to the corresponding 64-bit X fields, if the 64-bit field is originally
> - * zero. Adhering to the ACPI specification, we completely ignore the 32-bit
> - * field if the 64-bit field is valid, regardless of whether the host OS is
> - * 32-bit or 64-bit.
> - *
> - * Possible additional checks:
> - *  (acpi_gbl_FADT.pm1_event_length >= 4)
> - *  (acpi_gbl_FADT.pm1_control_length >= 2)
> - *  (acpi_gbl_FADT.pm_timer_length >= 4)
> - *  Gpe block lengths must be multiple of 2
> + * to the corresponding 64-bit X fields. For compatibility with other ACPI
> + * implementations, we ignore the 64-bit field if the 32-bit field is valid,
> + * regardless of whether the host OS is 32-bit or 64-bit.
>   *
>   ******************************************************************************/
> 
>  static void acpi_tb_convert_fadt(void)
>  {
> -	char *name;
>  	struct acpi_generic_address *address64;
>  	u32 address32;
> -	u8 length;
>  	u32 i;
> 
>  	/*
> +	 * Expand the 32-bit FACS and DSDT addresses to 64-bit as necessary.
> +	 * Later code will always use the X 64-bit field. Also, check for an
> +	 * address mismatch between the 32-bit and 64-bit address fields
> +	 * (FIRMWARE_CTRL/X_FIRMWARE_CTRL, DSDT/X_DSDT) which would indicate
> +	 * the presence of two FACS or two DSDT tables.
> +	 */
> +	if (!acpi_gbl_FADT.Xfacs) {
> +		acpi_gbl_FADT.Xfacs = (u64) acpi_gbl_FADT.facs;
> +	} else if (acpi_gbl_FADT.facs &&
> +		   (acpi_gbl_FADT.Xfacs != (u64) acpi_gbl_FADT.facs)) {
> +		ACPI_WARNING((AE_INFO,
> +		    "32/64 FACS address mismatch in FADT - two FACS tables!"));
> +	}
> +
> +	if (!acpi_gbl_FADT.Xdsdt) {
> +		acpi_gbl_FADT.Xdsdt = (u64) acpi_gbl_FADT.dsdt;
> +	} else if (acpi_gbl_FADT.dsdt &&
> +		   (acpi_gbl_FADT.Xdsdt != (u64) acpi_gbl_FADT.dsdt)) {
> +		ACPI_WARNING((AE_INFO,
> +		    "32/64 DSDT address mismatch in FADT - two DSDT tables!"));
> +	}
> +
> +	/*
>  	 * For ACPI 1.0 FADTs (revision 1 or 2), ensure that reserved fields which
>  	 * should be zero are indeed zero. This will workaround BIOSs that
>  	 * inadvertently place values in these fields.
> @@ -470,24 +421,119 @@ static void acpi_tb_convert_fadt(void)
>  		acpi_gbl_FADT.boot_flags = 0;
>  	}
> 
> +	/* Update the local FADT table header length */
> +
> +	acpi_gbl_FADT.header.length = sizeof(struct acpi_table_fadt);
> +
>  	/*
> -	 * Now we can update the local FADT length to the length of the
> -	 * current FADT version as defined by the ACPI specification.
> -	 * Thus, we will have a common FADT internally.
> +	 * Expand the ACPI 1.0 32-bit addresses to the ACPI 2.0 64-bit "X"
> +	 * generic address structures as necessary. Later code will always use
> +	 * the 64-bit address structures.
> +	 *
> +	 * March 2009:
> +	 * We now always use the 32-bit address if it is valid (non-null). This
> +	 * is not in accordance with the ACPI specification which states that
> +	 * the 64-bit address supersedes the 32-bit version, but we do this for
> +	 * compatibility with other ACPI implementations. Most notably, in the
> +	 * case where both the 32 and 64 versions are non-null, we use the 32-bit
> +	 * version. This is the only address that is guaranteed to have been
> +	 * tested by the BIOS manufacturer.
>  	 */
> -	acpi_gbl_FADT.header.length = sizeof(struct acpi_table_fadt);
> +	for (i = 0; i < ACPI_FADT_INFO_ENTRIES; i++) {
> +		address32 = *ACPI_ADD_PTR(u32,
> +					  &acpi_gbl_FADT,
> +					  fadt_info_table[i].address32);
> +
> +		address64 = ACPI_ADD_PTR(struct acpi_generic_address,
> +					 &acpi_gbl_FADT,
> +					 fadt_info_table[i].address64);
> +
> +		/*
> +		 * If both 32- and 64-bit addresses are valid (non-zero),
> +		 * they must match.
> +		 */
> +		if (address64->address && address32 &&
> +		    (address64->address != (u64)address32)) {
> +			ACPI_BIOS_ERROR((AE_INFO,
> +					 "32/64X address mismatch in FADT/%s: "
> +					 "0x%8.8X/0x%8.8X%8.8X, using 32",
> +					 fadt_info_table[i].name, address32,
> +					 ACPI_FORMAT_UINT64(address64->
> +							    address)));
> +		}
> +
> +		/* Always use 32-bit address if it is valid (non-null) */
> +
> +		if (address32) {
> +			/*
> +			 * Copy the 32-bit address to the 64-bit GAS structure. The
> +			 * Space ID is always I/O for 32-bit legacy address fields
> +			*/
> +			acpi_tb_init_generic_address(address64,
> +						     ACPI_ADR_SPACE_SYSTEM_IO,
> +						     *ACPI_ADD_PTR(u8,
> +								   &acpi_gbl_FADT,
> +								   fadt_info_table
> +								   [i].length),
> +						     (u64) address32,
> +						     fadt_info_table[i].name);
> +		}
> +	}
> +}
> +
> +/*******************************************************************************
> + *
> + * FUNCTION:    acpi_tb_validate_fadt
> + *
> + * PARAMETERS:  table           - Pointer to the FADT to be validated
> + *
> + * RETURN:      None
> + *
> + * DESCRIPTION: Validate various important fields within the FADT. If a problem
> + *              is found, issue a message, but no status is returned.
> + *              Used by both the table manager and the disassembler.
> + *
> + * Possible additional checks:
> + * (acpi_gbl_FADT.pm1_event_length >= 4)
> + * (acpi_gbl_FADT.pm1_control_length >= 2)
> + * (acpi_gbl_FADT.pm_timer_length >= 4)
> + * Gpe block lengths must be multiple of 2
> + *
> + ******************************************************************************/
> +
> +static void acpi_tb_validate_fadt(void)
> +{
> +	char *name;
> +	struct acpi_generic_address *address64;
> +	u8 length;
> +	u32 i;
> 
>  	/*
> -	 * Expand the 32-bit FACS and DSDT addresses to 64-bit as necessary.
> -	 * Later ACPICA code will always use the X 64-bit field.
> +	 * Check for FACS and DSDT address mismatches. An address mismatch between
> +	 * the 32-bit and 64-bit address fields (FIRMWARE_CTRL/X_FIRMWARE_CTRL and
> +	 * DSDT/X_DSDT) would indicate the presence of two FACS or two DSDT tables.
>  	 */
> -	acpi_gbl_FADT.Xfacs = acpi_tb_select_address("FACS",
> -						     acpi_gbl_FADT.facs,
> -						     acpi_gbl_FADT.Xfacs);
> +	if (acpi_gbl_FADT.facs &&
> +	    (acpi_gbl_FADT.Xfacs != (u64)acpi_gbl_FADT.facs)) {
> +		ACPI_BIOS_WARNING((AE_INFO,
> +				   "32/64X FACS address mismatch in FADT - "
> +				   "0x%8.8X/0x%8.8X%8.8X, using 32",
> +				   acpi_gbl_FADT.facs,
> +				   ACPI_FORMAT_UINT64(acpi_gbl_FADT.Xfacs)));
> +
> +		acpi_gbl_FADT.Xfacs = (u64)acpi_gbl_FADT.facs;
> +	}
> +
> +	if (acpi_gbl_FADT.dsdt &&
> +	    (acpi_gbl_FADT.Xdsdt != (u64)acpi_gbl_FADT.dsdt)) {
> +		ACPI_BIOS_WARNING((AE_INFO,
> +				   "32/64X DSDT address mismatch in FADT - "
> +				   "0x%8.8X/0x%8.8X%8.8X, using 32",
> +				   acpi_gbl_FADT.dsdt,
> +				   ACPI_FORMAT_UINT64(acpi_gbl_FADT.Xdsdt)));
> 
> -	acpi_gbl_FADT.Xdsdt = acpi_tb_select_address("DSDT",
> -						     acpi_gbl_FADT.dsdt,
> -						     acpi_gbl_FADT.Xdsdt);
> +		acpi_gbl_FADT.Xdsdt = (u64)acpi_gbl_FADT.dsdt;
> +	}
> 
>  	/* If Hardware Reduced flag is set, we are all done */
> 
> @@ -499,95 +545,18 @@ static void acpi_tb_convert_fadt(void)
> 
>  	for (i = 0; i < ACPI_FADT_INFO_ENTRIES; i++) {
>  		/*
> -		 * Get the 32-bit and 64-bit addresses, as well as the register
> -		 * length and register name.
> +		 * Generate pointer to the 64-bit address, get the register
> +		 * length (width) and the register name
>  		 */
> -		address32 = *ACPI_ADD_PTR(u32,
> -					  &acpi_gbl_FADT,
> -					  fadt_info_table[i].address32);
> -
>  		address64 = ACPI_ADD_PTR(struct acpi_generic_address,
>  					 &acpi_gbl_FADT,
>  					 fadt_info_table[i].address64);
> -
> -		length = *ACPI_ADD_PTR(u8,
> -				       &acpi_gbl_FADT,
> -				       fadt_info_table[i].length);
> -
> +		length =
> +		    *ACPI_ADD_PTR(u8, &acpi_gbl_FADT,
> +				  fadt_info_table[i].length);
>  		name = fadt_info_table[i].name;
> 
>  		/*
> -		 * Expand the ACPI 1.0 32-bit addresses to the ACPI 2.0 64-bit "X"
> -		 * generic address structures as necessary. Later code will always use
> -		 * the 64-bit address structures.
> -		 *
> -		 * November 2013:
> -		 * Now always use the 64-bit address if it is valid (non-zero), in
> -		 * accordance with the ACPI specification which states that a 64-bit
> -		 * address supersedes the 32-bit version. This behavior can be
> -		 * overridden by the acpi_gbl_use32_bit_fadt_addresses flag.
> -		 *
> -		 * During 64-bit address construction and verification,
> -		 * these cases are handled:
> -		 *
> -		 * Address32 zero, Address64 [don't care]   - Use Address64
> -		 *
> -		 * Address32 non-zero, Address64 zero       - Copy/use Address32
> -		 * Address32 non-zero == Address64 non-zero - Use Address64
> -		 * Address32 non-zero != Address64 non-zero - Warning, use Address64
> -		 *
> -		 * Override: if acpi_gbl_use32_bit_fadt_addresses is TRUE, and:
> -		 * Address32 non-zero != Address64 non-zero - Warning, copy/use Address32
> -		 *
> -		 * Note: space_id is always I/O for 32-bit legacy address fields
> -		 */
> -		if (address32) {
> -			if (!address64->address) {
> -
> -				/* 64-bit address is zero, use 32-bit address */
> -
> -				acpi_tb_init_generic_address(address64,
> -							     ACPI_ADR_SPACE_SYSTEM_IO,
> -							     *ACPI_ADD_PTR(u8,
> -									   &acpi_gbl_FADT,
> -									   fadt_info_table
> -									   [i].
> -									   length),
> -							     (u64)address32,
> -							     name);
> -			} else if (address64->address != (u64)address32) {
> -
> -				/* Address mismatch */
> -
> -				ACPI_BIOS_WARNING((AE_INFO,
> -						   "32/64X address mismatch in FADT/%s: "
> -						   "0x%8.8X/0x%8.8X%8.8X, using %u-bit address",
> -						   name, address32,
> -						   ACPI_FORMAT_UINT64
> -						   (address64->address),
> -						   acpi_gbl_use32_bit_fadt_addresses
> -						   ? 32 : 64));
> -
> -				if (acpi_gbl_use32_bit_fadt_addresses) {
> -
> -					/* 32-bit address override */
> -
> -					acpi_tb_init_generic_address(address64,
> -								     ACPI_ADR_SPACE_SYSTEM_IO,
> -								     *ACPI_ADD_PTR
> -								     (u8,
> -								      &acpi_gbl_FADT,
> -								      fadt_info_table
> -								      [i].
> -								      length),
> -								     (u64)
> -								     address32,
> -								     name);
> -				}
> -			}
> -		}
> -
> -		/*
>  		 * For each extended field, check for length mismatch between the
>  		 * legacy length field and the corresponding 64-bit X length field.
>  		 * Note: If the legacy length field is > 0xFF bits, ignore this
> diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
> index 44f5e9749601..40d1bc88cc14 100644
> --- a/include/acpi/acpixf.h
> +++ b/include/acpi/acpixf.h
> @@ -82,7 +82,6 @@ extern u8 acpi_gbl_enable_interpreter_slack;
>  extern u32 acpi_gbl_trace_flags;
>  extern acpi_name acpi_gbl_trace_method_name;
>  extern u8 acpi_gbl_truncate_io_addresses;
> -extern u8 acpi_gbl_use32_bit_fadt_addresses;
>  extern u8 acpi_gbl_use_default_register_widths;
> 
>  /*
> --
> 1.8.4.5
> 

Powered by blists - more mailing lists