[PATCH] acpi: Disable explicit power state retrieval on fans

[PATCH] acpi: Disable explicit power state retrieval on fans

[Matthew Garrett]

https://bugzilla.redhat.com/show_bug.cgi?id=531916 describes a system
with a _PSC method for the fan that always returns "on". There's no
benefit in us always requesting the state of the fan when performing
transitions - we want to do everything we can to ensure that the fan turns
on when it should do, not risk hardware damage by believing the hardware
when it tells us the fan is already on. Given that the Leading Other OS(tm)
works fine on this machine, it seems likely that it behaves in much this
way.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/acpi/fan.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/fan.c b/drivers/acpi/fan.c
index f419849..835b55e 100644
--- a/drivers/acpi/fan.c
+++ b/drivers/acpi/fan.c
@@ -256,6 +256,7 @@ static int acpi_fan_add(struct acpi_device *device)
 		goto end;
 	}
 
+	device->power.flags.explicit_get = 0;
 	device->flags.force_power_state = 1;
 	acpi_bus_set_power(device->handle, state);
 	device->flags.force_power_state = 0;
-- 
1.6.5.2

Re: [PATCH] acpi: Disable explicit power state retrieval on fans

[Matthew Garrett]

Any feedback?

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

Re: [PATCH] acpi: Disable explicit power state retrieval on fans

[Zhang Rui]

On Wed, 2009-11-11 at 04:09 +0800, Matthew Garrett wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=531916 describes a system
> with a _PSC method for the fan that always returns "on". There's no
> benefit in us always requesting the state of the fan when performing
> transitions - we want to do everything we can to ensure that the fan turns
> on when it should do, not risk hardware damage by believing the hardware
> when it tells us the fan is already on. Given that the Leading Other OS(tm)
> works fine on this machine, it seems likely that it behaves in much this
> way.
> 
sounds reasonable.
But how can we get the power state if power resources is not available?

In acpi_bus_get_power, why not make acpi_power_get_inferred_state as the
first choice, instead of evaluating _PSC? like the patch attached.

---
 drivers/acpi/bus.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Index: linux-2.6/drivers/acpi/bus.c
