2012-05-01 06:38:53

by Oleksij Rempel

[permalink] [raw]
Subject: Re: [PATCH] ACPI / PCI: Make _SxD/_SxW check follow ACPI 4.0a spec

On 30.04.2012 23:43, Rafael J. Wysocki wrote:
> On Monday, April 30, 2012, Oleksij Rempel wrote:
>> On 30.04.2012 19:53, Alan Stern wrote:
>>> On Mon, 30 Apr 2012, Bjorn Helgaas wrote:
>>>
>>>> On Sun, Apr 29, 2012 at 2:44 PM, Rafael J. Wysocki<[email protected]> wrote:
>>>>> From: Oleksij Rempel<[email protected]>
>>>>>
>>>>> This patch makes _SxD/_SxW check follow the ACPI 4.0a specification
>>>>> more closely and fixes suspend bug found on ASUS Zenbook UX31E.
>>>>>
>>>>> Some OEM use _SxD fileds do blacklist brocken Dx states.
>>>>> If _SxD/_SxW return values are check before suspend as appropriate,
>>>>> some nasty suspend/resume issues may be avoided.
>>>>>
>>>>> References: https://bugzilla.kernel.org/show_bug.cgi?id=42728
>>>>> Signed-off-by: Oleksij Rempel<[email protected]>
>>>>> Signed-off-by: Rafael J. Wysocki<[email protected]>
>>>>> ---
>>>>>
>>>>> Bjorn, Len,
>>>>>
>>>>> This is -stable material and therefore v3.4 as well, IMO. �Please let me
>>>>> know if one of you can take it or whether you want me to handle it all the
>>>>> way to Linus.
>>>>
>>>> I'm OK with this from a PCI perspective. Most of the change is in
>>>> ACPI, so I propose that either you or Len take care of it.
>>>>
>>>> The second paragraph of the changelog has several typos
>>>> (fileds/fields, do/to, brocken/broken, etc).
>>>
>>> It also turns out that the normal wakeup mechanism doesn't work for the
>>> devices in question. Can this be detected by ACPI? We don't want to
>>> tell userspace that wakeup works when in fact it doesn't.
>>
>> hm... how about using pci config and acpi together. PCI config provides
>> map of Dx states and wakeup support of them. If pci says wakeup works
>> only on D0 and D3 and acpi say - we can use only D2 in S3, then there is
>> no wakeup.
>
> Not really. ACPI trumps PCI here, so if ACPI says we can use D2 in S3,
> then we can.
>
> ACPI device states are not the same as PCI device states. They usually map
> to each other directly, but they don't have to.

I mean not just the mapping.
I mean PCI:PME_SUP field. If it PME(D0+,D1-,D2-,D3hot+,D3cold+), and
acpi trying to avoid D3 states for this device. then is is same like
PME(D0+,D1-,D2-)? Or not?

According to spec.:
7.2 Device Power Management Objects (page 287)
_S3D - Highest D-state supported by the device in the S3 state
_S3W - Lowest D-state supported by the device in the S3 state which can
wake the system.
by definition if _S3W is specified then we can assume, the device can
wake? But _SxW is not defined.

Are there any other method to forbid the system use broken state, after
device was actually produced? Usual BIOS flash utility will probably no
rewrite the PCIs EEPROM. Only hope is ACPI, what is correct method to do
define it by ACPI?


2012-05-01 14:11:45

by Alan Stern

[permalink] [raw]
Subject: Re: [PATCH] ACPI / PCI: Make _SxD/_SxW check follow ACPI 4.0a spec

On Tue, 1 May 2012, Rafael J. Wysocki wrote:

> > I mean not just the mapping.
> > I mean PCI:PME_SUP field. If it PME(D0+,D1-,D2-,D3hot+,D3cold+), and
> > acpi trying to avoid D3 states for this device. then is is same like
> > PME(D0+,D1-,D2-)? Or not?
>
> Yes, if _S3D or _S3W are present. If they are not present and _PRW is,
> that means "don't care".
>
> > According to spec.:
> > 7.2 Device Power Management Objects (page 287)
> > _S3D - Highest D-state supported by the device in the S3 state
> > _S3W - Lowest D-state supported by the device in the S3 state which can
> > wake the system.
> > by definition if _S3W is specified then we can assume, the device can
> > wake? But _SxW is not defined.
>
> The device can wake up the system if _PRW is present for it (and for
> PCIe devices even that is not formally necessary).
>
> > Are there any other method to forbid the system use broken state, after
> > device was actually produced? Usual BIOS flash utility will probably no
> > rewrite the PCIs EEPROM. Only hope is ACPI, what is correct method to do
> > define it by ACPI?
>
> Define _S3D that will return 2 (for example) and _PRW returning 3 as the
> deepest sleep state the system may be woken up from. Then, we'll use
> D2 (after the @subject patch).
>
> The drawback is that the kernel will then think the device can wake up
> the system.

There also remains a question about runtime power states and resume.

