: ACPI: Skip the power state check in power transition

From: Zhao Yakui <[email protected]>

Skip the power state check in course of power transition by changing the default value of acpi_power_nocheck to 1. If so, it is unnecessary to add the boot option of "acpi.power_nocheck=1" or add it into DMI power check table to skip the power state check.

Of course the power state check still can be enabled by adding the boot option of "acpi.power_nocheck=0".

Index: linux-2.6/drivers/acpi/power.c
===================================================================

drivers/acpi/power.c +1 -2

@@ -54,7 +54,7 @@
 #define ACPI_POWER_RESOURCE_STATE_ON	0x01
 #define ACPI_POWER_RESOURCE_STATE_UNKNOWN 0xFF
 
-int acpi_power_nocheck;
+int acpi_power_nocheck = 1;
 module_param_named(power_nocheck, acpi_power_nocheck, bool, 000);
 
 static int acpi_power_add(struct acpi_device *device);
 
 
--

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

Quoted 7 lines · yakui_zhao

From: Zhao Yakui <[email protected]>

Skip the power state check in course of power transition by changing the default value of acpi_power_nocheck to 1. If so, it is unnecessary to add the boot option of "acpi.power_nocheck=1" or add it into DMI power check table to skip the power state check.

Quoted 2 lines

Of course the power state check still can be enabled by adding the boot option of "acpi.power_nocheck=0".

What is the value of keeping the "acpi.power_nocheck" option at all?

If we can get along without it, it'd be nice to just remove the whole thing.

Bjorn

Quoted 20 lines

Index: linux-2.6/drivers/acpi/power.c
===================================================================

drivers/acpi/power.c +1 -2

@@ -54,7 +54,7 @@
 #define ACPI_POWER_RESOURCE_STATE_ON	0x01
 #define ACPI_POWER_RESOURCE_STATE_UNKNOWN 0xFF
 
-int acpi_power_nocheck;
+int acpi_power_nocheck = 1;
 module_param_named(power_nocheck, acpi_power_nocheck, bool, 000);
 
 static int acpi_power_add(struct acpi_device *device);
 
 
--

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

In fact this object is defined in ACPI spec. And we had better follow that. IMO Linux ACPI does the right thing.
The boot option of "acpi.power_nocheck" is only to make Linux be compatible with windows.

The default behaviour should be to be compatible with Windows, regardless of what the spec says. There's an argument for providing a strict interpretation of the spec for testing purposes, but I don't see
any reason for it to be split up into dozens of individual kernel parameters

Quoted 6 lines · Matthew Garrett

The default behaviour should be to be compatible with Windows, regardless of what the spec says. There's an argument for providing a strict interpretation of the spec for testing purposes, but I don't see
any reason for it to be split up into dozens of individual kernel parameters

The ACPI 1.0 spec is followed by windows XP. And the power state is not checked in power transition under windows XP.
But we don't know whether it is still skipped on the new version windows.(For example: Windows 7).

If the module param is removed, we must delete the source code related with power state check. And if the power state is checked in power transition on windows 7, what we should do? It is not reasonable to add them again.

Maybe it is better to determine whether the power state check is skipped in power transition.

Quoted 12 lines · yakui_zhao

Quoted 6 lines · Matthew Garrett

The default behaviour should be to be compatible with Windows, regardless of what the spec says. There's an argument for providing a strict interpretation of the spec for testing purposes, but I don't see
any reason for it to be split up into dozens of individual kernel parameters

The ACPI 1.0 spec is followed by windows XP. And the power state is not checked in power transition under windows XP.
But we don't know whether it is still skipped on the new version windows.(For example: Windows 7).

Quoted 4 lines

If the module param is removed, we must delete the source code related with power state check. And if the power state is checked in power transition on windows 7, what we should do? It is not reasonable to add them again.

If Windows 7 changes the behaviour then the correct approach is to key this behaviour on whether the system firmware requests the Windows 7 OSI string. The code can be #if 0ed out until then, or placed under an acpi.strict kernel option that turns on all standards-compliant but windows-incompatible code.

Quoted 2 lines

Maybe it is better to determine whether the power state check is skipped in power transition.

We have a stated policy that Linux will default to being Windows compatible. You've demonstrated that in this case Linux isn't Windows compatible, which means that it's a bug. The correct behaviour for Linux here is to ignore the _STA value (or, indeed, not to call _STA at all in this path).

Quoted 24 lines · Matthew Garrett

Quoted 12 lines · yakui_zhao

Quoted 6 lines · Matthew Garrett

The default behaviour should be to be compatible with Windows, regardless of what the spec says. There's an argument for providing a strict interpretation of the spec for testing purposes, but I don't see
any reason for it to be split up into dozens of individual kernel parameters

The ACPI 1.0 spec is followed by windows XP. And the power state is not checked in power transition under windows XP.
But we don't know whether it is still skipped on the new version windows.(For example: Windows 7).

Quoted 4 lines

If the module param is removed, we must delete the source code related with power state check. And if the power state is checked in power transition on windows 7, what we should do? It is not reasonable to add them again.

