ACPI: do not fail suspend if unable to configure wakeup

PATCH In review Dmitry Torokhov 3 participants 11 messages Oct 15, 2014 – Nov 19, 2014 ACPI

Share & tools

Dmitry Torokhov

author

2014-10-15 22:57 UTC #0 lore raw

PATCH 1/1 ACPI: do not fail suspend if unable to configure wakeup

Newer kernels put i2c devices with ACPI companion in ACPI power domain and then ACPI will try to configure them for wakeup (if requested). Unfortunately on some Chromebooks firmware separates wakeup GPIO into a completely separate device (which is handled by the kernel as a sleep button), leaving the touchpads themselves not wakeup capable (as far as ACPI is concerned). This causes ACPI late suspend code to fail to configure them as wakeup sources and aborts entire suspend.

To work around this issues let's not abort entire suspend process if driver asked to be a wakeup source but ACPI can not satisfy that request.

Note that originally I tried to simply change the driver to not mark device as wakeup source, unfortunately then we do not know that we should not be powering down the device completely, otherwise we can't wake up.

Verified by making sure that "echo mem > /sys/power/state" works on Squawks.

Reviewed-by Benson Leung <[email protected]>

Signed-off-by Dmitry Torokhov <[email protected]>

---
 drivers/acpi/device_pm.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

drivers/acpi/device_pm.c +12 -4

index 67075f8..440bc3d 100644
@@ -871,6 +871,7 @@ int acpi_dev_suspend_late(struct device *dev)
 	struct acpi_device *adev = ACPI_COMPANION(dev);
 	u32 target_state;
 	bool wakeup;
+	bool can_wakeup;
 	int error;
 
 	if (!adev)

@@ -878,12 +879,19 @@ int acpi_dev_suspend_late(struct device *dev)
 
 	target_state = acpi_target_system_state();
 	wakeup = device_may_wakeup(dev);
-	error = acpi_device_wakeup(adev, target_state, wakeup);
-	if (wakeup && error)
-		return error;
+	can_wakeup = acpi_device_can_wakeup(adev);
+
+	if (can_wakeup) {
+		error = acpi_device_wakeup(adev, target_state, wakeup);
+		if (wakeup && error)
+			return error;
+	} else if (wakeup) {
+		dev_warn(dev,
+			 "device is not wakeup-capable, not enabling wakeup\n");
+	}
 
 	error = acpi_dev_pm_low_power(dev, adev, target_state);
-	if (error)
+	if (error && can_wakeup)
 		acpi_device_wakeup(adev, ACPI_STATE_UNKNOWN, false);
 
 	return error;

Signature · 5 lines

2.1.0.rc2.206.gedb03e5


-- 
Dmitry

Rafael J. Wysocki

maintainer

2014-10-16 08:10 UTC #1 lore raw

Hi,

Quoted 42 lines · Dmitry Torokhov

Newer kernels put i2c devices with ACPI companion in ACPI power domain and then ACPI will try to configure them for wakeup (if requested). Unfortunately on some Chromebooks firmware separates wakeup GPIO into a completely separate device (which is handled by the kernel as a sleep button), leaving the touchpads themselves not wakeup capable (as far as ACPI is concerned). This causes ACPI late suspend code to fail to configure them as wakeup sources and aborts entire suspend.
To work around this issues let's not abort entire suspend process if driver asked to be a wakeup source but ACPI can not satisfy that request.
Note that originally I tried to simply change the driver to not mark device as wakeup source, unfortunately then we do not know that we should not be powering down the device completely, otherwise we can't wake up.
Verified by making sure that "echo mem > /sys/power/state" works on Squawks.
Reviewed-by Benson Leung <[email protected]>
Signed-off-by Dmitry Torokhov <[email protected]>
---
 drivers/acpi/device_pm.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)
drivers/acpi/device_pm.c +1 -0
index 67075f8..440bc3d 100644
@@ -871,6 +871,7 @@ int acpi_dev_suspend_late(struct device *dev)
        struct acpi_device *adev = ACPI_COMPANION(dev);
        u32 target_state;
        bool wakeup;
+       bool can_wakeup;
        int error;
 
        if (!adev)
@@ -878,12 +879,19 @@ int acpi_dev_suspend_late(struct device *dev)
 
        target_state = acpi_target_system_state();
        wakeup = device_may_wakeup(dev);

drivers/acpi/device_pm.c int acpi_dev_suspend_late(struct device *dev) · line 888

Quoted 10 lines

drivers/acpi/device_pm.c +7 -3