Oleksij, with your patch, which state does the controller get put into
during runtime suspend, D2 or D3? (You may need to enable runtime
suspend by doing

echo auto >/sys/bus/pci/devices/0000:00:1d.0/power/control

in order to test this.) And if the controller is in runtime suspend,
does it resume correctly when you plug in a new USB device?

I'm pretty sure that without the patch, the controller gets put into D3
and resume does work.

Alan Stern

2012-05-01 14:22:40

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH] ACPI / PCI: Make _SxD/_SxW check follow ACPI 4.0a spec

On Tuesday, May 01, 2012, Oleksij Rempel (fishor) wrote:
> On 30.04.2012 23:43, Rafael J. Wysocki wrote:
> > On Monday, April 30, 2012, Oleksij Rempel wrote:
> >> On 30.04.2012 19:53, Alan Stern wrote:
> >>> On Mon, 30 Apr 2012, Bjorn Helgaas wrote:
> >>>
> >>>> On Sun, Apr 29, 2012 at 2:44 PM, Rafael J. Wysocki<[email protected]> wrote:
> >>>>> From: Oleksij Rempel<[email protected]>
> >>>>>
> >>>>> This patch makes _SxD/_SxW check follow the ACPI 4.0a specification
> >>>>> more closely and fixes suspend bug found on ASUS Zenbook UX31E.
> >>>>>
> >>>>> Some OEM use _SxD fileds do blacklist brocken Dx states.
> >>>>> If _SxD/_SxW return values are check before suspend as appropriate,
> >>>>> some nasty suspend/resume issues may be avoided.
> >>>>>
> >>>>> References: https://bugzilla.kernel.org/show_bug.cgi?id=42728
> >>>>> Signed-off-by: Oleksij Rempel<[email protected]>
> >>>>> Signed-off-by: Rafael J. Wysocki<[email protected]>
> >>>>> ---
> >>>>>
> >>>>> Bjorn, Len,
> >>>>>
> >>>>> This is -stable material and therefore v3.4 as well, IMO. �Please let me
> >>>>> know if one of you can take it or whether you want me to handle it all the
> >>>>> way to Linus.
> >>>>
> >>>> I'm OK with this from a PCI perspective. Most of the change is in
> >>>> ACPI, so I propose that either you or Len take care of it.
> >>>>
> >>>> The second paragraph of the changelog has several typos
> >>>> (fileds/fields, do/to, brocken/broken, etc).
> >>>
> >>> It also turns out that the normal wakeup mechanism doesn't work for the
> >>> devices in question. Can this be detected by ACPI? We don't want to
> >>> tell userspace that wakeup works when in fact it doesn't.
> >>
> >> hm... how about using pci config and acpi together. PCI config provides
> >> map of Dx states and wakeup support of them. If pci says wakeup works
> >> only on D0 and D3 and acpi say - we can use only D2 in S3, then there is
> >> no wakeup.
> >
> > Not really. ACPI trumps PCI here, so if ACPI says we can use D2 in S3,
> > then we can.
> >
> > ACPI device states are not the same as PCI device states. They usually map
> > to each other directly, but they don't have to.
>
> I mean not just the mapping.
> I mean PCI:PME_SUP field. If it PME(D0+,D1-,D2-,D3hot+,D3cold+), and
> acpi trying to avoid D3 states for this device. then is is same like
> PME(D0+,D1-,D2-)? Or not?

Yes, if _S3D or _S3W are present. If they are not present and _PRW is,
that means "don't care".

> According to spec.:
> 7.2 Device Power Management Objects (page 287)
> _S3D - Highest D-state supported by the device in the S3 state
> _S3W - Lowest D-state supported by the device in the S3 state which can
> wake the system.
> by definition if _S3W is specified then we can assume, the device can
> wake? But _SxW is not defined.

The device can wake up the system if _PRW is present for it (and for
PCIe devices even that is not formally necessary).

> Are there any other method to forbid the system use broken state, after
> device was actually produced? Usual BIOS flash utility will probably no
> rewrite the PCIs EEPROM. Only hope is ACPI, what is correct method to do
> define it by ACPI?

Define _S3D that will return 2 (for example) and _PRW returning 3 as the
deepest sleep state the system may be woken up from. Then, we'll use
D2 (after the @subject patch).

The drawback is that the kernel will then think the device can wake up
the system.

Thanks,
Rafael

2012-05-01 16:27:23

by Oleksij Rempel

[permalink] [raw]
Subject: Re: [PATCH] ACPI / PCI: Make _SxD/_SxW check follow ACPI 4.0a spec

