Re: [PATCH 3/4] ACPI: Add "acpi.power_nocheck=1" to disable power state check in power transitio

[Thomas Renninger <trenn@suse.de>]

On Monday 14 July 2008 09:39:34 Zhao Yakui wrote:
> From: Zhao Yakui <yakui.zhao@intel.com>
>
>    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 and
> checks the device power state very strictly in power transition, the device
> can't be transited to the correct power state.
>
>    So the boot option of "acpi.power_nocheck=1" is added to avoid checking
> the device power state in the course of device power transition.
>
> http://bugzilla.kernel.org/show_bug.cgi?id=8049
> http://bugzilla.kernel.org/show_bug.cgi?id=11000
>
> Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> Signed-off-by: Li Shaohua <shaohua.li@intel.com>
>
> ---
>  Documentation/kernel-parameters.txt |    7 ++++++
>  drivers/acpi/bus.c                  |   14 +++++++++++-
>  drivers/acpi/power.c                |   42
> ++++++++++++++++++++++++++---------- include/acpi/acpi_drivers.h         | 
>   1
>  4 files changed, 52 insertions(+), 12 deletions(-)
>
> Index: linux-2.6/Documentation/kernel-parameters.txt
> ===================================================================
> --- linux-2.6.orig/Documentation/kernel-parameters.txt
> +++ linux-2.6/Documentation/kernel-parameters.txt
> @@ -224,6 +224,13 @@ and is between 256 and 4096 characters.
>  			The number can be in decimal or prefixed with 0x in hex.
>  			Warning: Many of these options can produce a lot of
>  			output and make your system unusable. Be very careful.
> +	acpi.power_nocheck=	[HW,ACPI]
> +			Format: 1/0 enable/disable the check of power state.
> +			On some bogus BIOS the _PSC object/_STA object of
> +			power resource can't return the correct device power
> +			state. In such case it is unneccessary to check its
> +			power state again in power transition.
> +			1 : disable the power state check
>
>  	acpi_pm_good	[X86-32,X86-64]
>  			Override the pmtimer bug detection: force the kernel
> Index: linux-2.6/drivers/acpi/power.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/power.c
> +++ linux-2.6/drivers/acpi/power.c
> @@ -54,6 +54,14 @@ ACPI_MODULE_NAME("power");
>  #define ACPI_POWER_RESOURCE_STATE_OFF	0x00
>  #define ACPI_POWER_RESOURCE_STATE_ON	0x01
>  #define ACPI_POWER_RESOURCE_STATE_UNKNOWN 0xFF
> +
> +#ifdef MODULE_PARAM_PREFIX
> +#undef MODULE_PARAM_PREFIX
> +#endif
> +#define MODULE_PARAM_PREFIX "acpi."
> +int acpi_power_nocheck;
> +module_param_named(power_nocheck, acpi_power_nocheck, bool, 000);
> +
>  static int acpi_power_add(struct acpi_device *device);
>  static int acpi_power_remove(struct acpi_device *device, int type);
>  static int acpi_power_resume(struct acpi_device *device);
> @@ -228,12 +236,18 @@ static int acpi_power_on(acpi_handle han
>  	if (ACPI_FAILURE(status))
>  		return -ENODEV;
>
> -	result = acpi_power_get_state(resource->device->handle, &state);
> -	if (result)
> -		return result;
> -	if (state != ACPI_POWER_RESOURCE_STATE_ON)
> -		return -ENOEXEC;
> -
> +	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;

A fat ACPI_EXCEPTION that the power state is bogus and already point to the 
acpi.power_nocheck boot param before returning -ENOEXEC would help to find 
this boot param for affected machines?

I wonder whether this (acpi.power_nocheck=1) could even get the default, maybe 
after waiting for reponse about how many machines really show this and why...

The current implementation sets:
               device->power.state = ACPI_STATE_UNKNOWN;
if -ENOEXEC is returned (or other errors happen) thermal management is rather 
blown up then? Setting a sane default (as you did if the boot param is 
passed) if things go wrong and print a warning sounds reasonable in general? 

While going through power.c it looks like there could be more warnings or more 
better fallbacks added:
in acpi_power_add():
       switch (state) {
        case ACPI_POWER_RESOURCE_STATE_ON:
                device->power.state = ACPI_STATE_D0;
                break;
        case ACPI_POWER_RESOURCE_STATE_OFF:
                device->power.state = ACPI_STATE_D3;
                break;
        default:
                device->power.state = ACPI_STATE_UNKNOWN;
                break;
        }
The last one is a BIOS bug.
Also the power state must never be ACPI_STATE_UNKNOWN or the whole driver is 
rather useless?
E.g. the check in acpi_power_transition() indicates that if the state is 
ACPI_STATE_UNKNOWN things are rather broken:
        if ((device->power.state < ACPI_STATE_D0)
            || (device->power.state > ACPI_STATE_D3))
                return -ENODEV;

What do you think about this cleanup patch.
I wonder whether it makes sense to not only throw an error message at places 
where ACPI_STATE_UNKNOWN is set, but also set ACPI_STATE_D3 (or 
ACPI_STATE_D0, whatever is expected) and try to keep power resource 
management up, even on bogus BIOS values?

---
 drivers/acpi/power.c |   20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

Index: pci-2.6/drivers/acpi/power.c
===================================================================
--- pci-2.6.orig/drivers/acpi/power.c
+++ pci-2.6/drivers/acpi/power.c
@@ -337,7 +337,7 @@ int acpi_device_sleep_wake(struct acpi_d
 	if (ACPI_SUCCESS(status)) {
 		return 0;
 	} else if (status != AE_NOT_FOUND) {
-		printk(KERN_ERR PREFIX "_DSW execution failed\n");
+		ACPI_EXCEPTION((AE_INFO, status, "_DSW execution failed\n"));
 		dev->wakeup.flags.valid = 0;
 		return -ENODEV;
 	}
@@ -347,7 +347,7 @@ int acpi_device_sleep_wake(struct acpi_d
 	in_arg[0].integer.value = enable;
 	status = acpi_evaluate_object(dev->handle, "_PSW", &arg_list, NULL);
 	if (ACPI_FAILURE(status) && (status != AE_NOT_FOUND)) {
-		printk(KERN_ERR PREFIX "_PSW execution failed\n");
+		ACPI_EXCEPTION((AE_INFO, status, "_PSW execution failed\n"));
 		dev->wakeup.flags.valid = 0;
 		return -ENODEV;
 	}
@@ -379,7 +379,11 @@ int acpi_enable_wakeup_device_power(stru
 	for (i = 0; i < dev->wakeup.resources.count; i++) {
 		int ret = acpi_power_on(dev->wakeup.resources.handles[i], dev);
 		if (ret) {
-			printk(KERN_ERR PREFIX "Transition power state\n");
+			/* ToDo make use of acpi_status in acpi_power_on and
+			 *  others for better error tracking
+			 */
+			ACPI_EXCEPTION((AE_INFO, AE_ERROR,
+					"Transition power state\n"));
 			dev->wakeup.flags.valid = 0;
 			return -ENODEV;
 		}
@@ -462,8 +466,13 @@ int acpi_power_get_inferred_state(struct
 			continue;
 
 		result = acpi_power_get_list_state(list, &list_state);
-		if (result)
+		if (result) {
+			ACPI_EXCEPTION((AE_INFO, AE_ERROR,
+					"Could not infer state for"
+					"power resource [%s]\n",
+					acpi_device_bid(device)));
 			return result;
+		}
 
 		if (list_state == ACPI_POWER_RESOURCE_STATE_ON) {
 			device->power.state = i;
@@ -680,6 +689,9 @@ static int acpi_power_add(struct acpi_de
 		device->power.state = ACPI_STATE_D3;
 		break;
 	default:
+		ACPI_EXCEPTION((AE_INFO, AE_ERROR,
+				"Power resource [%s] returned bogus value\n",
+				acpi_device_bid(device)));
 		device->power.state = ACPI_STATE_UNKNOWN;
 		break;
 	}


       Thomas

>
> @@ -279,11 +293,17 @@ static int acpi_power_off_device(acpi_ha
>  	if (ACPI_FAILURE(status))
>  		return -ENODEV;
>
> -	result = acpi_power_get_state(handle, &state);
> -	if (result)
> -		return result;
> -	if (state != ACPI_POWER_RESOURCE_STATE_OFF)
> -		return -ENOEXEC;
> +	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;
> Index: linux-2.6/include/acpi/acpi_drivers.h
> ===================================================================
> --- linux-2.6.orig/include/acpi/acpi_drivers.h
> +++ linux-2.6/include/acpi/acpi_drivers.h
> @@ -91,6 +91,7 @@ int acpi_enable_wakeup_device_power(stru
>  int acpi_disable_wakeup_device_power(struct acpi_device *dev);
>  int acpi_power_get_inferred_state(struct acpi_device *device);
>  int acpi_power_transition(struct acpi_device *device, int state);
> +extern int acpi_power_nocheck;
>  #endif
>
>  /*
> --------------------------------------------------------------------------
> Index: linux-2.6/drivers/acpi/bus.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/bus.c
> +++ linux-2.6/drivers/acpi/bus.c
> @@ -223,7 +223,19 @@ int acpi_bus_set_power(acpi_handle handl
>  	/*
>  	 * Get device's current power state
>  	 */
> -	acpi_bus_get_power(device->handle, &device->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));
>
>
> --
> 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