-       error = acpi_device_wakeup(adev, target_state, wakeup);
-       if (wakeup && error)
-               return error;
+       can_wakeup = acpi_device_can_wakeup(adev);
+
+       if (can_wakeup) {
+               error = acpi_device_wakeup(adev, target_state, wakeup);
+               if (wakeup && error)
+                       return error;
+       } else if (wakeup) {

I think we just need to return an error code in that case, because otherwise this is potentially dangerous (worst case, it may be impossible to wake up the machine at all after that).

drivers/acpi/device_pm.c int acpi_dev_suspend_late(struct device *dev)

Quoted 11 lines

drivers/acpi/device_pm.c +4 -2

+               dev_warn(dev,
+                        "device is not wakeup-capable, not enabling wakeup\n");
+       }
 
        error = acpi_dev_pm_low_power(dev, adev, target_state);
-       if (error)
+       if (error && can_wakeup)
                acpi_device_wakeup(adev, ACPI_STATE_UNKNOWN, false);
 
        return error;
--

Rafael

Dmitry Torokhov

author

2014-10-16 16:05 UTC #2 lore raw

Hi Rafael,

Quoted 43 lines · Rafael J. Wysocki

Hi,
Quoted 40 lines · Dmitry Torokhov
Newer kernels put i2c devices with ACPI companion in ACPI power domain and then ACPI will try to configure them for wakeup (if requested). Unfortunately on some Chromebooks firmware separates wakeup GPIO into a completely separate device (which is handled by the kernel as a sleep button), leaving the touchpads themselves not wakeup capable (as far as ACPI is concerned). This causes ACPI late suspend code to fail to configure them as wakeup sources and aborts entire suspend.
To work around this issues let's not abort entire suspend process if driver asked to be a wakeup source but ACPI can not satisfy that request.
Note that originally I tried to simply change the driver to not mark device as wakeup source, unfortunately then we do not know that we should not be powering down the device completely, otherwise we can't wake up.
Verified by making sure that "echo mem > /sys/power/state" works on Squawks.
Reviewed-by Benson Leung <[email protected]>
Signed-off-by Dmitry Torokhov <[email protected]>
---
 drivers/acpi/device_pm.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)
drivers/acpi/device_pm.c +1 -0
index 67075f8..440bc3d 100644
@@ -871,6 +871,7 @@ int acpi_dev_suspend_late(struct device *dev)
        struct acpi_device *adev = ACPI_COMPANION(dev);
        u32 target_state;
        bool wakeup;
+       bool can_wakeup;
        int error;
 
        if (!adev)
@@ -878,12 +879,19 @@ int acpi_dev_suspend_late(struct device *dev)
 

Quoted 14 lines

drivers/acpi/device_pm.c int acpi_dev_suspend_late(struct device *dev) · line 888

Quoted 12 lines

drivers/acpi/device_pm.c +7 -3

        target_state = acpi_target_system_state();
        wakeup = device_may_wakeup(dev);
-       error = acpi_device_wakeup(adev, target_state, wakeup);
-       if (wakeup && error)
-               return error;
+       can_wakeup = acpi_device_can_wakeup(adev);
+
+       if (can_wakeup) {
+               error = acpi_device_wakeup(adev, target_state, wakeup);
+               if (wakeup && error)
+                       return error;
+       } else if (wakeup) {

I think we just need to return an error code in that case, because otherwise

We used to return error and that error aborted the suspend altogether, which prompted creating this patch.

Quoted 2 lines

this is potentially dangerous (worst case, it may be impossible to wake up the machine at all after that).

Yes, there is such potential, but that kind of error (no working wakeup sources) will be discovered before a box is shipped. Right now we have boxes in the wild that suspend fine with 3.10 and refuse to suspend with 3.14 because between 3.10 and 3.14 we started placing i2c devices with ACPI companions into ACPI power domain and ACPI power domain is now trying to configure them as wakeup sources and fails.

Thanks.

Signature · 1 line

Dmitry

Dmitry Torokhov

author

2014-11-18 18:00 UTC #3 lore raw

Hi Rafael,

Quoted 67 lines · Dmitry Torokhov

Hi Rafael,
Quoted 43 lines · Rafael J. Wysocki
Hi,
Quoted 40 lines · Dmitry Torokhov
Newer kernels put i2c devices with ACPI companion in ACPI power domain and then ACPI will try to configure them for wakeup (if requested). Unfortunately on some Chromebooks firmware separates wakeup GPIO into a completely separate device (which is handled by the kernel as a sleep button), leaving the touchpads themselves not wakeup capable (as far as ACPI is concerned). This causes ACPI late suspend code to fail to configure them as wakeup sources and aborts entire suspend.
To work around this issues let's not abort entire suspend process if driver asked to be a wakeup source but ACPI can not satisfy that request.
Note that originally I tried to simply change the driver to not mark device as wakeup source, unfortunately then we do not know that we should not be powering down the device completely, otherwise we can't wake up.
Verified by making sure that "echo mem > /sys/power/state" works on Squawks.
Reviewed-by Benson Leung <[email protected]>
Signed-off-by Dmitry Torokhov <[email protected]>
---
 drivers/acpi/device_pm.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)
