Re: [PATCH v3 1/1] x86/acpi: Use FADT flags to determine the PMTMR width

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxxx>
> > ---
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Cc: Wei Liu <wl@xxxxxxx>
> > Cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> > Cc: marmarek@xxxxxxxxxxxxxxxxxxxxxx
> > Cc: contact@xxxxxxxxxxxx
> > Cc: jakub@xxxxxxxxxxx
> > Cc: j.nowak26@xxxxxxxxxxxxxxxxx
> > 
> > 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.