If Windows 7 changes the behaviour then the correct approach is to key this behaviour on whether the system firmware requests the Windows 7 OSI string. The code can be #if 0ed out until then, or placed under an acpi.strict kernel option that turns on all standards-compliant but windows-incompatible code.

Quoted 8 lines

Quoted 2 lines

Maybe it is better to determine whether the power state check is skipped in power transition.

We have a stated policy that Linux will default to being Windows compatible. You've demonstrated that in this case Linux isn't Windows compatible, which means that it's a bug. The correct behaviour for Linux here is to ignore the _STA value (or, indeed, not to call _STA at all in this path).

I think we should use a patch like the one below.

I'd *like* to remove this whole chunk, which would allow us to remove acpi_power_nocheck, the DMI table, and the kernel parameter completely:

        /*
         * Get device's current power state
         */
        if (!acpi_power_nocheck) {
                /*
                 * Maybe the incorrect power state is returned on the bogus
                 * bios, which is different with the real power state.
                 * For example: the bios returns D0 state and the real power
                 * state is D3. OS expects to set the device to D0 state. In
                 * such case if OS uses the power state returned by the BIOS,
                 * the device can't be transisted to the correct power state.
                 * So if the acpi_power_nocheck is set, it is unnecessary to
                 * get the power state by calling acpi_bus_get_power.
                 */
                acpi_bus_get_power(device->handle, &device->power.state);
        }
        if ((state == device->power.state) && !device->flags.force_power_state) {
                ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device is already at D%d\n",
                                  state));
                return 0;
        }

Then we could also remove the "force_power_state" ugliness. But I'm afraid of breaking something because I don't understand the subtleties of power transitions.

ACPI: never check power state after _ON/_OFF

From: Bjorn Helgaas <[email protected]>

We used to evaluate _STA to check the power state of a device after running _ON or _OFF. But as far as I can tell, there's no benefit to evaluating _STA, and sometimes we trip over bugs when BIOSes don't implement _STA correctly.

Yakui says Windows XP doesn't evaluate _STA during power transition. So let's skip it in Linux, too.

References:

    http://bugzilla.kernel.org/show_bug.cgi?id=13243
    http://marc.info/?l=linux-acpi&m=124166053803753&w=2
    http://marc.info/?l=linux-acpi&m=124175761408256&w=2
    http://marc.info/?l=linux-acpi&m=124210593114061&w=2

Signed-off-by Bjorn Helgaas <[email protected]>

CC Yakui Zhao <[email protected]>

CC Matthew Garrett <[email protected]>

CC "Rafael J. Wysocki" <[email protected]>

CC Witold Szczeponik <[email protected]>

CC Len Brown <[email protected]>

---
 drivers/acpi/power.c |   28 ++--------------------------
 1 files changed, 2 insertions(+), 26 deletions(-)

drivers/acpi/power.c +2 -26

index 56665a6..d74365d 100644
@@ -194,7 +194,7 @@ static int acpi_power_get_list_state(struct acpi_handle_list *list, int *state)
 
 static int acpi_power_on(acpi_handle handle, struct acpi_device *dev)
 {
-	int result = 0, state;
+	int result = 0;
 	int found = 0;
 	acpi_status status = AE_OK;
 	struct acpi_power_resource *resource = NULL;

@@ -236,18 +236,6 @@ static int acpi_power_on(acpi_handle handle, struct acpi_device *dev)
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
 
-	if (!acpi_power_nocheck) {
-		/*
-		 * If acpi_power_nocheck is set, it is unnecessary to check
-		 * the power state after power transition.
-		 */
-		result = acpi_power_get_state(resource->device->handle,
-				&state);
-		if (result)
-			return result;
-		if (state != ACPI_POWER_RESOURCE_STATE_ON)
-			return -ENOEXEC;
-	}
 	/* Update the power resource's _device_ power state */
 	resource->device->power.state = ACPI_STATE_D0;
 

@@ -258,7 +246,7 @@ static int acpi_power_on(acpi_handle handle, struct acpi_device *dev)
 
 static int acpi_power_off_device(acpi_handle handle, struct acpi_device *dev)
 {
-	int result = 0, state;
+	int result = 0;
 	acpi_status status = AE_OK;
 	struct acpi_power_resource *resource = NULL;
 	struct list_head *node, *next;

@@ -293,18 +281,6 @@ static int acpi_power_off_device(acpi_handle handle, struct acpi_device *dev)
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
 
-	if (!acpi_power_nocheck) {
-		/*
-		 * If acpi_power_nocheck is set, it is unnecessary to check
-		 * the power state after power transition.
-		 */
-		result = acpi_power_get_state(handle, &state);
-		if (result)
-			return result;
-		if (state != ACPI_POWER_RESOURCE_STATE_OFF)
-			return -ENOEXEC;
-	}
-
 	/* Update the power resource's _device_ power state */
 	resource->device->power.state = ACPI_STATE_D3;
 

The flag of "force_power_state" can't be removed. For example: The ACPI fan is in Off state. But the bogus bios reports that it is in D0 state. If there is no flag of "force_power_state", we can't turn on the FAN device.

Quoted 25 lines · Bjorn Helgaas

I'd *like* to remove this whole chunk, which would allow us to remove acpi_power_nocheck, the DMI table, and the kernel parameter completely:

        /*
         * Get device's current power state
         */
        if (!acpi_power_nocheck) {
                /*
                 * Maybe the incorrect power state is returned on the bogus
                 * bios, which is different with the real power state.
                 * For example: the bios returns D0 state and the real power
                 * state is D3. OS expects to set the device to D0 state. In
                 * such case if OS uses the power state returned by the BIOS,
                 * the device can't be transisted to the correct power state.
                 * So if the acpi_power_nocheck is set, it is unnecessary to
                 * get the power state by calling acpi_bus_get_power.
                 */
                acpi_bus_get_power(device->handle, &device->power.state);
        }
        if ((state == device->power.state) && !device->flags.force_power_state) {
                ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device is already at D%d\n",
                                  state));
                return 0;
        }

