RE: Endless ACPI errors on Linus tree (5b664cb235)

[Moore, Robert]

Couple of thoughts.

I think we must use the V2 field if it is present and the address value
is > 32 bits.

What if the V2 address is < 32 bits but does not match the V1 address?

>-----Original Message-----
>From: Jan Beulich [mailto:jbeulich@xxxxxxxxxx]
>Sent: Wednesday, July 23, 2008 4:28 AM
>To: Andi Kleen; Len Brown
>Cc: Moore, Robert; Andrew Paprocki; Takashi Iwai; LKML
>Subject: Re: Endless ACPI errors on Linus tree (5b664cb235)
>
>>>> Andi Kleen <andi@xxxxxxxxxxxxxx> 21.07.08 10:43 >>>
>>> That would basically mean there's no way to use the v2 fields here -
all
>>> the code could do is report the inconsitency, but it would have to
use
>>> the v1 field in any case. That's really a shame, and as before I'm
>>> somewhat hesitant to do so. The only way I could easily find myself
>>> doing this would be to introduce a command line option controlling
the
>>> behavior here, but that would need to be done outside of the ACPICA
>>> code I'd assume.
>>>
>>> Andi, Bob, Len?
>>
>>I think we should only use v1 FADT fields for now and ignore v2.
>
>Here we go:
>
>Subject: ACPI: fix FADT parsing (v3)
>
>The (1.0 inherited) separate length fields in the FADT are byte
>granular. Further, PM1a/b may live in distinct address spaces, and they
>also could have different widths (if using the respective v2 fields was
>okay, which in practice it apparently isn't). acpi_tb_convert_fadt()
>should account for all of these conditions.
>
>Signed-off-by: Jan Beulich <jbeulich@xxxxxxxxxx>
>
>---
> drivers/acpi/tables/tbfadt.c |   32 +++++++++++++++++++++++++-------
> 1 file changed, 25 insertions(+), 7 deletions(-)
>
>--- linux-2.6.26/drivers/acpi/tables/tbfadt.c	2008-07-13
>23:51:29.000000000 +0200
>+++ 2.6.26-acpi-fadt-parse/drivers/acpi/tables/tbfadt.c
2008-07-23
>13:21:22.000000000 +0200
>@@ -50,7 +50,7 @@ ACPI_MODULE_NAME("tbfadt")
> /* Local prototypes */
> static void inline
> acpi_tb_init_generic_address(struct acpi_generic_address
*generic_address,
>-			     u8 bit_width, u64 address);
>+			     u8 byte_width, u64 address);
>
> static void acpi_tb_convert_fadt(void);
>
>@@ -111,7 +111,7 @@ static struct acpi_fadt_info fadt_info_t
>  * FUNCTION:    acpi_tb_init_generic_address
>  *
>  * PARAMETERS:  generic_address     - GAS struct to be initialized
>- *              bit_width           - Width of this register
>+ *              byte_width          - Width of this register
>  *              Address             - Address of the register
>  *
>  * RETURN:      None
>@@ -124,7 +124,7 @@ static struct acpi_fadt_info fadt_info_t
>
> static void inline
> acpi_tb_init_generic_address(struct acpi_generic_address
*generic_address,
>-			     u8 bit_width, u64 address)
>+			     u8 byte_width, u64 address)
> {
>
> 	/*
>@@ -136,7 +136,7 @@ acpi_tb_init_generic_address(struct acpi
> 	/* All other fields are byte-wide */
>
> 	generic_address->space_id = ACPI_ADR_SPACE_SYSTEM_IO;
>-	generic_address->bit_width = bit_width;
>+	generic_address->bit_width = byte_width << 3;
> 	generic_address->bit_offset = 0;
> 	generic_address->access_width = 0;
> }
>@@ -342,9 +342,20 @@ static void acpi_tb_convert_fadt(void)
> 	 * useful to calculate them once, here.
> 	 *
> 	 * The PM event blocks are split into two register blocks, first
is
>the
>-	 * PM Status Register block, followed immediately by the PM
Enable
>Register
>-	 * block. Each is of length (pm1_event_length/2)
>+	 * PM Status Register block, followed immediately by the PM
Enable
>+	 * Register block. Each is of length
(xpm1x_event_block.bit_width/2).
>+	 *
>+	 * On various systems the v2 fields (and particularly the bit
widths)
>+	 * cannot be relied upon, though. Hence resort to using the v1
length
>+	 * here (and warn about the inconsistency).
> 	 */
>+	if (acpi_gbl_FADT.xpm1a_event_block.bit_width
>+	    != acpi_gbl_FADT.pm1_event_length * 8)
>+		printk(KERN_WARNING "FADT: "
>+		       "X_PM1a_EVT_BLK.bit_width (%u) does not match"
>+		       " PM1_EVT_LEN (%u)\n",
>+		       acpi_gbl_FADT.xpm1a_event_block.bit_width,
>+		       acpi_gbl_FADT.pm1_event_length);
> 	pm1_register_length = (u8)
>ACPI_DIV_2(acpi_gbl_FADT.pm1_event_length);
>
> 	/* The PM1A register block is required */
>@@ -360,13 +371,20 @@ static void acpi_tb_convert_fadt(void)
> 	/* The PM1B register block is optional, ignore if not present */
>
> 	if (acpi_gbl_FADT.xpm1b_event_block.address) {
>+		if (acpi_gbl_FADT.xpm1b_event_block.bit_width
>+		    != acpi_gbl_FADT.pm1_event_length * 8)
>+			printk(KERN_WARNING "FADT: "
>+			       "X_PM1b_EVT_BLK.bit_width (%u) does not
match"
>+			       " PM1_EVT_LEN (%u)\n",
>+
acpi_gbl_FADT.xpm1b_event_block.bit_width,
>+			       acpi_gbl_FADT.pm1_event_length);
> 		acpi_tb_init_generic_address(&acpi_gbl_xpm1b_enable,
> 					     pm1_register_length,
>
(acpi_gbl_FADT.xpm1b_event_block.
> 					      address +
pm1_register_length));
> 		/* Don't forget to copy space_id of the GAS */
> 		acpi_gbl_xpm1b_enable.space_id =
>-		    acpi_gbl_FADT.xpm1a_event_block.space_id;
>+		    acpi_gbl_FADT.xpm1b_event_block.space_id;
>
> 	}
> }
>

--
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/