drivers/acpi/device_pm.c +1 -0
index 67075f8..440bc3d 100644
@@ -871,6 +871,7 @@ int acpi_dev_suspend_late(struct device *dev)
        struct acpi_device *adev = ACPI_COMPANION(dev);
        u32 target_state;
        bool wakeup;
+       bool can_wakeup;
        int error;
 
        if (!adev)
@@ -878,12 +879,19 @@ int acpi_dev_suspend_late(struct device *dev)
 
Quoted 14 lines
drivers/acpi/device_pm.c int acpi_dev_suspend_late(struct device *dev) · line 888
Quoted 12 lines
drivers/acpi/device_pm.c +7 -3
        target_state = acpi_target_system_state();
        wakeup = device_may_wakeup(dev);
-       error = acpi_device_wakeup(adev, target_state, wakeup);
-       if (wakeup && error)
-               return error;
+       can_wakeup = acpi_device_can_wakeup(adev);
+
+       if (can_wakeup) {
+               error = acpi_device_wakeup(adev, target_state, wakeup);
+               if (wakeup && error)
+                       return error;
+       } else if (wakeup) {
I think we just need to return an error code in that case, because otherwise
We used to return error and that error aborted the suspend altogether, which prompted creating this patch.
Quoted 2 lines
this is potentially dangerous (worst case, it may be impossible to wake up the machine at all after that).

Quoted 6 lines

Yes, there is such potential, but that kind of error (no working wakeup sources) will be discovered before a box is shipped. Right now we have boxes in the wild that suspend fine with 3.10 and refuse to suspend with 3.14 because between 3.10 and 3.14 we started placing i2c devices with ACPI companions into ACPI power domain and ACPI power domain is now trying to configure them as wakeup sources and fails.

A gentle ping on the patch - without it (or something else) we basically have a regression on shipped hardware: Chromebooks that were suspending fine with 3.10 refuse to suspend with 3.14.

Thanks.

Signature · 1 line

Dmitry

Rafael J. Wysocki

2014-11-18 23:22 UTC #4 lore raw

Quoted 77 lines · Dmitry Torokhov

Hi Rafael,
Quoted 67 lines · Dmitry Torokhov
Hi Rafael,
Quoted 43 lines · Rafael J. Wysocki
Hi,
Quoted 40 lines · Dmitry Torokhov
Newer kernels put i2c devices with ACPI companion in ACPI power domain and then ACPI will try to configure them for wakeup (if requested). Unfortunately on some Chromebooks firmware separates wakeup GPIO into a completely separate device (which is handled by the kernel as a sleep button), leaving the touchpads themselves not wakeup capable (as far as ACPI is concerned). This causes ACPI late suspend code to fail to configure them as wakeup sources and aborts entire suspend.
To work around this issues let's not abort entire suspend process if driver asked to be a wakeup source but ACPI can not satisfy that request.
Note that originally I tried to simply change the driver to not mark device as wakeup source, unfortunately then we do not know that we should not be powering down the device completely, otherwise we can't wake up.
Verified by making sure that "echo mem > /sys/power/state" works on Squawks.
Reviewed-by Benson Leung <[email protected]>
Signed-off-by Dmitry Torokhov <[email protected]>
---
 drivers/acpi/device_pm.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)
drivers/acpi/device_pm.c +1 -0
index 67075f8..440bc3d 100644
@@ -871,6 +871,7 @@ int acpi_dev_suspend_late(struct device *dev)
        struct acpi_device *adev = ACPI_COMPANION(dev);
        u32 target_state;
        bool wakeup;
+       bool can_wakeup;
        int error;
 
        if (!adev)
@@ -878,12 +879,19 @@ int acpi_dev_suspend_late(struct device *dev)
 
Quoted 14 lines
drivers/acpi/device_pm.c int acpi_dev_suspend_late(struct device *dev) · line 888
Quoted 12 lines
drivers/acpi/device_pm.c +7 -3
        target_state = acpi_target_system_state();
        wakeup = device_may_wakeup(dev);
