New: regression from 2.6.29 : can't suspend on a compaq nc6000, suspend_device(): pnp_bus_suspend+0x0/0x6b returns -5

Bugme-new In review yakui_zhao 6 participants 14 messages May 7, 2009 – May 12, 2009 4 messages not in the archive

Subsystems (2)

ACPI Pnp support

lore

Share & tools

lore

yakui_zhao

author

2009-05-07 01:42 UTC #4 lore raw

In reply to a message not in the archive

Quoted 29 lines · Bjorn Helgaas

Quoted 23 lines · Witold Szczeponik
Quoted 4 lines · Andrew Morton
Quoted 1 line · On Mon, 4 May 2009 15:03:54 GMT [email protected] 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.
Quoted 2 lines
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.

Quoted 7 lines

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.

Quoted 2 lines

Method (_OFF, 0, NotSerialized)

                                Store (0x00, Local0)

Quoted 1 line

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.

Quoted 7 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Witold Szczeponik

2009-05-07 19:05 UTC #5 lore raw

Quoted 10 lines · yakui_zhao

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.
Quoted 2 lines
Method (_OFF, 0, NotSerialized)
                        {
                                Store (0x00, Local0)
Quoted 1 line
                        }

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

Quoted 5 lines

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

yakui_zhao

author

2009-05-08 01:29 UTC #6 lore raw

Quoted 13 lines · Witold Szczeponik

Quoted 10 lines · yakui_zhao
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.
Quoted 2 lines
Method (_OFF, 0, NotSerialized)
                        {
                                Store (0x00, Local0)
Quoted 1 line
                        }

Quoted 6 lines

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.

Quoted 11 lines

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

Len Brown

reviewer

2009-05-08 04:40 UTC #7 lore raw

PATCH 1/1 ACPI: suspend: don't let device _PS3 failure prevent suspend

In reply to Witold Szczeponik

From: Len Brown <[email protected]>

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 <[email protected]>

---
 drivers/pnp/pnpacpi/core.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

drivers/pnp/pnpacpi/core.c +3 -5

index 9a3a682..9496494 100644
@@ -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;

Signature · 1 line

1.6.3

Henrique de Moraes Holschuh

2009-05-08 12:36 UTC #8 lore raw

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

Quoted 3 lines · Len Brown

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?

Signature · 4 lines

  "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

Witold Szczeponik

2009-05-10 20:48 UTC #9 lore raw

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

Quoted 7 lines · Henrique de Moraes Holschuh

Quoted 3 lines · Len Brown
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

yakui_zhao

author

2009-05-11 02:41 UTC #10 lore raw

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

Quoted 7 lines · Witold Szczeponik

Quoted 5 lines · Henrique de Moraes Holschuh
Quoted 3 lines · Len Brown
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.

Quoted 6 lines

Quoted 2 lines
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.

Quoted 2 lines

--- Witold

Rafael J. Wysocki

2009-05-11 15:21 UTC #11 lore raw

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

Quoted 14 lines · yakui_zhao

Quoted 7 lines · Witold Szczeponik
Quoted 5 lines · Henrique de Moraes Holschuh
Quoted 3 lines · Len Brown
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.
Quoted 6 lines
Quoted 2 lines
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.

Quoted 6 lines

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

yakui_zhao

author

2009-05-12 00:46 UTC #12 lore raw

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

Quoted 19 lines · Rafael J. Wysocki

Quoted 18 lines · yakui_zhao
Quoted 7 lines · Witold Szczeponik
Quoted 5 lines · Henrique de Moraes Holschuh
Quoted 3 lines · Len Brown
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.
Quoted 6 lines
Quoted 2 lines
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.

Quoted 5 lines

Quoted 2 lines
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?

Quoted 2 lines

Rafael

Matthew Garrett

2009-05-12 02:02 UTC #13 lore raw

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

Quoted 4 lines · yakui_zhao

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?

Signature · 1 line

Matthew Garrett | [email protected]

yakui_zhao

author

2009-05-12 02:24 UTC #14 lore raw

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

Quoted 9 lines · Matthew Garrett

Quoted 4 lines · yakui_zhao
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

Quoted 1 line

Matthew Garrett

2009-05-12 02:50 UTC #15 lore raw

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

Quoted 6 lines · yakui_zhao

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.

Quoted 3 lines

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.

Signature · 1 line

Matthew Garrett | [email protected]

yakui_zhao

author

2009-05-12 03:04 UTC #16 lore raw

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

Quoted 5 lines · Matthew Garrett

Quoted 4 lines · yakui_zhao
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.

Quoted 8 lines

Quoted 2 lines
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.

Quoted 8 lines

Quoted 4 lines
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

2009-05-12 04:12 UTC #17 lore raw

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

Quoted 3 lines · yakui_zhao

It will be OK to change the default value of "power_nocheck" to 1.
I will do this.

Sounds good. Thanks!

Signature · 1 line

Matthew Garrett | [email protected]