diff mbox

ACPI: suspend: don't let device _PS3 failure prevent suspend

Message ID alpine.LFD.2.00.0905080037240.20287@localhost.localdomain (mailing list archive)
State Accepted
Headers show

Commit Message

Len Brown May 8, 2009, 4:39 a.m. UTC
From: Len Brown <len.brown@intel.com>

6328a57401dc5f5cf9931738eb7268fcd8058c49
"Enable PNPACPI _PSx Support, v3"

added a call to acpi_bus_set_power(handle, ACPI_STATE_D3)
to pnpacpi_disable_resource() before the existing call
to evaluate _DIS on the device.

This caused suspend to fail on the system in
http://bugzilla.kernel.org/show_bug.cgi?id=13243
because the sanity check to verify we entered _PS3
failed on the serial port.

As a work-around, that sanity check can be disabled
system-wide with "acpi.power_nocheck=1"

Or perhaps we should just shrug off the _PS3 failure
and carry on with _DIS like we used to -- which is
what this patch does.

Signed-off-by: Len Brown <len.brown@intel.com>
---
 drivers/pnp/pnpacpi/core.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

Comments

Henrique de Moraes Holschuh May 8, 2009, 12:23 p.m. UTC | #1
On Fri, 08 May 2009, Len Brown wrote:
> Or perhaps we should just shrug off the _PS3 failure
> and carry on with _DIS like we used to -- which is
> what this patch does.

Maybe "printk and carry on" would be better?
Witold Szczeponik May 10, 2009, 8:48 p.m. UTC | #2
Henrique de Moraes Holschuh schrieb:

> On Fri, 08 May 2009, Len Brown wrote:
>> Or perhaps we should just shrug off the _PS3 failure
>> and carry on with _DIS like we used to -- which is
>> what this patch does.
> 
> Maybe "printk and carry on" would be better?
> 

And there is very similar code just a few lines above: where we turn on 
the device by setting it to D0.  There, too, we may want to skip the 
check whether or not the transition was successful or not.

--- Witold
--
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
Zhao, Yakui May 11, 2009, 2:43 a.m. UTC | #3
On Mon, 2009-05-11 at 04:48 +0800, Witold Szczeponik wrote:
> Henrique de Moraes Holschuh schrieb:
> 
> > On Fri, 08 May 2009, Len Brown wrote:
> >> Or perhaps we should just shrug off the _PS3 failure
> >> and carry on with _DIS like we used to -- which is
> >> what this patch does.
> > 
> > Maybe "printk and carry on" would be better?
> > 
> 
> And there is very similar code just a few lines above: where we turn on 
> the device by setting it to D0.  There, too, we may want to skip the 
> check whether or not the transition was successful or not.
What you said is right.
It sounds reasonable that we skip the power state check while transiting
it to D0 state.

In fact the power state check can be skipped by adding the boot option
of "acpi.power_nocheck=1".

thanks.
> 
> --- Witold

--
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
Rafael Wysocki May 11, 2009, 3:21 p.m. UTC | #4
On Monday 11 May 2009, yakui_zhao wrote:
> On Mon, 2009-05-11 at 04:48 +0800, Witold Szczeponik wrote:
> > Henrique de Moraes Holschuh schrieb:
> > 
> > > On Fri, 08 May 2009, Len Brown wrote:
> > >> Or perhaps we should just shrug off the _PS3 failure
> > >> and carry on with _DIS like we used to -- which is
> > >> what this patch does.
> > > 
> > > Maybe "printk and carry on" would be better?
> > > 
> > 
> > And there is very similar code just a few lines above: where we turn on 
> > the device by setting it to D0.  There, too, we may want to skip the 
> > check whether or not the transition was successful or not.
> What you said is right.
> It sounds reasonable that we skip the power state check while transiting
> it to D0 state.
> 
> In fact the power state check can be skipped by adding the boot option
> of "acpi.power_nocheck=1".

Can we avoid adding the boot option?  I'd very much prefer not to add boot
options if not really necessary.

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
Zhao, Yakui May 12, 2009, 12:48 a.m. UTC | #5
On Mon, 2009-05-11 at 23:21 +0800, Rafael J. Wysocki wrote:
> On Monday 11 May 2009, yakui_zhao wrote:
> > On Mon, 2009-05-11 at 04:48 +0800, Witold Szczeponik wrote:
> > > Henrique de Moraes Holschuh schrieb:
> > > 
> > > > On Fri, 08 May 2009, Len Brown wrote:
> > > >> Or perhaps we should just shrug off the _PS3 failure
> > > >> and carry on with _DIS like we used to -- which is
> > > >> what this patch does.
> > > > 
> > > > Maybe "printk and carry on" would be better?
> > > > 
> > > 
> > > And there is very similar code just a few lines above: where we turn on 
> > > the device by setting it to D0.  There, too, we may want to skip the 
> > > check whether or not the transition was successful or not.
> > What you said is right.
> > It sounds reasonable that we skip the power state check while transiting
> > it to D0 state.
> > 
> > In fact the power state check can be skipped by adding the boot option
> > of "acpi.power_nocheck=1".
> 
> Can we avoid adding the boot option?  I'd very much prefer not to add boot
> options if not really necessary.
One is to add it into the DMI power check table so that the default
value of power_nocheck is 1. 

