* [patch 02/10] acpi: fix FADT parsing
@ 2008-05-01 9:52 akpm
2008-05-01 17:38 ` Thomas Renninger
0 siblings, 1 reply; 4+ messages in thread
From: akpm @ 2008-05-01 9:52 UTC (permalink / raw)
To: lenb; +Cc: linux-acpi, akpm, jbeulich
From: "Jan Beulich" <jbeulich@novell.com>
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.
Apart from these changes I'm puzzled by the fact that, not just for
acpi_gbl_xpm1{a,b}_enable, acpi_hw_low_level_{read,write}() get an explicit
size passed rather than using the size found in the passed GAS. What happens
on a platform that defines PM1{a,b} wider than 16 bits? Of course,
acpi_hw_low_level_{read,write}() at present are entirely un-prepared to deal
with sizes other than 8, 16, or 32, not to speak of a non-zero bit_offset or
access_width...
Signed-off-by: Jan Beulich <jbeulich@novell.com>
Cc: Len Brown <lenb@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
drivers/acpi/tables/tbfadt.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff -puN drivers/acpi/tables/tbfadt.c~acpi-fix-fadt-parsing drivers/acpi/tables/tbfadt.c
--- a/drivers/acpi/tables/tbfadt.c~acpi-fix-fadt-parsing
+++ a/drivers/acpi/tables/tbfadt.c
@@ -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,11 @@ 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);
+ WARN_ON(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);
/* The PM1A register block is required */
@@ -360,13 +362,17 @@ 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) {
+ WARN_ON(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);
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;
}
}
_
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [patch 02/10] acpi: fix FADT parsing
2008-05-01 9:52 [patch 02/10] acpi: fix FADT parsing akpm
@ 2008-05-01 17:38 ` Thomas Renninger
2008-05-04 9:16 ` Zhao Yakui
0 siblings, 1 reply; 4+ messages in thread
From: Thomas Renninger @ 2008-05-01 17:38 UTC (permalink / raw)
To: akpm, Moore, Robert; +Cc: lenb, linux-acpi, jbeulich, Zhao Yakui
I wonder whether this could be the real fix for the ThinkPads where it
helped to read FADT entries through RSDT instead of XSDT?
Could this be related?
http://bugzilla.kernel.org/show_bug.cgi?id=8246
Hmm, I'll post the patch there, they should give it a try...
Bob, do you mind giving this a review and push it into your acpica tree
if it looks ok to you?
This looks like a pretty important fix.
Thanks,
Thomas
On Thu, 2008-05-01 at 02:52 -0700, akpm@linux-foundation.org wrote:
> From: "Jan Beulich" <jbeulich@novell.com>
>
> 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.
>
> Apart from these changes I'm puzzled by the fact that, not just for
> acpi_gbl_xpm1{a,b}_enable, acpi_hw_low_level_{read,write}() get an explicit
> size passed rather than using the size found in the passed GAS. What happens
> on a platform that defines PM1{a,b} wider than 16 bits? Of course,
> acpi_hw_low_level_{read,write}() at present are entirely un-prepared to deal
> with sizes other than 8, 16, or 32, not to speak of a non-zero bit_offset or
> access_width...
>
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> Cc: Len Brown <lenb@kernel.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
>
> drivers/acpi/tables/tbfadt.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff -puN drivers/acpi/tables/tbfadt.c~acpi-fix-fadt-parsing drivers/acpi/tables/tbfadt.c
> --- a/drivers/acpi/tables/tbfadt.c~acpi-fix-fadt-parsing
> +++ a/drivers/acpi/tables/tbfadt.c
> @@ -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,11 @@ 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);
> + WARN_ON(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);
>
> /* The PM1A register block is required */
>
> @@ -360,13 +362,17 @@ 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) {
> + WARN_ON(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);
> 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-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [patch 02/10] acpi: fix FADT parsing
2008-05-01 17:38 ` Thomas Renninger
@ 2008-05-04 9:16 ` Zhao Yakui
0 siblings, 0 replies; 4+ messages in thread
From: Zhao Yakui @ 2008-05-04 9:16 UTC (permalink / raw)
To: trenn; +Cc: akpm, Moore, Robert, lenb, linux-acpi, jbeulich
On Thu, 2008-05-01 at 19:38 +0200, Thomas Renninger wrote:
> I wonder whether this could be the real fix for the ThinkPads where it
> helped to read FADT entries through RSDT instead of XSDT?
> Could this be related?
> http://bugzilla.kernel.org/show_bug.cgi?id=8246
>
> Hmm, I'll post the patch there, they should give it a try...
>
Although this patch can't fix the issue on the Thinkpads laptop of bug
8246, it fixes the error about the bit_width of generic_address.
> Bob, do you mind giving this a review and push it into your acpica tree
> if it looks ok to you?
> This looks like a pretty important fix.
>
> Thanks,
>
> Thomas
>
> On Thu, 2008-05-01 at 02:52 -0700, akpm@linux-foundation.org wrote:
> > From: "Jan Beulich" <jbeulich@novell.com>
> >
> > 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.
> >
> > Apart from these changes I'm puzzled by the fact that, not just for
> > acpi_gbl_xpm1{a,b}_enable, acpi_hw_low_level_{read,write}() get an explicit
> > size passed rather than using the size found in the passed GAS. What happens
> > on a platform that defines PM1{a,b} wider than 16 bits? Of course,
> > acpi_hw_low_level_{read,write}() at present are entirely un-prepared to deal
> > with sizes other than 8, 16, or 32, not to speak of a non-zero bit_offset or
> > access_width...
> >
> > Signed-off-by: Jan Beulich <jbeulich@novell.com>
> > Cc: Len Brown <lenb@kernel.org>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > ---
> >
> > drivers/acpi/tables/tbfadt.c | 16 +++++++++++-----
> > 1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff -puN drivers/acpi/tables/tbfadt.c~acpi-fix-fadt-parsing drivers/acpi/tables/tbfadt.c
> > --- a/drivers/acpi/tables/tbfadt.c~acpi-fix-fadt-parsing
> > +++ a/drivers/acpi/tables/tbfadt.c
> > @@ -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,11 @@ 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);
> > + WARN_ON(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);
> >
> > /* The PM1A register block is required */
> >
> > @@ -360,13 +362,17 @@ 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) {
> > + WARN_ON(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);
> > 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-acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* [patch 02/10] acpi: fix FADT parsing
@ 2008-05-02 20:31 akpm
0 siblings, 0 replies; 4+ messages in thread
From: akpm @ 2008-05-02 20:31 UTC (permalink / raw)
To: lenb; +Cc: linux-acpi, akpm, jbeulich
From: "Jan Beulich" <jbeulich@novell.com>
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.
Apart from these changes I'm puzzled by the fact that, not just for
acpi_gbl_xpm1{a,b}_enable, acpi_hw_low_level_{read,write}() get an explicit
size passed rather than using the size found in the passed GAS. What happens
on a platform that defines PM1{a,b} wider than 16 bits? Of course,
acpi_hw_low_level_{read,write}() at present are entirely un-prepared to deal
with sizes other than 8, 16, or 32, not to speak of a non-zero bit_offset or
access_width...
Signed-off-by: Jan Beulich <jbeulich@novell.com>
Cc: Len Brown <lenb@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
drivers/acpi/tables/tbfadt.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff -puN drivers/acpi/tables/tbfadt.c~acpi-fix-fadt-parsing drivers/acpi/tables/tbfadt.c
--- a/drivers/acpi/tables/tbfadt.c~acpi-fix-fadt-parsing
+++ a/drivers/acpi/tables/tbfadt.c
@@ -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,11 @@ 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);
+ WARN_ON(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);
/* The PM1A register block is required */
@@ -360,13 +362,17 @@ 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) {
+ WARN_ON(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);
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;
}
}
_
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2008-05-04 1:25 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-05-01 9:52 [patch 02/10] acpi: fix FADT parsing akpm
2008-05-01 17:38 ` Thomas Renninger
2008-05-04 9:16 ` Zhao Yakui
-- strict thread matches above, loose matches on Subject: below --
2008-05-02 20:31 akpm
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox