Re: [Bugme-new] [Bug 13243] New: regression from 2.6.29 : can't suspend on a compaq nc6000, suspend_device(): pnp_bus_suspend+0x0/0x6b returns -5

Re: [Bugme-new] [Bug 13243] New: regression from 2.6.29 : can't suspend on a compaq nc6000, suspend_device(): pnp_bus_suspend+0x0/0x6b returns -5

[yakui_zhao]

On Thu, 2009-05-07 at 06:16 +0800, Bjorn Helgaas wrote:
> On Wednesday 06 May 2009 03:24:29 pm Witold Szczeponik wrote:
> > Andrew Morton schrieb:
> > > On Mon, 4 May 2009 15:03:54 GMT
> > > bugzilla-daemon@bugzilla.kernel.org wrote:
> > > 
> > >> http://bugzilla.kernel.org/show_bug.cgi?id=13243
> > 
> > Zhao, Len,
> > 
> > I tried to verify the problem on my TP 600E: suspend worked without any 
> > problems.  I will install the latest RC asap, but as the 600E is a 
> > relatively slow machine, this will take time. :-) Once I have a 
> > 2.6.30-rc4 kernel, I will update this message.
> > 
> > However, I do not think that the problem lies within the PNPACPI layer. 
> >   The code that was included in my patch seems to expose the behavior we 
> > are seeing, not causing it.  Looking at the report in bug 13243, the 
> > ACPI layer is not able to transition the device (a COMx port) from 
> > "whatever" to D3.  This is indicated by message "ACPI: Transitioning 
> > device [C169] to D3" (the message is misleadig: it is displayed only 
> > when the transition failed).  My patch returns the error code to the PNP 
> > layer which is trying to suspend the machine.  It is here where the 
> > error is exposed.
> > 
> > Björn, I CC-ed you because this might be caused by something in the PNP 
> > layer.
> 
> Cedric identified 6328a57401dc5f5c as the point where this broke.
> 
> Prior to that commit, pnpacpi_disable_resources() did not set the
> power state of the device to D3.  After that commit, we try to set
> the device to D3 and return an error if that fails
> 
> In this case, the C169 device has _PR0 but no _PSx methods.  I don't
> think it makes sense to try to set such a device to D3, so we probably
> shouldn't return an error.
What you said is right. After applying that commit, the C169 device will
be switched to D3. And when the C169 device is switched to D3, the power
resource(C16D) will be turned off.
   But unfortunately the _OFF object of C16D is bogus. In such case the
_STA method can't reflect the correct status of power resource and OS
will complain that the C169 device can't be switched to D3 state.
   > Method (_OFF, 0, NotSerialized)
   >                         {
                                Store (0x00, Local0)
   >                         }
	
   
   If we add the boot option of "acpi.power_nocheck=1", the status check
will be skipped in course of D0/D3 state transition.

Hi, Cedric
    will you please try the boot option of "acpi.power_nocheck=1" and
see whether the issue still exists?
    Please also attach the output of dmidecode so that we add the box to
dmi check table.

    Thanks.
 
> 
> I don't know if that means acpi_bus_get_flags() should stop looking
> for _PR0, or if we should get rid of the power_manageable flag, or
> if acpi_bus_set_power() should silently succeed if the device doesn't
> support power states, or what.
> 
> Bjorn

--
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: [Bugme-new] [Bug 13243] New: regression from 2.6.29 : can't suspend on a compaq nc6000, suspend_device(): pnp_bus_suspend+0x0/0x6b returns -5

[Witold Szczeponik]

yakui_zhao schrieb:

> What you said is right. After applying that commit, the C169 device will
> be switched to D3. And when the C169 device is switched to D3, the power
> resource(C16D) will be turned off.
>    But unfortunately the _OFF object of C16D is bogus. In such case the
> _STA method can't reflect the correct status of power resource and OS
> will complain that the C169 device can't be switched to D3 state.
>    > Method (_OFF, 0, NotSerialized)
>    >                         {
>                                 Store (0x00, Local0)
>    >                         }

I think this is the key to understanding what is going on here: it seems 
as if the BIOS does not allow for disabling the power resource through 
ACPI.  I suspect the Linux' ACPI core tries to set the power resource to 
D3, then evaluates _STA, and relizes that something went wrong.  But I 
am guessing here...  Even more speculation: Windows either does not care 
or makes the transition from D0 to D3 only once...

--- Witold

> 	
>    
>    If we add the boot option of "acpi.power_nocheck=1", the status check
> will be skipped in course of D0/D3 state transition.
> 

Re: [Bugme-new] [Bug 13243] New: regression from 2.6.29 : can't suspend on a compaq nc6000, suspend_device(): pnp_bus_suspend+0x0/0x6b returns -5

[yakui_zhao]

On Fri, 2009-05-08 at 03:05 +0800, Witold Szczeponik wrote:
> yakui_zhao schrieb:
> 
> > What you said is right. After applying that commit, the C169 device will
> > be switched to D3. And when the C169 device is switched to D3, the power
> > resource(C16D) will be turned off.
> >    But unfortunately the _OFF object of C16D is bogus. In such case the
> > _STA method can't reflect the correct status of power resource and OS
> > will complain that the C169 device can't be switched to D3 state.
> >    > Method (_OFF, 0, NotSerialized)
> >    >                         {
> >                                 Store (0x00, Local0)
> >    >                         }
> 
> I think this is the key to understanding what is going on here: it seems 
> as if the BIOS does not allow for disabling the power resource through 
> ACPI.  I suspect the Linux' ACPI core tries to set the power resource to 
> D3, then evaluates _STA, and relizes that something went wrong.  But I 
> am guessing here...  Even more speculation: Windows either does not care 
> or makes the transition from D0 to D3 only once..
Your guess is right.
If there is no boot option of "acpi.power_nocheck=1", the _STA method
will be evaluated to check whether the power resource is switched to the
correct state after the _ON/_OFF object is evaluated.

if the boot option of "acpi.power_nocheck=1" is added, the status check
will be skipped after the _ON/_OFF object is evaluated.

Thanks.
> .
> 
> --- Witold
> 
> > 	
> >    
> >    If we add the boot option of "acpi.power_nocheck=1", the status check
> > will be skipped in course of D0/D3 state transition.
> > 
> 
> 

[PATCH] ACPI: suspend: don't let device _PS3 failure prevent suspend

[Len Brown]

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

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

Re: [PATCH] ACPI: suspend: don't let device _PS3 failure prevent suspend

[Henrique de Moraes Holschuh]

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?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] ACPI: suspend: don't let device _PS3 failure prevent suspend

[Witold Szczeponik]

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

Re: [PATCH] ACPI: suspend: don't let device _PS3 failure prevent suspend

[yakui_zhao]

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

Re: [PATCH] ACPI: suspend: don't let device _PS3 failure prevent suspend

[Rafael J. Wysocki]

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

Re: [PATCH] ACPI: suspend: don't let device _PS3 failure prevent suspend

[yakui_zhao]

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

Re: [PATCH] ACPI: suspend: don't let device _PS3 failure prevent suspend

[Matthew Garrett]

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?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] ACPI: suspend: don't let device _PS3 failure prevent suspend

[yakui_zhao]

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


> 

Re: [PATCH] ACPI: suspend: don't let device _PS3 failure prevent suspend

[Matthew Garrett]

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.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] ACPI: suspend: don't let device _PS3 failure prevent suspend

[yakui_zhao]

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.
> 

Re: [PATCH] ACPI: suspend: don't let device _PS3 failure prevent suspend

[Matthew Garrett]

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!

-- 
Matthew Garrett | mjg59@srcf.ucam.org