[PATCH] ACPI / PCI: Make _SxD/_SxW check follow ACPI 4.0a spec

[PATCH] ACPI / PCI: Make _SxD/_SxW check follow ACPI 4.0a spec

[Rafael J. Wysocki]

From: Oleksij Rempel <bug-track@fisher-privat.net>

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 <bug-track@fisher-privat.net>
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---

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.

Thanks,
Rafael

---
 drivers/acpi/sleep.c |   22 +++++++++++++++++++++-
 drivers/pci/pci.c    |   12 +++++++++++-
 2 files changed, 32 insertions(+), 2 deletions(-)

Index: linux/drivers/acpi/sleep.c
===================================================================
--- linux.orig/drivers/acpi/sleep.c
+++ linux/drivers/acpi/sleep.c
@@ -714,10 +714,30 @@ int acpi_pm_device_sleep_state(struct de
 	 *
 	 * NOTE: We rely on acpi_evaluate_integer() not clobbering the integer
 	 * provided -- that's our fault recovery, we ignore retval.
+	 *
+	 * According to ACPI 4.0a (April 5, 2010), page 294,
+	 * Table 7-7 S3 Action / Result Table, we need to evaluate _SxW in
+	 * addition to _SxD if the device is configured for wakeup:
+	 * Desired Action    | _S3D | _PRW | _S3W | Resultant D-state
+	 * Enter S3          |  N/A |  D/C |  N/A | OSPM decides
+	 * Enter S3, No Wake |   2  |  D/C |  D/C | Enter D2 or D3
+	 * Enter S3, Wake    |   2  |   3  |  N/A | Enter D2
+	 * Enter S3, Wake    |   2  |   3  |   3  | Enter D2 or D3
+	 * Enter S3, Wake    |  N/A |   3  |   2  | Enter D0, D1 or D2
 	 */
-	if (acpi_target_sleep_state > ACPI_STATE_S0)
+	if (acpi_target_sleep_state > ACPI_STATE_S0) {
+		acpi_status status;
+
 		acpi_evaluate_integer(handle, acpi_method, NULL, &d_min);
 
+		if (device_may_wakeup(dev)) {
+			acpi_method[3] = 'W';
+			status = acpi_evaluate_integer(handle, acpi_method,
+						       NULL, &d_max);
+			if (ACPI_FAILURE(status))
+				d_max = d_min;
+		}
+	}
 	/*
 	 * If _PRW says we can wake up the system from the target sleep state,
 	 * the D-state returned by _SxD is sufficient for that (we assume a
Index: linux/drivers/pci/pci.c
===================================================================
--- linux.orig/drivers/pci/pci.c
+++ linux/drivers/pci/pci.c
@@ -1691,10 +1691,20 @@ pci_power_t pci_target_state(struct pci_
 {
 	pci_power_t target_state = PCI_D3hot;
 
-	if (platform_pci_power_manageable(dev)) {
+	/*
+	 * According to ACPI 4.0a,7.2 Device Power Management Objects, device
+	 * with wake capability should have _PRW or _PSW object and can have
+	 * _SxD or _SxW object.
+	 * It looks like some OEMs use this fields to avoid buggy Dx states
+	 * of devices, so we need to check for _PRW or _PSW and see if _SxD or
+	 * _SxW indicate to overwrite Dx.
+	 */
+	if (platform_pci_power_manageable(dev)
+	    || platform_pci_can_wakeup(dev)) {
 		/*
 		 * Call the platform to choose the target state of the device
 		 * and enable wake-up from this state if supported.
+		 * (Check _SxD and _SxW)
 		 */
 		pci_power_t state = platform_pci_choose_state(dev);
 

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

[Bjorn Helgaas]

On Sun, Apr 29, 2012 at 2:44 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> From: Oleksij Rempel <bug-track@fisher-privat.net>
>
> 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 <bug-track@fisher-privat.net>
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>
> 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).

> ---
>  drivers/acpi/sleep.c |   22 +++++++++++++++++++++-
>  drivers/pci/pci.c    |   12 +++++++++++-
>  2 files changed, 32 insertions(+), 2 deletions(-)
>
> Index: linux/drivers/acpi/sleep.c
> ===================================================================
> --- linux.orig/drivers/acpi/sleep.c
> +++ linux/drivers/acpi/sleep.c
> @@ -714,10 +714,30 @@ int acpi_pm_device_sleep_state(struct de
>         *
>         * NOTE: We rely on acpi_evaluate_integer() not clobbering the integer
>         * provided -- that's our fault recovery, we ignore retval.
> +        *
> +        * According to ACPI 4.0a (April 5, 2010), page 294,
> +        * Table 7-7 S3 Action / Result Table, we need to evaluate _SxW in
> +        * addition to _SxD if the device is configured for wakeup:
> +        * Desired Action    | _S3D | _PRW | _S3W | Resultant D-state
> +        * Enter S3          |  N/A |  D/C |  N/A | OSPM decides
> +        * Enter S3, No Wake |   2  |  D/C |  D/C | Enter D2 or D3
> +        * Enter S3, Wake    |   2  |   3  |  N/A | Enter D2
> +        * Enter S3, Wake    |   2  |   3  |   3  | Enter D2 or D3
> +        * Enter S3, Wake    |  N/A |   3  |   2  | Enter D0, D1 or D2
>         */
> -       if (acpi_target_sleep_state > ACPI_STATE_S0)
> +       if (acpi_target_sleep_state > ACPI_STATE_S0) {
> +               acpi_status status;
> +
>                acpi_evaluate_integer(handle, acpi_method, NULL, &d_min);
>
> +               if (device_may_wakeup(dev)) {
> +                       acpi_method[3] = 'W';
> +                       status = acpi_evaluate_integer(handle, acpi_method,
> +                                                      NULL, &d_max);
> +                       if (ACPI_FAILURE(status))
> +                               d_max = d_min;
> +               }
> +       }
>        /*
>         * If _PRW says we can wake up the system from the target sleep state,
>         * the D-state returned by _SxD is sufficient for that (we assume a
> Index: linux/drivers/pci/pci.c
> ===================================================================
> --- linux.orig/drivers/pci/pci.c
> +++ linux/drivers/pci/pci.c
> @@ -1691,10 +1691,20 @@ pci_power_t pci_target_state(struct pci_
>  {
>        pci_power_t target_state = PCI_D3hot;
>
> -       if (platform_pci_power_manageable(dev)) {
> +       /*
> +        * According to ACPI 4.0a,7.2 Device Power Management Objects, device
> +        * with wake capability should have _PRW or _PSW object and can have
> +        * _SxD or _SxW object.
> +        * It looks like some OEMs use this fields to avoid buggy Dx states
> +        * of devices, so we need to check for _PRW or _PSW and see if _SxD or
> +        * _SxW indicate to overwrite Dx.
> +        */
> +       if (platform_pci_power_manageable(dev)
> +           || platform_pci_can_wakeup(dev)) {
>                /*
>                 * Call the platform to choose the target state of the device
>                 * and enable wake-up from this state if supported.
> +                * (Check _SxD and _SxW)
>                 */
>                pci_power_t state = platform_pci_choose_state(dev);
>

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

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 1386 bytes --]

On Mon, 30 Apr 2012, Bjorn Helgaas wrote:

> On Sun, Apr 29, 2012 at 2:44 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > From: Oleksij Rempel <bug-track@fisher-privat.net>
> >
> > 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 <bug-track@fisher-privat.net>
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >
> > 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.

Alan Stern

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

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 1353 bytes --]

On Mon, 30 Apr 2012, Bjorn Helgaas wrote:

> On Sun, Apr 29, 2012 at 2:44 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > From: Oleksij Rempel <bug-track@fisher-privat.net>
> >
> > 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 <bug-track@fisher-privat.net>
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >
> > 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.

Alan Stern

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

[Oleksij Rempel]

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 <rjw@sisk.pl> wrote:
>>> From: Oleksij Rempel <bug-track@fisher-privat.net>
>>>
>>> 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 <bug-track@fisher-privat.net>
>>> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
>>> ---
>>>
>>> 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.

-- 
Regards,
Oleksij

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

[Rafael J. Wysocki]

On Monday, April 30, 2012, Alan Stern wrote:
> On Mon, 30 Apr 2012, Bjorn Helgaas wrote:
> 
> > On Sun, Apr 29, 2012 at 2:44 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > From: Oleksij Rempel <bug-track@fisher-privat.net>
> > >
> > > 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 <bug-track@fisher-privat.net>
> > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > ---
> > >
> > > 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.

No, we don't, but if there's _PRW and/or _DSW/_PSW for the device, we really
have to assume that the device is able to wake up the system.  Moreover, if
those methods don't return error codes, we have no way to say there's anything
wrong.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Rafael J. Wysocki]

