Re: acpi: broken suspend to RAM with v4.7-rc1

["Rafael J. Wysocki" <rjw@rjwysocki.net>]

[-- Attachment #1: Type: text/plain, Size: 2551 bytes --]

On Saturday, June 11, 2016 01:49:22 PM Andrey Skvortsov wrote:
> On 10 Jun, Rafael J. Wysocki wrote:
> > On Friday, June 10, 2016 11:32:10 PM Andrey Skvortsov wrote:
> > > Hi,
> > > 
> > > On my laptop (DELL Vostro 1500) in v4.7-rc1 is broken suspend to RAM.
> > > Laptop doesn't finish suspend to RAM process (disks are off, but WiFi
> > > and Power LEDs are still on). The only way to get it out of this
> > > state, is to turn the power off.
> > > 
> > > I've bisected the issue to commit 66b1ed5aa8dd25
> > > [ACPICA: ACPI 2.0, Hardware: Add access_width/bit_offset support for acpi_h=
> > > w_write()].
> > > 
> > > If I revert this commit in v4.7-rc1 (or v4.7-rc2), suspend to RAM is
> > > working again.
> > > 
> > > The cause of this problem is that after this commit write to PM1A Control B=
> > > lock
> > > (16-bit register) is done using two 8-bit writes. If I force this write
> > > to be 16-bit, then all is working as before.
> > > 
> > > To get it working 'access_width' for PM1A Control Block needs to be 2 (16-b=
> > > it), but it's=20
> > > 1 (8-bit).
> > > 
> > > The root of the problem seems to be not the commit 66b1ed5aa8dd25 itself, b=
> > > ut the ACPI
> > > tables in BIOS where wrong access_width comes from. I fixed problem in FACP=
> > >  table,
> > > put it in initrd to override FACP table from BIOS. This fixed the issue,
> > > suspend to RAM is working now again.
> > > 
> > > But I'm not sure whether is this proper fix for this problem.
> > > Is there any place in the kernel, where such ACPI quirks are placed?
> > 
> > Well, if the commit in question caused a problem to happen for you, it also
> > might cause similar problems to happen elsewhere.
> > 
> > It looks like we'll need to revert that commit.
> Hi,
> 
> or maybe to reset access_width AnyAcc from FACP table only for PM1A control
> register or even for all registers? This will fix the issue too.

That's a good idea actually.

> diff --git a/drivers/acpi/acpica/tbfadt.c b/drivers/acpi/acpica/tbfadt.c
> index 6208069..a476e94 100644
> --- a/drivers/acpi/acpica/tbfadt.c
> +++ b/drivers/acpi/acpica/tbfadt.c
> @@ -714,7 +714,14 @@ static void acpi_tb_setup_fadt_registers(void)
>                         }
>                 }
>         }
> 
> +       /*
> +        * Reset access_width in the GAS for PM1A control register to
> +        * undefined value. Because in some cases this field contains
> +        * wrong value.
> +        */
> +       acpi_gbl_FADT.xpm1a_control_block.access_width = 0;

OK, let's see what Bob and Lv think about that.

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]