-       error = acpi_device_wakeup(adev, target_state, wakeup);
-       if (wakeup && error)
-               return error;
+       can_wakeup = acpi_device_can_wakeup(adev);
+
+       if (can_wakeup) {
+               error = acpi_device_wakeup(adev, target_state, wakeup);
+               if (wakeup && error)
+                       return error;
+       } else if (wakeup) {
I think we just need to return an error code in that case, because otherwise
We used to return error and that error aborted the suspend altogether, which prompted creating this patch.
Quoted 2 lines
this is potentially dangerous (worst case, it may be impossible to wake up the machine at all after that).
Quoted 6 lines
Yes, there is such potential, but that kind of error (no working wakeup sources) will be discovered before a box is shipped. Right now we have boxes in the wild that suspend fine with 3.10 and refuse to suspend with 3.14 because between 3.10 and 3.14 we started placing i2c devices with ACPI companions into ACPI power domain and ACPI power domain is now trying to configure them as wakeup sources and fails.

Quoted 3 lines

A gentle ping on the patch - without it (or something else) we basically have a regression on shipped hardware: Chromebooks that were suspending fine with 3.10 refuse to suspend with 3.14.

It fell of my radar, sorry about that.

So the error here is that device_may_wakeup(dev) returns true, because the device is technically wakeup-capable, but the wakeup is not via ACPI?

I'd say this is rather not in accordance with the spec, but that means we need to simply ignore 'wakeup' if acpi_device_can_wakeup(adev) returns false.

So what about the appended patch?

Rafael

---
 drivers/acpi/device_pm.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/acpi/device_pm.c +1 -1

Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
@@ -878,7 +878,7 @@ int acpi_dev_suspend_late(struct device
 		return 0;
 
 	target_state = acpi_target_system_state();
-	wakeup = device_may_wakeup(dev);
+	wakeup = device_may_wakeup(dev) && acpi_device_can_wakeup(adev);
 	error = acpi_device_wakeup(adev, target_state, wakeup);
 	if (wakeup && error)
 		return error;

Dmitry Torokhov

author

2014-11-18 23:31 UTC #5 lore raw

Quoted 82 lines · Rafael J. Wysocki

Quoted 77 lines · Dmitry Torokhov
Hi Rafael,
Quoted 67 lines · Dmitry Torokhov
Hi Rafael,
Quoted 43 lines · Rafael J. Wysocki
Hi,
Quoted 40 lines · Dmitry Torokhov
Newer kernels put i2c devices with ACPI companion in ACPI power domain and then ACPI will try to configure them for wakeup (if requested). Unfortunately on some Chromebooks firmware separates wakeup GPIO into a completely separate device (which is handled by the kernel as a sleep button), leaving the touchpads themselves not wakeup capable (as far as ACPI is concerned). This causes ACPI late suspend code to fail to configure them as wakeup sources and aborts entire suspend.
To work around this issues let's not abort entire suspend process if driver asked to be a wakeup source but ACPI can not satisfy that request.
Note that originally I tried to simply change the driver to not mark device as wakeup source, unfortunately then we do not know that we should not be powering down the device completely, otherwise we can't wake up.
Verified by making sure that "echo mem > /sys/power/state" works on Squawks.
Reviewed-by Benson Leung <[email protected]>
Signed-off-by Dmitry Torokhov <[email protected]>
---
 drivers/acpi/device_pm.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)
drivers/acpi/device_pm.c +1 -0
index 67075f8..440bc3d 100644
@@ -871,6 +871,7 @@ int acpi_dev_suspend_late(struct device *dev)
        struct acpi_device *adev = ACPI_COMPANION(dev);
        u32 target_state;
        bool wakeup;
+       bool can_wakeup;
        int error;
 
        if (!adev)
@@ -878,12 +879,19 @@ int acpi_dev_suspend_late(struct device *dev)
 
Quoted 14 lines
drivers/acpi/device_pm.c int acpi_dev_suspend_late(struct device *dev) · line 888
Quoted 12 lines
drivers/acpi/device_pm.c +7 -3
        target_state = acpi_target_system_state();
        wakeup = device_may_wakeup(dev);
-       error = acpi_device_wakeup(adev, target_state, wakeup);
-       if (wakeup && error)
-               return error;
+       can_wakeup = acpi_device_can_wakeup(adev);
+
+       if (can_wakeup) {
+               error = acpi_device_wakeup(adev, target_state, wakeup);
+               if (wakeup && error)
+                       return error;
+       } else if (wakeup) {
I think we just need to return an error code in that case, because otherwise
We used to return error and that error aborted the suspend altogether, which prompted creating this patch.
Quoted 2 lines
this is potentially dangerous (worst case, it may be impossible to wake up the machine at all after that).
Quoted 6 lines
Yes, there is such potential, but that kind of error (no working wakeup sources) will be discovered before a box is shipped. Right now we have boxes in the wild that suspend fine with 3.10 and refuse to suspend with 3.14 because between 3.10 and 3.14 we started placing i2c devices with ACPI companions into ACPI power domain and ACPI power domain is now trying to configure them as wakeup sources and fails.
Quoted 3 lines
A gentle ping on the patch - without it (or something else) we basically have a regression on shipped hardware: Chromebooks that were suspending fine with 3.10 refuse to suspend with 3.14.

Quoted 6 lines

It fell of my radar, sorry about that.
So the error here is that device_may_wakeup(dev) returns true, because the device is technically wakeup-capable, but the wakeup is not via ACPI?
I'd say this is rather not in accordance with the spec,

Well, specs, they are mostly like... guidelines ;)

Quoted 4 lines

but that means we
need to simply ignore 'wakeup' if acpi_device_can_wakeup(adev) returns false.
So what about the appended patch?

Would work as well with the difference that your version is silent so next time firmware engineers come up with similar stuff it will stay unnoticed.

I am happy with either version.

Thanks.

Signature · 1 line

Dmitry

Rafael J. Wysocki

2014-11-18 23:39 UTC #6 lore raw

Quoted 83 lines · Dmitry Torokhov

Quoted 82 lines · Rafael J. Wysocki
Quoted 80 lines · Dmitry Torokhov
Hi Rafael,
Quoted 67 lines · Dmitry Torokhov
Hi Rafael,
Quoted 43 lines · Rafael J. Wysocki
Hi,
Quoted 40 lines · Dmitry Torokhov
Newer kernels put i2c devices with ACPI companion in ACPI power domain and then ACPI will try to configure them for wakeup (if requested). Unfortunately on some Chromebooks firmware separates wakeup GPIO into a completely separate device (which is handled by the kernel as a sleep button), leaving the touchpads themselves not wakeup capable (as far as ACPI is concerned). This causes ACPI late suspend code to fail to configure them as wakeup sources and aborts entire suspend.
To work around this issues let's not abort entire suspend process if driver asked to be a wakeup source but ACPI can not satisfy that request.
Note that originally I tried to simply change the driver to not mark device as wakeup source, unfortunately then we do not know that we should not be powering down the device completely, otherwise we can't wake up.
Verified by making sure that "echo mem > /sys/power/state" works on Squawks.
Reviewed-by Benson Leung <[email protected]>
Signed-off-by Dmitry Torokhov <[email protected]>
---
 drivers/acpi/device_pm.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)