On Monday, April 30, 2012, Alan Stern wrote:
> On Mon, 30 Apr 2012, Bjorn Helgaas wrote:
> 
> > On Sun, Apr 29, 2012 at 2:44 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > From: Oleksij Rempel <bug-track@fisher-privat.net>
> > >
> > > 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 <bug-track@fisher-privat.net>
> > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > ---
> > >
> > > 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.

No, we don't, but if there's _PRW and/or _DSW/_PSW for the device, we really
have to assume that the device is able to wake up the system.  Moreover, if
those methods don't return error codes, we have no way to say there's anything
wrong.

Thanks,
Rafael

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

[Bjorn Helgaas]

On Mon, Apr 30, 2012 at 3:41 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Monday, April 30, 2012, Bjorn Helgaas wrote:
>> On Sun, Apr 29, 2012 at 2:44 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > From: Oleksij Rempel <bug-track@fisher-privat.net>
>> >
>> > 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 <bug-track@fisher-privat.net>
>> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
>> > ---
>> >
>> > 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).
>
> Thanks for spotting that, the version below should be better.
>
> I'll wait for Len to respond till Friday and will take care of the patch myself
> if he doesn't say anything.
>
> May I assume an ACK from you?

Yep, sorry :)

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
> From: Oleksij Rempel <bug-track@fisher-privat.net>
> Subject: ACPI / PCI: Make _SxD/_SxW check follow ACPI 4.0a spec
>
> 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 fields to blacklist broken device Dx states, so
> if _SxD/_SxW return values are checked 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 <bug-track@fisher-privat.net>
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/acpi/sleep.c |   22 +++++++++++++++++++++-
>  drivers/pci/pci.c    |   12 +++++++++++-
>  2 files changed, 32 insertions(+), 2 deletions(-)
>
> Index: linux/drivers/acpi/sleep.c
> ===================================================================
> --- linux.orig/drivers/acpi/sleep.c
> +++ linux/drivers/acpi/sleep.c
> @@ -720,10 +720,30 @@ int acpi_pm_device_sleep_state(struct de
>         *
>         * NOTE: We rely on acpi_evaluate_integer() not clobbering the integer
>         * provided -- that's our fault recovery, we ignore retval.
> +        *
> +        * According to ACPI 4.0a (April 5, 2010), page 294,
> +        * Table 7-7 S3 Action / Result Table, we need to evaluate _SxW in
> +        * addition to _SxD if the device is configured for wakeup:
> +        * Desired Action    | _S3D | _PRW | _S3W | Resultant D-state
> +        * Enter S3          |  N/A |  D/C |  N/A | OSPM decides
> +        * Enter S3, No Wake |   2  |  D/C |  D/C | Enter D2 or D3
> +        * Enter S3, Wake    |   2  |   3  |  N/A | Enter D2
> +        * Enter S3, Wake    |   2  |   3  |   3  | Enter D2 or D3
> +        * Enter S3, Wake    |  N/A |   3  |   2  | Enter D0, D1 or D2
>         */
> -       if (acpi_target_sleep_state > ACPI_STATE_S0)
> +       if (acpi_target_sleep_state > ACPI_STATE_S0) {
> +               acpi_status status;
> +
>                acpi_evaluate_integer(handle, acpi_method, NULL, &d_min);
>
> +               if (device_may_wakeup(dev)) {
> +                       acpi_method[3] = 'W';
> +                       status = acpi_evaluate_integer(handle, acpi_method,
> +                                                      NULL, &d_max);
> +                       if (ACPI_FAILURE(status))
> +                               d_max = d_min;
> +               }
> +       }
>        /*
>         * If _PRW says we can wake up the system from the target sleep state,
>         * the D-state returned by _SxD is sufficient for that (we assume a
> Index: linux/drivers/pci/pci.c
> ===================================================================
> --- linux.orig/drivers/pci/pci.c
> +++ linux/drivers/pci/pci.c
> @@ -1691,10 +1691,20 @@ pci_power_t pci_target_state(struct pci_
>  {
>        pci_power_t target_state = PCI_D3hot;
>
> -       if (platform_pci_power_manageable(dev)) {
> +       /*
> +        * According to ACPI 4.0a,7.2 Device Power Management Objects, device
> +        * with wake capability should have _PRW or _PSW object and can have
> +        * _SxD or _SxW object.
> +        * It looks like some OEMs use this fields to avoid buggy Dx states
> +        * of devices, so we need to check for _PRW or _PSW and see if _SxD or
> +        * _SxW indicate to overwrite Dx.
> +        */
> +       if (platform_pci_power_manageable(dev)
> +           || platform_pci_can_wakeup(dev)) {
>                /*
>                 * Call the platform to choose the target state of the device
>                 * and enable wake-up from this state if supported.
> +                * (Check _SxD and _SxW)
>                 */
>                pci_power_t state = platform_pci_choose_state(dev);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bjorn Helgaas]

On Mon, Apr 30, 2012 at 3:41 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Monday, April 30, 2012, Bjorn Helgaas wrote:
>> On Sun, Apr 29, 2012 at 2:44 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > From: Oleksij Rempel <bug-track@fisher-privat.net>
>> >
>> > 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 <bug-track@fisher-privat.net>
>> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
>> > ---
>> >
>> > 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).
>
> Thanks for spotting that, the version below should be better.
>
> I'll wait for Len to respond till Friday and will take care of the patch myself
> if he doesn't say anything.
>
> May I assume an ACK from you?

