| Message ID | de0e33d2be0924e66a220678a7683098df70c2b5.1592603468.git.gorbak25@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix broken suspend on some machines | expand |
On 20.06.2020 00:00, Grzegorz Uriasz wrote: > @@ -490,8 +497,12 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table) > */ > if (!pmtmr_ioport) { > pmtmr_ioport = fadt->pm_timer_block; > - pmtmr_width = fadt->pm_timer_length == 4 ? 24 : 0; > + pmtmr_width = fadt->pm_timer_length == 4 ? 32 : 0; > } > + if (pmtmr_width < 32 && fadt->flags & ACPI_FADT_32BIT_TIMER) > + printk(KERN_WARNING PREFIX "PM-Timer is too short\n"); Indentation is screwed up here. > --- a/xen/arch/x86/time.c > +++ b/xen/arch/x86/time.c > @@ -457,16 +457,13 @@ static u64 read_pmtimer_count(void) > static s64 __init init_pmtimer(struct platform_timesource *pts) > { > u64 start; > - u32 count, target, mask = 0xffffff; > + u32 count, target, mask; > > - if ( !pmtmr_ioport || !pmtmr_width ) > + if ( !pmtmr_ioport || (pmtmr_width != 24 && pmtmr_width != 32) ) > return 0; > > - if ( pmtmr_width == 32 ) > - { > - pts->counter_bits = 32; > - mask = 0xffffffff; > - } > + pts->counter_bits = pmtmr_width; > + mask = (1ull << pmtmr_width) - 1; "mask" being of "u32" type, the use of 1ull is certainly a little odd here, and one of the reasons why I think this extra "cleanup" would better go in a separate patch. Seeing that besides cosmetic aspects the patch looks okay now, I'd be willing to leave the bigger adjustment mostly as is, albeit I'd prefer the 1ull to go away, by instead switching to e.g. mask = 0xffffffff >> (32 - pmtmr_width); With the adjustments (which I'd be happy to make while committing, provided you agree) Reviewed-by: Jan Beulich <jbeulich@suse.com> Also Cc-ing Paul for a possible release ack (considering this is a backporting candidate). Jan
On Fri, Jun 19, 2020 at 10:00:16PM +0000, Grzegorz Uriasz wrote: > On some computers the bit width of the PM Timer as reported > by ACPI is 32 bits when in fact the FADT flags report correctly > that the timer is 24 bits wide. On affected machines such as the > ASUS FX504GM and never gaming laptops this results in the inability > to resume the machine from suspend. Without this patch suspend is > broken on affected machines and even if a machine manages to resume > correctly then the kernel time and xen timers are trashed. > > Signed-off-by: Grzegorz Uriasz <gorbak25@gmail.com> > --- > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: Wei Liu <wl@xen.org> > Cc: "Roger Pau Monné" <roger.pau@citrix.com> > Cc: marmarek@invisiblethingslab.com > Cc: contact@puzio.waw.pl > Cc: jakub@bartmin.ski > Cc: j.nowak26@student.uw.edu.pl > > Changes in v2: > - Check pm timer access width > - Proper timer width is set when xpm block is not present > - Cleanup timer initialization > > Changes in v3: > - Check pm timer bit offset > - Warn about acpi firmware bugs > - Drop int cast in init_pmtimer > - Merge if's in init_pmtimer > --- > xen/arch/x86/acpi/boot.c | 19 +++++++++++++++---- > xen/arch/x86/time.c | 12 ++++-------- > xen/include/acpi/acmacros.h | 8 ++++++++ > 3 files changed, 27 insertions(+), 12 deletions(-) > > diff --git a/xen/arch/x86/acpi/boot.c b/xen/arch/x86/acpi/boot.c > index bcba52e232..24fd236eb5 100644 > --- a/xen/arch/x86/acpi/boot.c > +++ b/xen/arch/x86/acpi/boot.c > @@ -475,10 +475,17 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table) > > #ifdef CONFIG_X86_PM_TIMER > /* detect the location of the ACPI PM Timer */ > - if (fadt->header.revision >= FADT2_REVISION_ID) { > + if (fadt->header.revision >= FADT2_REVISION_ID && > + fadt->xpm_timer_block.space_id == ACPI_ADR_SPACE_SYSTEM_IO) { > /* FADT rev. 2 */ > - if (fadt->xpm_timer_block.space_id == > - ACPI_ADR_SPACE_SYSTEM_IO) { > + if (fadt->xpm_timer_block.access_width != 0 && > + ACPI_ACCESS_BIT_WIDTH(fadt->xpm_timer_block.access_width) != 32) > + printk(KERN_WARNING PREFIX "PM-Timer has invalid access width(%u)\n", > + fadt->xpm_timer_block.access_width); > + else if (fadt->xpm_timer_block.bit_offset != 0) This should be a plain if instead of an else if, it's possible the block has both the access_width and the bit_offset wrong. > + printk(KERN_WARNING PREFIX "PM-Timer has invalid bit offset(%u)\n", > + fadt->xpm_timer_block.bit_offset); > + else { > pmtmr_ioport = fadt->xpm_timer_block.address; > pmtmr_width = fadt->xpm_timer_block.bit_width; > } > @@ -490,8 +497,12 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table) > */ > if (!pmtmr_ioport) { > pmtmr_ioport = fadt->pm_timer_block; > - pmtmr_width = fadt->pm_timer_length == 4 ? 24 : 0; > + pmtmr_width = fadt->pm_timer_length == 4 ? 32 : 0; > } > + if (pmtmr_width < 32 && fadt->flags & ACPI_FADT_32BIT_TIMER) > + printk(KERN_WARNING PREFIX "PM-Timer is too short\n"); Indentation. Also this should be a fatal error likely? You should set pmtmr_ioport = 0? > + if (pmtmr_width > 24 && !(fadt->flags & ACPI_FADT_32BIT_TIMER)) > + pmtmr_width = 24; > if (pmtmr_ioport) > printk(KERN_INFO PREFIX "PM-Timer IO Port: %#x (%u bits)\n", > pmtmr_ioport, pmtmr_width); > diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c > index d643590c0a..8a45514908 100644 > --- a/xen/arch/x86/time.c > +++ b/xen/arch/x86/time.c > @@ -457,16 +457,13 @@ static u64 read_pmtimer_count(void) > static s64 __init init_pmtimer(struct platform_timesource *pts) > { > u64 start; > - u32 count, target, mask = 0xffffff; > + u32 count, target, mask; > > - if ( !pmtmr_ioport || !pmtmr_width ) > + if ( !pmtmr_ioport || (pmtmr_width != 24 && pmtmr_width != 32) ) > return 0; > > - if ( pmtmr_width == 32 ) > - { > - pts->counter_bits = 32; > - mask = 0xffffffff; > - } > + pts->counter_bits = pmtmr_width; > + mask = (1ull << pmtmr_width) - 1; > > count = inl(pmtmr_ioport) & mask; > start = rdtsc_ordered(); > @@ -486,7 +483,6 @@ static struct platform_timesource __initdata plt_pmtimer = > .name = "ACPI PM Timer", > .frequency = ACPI_PM_FREQUENCY, > .read_counter = read_pmtimer_count, > - .counter_bits = 24, > .init = init_pmtimer > }; > > diff --git a/xen/include/acpi/acmacros.h b/xen/include/acpi/acmacros.h > index 6765535053..86c503c20f 100644 > --- a/xen/include/acpi/acmacros.h > +++ b/xen/include/acpi/acmacros.h > @@ -121,6 +121,14 @@ > #define ACPI_COMPARE_NAME(a,b) (!ACPI_STRNCMP (ACPI_CAST_PTR (char,(a)), ACPI_CAST_PTR (char,(b)), ACPI_NAME_SIZE)) > #endif > > +/* > + * Algorithm to obtain access bit or byte width. > + * Can be used with access_width of struct acpi_generic_address and access_size of > + * struct acpi_resource_generic_register. > + */ > +#define ACPI_ACCESS_BIT_WIDTH(size) (1 << ((size) + 2)) > +#define ACPI_ACCESS_BYTE_WIDTH(size) (1 << ((size) - 1)) Note sure I would introduce this last one, since it's not used by the code. Thanks, Roger.
On Mon, Jun 22, 2020 at 10:54:00AM +0200, Roger Pau Monné wrote: > On Fri, Jun 19, 2020 at 10:00:16PM +0000, Grzegorz Uriasz wrote: > > On some computers the bit width of the PM Timer as reported > > by ACPI is 32 bits when in fact the FADT flags report correctly > > that the timer is 24 bits wide. On affected machines such as the > > ASUS FX504GM and never gaming laptops this results in the inability > > to resume the machine from suspend. Without this patch suspend is > > broken on affected machines and even if a machine manages to resume > > correctly then the kernel time and xen timers are trashed. > > > > Signed-off-by: Grzegorz Uriasz <gorbak25@gmail.com> > > --- > > Cc: Jan Beulich <jbeulich@suse.com> > > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > > Cc: Wei Liu <wl@xen.org> > > Cc: "Roger Pau Monné" <roger.pau@citrix.com> > > Cc: marmarek@invisiblethingslab.com > > Cc: contact@puzio.waw.pl > > Cc: jakub@bartmin.ski > > Cc: j.nowak26@student.uw.edu.pl > > > > Changes in v2: > > - Check pm timer access width > > - Proper timer width is set when xpm block is not present > > - Cleanup timer initialization > > > > Changes in v3: > > - Check pm timer bit offset > > - Warn about acpi firmware bugs > > - Drop int cast in init_pmtimer > > - Merge if's in init_pmtimer > > --- > > xen/arch/x86/acpi/boot.c | 19 +++++++++++++++---- > > xen/arch/x86/time.c | 12 ++++-------- > > xen/include/acpi/acmacros.h | 8 ++++++++ > > 3 files changed, 27 insertions(+), 12 deletions(-) > > > > diff --git a/xen/arch/x86/acpi/boot.c b/xen/arch/x86/acpi/boot.c > > index bcba52e232..24fd236eb5 100644 > > --- a/xen/arch/x86/acpi/boot.c > > +++ b/xen/arch/x86/acpi/boot.c > > @@ -475,10 +475,17 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table) > > > > #ifdef CONFIG_X86_PM_TIMER > > /* detect the location of the ACPI PM Timer */ > > - if (fadt->header.revision >= FADT2_REVISION_ID) { > > + if (fadt->header.revision >= FADT2_REVISION_ID && > > + fadt->xpm_timer_block.space_id == ACPI_ADR_SPACE_SYSTEM_IO) { > > /* FADT rev. 2 */ > > - if (fadt->xpm_timer_block.space_id == > > - ACPI_ADR_SPACE_SYSTEM_IO) { > > + if (fadt->xpm_timer_block.access_width != 0 && > > + ACPI_ACCESS_BIT_WIDTH(fadt->xpm_timer_block.access_width) != 32) > > + printk(KERN_WARNING PREFIX "PM-Timer has invalid access width(%u)\n", > > + fadt->xpm_timer_block.access_width); > > + else if (fadt->xpm_timer_block.bit_offset != 0) > > This should be a plain if instead of an else if, it's possible the > block has both the access_width and the bit_offset wrong. My bad, realized this avoids setting pmtmr_ioport. Roger.
On 22.06.2020 10:54, Roger Pau Monné wrote: > On Fri, Jun 19, 2020 at 10:00:16PM +0000, Grzegorz Uriasz wrote: >> @@ -490,8 +497,12 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table) >> */ >> if (!pmtmr_ioport) { >> pmtmr_ioport = fadt->pm_timer_block; >> - pmtmr_width = fadt->pm_timer_length == 4 ? 24 : 0; >> + pmtmr_width = fadt->pm_timer_length == 4 ? 32 : 0; >> } >> + if (pmtmr_width < 32 && fadt->flags & ACPI_FADT_32BIT_TIMER) >> + printk(KERN_WARNING PREFIX "PM-Timer is too short\n"); > > Indentation. Also this should be a fatal error likely? You should set > pmtmr_ioport = 0? Not sure, to be honest. It's entirely fuzzy what mode to use in this case, yet assuming a working 24-bit timer looks valid in this case. And indeed we'd use the timer (with a 24-bit mask) exactly when pmtmr_width == 24 (alongside the bogus set ACPI_FADT_32BIT_TIMER bit). What I do notice only now though is that inside the if() there's a pair of parentheses missing. Jan
On 22.06.2020 10:49, Jan Beulich wrote: > On 20.06.2020 00:00, Grzegorz Uriasz wrote: >> @@ -490,8 +497,12 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table) >> */ >> if (!pmtmr_ioport) { >> pmtmr_ioport = fadt->pm_timer_block; >> - pmtmr_width = fadt->pm_timer_length == 4 ? 24 : 0; >> + pmtmr_width = fadt->pm_timer_length == 4 ? 32 : 0; >> } >> + if (pmtmr_width < 32 && fadt->flags & ACPI_FADT_32BIT_TIMER) >> + printk(KERN_WARNING PREFIX "PM-Timer is too short\n"); > > Indentation is screwed up here. > >> --- a/xen/arch/x86/time.c >> +++ b/xen/arch/x86/time.c >> @@ -457,16 +457,13 @@ static u64 read_pmtimer_count(void) >> static s64 __init init_pmtimer(struct platform_timesource *pts) >> { >> u64 start; >> - u32 count, target, mask = 0xffffff; >> + u32 count, target, mask; >> >> - if ( !pmtmr_ioport || !pmtmr_width ) >> + if ( !pmtmr_ioport || (pmtmr_width != 24 && pmtmr_width != 32) ) >> return 0; >> >> - if ( pmtmr_width == 32 ) >> - { >> - pts->counter_bits = 32; >> - mask = 0xffffffff; >> - } >> + pts->counter_bits = pmtmr_width; >> + mask = (1ull << pmtmr_width) - 1; > > "mask" being of "u32" type, the use of 1ull is certainly a little odd > here, and one of the reasons why I think this extra "cleanup" would > better go in a separate patch. > > Seeing that besides cosmetic aspects the patch looks okay now, I'd be > willing to leave the bigger adjustment mostly as is, albeit I'd > prefer the 1ull to go away, by instead switching to e.g. > > mask = 0xffffffff >> (32 - pmtmr_width); > > With the adjustments (which I'd be happy to make while committing, > provided you agree) > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > Also Cc-ing Paul for a possible release ack (considering this is a > backporting candidate). Paul? Thanks, Jan
> -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 24 June 2020 16:32 > To: Paul Durrant <paul@xen.org> > Cc: Grzegorz Uriasz <gorbak25@gmail.com>; Wei Liu <wl@xen.org>; jakub@bartmin.ski; Andrew Cooper > <andrew.cooper3@citrix.com>; marmarek@invisiblethingslab.com; j.nowak26@student.uw.edu.pl; xen- > devel@lists.xenproject.org; contact@puzio.waw.pl; Roger Pau Monné <roger.pau@citrix.com> > Subject: Ping: [PATCH v3 1/1] x86/acpi: Use FADT flags to determine the PMTMR width > > On 22.06.2020 10:49, Jan Beulich wrote: > > On 20.06.2020 00:00, Grzegorz Uriasz wrote: > >> @@ -490,8 +497,12 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table) > >> */ > >> if (!pmtmr_ioport) { > >> pmtmr_ioport = fadt->pm_timer_block; > >> - pmtmr_width = fadt->pm_timer_length == 4 ? 24 : 0; > >> + pmtmr_width = fadt->pm_timer_length == 4 ? 32 : 0; > >> } > >> + if (pmtmr_width < 32 && fadt->flags & ACPI_FADT_32BIT_TIMER) > >> + printk(KERN_WARNING PREFIX "PM-Timer is too short\n"); > > > > Indentation is screwed up here. > > > >> --- a/xen/arch/x86/time.c > >> +++ b/xen/arch/x86/time.c > >> @@ -457,16 +457,13 @@ static u64 read_pmtimer_count(void) > >> static s64 __init init_pmtimer(struct platform_timesource *pts) > >> { > >> u64 start; > >> - u32 count, target, mask = 0xffffff; > >> + u32 count, target, mask; > >> > >> - if ( !pmtmr_ioport || !pmtmr_width ) > >> + if ( !pmtmr_ioport || (pmtmr_width != 24 && pmtmr_width != 32) ) > >> return 0; > >> > >> - if ( pmtmr_width == 32 ) > >> - { > >> - pts->counter_bits = 32; > >> - mask = 0xffffffff; > >> - } > >> + pts->counter_bits = pmtmr_width; > >> + mask = (1ull << pmtmr_width) - 1; > > > > "mask" being of "u32" type, the use of 1ull is certainly a little odd > > here, and one of the reasons why I think this extra "cleanup" would > > better go in a separate patch. > > > > Seeing that besides cosmetic aspects the patch looks okay now, I'd be > > willing to leave the bigger adjustment mostly as is, albeit I'd > > prefer the 1ull to go away, by instead switching to e.g. > > > > mask = 0xffffffff >> (32 - pmtmr_width); > > > > With the adjustments (which I'd be happy to make while committing, > > provided you agree) > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > > > Also Cc-ing Paul for a possible release ack (considering this is a > > backporting candidate). > > Paul? > Looks ok. Release-acked-by: Paul Durrant <paul@xen.org> > Thanks, Jan
diff --git a/xen/arch/x86/acpi/boot.c b/xen/arch/x86/acpi/boot.c index bcba52e232..24fd236eb5 100644 --- a/xen/arch/x86/acpi/boot.c +++ b/xen/arch/x86/acpi/boot.c @@ -475,10 +475,17 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table) #ifdef CONFIG_X86_PM_TIMER /* detect the location of the ACPI PM Timer */ - if (fadt->header.revision >= FADT2_REVISION_ID) { + if (fadt->header.revision >= FADT2_REVISION_ID && + fadt->xpm_timer_block.space_id == ACPI_ADR_SPACE_SYSTEM_IO) { /* FADT rev. 2 */ - if (fadt->xpm_timer_block.space_id == - ACPI_ADR_SPACE_SYSTEM_IO) { + if (fadt->xpm_timer_block.access_width != 0 && + ACPI_ACCESS_BIT_WIDTH(fadt->xpm_timer_block.access_width) != 32) + printk(KERN_WARNING PREFIX "PM-Timer has invalid access width(%u)\n", + fadt->xpm_timer_block.access_width); + else if (fadt->xpm_timer_block.bit_offset != 0) + printk(KERN_WARNING PREFIX "PM-Timer has invalid bit offset(%u)\n", + fadt->xpm_timer_block.bit_offset); + else { pmtmr_ioport = fadt->xpm_timer_block.address; pmtmr_width = fadt->xpm_timer_block.bit_width; } @@ -490,8 +497,12 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table) */ if (!pmtmr_ioport) { pmtmr_ioport = fadt->pm_timer_block; - pmtmr_width = fadt->pm_timer_length == 4 ? 24 : 0; + pmtmr_width = fadt->pm_timer_length == 4 ? 32 : 0; } + if (pmtmr_width < 32 && fadt->flags & ACPI_FADT_32BIT_TIMER) + printk(KERN_WARNING PREFIX "PM-Timer is too short\n"); + if (pmtmr_width > 24 && !(fadt->flags & ACPI_FADT_32BIT_TIMER)) + pmtmr_width = 24; if (pmtmr_ioport) printk(KERN_INFO PREFIX "PM-Timer IO Port: %#x (%u bits)\n", pmtmr_ioport, pmtmr_width); diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c index d643590c0a..8a45514908 100644 --- a/xen/arch/x86/time.c +++ b/xen/arch/x86/time.c @@ -457,16 +457,13 @@ static u64 read_pmtimer_count(void) static s64 __init init_pmtimer(struct platform_timesource *pts) { u64 start; - u32 count, target, mask = 0xffffff; + u32 count, target, mask; - if ( !pmtmr_ioport || !pmtmr_width ) + if ( !pmtmr_ioport || (pmtmr_width != 24 && pmtmr_width != 32) ) return 0; - if ( pmtmr_width == 32 ) - { - pts->counter_bits = 32; - mask = 0xffffffff; - } + pts->counter_bits = pmtmr_width; + mask = (1ull << pmtmr_width) - 1; count = inl(pmtmr_ioport) & mask; start = rdtsc_ordered(); @@ -486,7 +483,6 @@ static struct platform_timesource __initdata plt_pmtimer = .name = "ACPI PM Timer", .frequency = ACPI_PM_FREQUENCY, .read_counter = read_pmtimer_count, - .counter_bits = 24, .init = init_pmtimer }; diff --git a/xen/include/acpi/acmacros.h b/xen/include/acpi/acmacros.h index 6765535053..86c503c20f 100644 --- a/xen/include/acpi/acmacros.h +++ b/xen/include/acpi/acmacros.h @@ -121,6 +121,14 @@ #define ACPI_COMPARE_NAME(a,b) (!ACPI_STRNCMP (ACPI_CAST_PTR (char,(a)), ACPI_CAST_PTR (char,(b)), ACPI_NAME_SIZE)) #endif +/* + * Algorithm to obtain access bit or byte width. + * Can be used with access_width of struct acpi_generic_address and access_size of + * struct acpi_resource_generic_register. + */ +#define ACPI_ACCESS_BIT_WIDTH(size) (1 << ((size) + 2)) +#define ACPI_ACCESS_BYTE_WIDTH(size) (1 << ((size) - 1)) + /* * Macros for moving data around to/from buffers that are possibly unaligned. * If the hardware supports the transfer of unaligned data, just do the store.
On some computers the bit width of the PM Timer as reported by ACPI is 32 bits when in fact the FADT flags report correctly that the timer is 24 bits wide. On affected machines such as the ASUS FX504GM and never gaming laptops this results in the inability to resume the machine from suspend. Without this patch suspend is broken on affected machines and even if a machine manages to resume correctly then the kernel time and xen timers are trashed. Signed-off-by: Grzegorz Uriasz <gorbak25@gmail.com> --- Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Wei Liu <wl@xen.org> Cc: "Roger Pau Monné" <roger.pau@citrix.com> Cc: marmarek@invisiblethingslab.com Cc: contact@puzio.waw.pl Cc: jakub@bartmin.ski Cc: j.nowak26@student.uw.edu.pl Changes in v2: - Check pm timer access width - Proper timer width is set when xpm block is not present - Cleanup timer initialization Changes in v3: - Check pm timer bit offset - Warn about acpi firmware bugs - Drop int cast in init_pmtimer - Merge if's in init_pmtimer --- xen/arch/x86/acpi/boot.c | 19 +++++++++++++++---- xen/arch/x86/time.c | 12 ++++-------- xen/include/acpi/acmacros.h | 8 ++++++++ 3 files changed, 27 insertions(+), 12 deletions(-)