drivers/acpi/device_pm.c +1 -0
index 67075f8..440bc3d 100644
@@ -871,6 +871,7 @@ int acpi_dev_suspend_late(struct device *dev)
        struct acpi_device *adev = ACPI_COMPANION(dev);
        u32 target_state;
        bool wakeup;
+       bool can_wakeup;
        int error;
 
        if (!adev)
@@ -878,12 +879,19 @@ int acpi_dev_suspend_late(struct device *dev)
 
Quoted 14 lines
drivers/acpi/device_pm.c int acpi_dev_suspend_late(struct device *dev) · line 888
Quoted 12 lines
drivers/acpi/device_pm.c +7 -3
        target_state = acpi_target_system_state();
        wakeup = device_may_wakeup(dev);
-       error = acpi_device_wakeup(adev, target_state, wakeup);
-       if (wakeup && error)
-               return error;
+       can_wakeup = acpi_device_can_wakeup(adev);
+
+       if (can_wakeup) {
+               error = acpi_device_wakeup(adev, target_state, wakeup);
+               if (wakeup && error)
+                       return error;
+       } else if (wakeup) {
I think we just need to return an error code in that case, because otherwise
We used to return error and that error aborted the suspend altogether, which prompted creating this patch.
Quoted 2 lines
this is potentially dangerous (worst case, it may be impossible to wake up the machine at all after that).
Quoted 6 lines
Yes, there is such potential, but that kind of error (no working wakeup sources) will be discovered before a box is shipped. Right now we have boxes in the wild that suspend fine with 3.10 and refuse to suspend with 3.14 because between 3.10 and 3.14 we started placing i2c devices with ACPI companions into ACPI power domain and ACPI power domain is now trying to configure them as wakeup sources and fails.
A gentle ping on the patch - without it (or something else) we basically have a regression on shipped hardware: Chromebooks that were suspending fine with 3.10 refuse to suspend with 3.14.

Quoted 8 lines

Quoted 6 lines
It fell of my radar, sorry about that.
So the error here is that device_may_wakeup(dev) returns true, because the device is technically wakeup-capable, but the wakeup is not via ACPI?
I'd say this is rather not in accordance with the spec,
Well, specs, they are mostly like... guidelines ;)

Just don't tell me ...

Quoted 7 lines

Quoted 4 lines
but that means we
need to simply ignore 'wakeup' if acpi_device_can_wakeup(adev) returns false.
So what about the appended patch?
Would work as well with the difference that your version is silent so next time firmware engineers come up with similar stuff it will stay unnoticed.

We need to accept this kind of breakage already and the warning can easily get ignored during validation ("wakes up" == "works"). Past that, the firmware engineers in question don't care any more.

Quoted 1 line

I am happy with either version.

OK, I'll apply the simpler one, then. :-)

Rafael

Rafael J. Wysocki

2014-11-18 23:59 UTC #7 lore raw

Quoted 103 lines · Rafael J. Wysocki