Yep, sorry :)

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
> From: Oleksij Rempel <bug-track@fisher-privat.net>
> Subject: ACPI / PCI: Make _SxD/_SxW check follow ACPI 4.0a spec
>
> 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 fields to blacklist broken device Dx states, so
> if _SxD/_SxW return values are checked 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 <bug-track@fisher-privat.net>
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/acpi/sleep.c |   22 +++++++++++++++++++++-
>  drivers/pci/pci.c    |   12 +++++++++++-
>  2 files changed, 32 insertions(+), 2 deletions(-)
>
> Index: linux/drivers/acpi/sleep.c
> ===================================================================
> --- linux.orig/drivers/acpi/sleep.c
> +++ linux/drivers/acpi/sleep.c
> @@ -720,10 +720,30 @@ int acpi_pm_device_sleep_state(struct de
>         *
>         * NOTE: We rely on acpi_evaluate_integer() not clobbering the integer
>         * provided -- that's our fault recovery, we ignore retval.
> +        *
> +        * According to ACPI 4.0a (April 5, 2010), page 294,
> +        * Table 7-7 S3 Action / Result Table, we need to evaluate _SxW in
> +        * addition to _SxD if the device is configured for wakeup:
> +        * Desired Action    | _S3D | _PRW | _S3W | Resultant D-state
> +        * Enter S3          |  N/A |  D/C |  N/A | OSPM decides
> +        * Enter S3, No Wake |   2  |  D/C |  D/C | Enter D2 or D3
> +        * Enter S3, Wake    |   2  |   3  |  N/A | Enter D2
> +        * Enter S3, Wake    |   2  |   3  |   3  | Enter D2 or D3
> +        * Enter S3, Wake    |  N/A |   3  |   2  | Enter D0, D1 or D2
>         */
> -       if (acpi_target_sleep_state > ACPI_STATE_S0)
> +       if (acpi_target_sleep_state > ACPI_STATE_S0) {
> +               acpi_status status;
> +
>                acpi_evaluate_integer(handle, acpi_method, NULL, &d_min);
>
> +               if (device_may_wakeup(dev)) {
> +                       acpi_method[3] = 'W';
> +                       status = acpi_evaluate_integer(handle, acpi_method,
> +                                                      NULL, &d_max);
> +                       if (ACPI_FAILURE(status))
> +                               d_max = d_min;
> +               }
> +       }
>        /*
>         * If _PRW says we can wake up the system from the target sleep state,
>         * the D-state returned by _SxD is sufficient for that (we assume a
> Index: linux/drivers/pci/pci.c
> ===================================================================
> --- linux.orig/drivers/pci/pci.c
> +++ linux/drivers/pci/pci.c
> @@ -1691,10 +1691,20 @@ pci_power_t pci_target_state(struct pci_
>  {
>        pci_power_t target_state = PCI_D3hot;
>
> -       if (platform_pci_power_manageable(dev)) {
> +       /*
> +        * According to ACPI 4.0a,7.2 Device Power Management Objects, device
> +        * with wake capability should have _PRW or _PSW object and can have
> +        * _SxD or _SxW object.
> +        * It looks like some OEMs use this fields to avoid buggy Dx states
> +        * of devices, so we need to check for _PRW or _PSW and see if _SxD or
> +        * _SxW indicate to overwrite Dx.
> +        */
> +       if (platform_pci_power_manageable(dev)
> +           || platform_pci_can_wakeup(dev)) {
>                /*
>                 * Call the platform to choose the target state of the device
>                 * and enable wake-up from this state if supported.
> +                * (Check _SxD and _SxW)
>                 */
>                pci_power_t state = platform_pci_choose_state(dev);
>

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

[Rafael J. Wysocki]

On Monday, April 30, 2012, Bjorn Helgaas wrote:
> On Sun, Apr 29, 2012 at 2:44 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > From: Oleksij Rempel <bug-track@fisher-privat.net>
> >
> > 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 <bug-track@fisher-privat.net>
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >
> > 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).

Thanks for spotting that, the version below should be better.

I'll wait for Len to respond till Friday and will take care of the patch myself
if he doesn't say anything.

May I assume an ACK from you?

Rafael

---
From: Oleksij Rempel <bug-track@fisher-privat.net>
Subject: ACPI / PCI: Make _SxD/_SxW check follow ACPI 4.0a spec

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 fields to blacklist broken device Dx states, so
if _SxD/_SxW return values are checked 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 <bug-track@fisher-privat.net>
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/sleep.c |   22 +++++++++++++++++++++-
 drivers/pci/pci.c    |   12 +++++++++++-
 2 files changed, 32 insertions(+), 2 deletions(-)

Index: linux/drivers/acpi/sleep.c
===================================================================
--- linux.orig/drivers/acpi/sleep.c
+++ linux/drivers/acpi/sleep.c
@@ -720,10 +720,30 @@ int acpi_pm_device_sleep_state(struct de
 	 *
 	 * NOTE: We rely on acpi_evaluate_integer() not clobbering the integer
 	 * provided -- that's our fault recovery, we ignore retval.
+	 *
+	 * According to ACPI 4.0a (April 5, 2010), page 294,
+	 * Table 7-7 S3 Action / Result Table, we need to evaluate _SxW in
+	 * addition to _SxD if the device is configured for wakeup:
+	 * Desired Action    | _S3D | _PRW | _S3W | Resultant D-state
+	 * Enter S3          |  N/A |  D/C |  N/A | OSPM decides
+	 * Enter S3, No Wake |   2  |  D/C |  D/C | Enter D2 or D3
+	 * Enter S3, Wake    |   2  |   3  |  N/A | Enter D2
+	 * Enter S3, Wake    |   2  |   3  |   3  | Enter D2 or D3
+	 * Enter S3, Wake    |  N/A |   3  |   2  | Enter D0, D1 or D2
 	 */
-	if (acpi_target_sleep_state > ACPI_STATE_S0)
+	if (acpi_target_sleep_state > ACPI_STATE_S0) {
+		acpi_status status;
+
 		acpi_evaluate_integer(handle, acpi_method, NULL, &d_min);
 
+		if (device_may_wakeup(dev)) {
+			acpi_method[3] = 'W';
+			status = acpi_evaluate_integer(handle, acpi_method,
+						       NULL, &d_max);
+			if (ACPI_FAILURE(status))
+				d_max = d_min;
+		}
+	}
 	/*
 	 * If _PRW says we can wake up the system from the target sleep state,
 	 * the D-state returned by _SxD is sufficient for that (we assume a
Index: linux/drivers/pci/pci.c
===================================================================
--- linux.orig/drivers/pci/pci.c
+++ linux/drivers/pci/pci.c
@@ -1691,10 +1691,20 @@ pci_power_t pci_target_state(struct pci_
 {
 	pci_power_t target_state = PCI_D3hot;
 
-	if (platform_pci_power_manageable(dev)) {
+	/*
+	 * According to ACPI 4.0a,7.2 Device Power Management Objects, device
+	 * with wake capability should have _PRW or _PSW object and can have
+	 * _SxD or _SxW object.
+	 * It looks like some OEMs use this fields to avoid buggy Dx states
+	 * of devices, so we need to check for _PRW or _PSW and see if _SxD or
+	 * _SxW indicate to overwrite Dx.
+	 */
+	if (platform_pci_power_manageable(dev)
+	    || platform_pci_can_wakeup(dev)) {
 		/*
 		 * Call the platform to choose the target state of the device
 		 * and enable wake-up from this state if supported.
+		 * (Check _SxD and _SxW)
 		 */
 		pci_power_t state = platform_pci_choose_state(dev);
 

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

[Rafael J. Wysocki]

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 <rjw@sisk.pl> wrote:
> >>> From: Oleksij Rempel <bug-track@fisher-privat.net>
> >>>
> >>> 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 <bug-track@fisher-privat.net>
> >>> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> >>> ---
> >>>
> >>> 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.

Thanks,
Rafael

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

[Oleksij Rempel (fishor)]

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<rjw@sisk.pl>  wrote:
>>>>> From: Oleksij Rempel<bug-track@fisher-privat.net>
>>>>>
>>>>> 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<bug-track@fisher-privat.net>
>>>>> Signed-off-by: Rafael J. Wysocki<rjw@sisk.pl>
>>>>> ---
>>>>>
>>>>> 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?

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

[Rafael J. Wysocki]

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<rjw@sisk.pl>  wrote:
> >>>>> From: Oleksij Rempel<bug-track@fisher-privat.net>
> >>>>>
> >>>>> 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<bug-track@fisher-privat.net>
> >>>>> Signed-off-by: Rafael J. Wysocki<rjw@sisk.pl>
> >>>>> ---
> >>>>>
> >>>>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Rafael J. Wysocki]

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<rjw@sisk.pl>  wrote:
> >>>>> From: Oleksij Rempel<bug-track@fisher-privat.net>
> >>>>>
> >>>>> 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<bug-track@fisher-privat.net>
> >>>>> Signed-off-by: Rafael J. Wysocki<rjw@sisk.pl>
> >>>>> ---
> >>>>>
> >>>>> 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

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

[Alan Stern]

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

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

[Alan Stern]

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

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

[Oleksij Rempel (fishor)]

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.

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

[Alan Stern]

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

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

[Alan Stern]

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

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

[Oleksij Rempel (fishor)]

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

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

[Rafael J. Wysocki]

On Monday, April 30, 2012, Rafael J. Wysocki wrote:
> On Monday, April 30, 2012, Bjorn Helgaas wrote:
> > On Sun, Apr 29, 2012 at 2:44 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > From: Oleksij Rempel <bug-track@fisher-privat.net>
> > >
> > > 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 <bug-track@fisher-privat.net>
> > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > ---
> > >
> > > 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).
> 
> Thanks for spotting that, the version below should be better.
> 
> I'll wait for Len to respond till Friday and will take care of the patch myself
> if he doesn't say anything.
> 
> May I assume an ACK from you?
> 
> Rafael
> 
> ---
> From: Oleksij Rempel <bug-track@fisher-privat.net>
> Subject: ACPI / PCI: Make _SxD/_SxW check follow ACPI 4.0a spec
> 
> 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 fields to blacklist broken device Dx states, so
> if _SxD/_SxW return values are checked 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 <bug-track@fisher-privat.net>
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Len, this is an important fix, any chance to take it for v3.5 and -stable?
Or do you want me to do that?

Rafael


> ---
>  drivers/acpi/sleep.c |   22 +++++++++++++++++++++-
>  drivers/pci/pci.c    |   12 +++++++++++-
>  2 files changed, 32 insertions(+), 2 deletions(-)
> 
> Index: linux/drivers/acpi/sleep.c
> ===================================================================
> --- linux.orig/drivers/acpi/sleep.c
> +++ linux/drivers/acpi/sleep.c
> @@ -720,10 +720,30 @@ int acpi_pm_device_sleep_state(struct de
>  	 *
>  	 * NOTE: We rely on acpi_evaluate_integer() not clobbering the integer
>  	 * provided -- that's our fault recovery, we ignore retval.
> +	 *
> +	 * According to ACPI 4.0a (April 5, 2010), page 294,
> +	 * Table 7-7 S3 Action / Result Table, we need to evaluate _SxW in
> +	 * addition to _SxD if the device is configured for wakeup:
> +	 * Desired Action    | _S3D | _PRW | _S3W | Resultant D-state
> +	 * Enter S3          |  N/A |  D/C |  N/A | OSPM decides
> +	 * Enter S3, No Wake |   2  |  D/C |  D/C | Enter D2 or D3
> +	 * Enter S3, Wake    |   2  |   3  |  N/A | Enter D2
> +	 * Enter S3, Wake    |   2  |   3  |   3  | Enter D2 or D3
> +	 * Enter S3, Wake    |  N/A |   3  |   2  | Enter D0, D1 or D2
>  	 */
> -	if (acpi_target_sleep_state > ACPI_STATE_S0)
> +	if (acpi_target_sleep_state > ACPI_STATE_S0) {
> +		acpi_status status;
> +
>  		acpi_evaluate_integer(handle, acpi_method, NULL, &d_min);
>  
> +		if (device_may_wakeup(dev)) {
> +			acpi_method[3] = 'W';
> +			status = acpi_evaluate_integer(handle, acpi_method,
> +						       NULL, &d_max);
> +			if (ACPI_FAILURE(status))
> +				d_max = d_min;
> +		}
> +	}
>  	/*
>  	 * If _PRW says we can wake up the system from the target sleep state,
>  	 * the D-state returned by _SxD is sufficient for that (we assume a
> Index: linux/drivers/pci/pci.c
> ===================================================================
> --- linux.orig/drivers/pci/pci.c
> +++ linux/drivers/pci/pci.c
> @@ -1691,10 +1691,20 @@ pci_power_t pci_target_state(struct pci_
>  {
>  	pci_power_t target_state = PCI_D3hot;
>  
> -	if (platform_pci_power_manageable(dev)) {
> +	/*
> +	 * According to ACPI 4.0a,7.2 Device Power Management Objects, device
> +	 * with wake capability should have _PRW or _PSW object and can have
> +	 * _SxD or _SxW object.
> +	 * It looks like some OEMs use this fields to avoid buggy Dx states
> +	 * of devices, so we need to check for _PRW or _PSW and see if _SxD or
> +	 * _SxW indicate to overwrite Dx.
> +	 */
> +	if (platform_pci_power_manageable(dev)
> +	    || platform_pci_can_wakeup(dev)) {
>  		/*
>  		 * Call the platform to choose the target state of the device
>  		 * and enable wake-up from this state if supported.
> +		 * (Check _SxD and _SxW)
>  		 */
>  		pci_power_t state = platform_pci_choose_state(dev);
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

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

[Rafael J. Wysocki]

On Monday, April 30, 2012, Rafael J. Wysocki wrote:
> On Monday, April 30, 2012, Bjorn Helgaas wrote:
> > On Sun, Apr 29, 2012 at 2:44 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > From: Oleksij Rempel <bug-track@fisher-privat.net>
> > >
> > > 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 <bug-track@fisher-privat.net>
> > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > ---
> > >
> > > 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).
> 
> Thanks for spotting that, the version below should be better.
> 
> I'll wait for Len to respond till Friday and will take care of the patch myself
> if he doesn't say anything.
> 
> May I assume an ACK from you?
> 
> Rafael
> 
> ---
> From: Oleksij Rempel <bug-track@fisher-privat.net>
> Subject: ACPI / PCI: Make _SxD/_SxW check follow ACPI 4.0a spec
> 
> 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 fields to blacklist broken device Dx states, so
> if _SxD/_SxW return values are checked 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 <bug-track@fisher-privat.net>
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Len, this is an important fix, any chance to take it for v3.5 and -stable?
Or do you want me to do that?

Rafael


> ---
>  drivers/acpi/sleep.c |   22 +++++++++++++++++++++-
>  drivers/pci/pci.c    |   12 +++++++++++-
>  2 files changed, 32 insertions(+), 2 deletions(-)
> 
> Index: linux/drivers/acpi/sleep.c
> ===================================================================
> --- linux.orig/drivers/acpi/sleep.c
> +++ linux/drivers/acpi/sleep.c
> @@ -720,10 +720,30 @@ int acpi_pm_device_sleep_state(struct de
>  	 *
>  	 * NOTE: We rely on acpi_evaluate_integer() not clobbering the integer
>  	 * provided -- that's our fault recovery, we ignore retval.
> +	 *
> +	 * According to ACPI 4.0a (April 5, 2010), page 294,
> +	 * Table 7-7 S3 Action / Result Table, we need to evaluate _SxW in
> +	 * addition to _SxD if the device is configured for wakeup:
> +	 * Desired Action    | _S3D | _PRW | _S3W | Resultant D-state
> +	 * Enter S3          |  N/A |  D/C |  N/A | OSPM decides
> +	 * Enter S3, No Wake |   2  |  D/C |  D/C | Enter D2 or D3
> +	 * Enter S3, Wake    |   2  |   3  |  N/A | Enter D2
> +	 * Enter S3, Wake    |   2  |   3  |   3  | Enter D2 or D3
> +	 * Enter S3, Wake    |  N/A |   3  |   2  | Enter D0, D1 or D2
>  	 */
> -	if (acpi_target_sleep_state > ACPI_STATE_S0)
> +	if (acpi_target_sleep_state > ACPI_STATE_S0) {
> +		acpi_status status;
> +
>  		acpi_evaluate_integer(handle, acpi_method, NULL, &d_min);
>  
> +		if (device_may_wakeup(dev)) {
> +			acpi_method[3] = 'W';
> +			status = acpi_evaluate_integer(handle, acpi_method,
> +						       NULL, &d_max);
> +			if (ACPI_FAILURE(status))
> +				d_max = d_min;
> +		}
> +	}
>  	/*
>  	 * If _PRW says we can wake up the system from the target sleep state,
>  	 * the D-state returned by _SxD is sufficient for that (we assume a
> Index: linux/drivers/pci/pci.c
> ===================================================================
> --- linux.orig/drivers/pci/pci.c
> +++ linux/drivers/pci/pci.c
> @@ -1691,10 +1691,20 @@ pci_power_t pci_target_state(struct pci_
>  {
>  	pci_power_t target_state = PCI_D3hot;
>  
> -	if (platform_pci_power_manageable(dev)) {
> +	/*
> +	 * According to ACPI 4.0a,7.2 Device Power Management Objects, device
> +	 * with wake capability should have _PRW or _PSW object and can have
> +	 * _SxD or _SxW object.
> +	 * It looks like some OEMs use this fields to avoid buggy Dx states
> +	 * of devices, so we need to check for _PRW or _PSW and see if _SxD or
> +	 * _SxW indicate to overwrite Dx.
> +	 */
> +	if (platform_pci_power_manageable(dev)
> +	    || platform_pci_can_wakeup(dev)) {
>  		/*
>  		 * Call the platform to choose the target state of the device
>  		 * and enable wake-up from this state if supported.
> +		 * (Check _SxD and _SxW)
>  		 */
>  		pci_power_t state = platform_pci_choose_state(dev);
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

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

[Alan Stern]

Oleksij:

Please take a look at this bug report:

	https://bugzilla.kernel.org/show_bug.cgi?id=43278

Apparently your patch breaks wakeup on this machine by preventing the 
USB host controllers from being put into D3.

Alan Stern

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

[Rafael J. Wysocki]

On Friday, May 25, 2012, Alan Stern wrote:
> Oleksij:
> 
> Please take a look at this bug report:
> 
> 	https://bugzilla.kernel.org/show_bug.cgi?id=43278
> 
> Apparently your patch breaks wakeup on this machine by preventing the 
> USB host controllers from being put into D3.

I think the patch is incorrect, actually.

First, if you look at the first hunk:

-       if (acpi_target_sleep_state > ACPI_STATE_S0)
+       if (acpi_target_sleep_state > ACPI_STATE_S0) {
+               acpi_status status;
+
                acpi_evaluate_integer(handle, acpi_method, NULL, &d_min);
 
+               if (device_may_wakeup(dev)) {
+                       acpi_method[3] = 'W';
+                       status = acpi_evaluate_integer(handle, acpi_method,
+                                                      NULL, &d_max);
+                       if (ACPI_FAILURE(status))
+                               d_max = d_min;
+               }
+       }

it will do something like this: if the device is wakeup-capable, get d_max
from _SxW, unless it fails.  However, the code just below in that function:

	if (acpi_target_sleep_state == ACPI_STATE_S0 ||
	    (device_may_wakeup(dev) &&
	     adev->wakeup.sleep_state <= acpi_target_sleep_state)) {
		acpi_status status;

		acpi_method[3] = 'W';
		status = acpi_evaluate_integer(handle, acpi_method, NULL,
						&d_max);
		if (ACPI_FAILURE(status)) {
			if (acpi_target_sleep_state != ACPI_STATE_S0 ||
			    status != AE_NOT_FOUND)
				d_max = d_min;
		} else if (d_max < d_min) {
			/* Warn the user of the broken DSDT */
			printk(KERN_WARNING "ACPI: Wrong value from %s\n",
				acpi_method);
			/* Sanitize it */
			d_min = d_max;
		}
	}

does _exactly_ the same thing (it only has a more sophisticated error code path).

It might check adev->wakeup.flags.valid instead of device_may_wakeup(dev),
but that's a different story.

So the only difference made by the patch is te hunk in pci_target_state().

Thanks,
Rafael

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

[Oleksij Rempel (fishor)]

Hi,

On 25.05.2012 22:00, Rafael J. Wysocki wrote:
> On Friday, May 25, 2012, Alan Stern wrote:
>> Oleksij:
>>
>> Please take a look at this bug report:
>>
>> 	https://bugzilla.kernel.org/show_bug.cgi?id=43278
>>
>> Apparently your patch breaks wakeup on this machine by preventing the
>> USB host controllers from being put into D3.
>
> I think the patch is incorrect, actually.
>
> First, if you look at the first hunk:
>
> -       if (acpi_target_sleep_state>  ACPI_STATE_S0)
> +       if (acpi_target_sleep_state>  ACPI_STATE_S0) {
> +               acpi_status status;
> +
>                  acpi_evaluate_integer(handle, acpi_method, NULL,&d_min);
>
> +               if (device_may_wakeup(dev)) {
> +                       acpi_method[3] = 'W';
> +                       status = acpi_evaluate_integer(handle, acpi_method,
> +                                                      NULL,&d_max);
> +                       if (ACPI_FAILURE(status))
> +                               d_max = d_min;
> +               }
> +       }
>
> it will do something like this: if the device is wakeup-capable, get d_max
> from _SxW, unless it fails.  However, the code just below in that function:
>
> 	if (acpi_target_sleep_state == ACPI_STATE_S0 ||
> 	    (device_may_wakeup(dev)&&
> 	adev->wakeup.sleep_state<= acpi_target_sleep_state)) {
> 		acpi_status status;
>
> 		acpi_method[3] = 'W';
> 		status = acpi_evaluate_integer(handle, acpi_method, NULL,
> 						&d_max);
> 		if (ACPI_FAILURE(status)) {
> 			if (acpi_target_sleep_state != ACPI_STATE_S0 ||
> 			    status != AE_NOT_FOUND)
> 				d_max = d_min;
> 		} else if (d_max<  d_min) {
> 			/* Warn the user of the broken DSDT */
> 			printk(KERN_WARNING "ACPI: Wrong value from %s\n",
> 				acpi_method);
> 			/* Sanitize it */
> 			d_min = d_max;
> 		}
> 	}
>
> does _exactly_ the same thing (it only has a more sophisticated error code path).

Not really correct. The code below check _SxW state on wake up. This 
code checks _SxW on suspend.

> It might check adev->wakeup.flags.valid instead of device_may_wakeup(dev),
> but that's a different story.
>
> So the only difference made by the patch is te hunk in pci_target_state().

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

[Oleksij Rempel]

Am 26.05.2012 08:03, schrieb Oleksij Rempel (fishor):
> Hi,
> 
> On 25.05.2012 22:00, Rafael J. Wysocki wrote:
>> On Friday, May 25, 2012, Alan Stern wrote:
>>> Oleksij:
>>>
>>> Please take a look at this bug report:
>>>
>>>     https://bugzilla.kernel.org/show_bug.cgi?id=43278
>>>
>>> Apparently your patch breaks wakeup on this machine by preventing the
>>> USB host controllers from being put into D3.
>>
>> I think the patch is incorrect, actually.
>>
>> First, if you look at the first hunk:
>>
>> -       if (acpi_target_sleep_state>  ACPI_STATE_S0)
>> +       if (acpi_target_sleep_state>  ACPI_STATE_S0) {
>> +               acpi_status status;
>> +
>>                  acpi_evaluate_integer(handle, acpi_method, NULL,&d_min);
>>
>> +               if (device_may_wakeup(dev)) {
>> +                       acpi_method[3] = 'W';
>> +                       status = acpi_evaluate_integer(handle,
>> acpi_method,
>> +                                                      NULL,&d_max);
>> +                       if (ACPI_FAILURE(status))
>> +                               d_max = d_min;
>> +               }
>> +       }
>>
>> it will do something like this: if the device is wakeup-capable, get
>> d_max
>> from _SxW, unless it fails.  However, the code just below in that
>> function:
>>
>>     if (acpi_target_sleep_state == ACPI_STATE_S0 ||
>>         (device_may_wakeup(dev)&&
>>     adev->wakeup.sleep_state<= acpi_target_sleep_state)) {
>>         acpi_status status;
>>
>>         acpi_method[3] = 'W';
>>         status = acpi_evaluate_integer(handle, acpi_method, NULL,
>>                         &d_max);
>>         if (ACPI_FAILURE(status)) {
>>             if (acpi_target_sleep_state != ACPI_STATE_S0 ||
>>                 status != AE_NOT_FOUND)
>>                 d_max = d_min;
>>         } else if (d_max<  d_min) {
>>             /* Warn the user of the broken DSDT */
>>             printk(KERN_WARNING "ACPI: Wrong value from %s\n",
>>                 acpi_method);
>>             /* Sanitize it */
>>             d_min = d_max;
>>         }
>>     }
>>
>> does _exactly_ the same thing (it only has a more sophisticated error
>> code path).
> 
> Not really correct. The code below check _SxW state on wake up. This
> code checks _SxW on suspend.

oops i misreaded the code. it really do the same. except this part is
not executed because of this check: "adev->wakeup.sleep_state <=
acpi_target_sleep_state"


-- 
Regards,
Oleksij

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

[Rafael J. Wysocki]

On Saturday, May 26, 2012, Oleksij Rempel wrote:
> Am 26.05.2012 08:03, schrieb Oleksij Rempel (fishor):
> > Hi,
> > 
> > On 25.05.2012 22:00, Rafael J. Wysocki wrote:
> >> On Friday, May 25, 2012, Alan Stern wrote:
> >>> Oleksij:
> >>>
> >>> Please take a look at this bug report:
> >>>
> >>>     https://bugzilla.kernel.org/show_bug.cgi?id=43278
> >>>
> >>> Apparently your patch breaks wakeup on this machine by preventing the
> >>> USB host controllers from being put into D3.
> >>
> >> I think the patch is incorrect, actually.
> >>
> >> First, if you look at the first hunk:
> >>
> >> -       if (acpi_target_sleep_state>  ACPI_STATE_S0)
> >> +       if (acpi_target_sleep_state>  ACPI_STATE_S0) {
> >> +               acpi_status status;
> >> +
> >>                  acpi_evaluate_integer(handle, acpi_method, NULL,&d_min);
> >>
> >> +               if (device_may_wakeup(dev)) {
> >> +                       acpi_method[3] = 'W';
> >> +                       status = acpi_evaluate_integer(handle,
> >> acpi_method,
> >> +                                                      NULL,&d_max);
> >> +                       if (ACPI_FAILURE(status))
> >> +                               d_max = d_min;
> >> +               }
> >> +       }
> >>
> >> it will do something like this: if the device is wakeup-capable, get
> >> d_max
> >> from _SxW, unless it fails.  However, the code just below in that
> >> function:
> >>
> >>     if (acpi_target_sleep_state == ACPI_STATE_S0 ||
> >>         (device_may_wakeup(dev)&&
> >>     adev->wakeup.sleep_state<= acpi_target_sleep_state)) {
> >>         acpi_status status;
> >>
> >>         acpi_method[3] = 'W';
> >>         status = acpi_evaluate_integer(handle, acpi_method, NULL,
> >>                         &d_max);
> >>         if (ACPI_FAILURE(status)) {
> >>             if (acpi_target_sleep_state != ACPI_STATE_S0 ||
> >>                 status != AE_NOT_FOUND)
> >>                 d_max = d_min;
> >>         } else if (d_max<  d_min) {
> >>             /* Warn the user of the broken DSDT */
> >>             printk(KERN_WARNING "ACPI: Wrong value from %s\n",
> >>                 acpi_method);
> >>             /* Sanitize it */
> >>             d_min = d_max;
> >>         }
> >>     }
> >>
> >> does _exactly_ the same thing (it only has a more sophisticated error
> >> code path).
> > 
> > Not really correct. The code below check _SxW state on wake up. This
> > code checks _SxW on suspend.
> 
> oops i misreaded the code. it really do the same. except this part is
> not executed because of this check: "adev->wakeup.sleep_state <=
> acpi_target_sleep_state"

Precisely.

Thanks,
Rafael

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

[Greg KH]

On Sun, Apr 29, 2012 at 10:44:16PM +0200, Rafael J. Wysocki wrote:
> From: Oleksij Rempel <bug-track@fisher-privat.net>
> 
> 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 <bug-track@fisher-privat.net>
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
> 
> 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.

What ever hapened to this patch?

greg k-h

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

[Rafael J. Wysocki]

On Wednesday, July 11, 2012, Greg KH wrote:
> On Sun, Apr 29, 2012 at 10:44:16PM +0200, Rafael J. Wysocki wrote:
> > From: Oleksij Rempel <bug-track@fisher-privat.net>
> > 
> > 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 <bug-track@fisher-privat.net>
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> > 
> > 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.
> 
> What ever hapened to this patch?

It was split and partially merged.  As far as the other part is concerned,
the jury is still out.

Thanks,
Rafael

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

[Greg KH]

On Wed, Jul 11, 2012 at 11:07:56AM +0200, Rafael J. Wysocki wrote:
> On Wednesday, July 11, 2012, Greg KH wrote:
> > On Sun, Apr 29, 2012 at 10:44:16PM +0200, Rafael J. Wysocki wrote:
> > > From: Oleksij Rempel <bug-track@fisher-privat.net>
> > > 
> > > 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 <bug-track@fisher-privat.net>
> > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > ---
> > > 
> > > 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.
> > 
> > What ever hapened to this patch?
> 
> It was split and partially merged.  As far as the other part is concerned,
> the jury is still out.

Ok, I'll wait for someone to ask for it to be added to the stable tree
before I do anything.

thanks,

greg k-h

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

[Rafael J. Wysocki]

On Wednesday, July 11, 2012, Greg KH wrote:
> On Wed, Jul 11, 2012 at 11:07:56AM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, July 11, 2012, Greg KH wrote:
> > > On Sun, Apr 29, 2012 at 10:44:16PM +0200, Rafael J. Wysocki wrote:
> > > > From: Oleksij Rempel <bug-track@fisher-privat.net>
> > > > 
> > > > 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 <bug-track@fisher-privat.net>
> > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > > ---
> > > > 
> > > > 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.
> > > 
> > > What ever hapened to this patch?
> > 
> > It was split and partially merged.  As far as the other part is concerned,
> > the jury is still out.
> 
> Ok, I'll wait for someone to ask for it to be added to the stable tree
> before I do anything.

In fact, it should have been marked as -stable material, so please add:

commit dbe9a2edd17d843d80faf2b99f20a691c1853418
Author: Rafael J. Wysocki <rjw@sisk.pl>
Date:   Tue May 29 21:21:07 2012 +0200

    ACPI / PM: Make acpi_pm_device_sleep_state() follow the specification

Thanks,
Rafael

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

[Greg KH]

On Wed, Jul 11, 2012 at 09:04:48PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, July 11, 2012, Greg KH wrote:
> > On Wed, Jul 11, 2012 at 11:07:56AM +0200, Rafael J. Wysocki wrote:
> > > On Wednesday, July 11, 2012, Greg KH wrote:
> > > > On Sun, Apr 29, 2012 at 10:44:16PM +0200, Rafael J. Wysocki wrote:
> > > > > From: Oleksij Rempel <bug-track@fisher-privat.net>
> > > > > 
> > > > > 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 <bug-track@fisher-privat.net>
> > > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > ---
> > > > > 
> > > > > 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.
> > > > 
> > > > What ever hapened to this patch?
> > > 
> > > It was split and partially merged.  As far as the other part is concerned,
> > > the jury is still out.
> > 
> > Ok, I'll wait for someone to ask for it to be added to the stable tree
> > before I do anything.
> 
> In fact, it should have been marked as -stable material, so please add:
> 
> commit dbe9a2edd17d843d80faf2b99f20a691c1853418
> Author: Rafael J. Wysocki <rjw@sisk.pl>
> Date:   Tue May 29 21:21:07 2012 +0200
> 
>     ACPI / PM: Make acpi_pm_device_sleep_state() follow the specification

Ah, the name changed, that's why I couldn't find it.  Thanks, I've now
queued it up.

greg k-h

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

[Rafael J. Wysocki]

On Wednesday, July 11, 2012, Greg KH wrote:
> On Wed, Jul 11, 2012 at 09:04:48PM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, July 11, 2012, Greg KH wrote:
> > > On Wed, Jul 11, 2012 at 11:07:56AM +0200, Rafael J. Wysocki wrote:
> > > > On Wednesday, July 11, 2012, Greg KH wrote:
> > > > > On Sun, Apr 29, 2012 at 10:44:16PM +0200, Rafael J. Wysocki wrote:
> > > > > > From: Oleksij Rempel <bug-track@fisher-privat.net>
> > > > > > 
> > > > > > 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 <bug-track@fisher-privat.net>
> > > > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > > ---
> > > > > > 
> > > > > > 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.
> > > > > 
> > > > > What ever hapened to this patch?
> > > > 
> > > > It was split and partially merged.  As far as the other part is concerned,
> > > > the jury is still out.
> > > 
> > > Ok, I'll wait for someone to ask for it to be added to the stable tree
> > > before I do anything.
> > 
> > In fact, it should have been marked as -stable material, so please add:
> > 
> > commit dbe9a2edd17d843d80faf2b99f20a691c1853418
> > Author: Rafael J. Wysocki <rjw@sisk.pl>
> > Date:   Tue May 29 21:21:07 2012 +0200
> > 
> >     ACPI / PM: Make acpi_pm_device_sleep_state() follow the specification
> 
> Ah, the name changed, that's why I couldn't find it.  Thanks, I've now
> queued it up.

Thanks!

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

[Ben Hutchings]

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

On Wed, 2012-07-11 at 21:04 +0200, Rafael J. Wysocki wrote:
> On Wednesday, July 11, 2012, Greg KH wrote:
> > On Wed, Jul 11, 2012 at 11:07:56AM +0200, Rafael J. Wysocki wrote:
> > > On Wednesday, July 11, 2012, Greg KH wrote:
> > > > On Sun, Apr 29, 2012 at 10:44:16PM +0200, Rafael J. Wysocki wrote:
> > > > > From: Oleksij Rempel <bug-track@fisher-privat.net>
> > > > > 
> > > > > 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 <bug-track@fisher-privat.net>
> > > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > ---
> > > > > 
> > > > > 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.
> > > > 
> > > > What ever hapened to this patch?
> > > 
> > > It was split and partially merged.  As far as the other part is concerned,
> > > the jury is still out.
> > 
> > Ok, I'll wait for someone to ask for it to be added to the stable tree
> > before I do anything.
> 
> In fact, it should have been marked as -stable material, so please add:
> 
> commit dbe9a2edd17d843d80faf2b99f20a691c1853418
> Author: Rafael J. Wysocki <rjw@sisk.pl>
> Date:   Tue May 29 21:21:07 2012 +0200
> 
>     ACPI / PM: Make acpi_pm_device_sleep_state() follow the specification

Queued up for 3.2.y.

Ben.

-- 
Ben Hutchings
The generation of random numbers is too important to be left to chance.
                                                            - Robert Coveyou

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