All of lore.kernel.org
 help / color / mirror / Atom feed
* acpi: broken suspend to RAM with v4.7-rc1
@ 2016-06-10 20:32 Andrey Skvortsov
  2016-06-10 21:24 ` Rafael J. Wysocki
  0 siblings, 1 reply; 23+ messages in thread
From: Andrey Skvortsov @ 2016-06-10 20:32 UTC (permalink / raw)
  To: Lv Zheng, Bob Moore, Rafael J. Wysocki, linux-acpi, devel,
	linux-kernel

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

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_hw_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 Block
(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-bit), but it's 
1 (8-bit).

The root of the problem seems to be not the commit 66b1ed5aa8dd25 itself, but 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?


-- 
Best regards,
Andrey Skvortsov



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [Devel] acpi: broken suspend to RAM with v4.7-rc1
  2016-06-27  8:49                   ` Andrey Skvortsov
@ 2016-06-28  5:10 ` Zheng, Lv
  -1 siblings, 0 replies; 23+ messages in thread
From: Zheng, Lv @ 2016-06-28  5:10 UTC (permalink / raw)
  To: devel

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

Hi,

> From: Andrey Skvortsov [mailto:andrej.skvortzov(a)gmail.com]
> Subject: Re: acpi: broken suspend to RAM with v4.7-rc1
> 
> On 27 Jun, Zheng, Lv wrote:
> > Hi,
> >
> > > From: Andrey Skvortsov [mailto:andrej.skvortzov(a)gmail.com]
> > > Subject: Re: acpi: broken suspend to RAM with v4.7-rc1
> > >
> > > On 24 Jun, Zheng, Lv wrote:
> > > > Hi,
> > > >
> > > > > From: Andrey Skvortsov [mailto:andrej.skvortzov(a)gmail.com]
> > > > > Subject: Re: acpi: broken suspend to RAM with v4.7-rc1
> > > > >
> > > > > Hi Lv,
> > > > >
> > > > > On 13 Jun, Zheng, Lv wrote:
> > > > > > > From: linux-acpi-owner(a)vger.kernel.org [mailto:linux-acpi-
> > > > > > > owner(a)vger.kernel.org] On Behalf Of Rafael J. Wysocki
> > > > > > > Subject: Re: acpi: broken suspend to RAM with v4.7-rc1
> > > > > > >
> > > > > > > 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_hw_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 Block (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-bit), but it's 1 (8-bit).
> > > > > > [Lv Zheng]
> > > > > > Could you send me the acpidump of the machine?
> > > > > Here it is
> > > > >
> > >
> https://dl.dropboxusercontent.com/u/24023626/dell_vostro_1500.acpid
> > > > > ump.bin
> > > > [Lv Zheng]
> > > > I've been trying to download it these days but all failed.
> > > > Could you send an off-list email to me with this attached?
> > > Strange. I've check now. The link above is working, but I see that
> > > part of the link above is moved to the next line.
> > [Lv Zheng]
> > Maybe this is just because of ISP firewall.
> >
> > > Anyway I resend you all files off-list.
> > [Lv Zheng]
> > Great!
> >
> > >
> > >
> > > > > > > > > > The root of the problem seems to be not the commit
> > > > > > > 66b1ed5aa8dd25
> > > > > > > > > > itself, but 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?
> > > > > > [Lv Zheng]
> > > > > > My question would be:
> > > > > > Does Windows behave correctly for this table?
> > > > > Yes, suspend to RAM is working under Windows Vista.
> > > > > IIRC it worked under Windows XP too.
> > > > >
> > > > > > However there is a real case showing that there are real tables
> need
> > > us to
> > > > > correctly support BitWidth/BitOffset.
> > > > > > Here is the information for you to refer:
> > > > > >
> > >
> http://permalink.gmane.org/gmane.linux.kernel.commits.head/313870
> > > > > >
> > > > > > > > >
> > > > > > > > > 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.
> > > > > > [Lv Zheng]
> > > > > > There is a commit in 4.7-rc2:
> > > > > >
> > > > >
> > >
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=
> > > > > 7f9bef9d
> > > > > > Have you tried if the problem still exists in 4.7-rc2?
> > > > > I've just tried v4.7-rc3. It contains commit 7f9bef9d and the
> problem
> > > > > exists there too.
> > > > [Lv Zheng]
> > > > IMO, for the time being, you can use quirks.
> > > > Booting your kernel with the following parameters:
> > > >
> > > > acpi=rsdt
> > > > Or
> > > > acpi_force_32bit_fadt_addr
> > > > Or
> > > > Both
> > >
> > > Rafael reverted commit, so I'm ok now.
> > >
> > > Actually acpi_force_32bit_fadt_addr will not help here, because it's
> take
> > > effect only if address64 != address32. But here these addresses are
> > > the same, therefore access_width is taken from extended address.
> > >
> > > http://lxr.free-
> electrons.com/source/drivers/acpi/acpica/tbfadt.c#L576
> > [Lv Zheng]
> > In addition to the address check, we may add access width check here.
> > I need to check this with the decision makers.
> Make sense. But then it's necessary to set default access width for
> registers from
> FADT for this check. Because in the old 32-bit part of FADT only address
> and
> length are defined, but not access width.
> 
> 
> > > acpi=rsdt helps. Thanks for the information about this option. I
> > > missed it, when I read documentation.
> > [Lv Zheng]
> > Great to know that at least 1 quirk works.
> > Back to this bug, it seems we should use fixed access_width for some
> pre-defined PM registers.
> This is what I meant by suggesting to reset xpm1a's
> access_width to zero (or maybe another registers from FADT too) in
> acpi_tb_setup_fadt_registers (see quoted diff above from my previous
> answer).
> If access_width is zero, then code works as before and access_width is
> calculated by acpi_hw_get_access_bit_width (will return reg->bit_width or
> max_bit_width(32) in our case).
> 
> Setting access_width to some pre-defined value is an option.
> But it's not as flexible, because ACPI specification doesn't define
> access width and some pre-defined PM registers have variable length
> and only the minimal register length is defined (PM1A Control Reg, PM1B
> Control
> Reg, PM1A event block, PM1B event block).
> 
> Previously you pointed to commit that requires usage of access
> width field.
> http://permalink.gmane.org/gmane.linux.kernel.commits.head/313870
> I see it's related to APEI, therefore setting access_width for
> registers in FADT will not affect it.

[Lv Zheng] 
Yes, I can see your point and it looks reasonable.
For now, I'm going to probe Windows behavior using qemu.

It looks I can achieve this by modifying:
hw/i386/acpi-build.c: http://git.qemu.org/?p=qemu.git;a=blob;f=hw/i386/acpi-build.c
hw/acpi/ich9.c: http://git.qemu.org/?p=qemu.git;a=blob;f=hw/acpi/ich9.c

The result may help us to make more accurate decision.

Thanks and best regards
-Lv

^ permalink raw reply	[flat|nested] 23+ messages in thread
end of thread, other threads:[~2016-08-10 18:13 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-06-10 20:32 acpi: broken suspend to RAM with v4.7-rc1 Andrey Skvortsov
2016-06-10 21:24 ` Rafael J. Wysocki
2016-06-11 10:49   ` Andrey Skvortsov
2016-06-11 11:56     ` Rafael J. Wysocki
2016-06-11 11:56       ` Rafael J. Wysocki
     [not found]       ` <744357E9AAD1214791ACBA4B0B9092633AA54819@SHSMSX101.ccr.corp.intel.com>
2016-06-13  8:50         ` [Devel] " Zheng, Lv
2016-06-13  8:50           ` Zheng, Lv
2016-06-13 10:07           ` Andrey Skvortsov
2016-06-15  0:24             ` Rafael J. Wysocki
2016-06-15  6:05               ` [Devel] " Zheng, Lv
2016-06-15  6:05                 ` Zheng, Lv
2016-06-15 20:02                 ` Andrey Skvortsov
2016-06-24  1:02             ` [Devel] " Zheng, Lv
2016-06-24  1:02               ` Zheng, Lv
2016-06-24 21:32               ` Andrey Skvortsov
2016-06-27  1:17                 ` [Devel] " Zheng, Lv
2016-06-27  1:17                   ` Zheng, Lv
2016-06-27  8:49                   ` Andrey Skvortsov
2016-08-05  1:06                 ` [Devel] " Zheng, Lv
2016-08-05  1:06                   ` Zheng, Lv
2016-08-10 17:51                   ` Andrey Skvortsov
  -- strict thread matches above, loose matches on Subject: below --
2016-06-28  5:10 [Devel] " Zheng, Lv
2016-06-28  5:10 ` Zheng, Lv

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.