Quoted 83 lines · Dmitry Torokhov
Quoted 82 lines · Rafael J. Wysocki
Quoted 80 lines · Dmitry Torokhov
Hi Rafael,
Quoted 67 lines · Dmitry Torokhov
Hi Rafael,
Quoted 43 lines · Rafael J. Wysocki
Hi,
Quoted 40 lines · Dmitry Torokhov
Newer kernels put i2c devices with ACPI companion in ACPI power domain and then ACPI will try to configure them for wakeup (if requested). Unfortunately on some Chromebooks firmware separates wakeup GPIO into a completely separate device (which is handled by the kernel as a sleep button), leaving the touchpads themselves not wakeup capable (as far as ACPI is concerned). This causes ACPI late suspend code to fail to configure them as wakeup sources and aborts entire suspend.
To work around this issues let's not abort entire suspend process if driver asked to be a wakeup source but ACPI can not satisfy that request.
Note that originally I tried to simply change the driver to not mark device as wakeup source, unfortunately then we do not know that we should not be powering down the device completely, otherwise we can't wake up.
Verified by making sure that "echo mem > /sys/power/state" works on Squawks.
Reviewed-by Benson Leung <[email protected]>
Signed-off-by Dmitry Torokhov <[email protected]>
---
 drivers/acpi/device_pm.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)
drivers/acpi/device_pm.c +1 -0
index 67075f8..440bc3d 100644
@@ -871,6 +871,7 @@ int acpi_dev_suspend_late(struct device *dev)
        struct acpi_device *adev = ACPI_COMPANION(dev);
        u32 target_state;
        bool wakeup;
+       bool can_wakeup;
        int error;
 
        if (!adev)
@@ -878,12 +879,19 @@ int acpi_dev_suspend_late(struct device *dev)
 
Quoted 14 lines
drivers/acpi/device_pm.c int acpi_dev_suspend_late(struct device *dev) · line 888
Quoted 12 lines
drivers/acpi/device_pm.c +7 -3
        target_state = acpi_target_system_state();
        wakeup = device_may_wakeup(dev);
-       error = acpi_device_wakeup(adev, target_state, wakeup);
-       if (wakeup && error)
-               return error;
+       can_wakeup = acpi_device_can_wakeup(adev);
+
+       if (can_wakeup) {
+               error = acpi_device_wakeup(adev, target_state, wakeup);
+               if (wakeup && error)
+                       return error;
+       } else if (wakeup) {
I think we just need to return an error code in that case, because otherwise
We used to return error and that error aborted the suspend altogether, which prompted creating this patch.
Quoted 2 lines
this is potentially dangerous (worst case, it may be impossible to wake up the machine at all after that).
Quoted 6 lines
Yes, there is such potential, but that kind of error (no working wakeup sources) will be discovered before a box is shipped. Right now we have boxes in the wild that suspend fine with 3.10 and refuse to suspend with 3.14 because between 3.10 and 3.14 we started placing i2c devices with ACPI companions into ACPI power domain and ACPI power domain is now trying to configure them as wakeup sources and fails.
A gentle ping on the patch - without it (or something else) we basically have a regression on shipped hardware: Chromebooks that were suspending fine with 3.10 refuse to suspend with 3.14.
Quoted 8 lines
Quoted 6 lines
It fell of my radar, sorry about that.
So the error here is that device_may_wakeup(dev) returns true, because the device is technically wakeup-capable, but the wakeup is not via ACPI?
I'd say this is rather not in accordance with the spec,
Well, specs, they are mostly like... guidelines ;)
Just don't tell me ...
Quoted 7 lines
Quoted 4 lines
but that means we
need to simply ignore 'wakeup' if acpi_device_can_wakeup(adev) returns false.
So what about the appended patch?
Would work as well with the difference that your version is silent so next time firmware engineers come up with similar stuff it will stay unnoticed.

Quoted 7 lines

We need to accept this kind of breakage already and the warning can easily get ignored during validation ("wakes up" == "works"). Past that, the firmware engineers in question don't care any more.
Quoted 1 line
I am happy with either version.
OK, I'll apply the simpler one, then. :-)

It goes again below with a changelog, but since technically it is a regression fix, I'd like to push it for "stable" too. I suppose that the commit that things stopped working after is a76e9bd89ae7 (i2c: attach/detach I2C client device to the ACPI power domain). Is that correct?

Rafael

--
From: Rafael J. Wysocki <[email protected]>
Subject: ACPI / PM: Ignore wakeup setting if the ACPI companion can't wake up

As reported by Dmitry, on some Chromebooks there are devices with corresponding ACPI objects and with unusual system wakeup configuration. Namely, they technically are wakeup-capable, but the wakeup is handled via a platform-specific out-of-band mechanism rather than by standard ACPI means. As a result, the device_may_wakeup(dev) called from acpi_dev_suspend_late() returns 'true' for them, but the wakeup.flags.valid flag is unset for the corresponding ACPI device objects, so acpi_dev_suspend_late() reproducibly returns an error for them causing the entire system suspend to be aborted. Consequently, system suspend doesn't work on those machines at all.

