ACPI / PCI: Make _SxD/_SxW check follow ACPI 4.0a spec
diff mbox

Message ID 201204292244.16467.rjw@sisk.pl
State Not Applicable, archived
Headers show

Commit Message

Rafael J. Wysocki April 29, 2012, 8:44 p.m. UTC
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(-)

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

Comments

Bjorn Helgaas April 30, 2012, 4:03 p.m. UTC | #1
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);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greg KH July 11, 2012, 12:08 a.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki July 11, 2012, 9:07 a.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greg KH July 11, 2012, 1:53 p.m. UTC | #4
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki July 11, 2012, 7:04 p.m. UTC | #5
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greg KH July 11, 2012, 7:20 p.m. UTC | #6
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki July 11, 2012, 7:29 p.m. UTC | #7
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!
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings July 12, 2012, 4:30 a.m. UTC | #8
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.

Patch
diff mbox

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);