[PATCH]: ACPI: Skip the power state check in power transition

[PATCH]: ACPI: Skip the power state check in power transition

[yakui_zhao]

From: Zhao Yakui <yakui.zhao@intel.com>

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

Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
Acked-by: Matthew Garrett <mjg59@srcf.ucam.org>
---
 drivers/acpi/power.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/drivers/acpi/power.c
===================================================================
--- linux-2.6.orig/drivers/acpi/power.c	2009-04-16 16:10:24.000000000 +0800
+++ linux-2.6/drivers/acpi/power.c	2009-05-12 13:12:27.000000000 +0800
@@ -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);

Re: [PATCH]: ACPI: Skip the power state check in power transition

[Bjorn Helgaas]

On Monday 11 May 2009 11:27:40 pm yakui_zhao wrote:
> From: Zhao Yakui <yakui.zhao@intel.com>
> 
> 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".

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

> Index: linux-2.6/drivers/acpi/power.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/power.c	2009-04-16 16:10:24.000000000 +0800
> +++ linux-2.6/drivers/acpi/power.c	2009-05-12 13:12:27.000000000 +0800
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH]: ACPI: Skip the power state check in power transition

[yakui_zhao]

On Tue, 2009-05-12 at 22:57 +0800, Bjorn Helgaas wrote:
> On Monday 11 May 2009 11:27:40 pm yakui_zhao wrote:
> > From: Zhao Yakui <yakui.zhao@intel.com>
> > 
> > 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".
> 
> 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.
Yes. If we delete the course of the power state check in power
transition, it will be unnecessary to add the boot option.

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.

At the same time if we want to make the power state check more strict,
we will have to re-add the source-code. 

So IMO it will be better that the power state check in power transition
is controlled by one module parameter.

Best regards.
    Yakui.

> 
> Bjorn
> 
> > Index: linux-2.6/drivers/acpi/power.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/acpi/power.c	2009-04-16 16:10:24.000000000 +0800
> > +++ linux-2.6/drivers/acpi/power.c	2009-05-12 13:12:27.000000000 +0800
> > @@ -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 majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 
> 

Re: [PATCH]: ACPI: Skip the power state check in power transition

[Matthew Garrett]

On Wed, May 13, 2009 at 11:13:04AM +0800, yakui_zhao wrote:

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

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

Re: [PATCH]: ACPI: Skip the power state check in power transition

[yakui_zhao]

On Wed, 2009-05-13 at 21:08 +0800, Matthew Garrett wrote:
> 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.

Thanks.

Re: [PATCH]: ACPI: Skip the power state check in power transition

[Matthew Garrett]

On Thu, May 14, 2009 at 09:47:39AM +0800, yakui_zhao wrote:
> On Wed, 2009-05-13 at 21:08 +0800, Matthew Garrett wrote:
> > 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.

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.

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

-- 

Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH]: ACPI: Skip the power state check in power transition

[Bjorn Helgaas]

On Thursday 14 May 2009 04:56:27 am Matthew Garrett wrote:
> On Thu, May 14, 2009 at 09:47:39AM +0800, yakui_zhao wrote:
> > On Wed, 2009-05-13 at 21:08 +0800, Matthew Garrett wrote:
> > > 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.
> 
> 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.
> 
> > 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 <bjorn.helgaas@hp.com>

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 <bjorn.helgaas@hp.com>
CC: Yakui Zhao <yakui.zhao@intel.com>
CC: Matthew Garrett <mjg59@srcf.ucam.org>
CC: "Rafael J. Wysocki" <rjw@sisk.pl>
CC: Witold Szczeponik <Witold.Szczeponik@gmx.net>
CC: Len Brown <lenb@kernel.org>
---
 drivers/acpi/power.c |   28 ++--------------------------
 1 files changed, 2 insertions(+), 26 deletions(-)

diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
index 56665a6..d74365d 100644
--- a/drivers/acpi/power.c
+++ b/drivers/acpi/power.c
@@ -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;
 

Re: [PATCH]: ACPI: Skip the power state check in power transition

[yakui_zhao]

On Fri, 2009-05-15 at 02:49 +0800, Bjorn Helgaas wrote:
> On Thursday 14 May 2009 04:56:27 am Matthew Garrett wrote:
> > On Thu, May 14, 2009 at 09:47:39AM +0800, yakui_zhao wrote:
> > > On Wed, 2009-05-13 at 21:08 +0800, Matthew Garrett wrote:
> > > > 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.
> > 
> > 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.
> > 
> > > 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.
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.
> 
> 
> ACPI: never check power state after _ON/_OFF
> 
> From: Bjorn Helgaas <bjorn.helgaas@hp.com>
> 
> 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.
> 
> 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 <bjorn.helgaas@hp.com>
> CC: Yakui Zhao <yakui.zhao@intel.com>
> CC: Matthew Garrett <mjg59@srcf.ucam.org>
> CC: "Rafael J. Wysocki" <rjw@sisk.pl>
> CC: Witold Szczeponik <Witold.Szczeponik@gmx.net>
> CC: Len Brown <lenb@kernel.org>
> ---
>  drivers/acpi/power.c |   28 ++--------------------------
>  1 files changed, 2 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
> index 56665a6..d74365d 100644
> --- a/drivers/acpi/power.c
> +++ b/drivers/acpi/power.c
> @@ -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;
>  

Re: [PATCH]: ACPI: Skip the power state check in power transition

[Matthew Garrett]

On Mon, May 18, 2009 at 10:08:23AM +0800, yakui_zhao wrote:

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

If Windows never calls _STA in this case, why do we care that it's lying 
to us?

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

Re: [PATCH]: ACPI: Skip the power state check in power transition

[Bjorn Helgaas]

On Sunday 17 May 2009 8:08:23 pm yakui_zhao wrote:
> On Fri, 2009-05-15 at 02:49 +0800, Bjorn Helgaas wrote:

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

OK, we need more investigation before removing this code from
acpi_bus_set_power(), so let's defer that.

> > ACPI: never check power state after _ON/_OFF
> > 
> > From: Bjorn Helgaas <bjorn.helgaas@hp.com>
> > 
> > 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.

I can't quite tell whether you think we still need the checks in
acpi_power_on() and acpi_power_off_device().  You proposed setting
"acpi_power_nocheck = 1" by default, which would cause us to bypass
the checks.

Removing the checks completely from those paths only means we wouldn't
be able to use "acpi.power_nocheck=0" to force checking the state after
a transition.  I really don't think users are going to do that, so I
don't think it's worth cluttering the code with those checks.

I'll post that patch again in a separate thread.

Bjorn

> > 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 <bjorn.helgaas@hp.com>
> > CC: Yakui Zhao <yakui.zhao@intel.com>
> > CC: Matthew Garrett <mjg59@srcf.ucam.org>
> > CC: "Rafael J. Wysocki" <rjw@sisk.pl>
> > CC: Witold Szczeponik <Witold.Szczeponik@gmx.net>
> > CC: Len Brown <lenb@kernel.org>
> > ---
> >  drivers/acpi/power.c |   28 ++--------------------------
> >  1 files changed, 2 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
> > index 56665a6..d74365d 100644
> > --- a/drivers/acpi/power.c
> > +++ b/drivers/acpi/power.c
> > @@ -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;
> >  
> 
> 

Re: [PATCH]: ACPI: Skip the power state check in power transition

[Len Brown]

I agree with Matthew and Bjorn.
This is the right thing to do, but we should take it a step further and
simplify the code.

thanks,
Len Brown, Intel Open Source Technology Center