Quoted 3 lines

Then we could also remove the "force_power_state" ugliness. But I'm afraid of breaking something because I don't understand the subtleties of power transitions.

The flag of "force_power_state" can't be removed. For example: The ACPI fan is in Off state. But the bogus bios reports that it is in D0 state. If there is no flag of "force_power_state", we can't turn on the FAN device.

Quoted 11 lines

ACPI: never check power state after _ON/_OFF

From: Bjorn Helgaas <[email protected]>

We used to evaluate _STA to check the power state of a device after running _ON or _OFF. But as far as I can tell, there's no benefit to evaluating _STA, and sometimes we trip over bugs when BIOSes don't implement _STA correctly.

Yakui says Windows XP doesn't evaluate _STA during power transition. So let's skip it in Linux, too.

It is also OK that the power state is never checked during power transition. I verify this on Windows XP. In such case we can delete the DMI check, kernel parameter, the code related with power state check.

But we don't know whether the power state check is still skipped during power transition on windows 7.

So IMO the better solution is still to keep the kernel parameter. It will break nothing. And the default value is to skip the power state check in power transition.

Quoted 76 lines

References:

    http://bugzilla.kernel.org/show_bug.cgi?id=13243
    http://marc.info/?l=linux-acpi&m=124166053803753&w=2
    http://marc.info/?l=linux-acpi&m=124175761408256&w=2
    http://marc.info/?l=linux-acpi&m=124210593114061&w=2

Signed-off-by Bjorn Helgaas <[email protected]>

CC Yakui Zhao <[email protected]>

CC Matthew Garrett <[email protected]>

CC "Rafael J. Wysocki" <[email protected]>

CC Witold Szczeponik <[email protected]>

CC Len Brown <[email protected]>

---
 drivers/acpi/power.c |   28 ++--------------------------
 1 files changed, 2 insertions(+), 26 deletions(-)

drivers/acpi/power.c +2 -26

index 56665a6..d74365d 100644
@@ -194,7 +194,7 @@ static int acpi_power_get_list_state(struct acpi_handle_list *list, int *state)
 
 static int acpi_power_on(acpi_handle handle, struct acpi_device *dev)
 {
-	int result = 0, state;
+	int result = 0;
 	int found = 0;
 	acpi_status status = AE_OK;
 	struct acpi_power_resource *resource = NULL;

@@ -236,18 +236,6 @@ static int acpi_power_on(acpi_handle handle, struct acpi_device *dev)
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
 
-	if (!acpi_power_nocheck) {
-		/*
-		 * If acpi_power_nocheck is set, it is unnecessary to check
-		 * the power state after power transition.
-		 */
-		result = acpi_power_get_state(resource->device->handle,
-				&state);
-		if (result)
-			return result;
-		if (state != ACPI_POWER_RESOURCE_STATE_ON)
-			return -ENOEXEC;
-	}
 	/* Update the power resource's _device_ power state */
 	resource->device->power.state = ACPI_STATE_D0;
 

@@ -258,7 +246,7 @@ static int acpi_power_on(acpi_handle handle, struct acpi_device *dev)
 
 static int acpi_power_off_device(acpi_handle handle, struct acpi_device *dev)
 {
-	int result = 0, state;
+	int result = 0;
 	acpi_status status = AE_OK;
 	struct acpi_power_resource *resource = NULL;
 	struct list_head *node, *next;

@@ -293,18 +281,6 @@ static int acpi_power_off_device(acpi_handle handle, struct acpi_device *dev)
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
 
-	if (!acpi_power_nocheck) {
-		/*
-		 * If acpi_power_nocheck is set, it is unnecessary to check
-		 * the power state after power transition.
-		 */
-		result = acpi_power_get_state(handle, &state);
-		if (result)
-			return result;
-		if (state != ACPI_POWER_RESOURCE_STATE_OFF)
-			return -ENOEXEC;
-	}
-
 	/* Update the power resource's _device_ power state */
 	resource->device->power.state = ACPI_STATE_D3;