On 01.05.2012 16:11, Alan Stern wrote:
> On Tue, 1 May 2012, Rafael J. Wysocki wrote:
>
>>> I mean not just the mapping.
>>> I mean PCI:PME_SUP field. If it PME(D0+,D1-,D2-,D3hot+,D3cold+), and
>>> acpi trying to avoid D3 states for this device. then is is same like
>>> PME(D0+,D1-,D2-)? Or not?
>>
>> Yes, if _S3D or _S3W are present. If they are not present and _PRW is,
>> that means "don't care".
>>
>>> According to spec.:
>>> 7.2 Device Power Management Objects (page 287)
>>> _S3D - Highest D-state supported by the device in the S3 state
>>> _S3W - Lowest D-state supported by the device in the S3 state which can
>>> wake the system.
>>> by definition if _S3W is specified then we can assume, the device can
>>> wake? But _SxW is not defined.
>>
>> The device can wake up the system if _PRW is present for it (and for
>> PCIe devices even that is not formally necessary).
>>
>>> Are there any other method to forbid the system use broken state, after
>>> device was actually produced? Usual BIOS flash utility will probably no
>>> rewrite the PCIs EEPROM. Only hope is ACPI, what is correct method to do
>>> define it by ACPI?
>>
>> Define _S3D that will return 2 (for example) and _PRW returning 3 as the
>> deepest sleep state the system may be woken up from. Then, we'll use
>> D2 (after the @subject patch).
>>
>> The drawback is that the kernel will then think the device can wake up
>> the system.
>
> There also remains a question about runtime power states and resume.
>
> Oleksij, with your patch, which state does the controller get put into
> during runtime suspend, D2 or D3? (You may need to enable runtime
> suspend by doing
>
> echo auto>/sys/bus/pci/devices/0000:00:1d.0/power/control
>
> in order to test this.) And if the controller is in runtime suspend,
> does it resume correctly when you plug in a new USB device?
>
> I'm pretty sure that without the patch, the controller gets put into D3
> and resume does work.

I do not know if device really suspended, but every thing works like
before. New usb devices are recognized and working.

2012-05-01 16:59:38

by Alan Stern

[permalink] [raw]
Subject: Re: [PATCH] ACPI / PCI: Make _SxD/_SxW check follow ACPI 4.0a spec

On Tue, 1 May 2012, Oleksij Rempel (fishor) wrote:

> > There also remains a question about runtime power states and resume.
> >
> > Oleksij, with your patch, which state does the controller get put into
> > during runtime suspend, D2 or D3? (You may need to enable runtime
> > suspend by doing
> >
> > echo auto>/sys/bus/pci/devices/0000:00:1d.0/power/control
> >
> > in order to test this.) And if the controller is in runtime suspend,
> > does it resume correctly when you plug in a new USB device?
> >
> > I'm pretty sure that without the patch, the controller gets put into D3
> > and resume does work.
>
> I do not know if device really suspended, but every thing works like
> before. New usb devices are recognized and working.

You ought to be able to tell the controller's state by looking at the
dmesg log (after doing the "echo" command above) or the output from
lspci -v.

Alan Stern

2012-05-02 04:10:42

by Oleksij Rempel

[permalink] [raw]
Subject: Re: [PATCH] ACPI / PCI: Make _SxD/_SxW check follow ACPI 4.0a spec

On 01.05.2012 18:59, Alan Stern wrote:
> On Tue, 1 May 2012, Oleksij Rempel (fishor) wrote:
>
>>> There also remains a question about runtime power states and resume.
>>>
>>> Oleksij, with your patch, which state does the controller get put into
>>> during runtime suspend, D2 or D3? (You may need to enable runtime
>>> suspend by doing
>>>
>>> echo auto>/sys/bus/pci/devices/0000:00:1d.0/power/control
>>>
>>> in order to test this.) And if the controller is in runtime suspend,
>>> does it resume correctly when you plug in a new USB device?
>>>
>>> I'm pretty sure that without the patch, the controller gets put into D3
>>> and resume does work.
>>
>> I do not know if device really suspended, but every thing works like
>> before. New usb devices are recognized and working.
>
> You ought to be able to tell the controller's state by looking at the
> dmesg log (after doing the "echo" command above) or the output from
> lspci -v.
>
> Alan Stern
>

looks like it stay in D0 after echo auto > contreol, or even after
"echo 0000:00:1d.0 > /sys/bus/pci/drivers/ehci_hcd/unbind"
Only difference i got is "AFStatus: TP+" after last command changed to
"TP-". Devices are powersupplied, but not recognized by the OS.


lspci -vvs 00:1d.0
00:1d.0 USB controller: Intel Corporation 6 Series/C200 Series Chipset
Family USB Enhanced Host Controller #1 (rev 05) (prog-if 20 [EHCI])
Subsystem: ASUSTeK Computer Inc. Device 1427
Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
Stepping- SERR- FastB2B- DisINTx-
Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort-
<TAbort- <MAbort- >SERR- <PERR- INTx-
Latency: 0
Interrupt: pin A routed to IRQ 23
Region 0: Memory at dfe07000 (32-bit, non-prefetchable) [size=1K]
Capabilities: [50] Power Management version 2
Flags: PMEClk- DSI- D1- D2- AuxCurrent=375mA
PME(D0+,D1-,D2-,D3hot+,D3cold+)
Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
Capabilities: [58] Debug port: BAR=1 offset=00a0
Capabilities: [98] PCI Advanced Features
AFCap: TP+ FLR+
AFCtrl: FLR-
AFStatus: TP+
Kernel driver in use: ehci_hcd
Kernel modules: ehci-hcd