Address the problem by ignoring the device_may_wakeup(dev) return value in acpi_dev_suspend_late() if the ACPI companion of the device in question has wakeup.flags.valid unset (in which case it is clear that the wakeup cannot happen via ACPI).

Reported-by Dmitry Torokhov <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/device_pm.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/acpi/device_pm.c +1 -1

Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
@@ -878,7 +878,7 @@ int acpi_dev_suspend_late(struct device
 		return 0;
 
 	target_state = acpi_target_system_state();
-	wakeup = device_may_wakeup(dev);
+	wakeup = device_may_wakeup(dev) && acpi_device_can_wakeup(adev);
 	error = acpi_device_wakeup(adev, target_state, wakeup);
 	if (wakeup && error)
 		return error;

Dmitry Torokhov

author

2014-11-19 00:14 UTC #8 lore raw

Quoted 4 lines · Rafael J. Wysocki

It goes again below with a changelog, but since technically it is a regression fix, I'd like to push it for "stable" too. I suppose that the commit that things stopped working after is a76e9bd89ae7 (i2c: attach/detach I2C client device to the ACPI power domain). Is that correct?

Yes.

Quoted 8 lines

Rafael
--
From: Rafael J. Wysocki <[email protected]>
Subject: ACPI / PM: Ignore wakeup setting if the ACPI companion can't wake up

Quoted 5 lines

As reported by Dmitry, on some Chromebooks there are devices with corresponding ACPI objects and with unusual system wakeup configuration. Namely, they technically are wakeup-capable, but the wakeup is handled via a platform-specific out-of-band mechanism rather than by standard ACPI means.

I think they are using standard ACPI wakeup methods, but in a very perverted
way: there is "shadow" ACPI sleep button corresponding to the GPIO assigned to the trackpad or touchscreen; it is just not tied to touchpad/touchscreen device in DSDT.

We won't have such set up again, but we do have existing boxes that are like that.

Quoted 33 lines

As a result, the
device_may_wakeup(dev) called from acpi_dev_suspend_late() returns 'true' for them, but the wakeup.flags.valid flag is unset for the corresponding ACPI device objects, so acpi_dev_suspend_late() reproducibly returns an error for them causing the entire system suspend to be aborted. Consequently, system suspend doesn't work on those machines at all.
Address the problem by ignoring the device_may_wakeup(dev) return value in acpi_dev_suspend_late() if the ACPI companion of the device in question has wakeup.flags.valid unset (in which case it is clear that the wakeup cannot happen via ACPI).
Reported-by Dmitry Torokhov <[email protected]>
Signed-off-by Rafael J. Wysocki <[email protected]>
---
 drivers/acpi/device_pm.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
drivers/acpi/device_pm.c +1 -1
@@ -878,7 +878,7 @@ int acpi_dev_suspend_late(struct device
                return 0;
 
        target_state = acpi_target_system_state();
-       wakeup = device_may_wakeup(dev);
+       wakeup = device_may_wakeup(dev) && acpi_device_can_wakeup(adev);
        error = acpi_device_wakeup(adev, target_state, wakeup);
        if (wakeup && error)
                return error;

Rafael J. Wysocki

2014-11-19 00:40 UTC #9 lore raw

Quoted 7 lines · Dmitry Torokhov

Quoted 4 lines · Rafael J. Wysocki
It goes again below with a changelog, but since technically it is a regression fix, I'd like to push it for "stable" too. I suppose that the commit that things stopped working after is a76e9bd89ae7 (i2c: attach/detach I2C client device to the ACPI power domain). Is that correct?
Yes.

Quoted 10 lines

Quoted 9 lines
--
From: Rafael J. Wysocki <[email protected]>
Subject: ACPI / PM: Ignore wakeup setting if the ACPI companion can't wake up
As reported by Dmitry, on some Chromebooks there are devices with corresponding ACPI objects and with unusual system wakeup configuration. Namely, they technically are wakeup-capable, but the wakeup is handled via a platform-specific out-of-band mechanism rather than by standard ACPI means.

Quoted 4 lines

I think they are using standard ACPI wakeup methods, but in a very perverted
way: there is "shadow" ACPI sleep button corresponding to the GPIO assigned to the trackpad or touchscreen; it is just not tied to touchpad/touchscreen device in DSDT.

This:

---
From: Rafael J. Wysocki <[email protected]>
Subject: ACPI / PM: Ignore wakeup setting if the ACPI companion can't wake up

As reported by Dmitry, on some Chromebooks there are devices with corresponding ACPI objects and with unusual system wakeup configuration. Namely, they technically are wakeup-capable, but the wakeup is handled via a platform-specific out-of-band mechanism and the ACPI PM layer has no information on the wakeup capability. As a result, device_may_wakeup(dev) called from acpi_dev_suspend_late() returns 'true' for those devices, but the wakeup.flags.valid flag is unset for the corresponding ACPI device objects, so acpi_device_wakeup() reproducibly fails for them causing acpi_dev_suspend_late() to return an error code. The entire system suspend is then aborted and the machines in question cannot suspend at all.

Address the problem by ignoring the device_may_wakeup(dev) return value in acpi_dev_suspend_late() if the ACPI companion of the device being handled has wakeup.flags.valid unset (in which case it is clear that the wakeup is supposed to be handled by other means).

This fixes a regression introduced by commit a76e9bd89ae7 (i2c:
attach/detach I2C client device to the ACPI power domain) as the affected systems could suspend and resume successfully before that commit.

Fixes a76e9bd89ae7 (i2c: attach/detach I2C client device to the ACPI power domain)

Reported-by Dmitry Torokhov <[email protected]>

Cc 3.13+ <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/device_pm.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/acpi/device_pm.c +1 -1

Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
@@ -878,7 +878,7 @@ int acpi_dev_suspend_late(struct device
 		return 0;
 
 	target_state = acpi_target_system_state();
-	wakeup = device_may_wakeup(dev);
+	wakeup = device_may_wakeup(dev) && acpi_device_can_wakeup(adev);
 	error = acpi_device_wakeup(adev, target_state, wakeup);
 	if (wakeup && error)
 		return error;

Dmitry Torokhov

author Reviewed-by

2014-11-19 00:48 UTC #10 lore raw

Quoted 51 lines · Rafael J. Wysocki

Quoted 7 lines · Dmitry Torokhov
Quoted 4 lines · Rafael J. Wysocki
It goes again below with a changelog, but since technically it is a regression fix, I'd like to push it for "stable" too. I suppose that the commit that things stopped working after is a76e9bd89ae7 (i2c: attach/detach I2C client device to the ACPI power domain). Is that correct?
Yes.
OK
Quoted 10 lines
Quoted 9 lines
--
From: Rafael J. Wysocki <[email protected]>
Subject: ACPI / PM: Ignore wakeup setting if the ACPI companion can't wake up
As reported by Dmitry, on some Chromebooks there are devices with corresponding ACPI objects and with unusual system wakeup configuration. Namely, they technically are wakeup-capable, but the wakeup is handled via a platform-specific out-of-band mechanism rather than by standard ACPI means.
Quoted 4 lines
I think they are using standard ACPI wakeup methods, but in a very perverted
way: there is "shadow" ACPI sleep button corresponding to the GPIO assigned to the trackpad or touchscreen; it is just not tied to touchpad/touchscreen device in DSDT.
OK
This:
---
From: Rafael J. Wysocki <[email protected]>
Subject: ACPI / PM: Ignore wakeup setting if the ACPI companion can't wake up
As reported by Dmitry, on some Chromebooks there are devices with corresponding ACPI objects and with unusual system wakeup configuration. Namely, they technically are wakeup-capable, but the wakeup is handled via a platform-specific out-of-band mechanism and the ACPI PM layer has no information on the wakeup capability. As a result, device_may_wakeup(dev) called from acpi_dev_suspend_late() returns 'true' for those devices, but the wakeup.flags.valid flag is unset for the corresponding ACPI device objects, so acpi_device_wakeup() reproducibly fails for them causing acpi_dev_suspend_late() to return an error code. The entire system suspend is then aborted and the machines in question cannot suspend at all.
Address the problem by ignoring the device_may_wakeup(dev) return value in acpi_dev_suspend_late() if the ACPI companion of the device being handled has wakeup.flags.valid unset (in which case it is clear that the wakeup is supposed to be handled by other means).

Quoted 7 lines

This fixes a regression introduced by commit a76e9bd89ae7 (i2c:
attach/detach I2C client device to the ACPI power domain) as the affected systems could suspend and resume successfully before that commit.
Fixes a76e9bd89ae7 (i2c: attach/detach I2C client device to the ACPI power domain)
Reported-by Dmitry Torokhov <[email protected]>

Excellent, thank you Rafael.

Reviewed-by Dmitry Torokhov <[email protected]>

Quoted 20 lines

Cc 3.13+ <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/device_pm.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-pm/drivers/acpi/device_pm.c
===================================================================

drivers/acpi/device_pm.c +1 -1

@@ -878,7 +878,7 @@ int acpi_dev_suspend_late(struct device
                return 0;
 
        target_state = acpi_target_system_state();
-       wakeup = device_may_wakeup(dev);
+       wakeup = device_may_wakeup(dev) && acpi_device_can_wakeup(adev);
        error = acpi_device_wakeup(adev, target_state, wakeup);
        if (wakeup && error)
                return error;