===================================================================
--- linux-2.6.orig/drivers/acpi/bus.c
+++ linux-2.6/drivers/acpi/bus.c
@@ -190,16 +190,17 @@ int acpi_bus_get_power(acpi_handle handl
 		 * Get the device's power state either directly (via _PSC) or
 		 * indirectly (via power resources).
 		 */
-		if (device->power.flags.explicit_get) {
+
+		if (device->power.flags.power_resources) {
+			result = acpi_power_get_inferred_state(device);
+			if (result)
+				return result;
+		} else if (device->power.flags.explicit_get) {
 			status = acpi_evaluate_integer(device->handle, "_PSC",
 						       NULL, &psc);
 			if (ACPI_FAILURE(status))
 				return -ENODEV;
 			device->power.state = (int)psc;
-		} else if (device->power.flags.power_resources) {
-			result = acpi_power_get_inferred_state(device);
-			if (result)
-				return result;
 		}
 
 		*state = device->power.state;


> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> ---
>  drivers/acpi/fan.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/acpi/fan.c b/drivers/acpi/fan.c
> index f419849..835b55e 100644
> --- a/drivers/acpi/fan.c
> +++ b/drivers/acpi/fan.c
> @@ -256,6 +256,7 @@ static int acpi_fan_add(struct acpi_device *device)
>  		goto end;
>  	}
>  
> +	device->power.flags.explicit_get = 0;
>  	device->flags.force_power_state = 1;
>  	acpi_bus_set_power(device->handle, state);
>  	device->flags.force_power_state = 0;

at least we should clear the explicit_get flag 6 lines above, i.e.
before getting the fan state?

thanks,
rui

Re: [PATCH] acpi: Disable explicit power state retrieval on fans

[Zhang Rui]

On Fri, 2009-12-11 at 11:52 +0800, Zhang Rui wrote:
> On Wed, 2009-11-11 at 04:09 +0800, Matthew Garrett wrote:
> > https://bugzilla.redhat.com/show_bug.cgi?id=531916 describes a system
> > with a _PSC method for the fan that always returns "on". There's no
> > benefit in us always requesting the state of the fan when performing
> > transitions - we want to do everything we can to ensure that the fan turns
> > on when it should do, not risk hardware damage by believing the hardware
> > when it tells us the fan is already on. Given that the Leading Other OS(tm)
> > works fine on this machine, it seems likely that it behaves in much this
> > way.
> > 
> sounds reasonable.
> But how can we get the power state if power resources is not available?
> 
> In acpi_bus_get_power, why not make acpi_power_get_inferred_state as the
> first choice, instead of evaluating _PSC? like the patch attached.
> 
Matthew,

how about this one?


If the ACPI power state can be got both directly and indirectly,
we prefer to get it indirectly.

https://bugzilla.redhat.com/show_bug.cgi?id=531916 describes a
system with a _PSC method for the fan that always returns "on".
There's no benefit in us always requesting the state of the fan
when performing transitions - we want to do everything we can
to ensure that the fan turns on when it should do, not risk
hardware damage by believing the hardware when it tells us the
fan is already on. Given that the Leading Other OS(tm) works fine
on this machine, it seems likely that it behaves in much this way.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/bus.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Index: linux-2.6/drivers/acpi/bus.c
===================================================================
--- linux-2.6.orig/drivers/acpi/bus.c
+++ linux-2.6/drivers/acpi/bus.c
@@ -190,16 +190,16 @@ int acpi_bus_get_power(acpi_handle handl
 		 * Get the device's power state either directly (via _PSC) or
 		 * indirectly (via power resources).
 		 */
-		if (device->power.flags.explicit_get) {
+		if (device->power.flags.power_resources) {
+			result = acpi_power_get_inferred_state(device);
+			if (result)
+				return result;
+		} else if (device->power.flags.explicit_get) {
 			status = acpi_evaluate_integer(device->handle, "_PSC",
 						       NULL, &psc);
 			if (ACPI_FAILURE(status))
 				return -ENODEV;
 			device->power.state = (int)psc;
-		} else if (device->power.flags.power_resources) {
-			result = acpi_power_get_inferred_state(device);
-			if (result)
-				return result;
 		}
 
 		*state = device->power.state;

Re: [PATCH] acpi: Disable explicit power state retrieval on fans

[Zhang Rui]

On Fri, 2009-12-18 at 03:13 +0800, Matthew Garrett wrote:
> On Thu, Dec 17, 2009 at 04:02:08PM +0800, Zhang Rui wrote:
> 
> > If the ACPI power state can be got both directly and indirectly,
> > we prefer to get it indirectly.
> 
> I guess this would be fine, but it seems to be a rather bigger change - 
> I'd feel safer limiting it to fans. We might as well give it a go, 
> though.
> 
only ACPI fan and several other devices that invokes
acpi_bus_set/get_power.
If it brings some problems, it won't take us a lot of efforts to debug.
I think it's a good candidate for -rc1.

thanks,
rui

Re: [PATCH] acpi: Disable explicit power state retrieval on fans

[Len Brown]

applied to acpi-test

thanks,
Len Brown, Intel Open Source Technology Center

On Thu, 17 Dec 2009, Zhang Rui wrote:

> On Fri, 2009-12-11 at 11:52 +0800, Zhang Rui wrote:
> > On Wed, 2009-11-11 at 04:09 +0800, Matthew Garrett wrote:
> > > https://bugzilla.redhat.com/show_bug.cgi?id=531916 describes a system
> > > with a _PSC method for the fan that always returns "on". There's no
> > > benefit in us always requesting the state of the fan when performing
> > > transitions - we want to do everything we can to ensure that the fan turns
> > > on when it should do, not risk hardware damage by believing the hardware
> > > when it tells us the fan is already on. Given that the Leading Other OS(tm)
> > > works fine on this machine, it seems likely that it behaves in much this
> > > way.
> > > 
> > sounds reasonable.
> > But how can we get the power state if power resources is not available?
> > 
> > In acpi_bus_get_power, why not make acpi_power_get_inferred_state as the
> > first choice, instead of evaluating _PSC? like the patch attached.
> > 
> Matthew,
> 
> how about this one?
> 
> 
> If the ACPI power state can be got both directly and indirectly,
> we prefer to get it indirectly.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=531916 describes a
> system with a _PSC method for the fan that always returns "on".
> There's no benefit in us always requesting the state of the fan
> when performing transitions - we want to do everything we can
> to ensure that the fan turns on when it should do, not risk
> hardware damage by believing the hardware when it tells us the
> fan is already on. Given that the Leading Other OS(tm) works fine
> on this machine, it seems likely that it behaves in much this way.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/acpi/bus.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> Index: linux-2.6/drivers/acpi/bus.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/bus.c
> +++ linux-2.6/drivers/acpi/bus.c
> @@ -190,16 +190,16 @@ int acpi_bus_get_power(acpi_handle handl
>  		 * Get the device's power state either directly (via _PSC) or
>  		 * indirectly (via power resources).
>  		 */
> -		if (device->power.flags.explicit_get) {
> +		if (device->power.flags.power_resources) {
> +			result = acpi_power_get_inferred_state(device);
> +			if (result)
> +				return result;
> +		} else if (device->power.flags.explicit_get) {
>  			status = acpi_evaluate_integer(device->handle, "_PSC",
>  						       NULL, &psc);
>  			if (ACPI_FAILURE(status))
>  				return -ENODEV;
>  			device->power.state = (int)psc;
> -		} else if (device->power.flags.power_resources) {
> -			result = acpi_power_get_inferred_state(device);
> -			if (result)
> -				return result;
>  		}
>  
>  		*state = device->power.state;
> 
> 
> 
> --
> 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
>