RE: [PATCH ] tbfadt.c: output warning only when 64bit 32bit addressof FACS/DSDT all have value but not equal to each other

[Moore, Robert]

Ethan,

Does this actually solve a problem that you have seen?

I ask because of the code that converts the FADT to the common internal format:

acpi_tb_convert_fadt (
...
	/*
	 * Expand the 32-bit FACS and DSDT addresses to 64-bit as necessary.
	 * Later code will always use the X 64-bit field.
	 */
	if (!acpi_gbl_FADT.Xfacs) {
		acpi_gbl_FADT.Xfacs = (u64) acpi_gbl_FADT.facs;
	}
	if (!acpi_gbl_FADT.Xdsdt) {
		acpi_gbl_FADT.Xdsdt = (u64) acpi_gbl_FADT.dsdt;
	}

This function is called before the call to validate_fadt, so if the 64-bit address was originally zero, it is set to be equal to the 32-bit address. Later, only the 64-bit address field is used.

So, after convert_fadt, we have the following possible conditions:

1) Both facs and Xfacs are zero
2) facs is zero, but Xfacs in non-zero
3) Both facs and Xfacs are non-zero but equal
4) Both facs and Xfacs are non-zero and not equal

Issues for each condition above:

1) There is no FACS
2) No error, we are going to use Xfacs anyway
3) No error, we will use Xfacs
4) This is the error condition, Xfacs != facs


The check for case (4) is what appears in validate_facs:

If ((facs > 0) && (facs != Xfacs))

I don't think we need to check the value of Xfacs against zero because we know we are in case (3) or (4) after the check for facs nonzero:

if
   (facs > 0) - if true, we are in case (3) or (4)
    if
       (facs != Xfacs) - if true, we now know we are in case (4) --> error.


Unless I'm missing something early this morning.
Bob


> -----Original Message-----
> From: Ethan Zhao [mailto:ethan.kernel@xxxxxxxxx]
> Sent: Monday, November 12, 2012 7:32 PM
> To: Brown, Len
> Cc: Moore, Robert; Zheng, Lv; LKML
> Subject: [PATCH ] tbfadt.c: output warning only when 64bit 32bit address
> of FACS/DSDT all have value but not equal to each other
> 
> Hi, Len, Robert,
> 
>        Please help to check the following patch, add more conditions when
> validate the 64bit 32bit FACS/DSDT address in FADT to follow ACPI spec, In
> the meantime, keep the compatibility and latitude.
> 
> 
> Thanks,
> Ethan
> 
> 
> 
> From c1116211a7b329c26b0370565c36b084ceb08f71 Mon Sep 17 00:00:00 2001
> From: ethan.zhao <ethan.kernel@xxxxxxxxx>
> Date: Tue, 13 Nov 2012 22:21:12 -0800
> Subject: [PATCH 642/642] To follow the ACPI spec 3,4&5 and keep the
> compatibility and latitude,only  output mismatch warning when 64bit
> address and 32bit address of FACS/DSDT are all  valid but not equal to
> each other.
> 
> 
> Signed-off-by: ethan.zhao <ethan.kernel@xxxxxxxxx>
> ---
>  drivers/acpi/acpica/tbfadt.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/acpica/tbfadt.c b/drivers/acpi/acpica/tbfadt.c
> index 3906518..f23f512 100644
> --- a/drivers/acpi/acpica/tbfadt.c
> +++ b/drivers/acpi/acpica/tbfadt.c
> @@ -512,7 +512,7 @@ static void acpi_tb_validate_fadt(void)
>          * 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.
>          */
> -       if (acpi_gbl_FADT.facs &&
> +       if ((acpi_gbl_FADT.facs && acpi_gbl_FADT.Xfacs) &&
>             (acpi_gbl_FADT.Xfacs != (u64)acpi_gbl_FADT.facs)) {
>                 ACPI_BIOS_WARNING((AE_INFO,
>                                    "32/64X FACS address mismatch in FADT -
> "
> @@ -523,7 +523,7 @@ static void acpi_tb_validate_fadt(void)
>                 acpi_gbl_FADT.Xfacs = (u64)acpi_gbl_FADT.facs;
>         }
> 
> -       if (acpi_gbl_FADT.dsdt &&
> +       if ((acpi_gbl_FADT.dsdt && acpi_gbl_FADT.Xdsdt) &&
>             (acpi_gbl_FADT.Xdsdt != (u64)acpi_gbl_FADT.dsdt)) {
>                 ACPI_BIOS_WARNING((AE_INFO,
>                                    "32/64X DSDT address mismatch in FADT -
> "
> --
> 1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/