I just saw this for the first time using a kernel built from the git
head. This is all the ACPI related printk info around the warning. If
more information is needed to figure out what this is, please let me
know.
[ 0.000000] ACPI: RSDP 000F7CE0, 0024 (r2 RS690 )
[ 0.000000] ACPI: XSDT 3DFE30C0, 004C (r1 RS690 AWRDACPI 42302E31 AWRD 0)
[ 0.000000] ACPI: FACP 3DFE7400, 00F4 (r3 RS690 AWRDACPI 42302E31 AWRD 0)
[ 0.000000] ------------[ cut here ]------------
[ 0.000000] WARNING: at drivers/acpi/tables/tbfadt.c:348
acpi_tb_create_local_fadt+0x147/0x2f4()
[ 0.000000] Pid: 0, comm: swapper Not tainted 2.6.26 #1
[ 0.000000] [<c01171bb>] warn_on_slowpath+0x3b/0x74
[ 0.000000] [<c01ede36>] acpi_os_vprintf+0x1d/0x20
[ 0.000000] [<c01ede46>] acpi_os_printf+0xd/0xe
[ 0.000000] [<c020120e>] acpi_ut_info+0x21/0x24
[ 0.000000] [<c0200016>] acpi_tb_print_table_header+0x96/0x9c
[ 0.000000] [<c020032b>] acpi_tb_create_local_fadt+0x147/0x2f4
[ 0.000000] [<c0200508>] acpi_tb_parse_fadt+0x30/0x6c
[ 0.000000] [<c03eeda5>] acpi_tb_parse_root_table+0x264/0x2ab
[ 0.000000] [<c03ee469>] acpi_table_init+0x14/0x6d
[ 0.000000] [<c03e3b28>] acpi_boot_table_init+0x2b/0xc3
[ 0.000000] [<c03dfcf4>] setup_arch+0x3fa/0x557
[ 0.000000] [<c03de7a9>] start_kernel+0x41/0x1ec
[ 0.000000] =======================
[ 0.000000] ---[ end trace 4eaa2a86a8e2da22 ]---
[ 0.000000] ACPI: DSDT 3DFE3240, 4146 (r1 RS690 AWRDACPI 1000
MSFT 100000E)
[ 0.000000] ACPI: FACS 3DFE0000, 0040
[ 0.000000] ACPI: SSDT 3DFE7600, 0136 (r1 PTLTD POWERNOW 1 LTP 1)
[ 0.000000] ACPI: HPET 3DFE7780, 0038 (r1 RS690 AWRDACPI 42302E31 AWRD 98)
[ 0.000000] ACPI: MCFG 3DFE7800, 003C (r1 RS690 AWRDACPI 42302E31 AWRD 0)
[ 0.000000] ACPI: APIC 3DFE7540, 0068 (r1 RS690 AWRDACPI 42302E31 AWRD 0)
On Wed, Jul 16, 2008 at 10:29 PM, Andrew Paprocki <[email protected]> wrote:
> I just saw this for the first time using a kernel built from the git
> head. This is all the ACPI related printk info around the warning. If
> more information is needed to figure out what this is, please let me
> know.
>
> [ 0.000000] ACPI: RSDP 000F7CE0, 0024 (r2 RS690 )
> [ 0.000000] ACPI: XSDT 3DFE30C0, 004C (r1 RS690 AWRDACPI 42302E31 AWRD 0)
> [ 0.000000] ACPI: FACP 3DFE7400, 00F4 (r3 RS690 AWRDACPI 42302E31 AWRD 0)
> [ 0.000000] ------------[ cut here ]------------
> [ 0.000000] WARNING: at drivers/acpi/tables/tbfadt.c:348
> acpi_tb_create_local_fadt+0x147/0x2f4()
> [ 0.000000] Pid: 0, comm: swapper Not tainted 2.6.26 #1
> [ 0.000000] [<c01171bb>] warn_on_slowpath+0x3b/0x74
> [ 0.000000] [<c01ede36>] acpi_os_vprintf+0x1d/0x20
> [ 0.000000] [<c01ede46>] acpi_os_printf+0xd/0xe
> [ 0.000000] [<c020120e>] acpi_ut_info+0x21/0x24
> [ 0.000000] [<c0200016>] acpi_tb_print_table_header+0x96/0x9c
> [ 0.000000] [<c020032b>] acpi_tb_create_local_fadt+0x147/0x2f4
> [ 0.000000] [<c0200508>] acpi_tb_parse_fadt+0x30/0x6c
> [ 0.000000] [<c03eeda5>] acpi_tb_parse_root_table+0x264/0x2ab
> [ 0.000000] [<c03ee469>] acpi_table_init+0x14/0x6d
> [ 0.000000] [<c03e3b28>] acpi_boot_table_init+0x2b/0xc3
> [ 0.000000] [<c03dfcf4>] setup_arch+0x3fa/0x557
> [ 0.000000] [<c03de7a9>] start_kernel+0x41/0x1ec
> [ 0.000000] =======================
> [ 0.000000] ---[ end trace 4eaa2a86a8e2da22 ]---
> [ 0.000000] ACPI: DSDT 3DFE3240, 4146 (r1 RS690 AWRDACPI 1000
> MSFT 100000E)
> [ 0.000000] ACPI: FACS 3DFE0000, 0040
> [ 0.000000] ACPI: SSDT 3DFE7600, 0136 (r1 PTLTD POWERNOW 1 LTP 1)
> [ 0.000000] ACPI: HPET 3DFE7780, 0038 (r1 RS690 AWRDACPI 42302E31 AWRD 98)
> [ 0.000000] ACPI: MCFG 3DFE7800, 003C (r1 RS690 AWRDACPI 42302E31 AWRD 0)
> [ 0.000000] ACPI: APIC 3DFE7540, 0068 (r1 RS690 AWRDACPI 42302E31 AWRD 0)
>
This most definitely broke because of:
commit 01a5bba576b9364b33f61f0cd9fa70c2cf5535e2
Author: Jan Beulich <[email protected]>
Date: Wed Jul 16 23:27:08 2008 +0200
Fix FADT parsing
I added printk()s and this is what is reported here:
printk(KERN_INFO
"xpm1a_event_block bit_width=%d pm1_register_length=%d\n",
acpi_gbl_FADT.xpm1a_event_block.bit_width, pm1_register_length);
acpi_tb_init_generic_address(&acpi_gbl_xpm1a_enable,
pm1_register_length,
(acpi_gbl_FADT.xpm1a_event_block.address +
pm1_register_length));
[ 0.000000] xpm1a_event_block bit_width=8 pm1_register_length=0
The bit width is not % 16, so the following patch addition a few lines
earlier fails:
WARN_ON(ACPI_MOD_16(acpi_gbl_FADT.xpm1a_event_block.bit_width));
Also, I noticed that the patch changed the definition of
acpi_tb_init_generic_address to name the parameter byte_width instead
of bit_width. The declaration at the top of the file and the
documentation still refer to it as bit_width.
I also added printk()s to the first call to
acpi_tb_init_generic_address ~ line 326 and the lengths passed to the
function at that point are:
[ 0.000000] fadt_info_table[i].length=88
[ 0.000000] fadt_info_table[i].length=89
[ 0.000000] fadt_info_table[i].length=93
>I added printk()s and this is what is reported here:
>
> printk(KERN_INFO
> "xpm1a_event_block bit_width=%d pm1_register_length=%d\n",
> acpi_gbl_FADT.xpm1a_event_block.bit_width, pm1_register_length);
> acpi_tb_init_generic_address(&acpi_gbl_xpm1a_enable,
> pm1_register_length,
> (acpi_gbl_FADT.xpm1a_event_block.address +
> pm1_register_length));
>
>[ 0.000000] xpm1a_event_block bit_width=8 pm1_register_length=0
>
>The bit width is not % 16, so the following patch addition a few lines
>earlier fails:
>
> WARN_ON(ACPI_MOD_16(acpi_gbl_FADT.xpm1a_event_block.bit_width));
So it's a firmware bug in the system you saw this on. The specification
is clear about the width being at least 16 bits, and the warning was added
to indicate the problem you now got: Dividing 8 by 16 yields zero for
pm1_register_length, which results in acpi_gbl_xpm1a_enable aliasing
the address of the respective status register. That won't work, hence
the warning.
I'd be hesitant to fix this (as I think we should be allowed to expect
ACPI tables to not be that fundamentally flawed these days), but of
course Len (or others) might be of a different opinion.
>Also, I noticed that the patch changed the definition of
>acpi_tb_init_generic_address to name the parameter byte_width instead
>of bit_width. The declaration at the top of the file and the
>documentation still refer to it as bit_width.
>
>I also added printk()s to the first call to
>acpi_tb_init_generic_address ~ line 326 and the lengths passed to the
>function at that point are:
>[ 0.000000] fadt_info_table[i].length=88
>[ 0.000000] fadt_info_table[i].length=89
>[ 0.000000] fadt_info_table[i].length=93
Hmm, indeed, I didn't notice the (pointless) earlier declaration, I realize
I failed to update the function description. Bob, could you fix this in
ACPICA without the need for me to send a patch against it (assuming
the base patch went into ACPICA)?
Jan
"Andrew Paprocki" <[email protected]> writes:
[cc Bob More. What is your opinion?]
> This most definitely broke because of:
>
> commit 01a5bba576b9364b33f61f0cd9fa70c2cf5535e2
> Author: Jan Beulich <[email protected]>
> Date: Wed Jul 16 23:27:08 2008 +0200
>
> Fix FADT parsing
>
> I added printk()s and this is what is reported here:
Thanks for the detailed analysis.
>
> printk(KERN_INFO
> "xpm1a_event_block bit_width=%d pm1_register_length=%d\n",
> acpi_gbl_FADT.xpm1a_event_block.bit_width, pm1_register_length);
> acpi_tb_init_generic_address(&acpi_gbl_xpm1a_enable,
> pm1_register_length,
> (acpi_gbl_FADT.xpm1a_event_block.address +
> pm1_register_length));
>
> [ 0.000000] xpm1a_event_block bit_width=8 pm1_register_length=0
>
> The bit width is not % 16, so the following patch addition a few lines
> earlier fails:
>
> WARN_ON(ACPI_MOD_16(acpi_gbl_FADT.xpm1a_event_block.bit_width));
I'll queue a revert for now until this can be resolved properly.
-Andi
Jan Beulich wrote:
> So it's a firmware bug in the system you saw this on. The specification
> is clear about the width being at least 16 bits, and the warning was added
> to indicate the problem you now got: Dividing 8 by 16 yields zero for
> pm1_register_length, which results in acpi_gbl_xpm1a_enable aliasing
> the address of the respective status register. That won't work, hence
> the warning.
When there are systems around where this register is 8 bits then we have
to handle it. Real systems beat the specification.
The question is just if the hardware is really 8 bits or if the table
is not just wrong. What does lspci say?
>> Also, I noticed that the patch changed the definition of
>> acpi_tb_init_generic_address to name the parameter byte_width instead
>> of bit_width. The declaration at the top of the file and the
>> documentation still refer to it as bit_width.
>>
>> I also added printk()s to the first call to
>> acpi_tb_init_generic_address ~ line 326 and the lengths passed to the
>> function at that point are:
>> [ 0.000000] fadt_info_table[i].length=88
>> [ 0.000000] fadt_info_table[i].length=89
>> [ 0.000000] fadt_info_table[i].length=93
>
> Hmm, indeed, I didn't notice the (pointless) earlier declaration, I realize
> I failed to update the function description. Bob, could you fix this in
> ACPICA without the need for me to send a patch against it (assuming
> the base patch went into ACPICA)?
No it went directly.
-Andi
>The question is just if the hardware is really 8 bits or if the table
>is not just wrong. What does lspci say?
What would lspci have to do with this? /proc/acpi/fadt (from an older
kernel if the new one fails to come up) would seem to help somewhat,
as that would allow comparing the v1 (byte-width) and v2 (bit-width)
values - if they're out of sync (and they likely are if the system worked
flawlessly with previous kernels), I'd be certain the tables are wrong.
Jan
Jan Beulich wrote:
>> The question is just if the hardware is really 8 bits or if the table
>> is not just wrong. What does lspci say?
>
> What would lspci have to do with this?
When it's an chipset with available data sheets one could check that.
> /proc/acpi/fadt (from an older
> kernel if the new one fails to come up) would seem to help somewhat,
> as that would allow comparing the v1 (byte-width) and v2 (bit-width)
> values - if they're out of sync (and they likely are if the system worked
> flawlessly with previous kernels), I'd be certain the tables are wrong.
Ok, but we can just get that from a table dump.
-Andi
On Thu, Jul 17, 2008 at 8:28 AM, Andi Kleen <[email protected]> wrote:
> Ok, but we can just get that from a table dump.
Output from acpidump is attached.
Also, in this new kernel, the ACPI THRM thermal zone reading for the
CPU is incorrect. In 2.6.26-rc8 read a valid value (e.g. 40C) but now
reports:
[ 0.373987] ACPI: Thermal Zone [THRM] (-127 C)
I'm not sure if that is related to this or if it is another patch. I
quickly scanned the git log and I don't see any recent change to
thermal.c itself.
Thanks,
-Andrew
>>> "Andrew Paprocki" <[email protected]> 17.07.08 15:03 >>>
>On Thu, Jul 17, 2008 at 8:28 AM, Andi Kleen <[email protected]> wrote:
>> Ok, but we can just get that from a table dump.
>
>Output from acpidump is attached.
Just as I suspected - the v1 field says 4 bytes, but the v2 field says 8 bits.
Jan
On Thu, Jul 17, 2008 at 9:58 AM, Jan Beulich <[email protected]> wrote:
>>>> "Andrew Paprocki" <[email protected]> 17.07.08 15:03 >>>
>>On Thu, Jul 17, 2008 at 8:28 AM, Andi Kleen <[email protected]> wrote:
>>> Ok, but we can just get that from a table dump.
>>
>>Output from acpidump is attached.
>
> Just as I suspected - the v1 field says 4 bytes, but the v2 field says 8 bits.
So does the BIOS owner need to fix the table? If you could give me the
exact text to pass along to the mobo mfr, I will forward it and I can
get them to release a new BIOS rev.
Thanks,
-Andrew
>>> "Andrew Paprocki" <[email protected]> 17.07.08 16:32 >>>
>On Thu, Jul 17, 2008 at 9:58 AM, Jan Beulich <[email protected]> wrote:
>>>>> "Andrew Paprocki" <[email protected]> 17.07.08 15:03 >>>
>>>On Thu, Jul 17, 2008 at 8:28 AM, Andi Kleen <[email protected]> wrote:
>>>> Ok, but we can just get that from a table dump.
>>>
>>>Output from acpidump is attached.
>>
>> Just as I suspected - the v1 field says 4 bytes, but the v2 field says 8 bits.
>
>So does the BIOS owner need to fix the table? If you could give me the
>exact text to pass along to the mobo mfr, I will forward it and I can
>get them to release a new BIOS rev.
I'm not sure how else to express what I already said above: They simply
need to get (in ACPI spec terms) the FADT's X_PM1a_EVT_BLK in sync
with PM1_EVT_LEN (and they should at once make sure other
X_PM*_BLK and X_GPE?_BLK fields are in sync with their respective
legacy fields, too - while looking at the dump, it seemed there were more
inconsistencies).
Jan
So far, in the number of the cases like this that I've seen, it's the v2
fields that have problems. Perhaps the heuristic should be something
like "if there is an inconsistency between the v1 and v2 fields, fall
back to v1".
Bob
>-----Original Message-----
>From: Jan Beulich [mailto:[email protected]]
>Sent: Thursday, July 17, 2008 8:30 AM
>To: Andrew Paprocki
>Cc: Moore, Robert; Len Brown; Andrew Morton; Andi Kleen; LKML
>Subject: Re: ACPI WARNING: at
>drivers/acpi/tables/tbfadt.c:348acpi_tb_create_local_fadt+0x147/0x2f4()
>
>>>> "Andrew Paprocki" <[email protected]> 17.07.08 16:32 >>>
>>On Thu, Jul 17, 2008 at 9:58 AM, Jan Beulich <[email protected]>
wrote:
>>>>>> "Andrew Paprocki" <[email protected]> 17.07.08 15:03 >>>
>>>>On Thu, Jul 17, 2008 at 8:28 AM, Andi Kleen <[email protected]>
wrote:
>>>>> Ok, but we can just get that from a table dump.
>>>>
>>>>Output from acpidump is attached.
>>>
>>> Just as I suspected - the v1 field says 4 bytes, but the v2 field
says 8
>bits.
>>
>>So does the BIOS owner need to fix the table? If you could give me the
>>exact text to pass along to the mobo mfr, I will forward it and I can
>>get them to release a new BIOS rev.
>
>I'm not sure how else to express what I already said above: They simply
>need to get (in ACPI spec terms) the FADT's X_PM1a_EVT_BLK in sync
>with PM1_EVT_LEN (and they should at once make sure other
>X_PM*_BLK and X_GPE?_BLK fields are in sync with their respective
>legacy fields, too - while looking at the dump, it seemed there were
more
>inconsistencies).
>
>Jan
Moore, Robert wrote:
> So far, in the number of the cases like this that I've seen, it's the v2
> fields that have problems. Perhaps the heuristic should be something
> like "if there is an inconsistency between the v1 and v2 fields, fall
> back to v1".
Yes that makes sense. Jan could you revise the patch to do that please?
Thanks,
-Andi
>>> Andi Kleen <[email protected]> 17.07.08 19:40 >>>
>Moore, Robert wrote:
>> So far, in the number of the cases like this that I've seen, it's the v2
>> fields that have problems. Perhaps the heuristic should be something
>> like "if there is an inconsistency between the v1 and v2 fields, fall
>> back to v1".
>
>Yes that makes sense. Jan could you revise the patch to do that please?
Sure, soon.
Jan
>>> "Moore, Robert" <[email protected]> 17.07.08 19:20 >>>
>So far, in the number of the cases like this that I've seen, it's the v2
>fields that have problems. Perhaps the heuristic should be something
>like "if there is an inconsistency between the v1 and v2 fields, fall
>back to v1".
While extending the patch to do so, I realize that other v2 fields are
used as-is, no matter whether their bit_width (or other fields) are
wrong. Is that perhaps why hardware/hwregs.c uses hard-coded
constants rather than the specified widths? If so (and if the v1 fields
are considered reliable), shouldn't the v2 ones be sanity-checked
against the v1 ones and then the specified widths be used as intended
by the spec?
Jan
>>> "Moore, Robert" <[email protected]> 17.07.08 19:20 >>>
>So far, in the number of the cases like this that I've seen, it's the v2
>fields that have problems. Perhaps the heuristic should be something
>like "if there is an inconsistency between the v1 and v2 fields, fall
>back to v1".
Here's the updated (i.e. replacement) patch. Andrew P., any chance you
could test this on your system?
The (1.0 inherited) separate length fields in the FADT are byte
granular. Further, PM1a/b may have distinct lengths and live in
distinct address spaces. acpi_tb_convert_fadt() should account for
all of these conditions.
Signed-off-by: Jan Beulich <[email protected]>
---
drivers/acpi/tables/tbfadt.c | 39 ++++++++++++++++++++++++++++++++-------
1 file changed, 32 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-18 10:43:55.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;
}
@@ -343,9 +343,21 @@ static void acpi_tb_convert_fadt(void)
*
* 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)
+ * block. Each is of length (xpm1x_event_block.bit_width/2)
*/
- pm1_register_length = (u8) ACPI_DIV_2(acpi_gbl_FADT.pm1_event_length);
+ if (!ACPI_MOD_16(acpi_gbl_FADT.xpm1a_event_block.bit_width))
+ pm1_register_length = (u8) ACPI_DIV_16(acpi_gbl_FADT
+ .xpm1a_event_block
+ .bit_width);
+ else {
+ printk(KERN_WARNING "FADT: "
+ "X_PM1a_EVT_BLK.bit_width=%u is invalid,"
+ " falling back to 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 +372,26 @@ 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_MOD_16(acpi_gbl_FADT.xpm1b_event_block.bit_width))
+ pm1_register_length = (u8) ACPI_DIV_16(acpi_gbl_FADT
+ .xpm1b_event_block
+ .bit_width);
+ else {
+ printk(KERN_WARNING "FADT: "
+ "X_PM1b_EVT_BLK.bit_width=%u is invalid,"
+ " falling back to PM1_EVT_LEN=%u\n",
+ acpi_gbl_FADT.xpm1b_event_block.bit_width,
+ acpi_gbl_FADT.pm1_event_length);
+ pm1_register_length = (u8) ACPI_DIV_2(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;
}
}
>shouldn't the v2 ones be sanity-checked
>against the v1 ones and then the specified widths be used as intended
>by the spec?
We are still investigating this. Suffice to say that we have not
encountered any hardware that actually uses different values. One of our
BIOS guys is going to determine what windows does with the bit_width
fields. At the same time, we will go ahead and figure out how it handles
the case when the v1 and v2(X) fields are in conflict.
>-----Original Message-----
>From: Jan Beulich [mailto:[email protected]]
>Sent: Friday, July 18, 2008 1:43 AM
>To: Moore, Robert; Andi Kleen
>Cc: Andrew Paprocki; Len Brown; Andrew Morton; LKML
>Subject: RE: ACPI WARNING:
>atdrivers/acpi/tables/tbfadt.c:348acpi_tb_create_local_fadt+0x147/0x2f4
()
>
>>>> "Moore, Robert" <[email protected]> 17.07.08 19:20 >>>
>>So far, in the number of the cases like this that I've seen, it's the
v2
>>fields that have problems. Perhaps the heuristic should be something
>>like "if there is an inconsistency between the v1 and v2 fields, fall
>>back to v1".
>
>While extending the patch to do so, I realize that other v2 fields are
>used as-is, no matter whether their bit_width (or other fields) are
>wrong. Is that perhaps why hardware/hwregs.c uses hard-coded
>constants rather than the specified widths? If so (and if the v1 fields
>are considered reliable), shouldn't the v2 ones be sanity-checked
>against the v1 ones and then the specified widths be used as intended
>by the spec?
>
>Jan