Another is that the default value of power_nocheck is changed to 1
instead of 0. 

In such case the power state check will be skipped in course of power
transition.

Is this OK?
> 
> 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
Matthew Garrett May 12, 2009, 2:01 a.m. UTC | #6
On Tue, May 12, 2009 at 08:48:25AM +0800, yakui_zhao wrote:

> In such case the power state check will be skipped in course of power
> transition.
> 
> Is this OK?

What's the real-world benefit to throwing an error in this case? What is 
the user or software supposed to do with it?
Zhao, Yakui May 12, 2009, 2:26 a.m. UTC | #7
On Tue, 2009-05-12 at 10:01 +0800, Matthew Garrett wrote:
> On Tue, May 12, 2009 at 08:48:25AM +0800, yakui_zhao wrote:
> 
> > In such case the power state check will be skipped in course of power
> > transition.
> > 
> > Is this OK?
> 
> What's the real-world benefit to throwing an error in this case? What is 
> the user or software supposed to do with it?
In fact this error is caused by the BIOS. And it tells us that such
issue had better be fixed by BIOS upgrading. 
For example: on the HP nc6000 box. The _OFF object of the power resource
is bogus.  And the _STA object can't reflect the correct status of the
power resource. 
   
Windows can work well on such broken box. And I find that the _STA
object of power resource is not called in course of power transition
with the help of KVM.

To be compatible with windows, we add such a
workaround("acpi.power_nocheck=1") to fix this issue.

If the power state check is always skipped in course of power
transition, there is no such error message. But it can't tell us that
this is a broken BIOS.

Best regards.
   Yakui


> 

--
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
Matthew Garrett May 12, 2009, 2:50 a.m. UTC | #8
On Tue, May 12, 2009 at 10:26:12AM +0800, yakui_zhao wrote:
> Windows can work well on such broken box. And I find that the _STA
> object of power resource is not called in course of power transition
> with the help of KVM.
> 
> To be compatible with windows, we add such a
> workaround("acpi.power_nocheck=1") to fix this issue.

If Windows doesn't call _STA when performing these power transitions 
then the default behaviour of Linux should be not to call _STA. We 
strive to maintain compatibility with Windows when it comes to driving 
the hardware - that means not forcing the user to add a boot option or 
trying to maintain a DMI table.

> If the power state check is always skipped in course of power
> transition, there is no such error message. But it can't tell us that
> this is a broken BIOS.

Failing someone's suspend in order to inform them that their ancient 
hardware has a broken BIOS doesn't seem like useful behaviour.
Zhao, Yakui May 12, 2009, 3:06 a.m. UTC | #9
On Tue, 2009-05-12 at 10:50 +0800, Matthew Garrett wrote:
> On Tue, May 12, 2009 at 10:26:12AM +0800, yakui_zhao wrote:
> > Windows can work well on such broken box. And I find that the _STA
> > object of power resource is not called in course of power transition
> > with the help of KVM.
> > 
> > To be compatible with windows, we add such a
> > workaround("acpi.power_nocheck=1") to fix this issue.
> 
> If Windows doesn't call _STA when performing these power transitions 
> then the default behaviour of Linux should be not to call _STA. We 
> strive to maintain compatibility with Windows when it comes to driving 
> the hardware - that means not forcing the user to add a boot option or 
> trying to maintain a DMI table.
It will be OK to change the default value of "power_nocheck" to 1.

I will do this.
> 
> > If the power state check is always skipped in course of power
> > transition, there is no such error message. But it can't tell us that
> > this is a broken BIOS.
> 
> Failing someone's suspend in order to inform them that their ancient 
> hardware has a broken BIOS doesn't seem like useful behaviour.
> 

--
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
Matthew Garrett May 12, 2009, 4:11 a.m. UTC | #10
On Tue, May 12, 2009 at 11:06:14AM +0800, yakui_zhao wrote:
> It will be OK to change the default value of "power_nocheck" to 1.
> 
> I will do this.

Sounds good. Thanks!
diff mbox

Patch

diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
index 9a3a682..9496494 100644
--- a/drivers/pnp/pnpacpi/core.c
+++ b/drivers/pnp/pnpacpi/core.c
@@ -110,11 +110,9 @@  static int pnpacpi_disable_resources(struct pnp_dev *dev)
 
 	/* acpi_unregister_gsi(pnp_irq(dev, 0)); */
 	ret = 0;
-	if (acpi_bus_power_manageable(handle)) {
-		ret = acpi_bus_set_power(handle, ACPI_STATE_D3);
-		if (ret)
-			return ret;
-	}
+	if (acpi_bus_power_manageable(handle))
+		acpi_bus_set_power(handle, ACPI_STATE_D3);
+		/* continue even if acpi_bus_set_power() fails */
 	if (ACPI_FAILURE(acpi_evaluate_object(handle, "_DIS", NULL, NULL)))
 		ret = -ENODEV;
 	return ret;