Re: [PATCH 12/12] ACPI: Fan: Drop force_power_state acpi_device option

Re: [PATCH 12/12] ACPI: Fan: Drop force_power_state acpi_device option

[Rafael J. Wysocki]

On Monday, 22 October 2007 12:19, Alexey Starikovskiy wrote:
> force_power_state was used as a workaround for invalid cached 
> power state of the device. We do not cache power state, so no need for
> workaround.
> 
> Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
> ---

Acked-by: Rafael J. Wysocki <rjw@sisk.pl>

>  drivers/acpi/bus.c      |    2 +-
>  drivers/acpi/fan.c      |   40 ----------------------------------------
>  include/acpi/acpi_bus.h |    3 +--
>  3 files changed, 2 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index fdee82d..49d432d 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -201,7 +201,7 @@ int acpi_bus_set_power(acpi_handle handle, int state)
>  	 * Get device's current power state
>  	 */
>  	acpi_bus_get_power(device->handle, &device->power.state);
> -	if ((state == device->power.state) && !device->flags.force_power_state) {
> +	if (state == device->power.state) {
>  		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device is already at D%d\n",
>  				  state));
>  		return 0;
> diff --git a/drivers/acpi/fan.c b/drivers/acpi/fan.c
> index a6e149d..a5a5532 100644
> --- a/drivers/acpi/fan.c
> +++ b/drivers/acpi/fan.c
> @@ -47,8 +47,6 @@ MODULE_LICENSE("GPL");
>  
>  static int acpi_fan_add(struct acpi_device *device);
>  static int acpi_fan_remove(struct acpi_device *device, int type);
> -static int acpi_fan_suspend(struct acpi_device *device, pm_message_t state);
> -static int acpi_fan_resume(struct acpi_device *device);
>  
>  static const struct acpi_device_id fan_device_ids[] = {
>  	{"PNP0C0B", 0},
> @@ -63,8 +61,6 @@ static struct acpi_driver acpi_fan_driver = {
>  	.ops = {
>  		.add = acpi_fan_add,
>  		.remove = acpi_fan_remove,
> -		.suspend = acpi_fan_suspend,
> -		.resume = acpi_fan_resume,
>  		},
>  };
>  
> @@ -195,10 +191,6 @@ static int acpi_fan_add(struct acpi_device *device)
>  		goto end;
>  	}
>  
> -	device->flags.force_power_state = 1;
> -	acpi_bus_set_power(device->handle, state);
> -	device->flags.force_power_state = 0;
> -
>  	result = acpi_fan_add_fs(device);
>  	if (result)
>  		goto end;
> @@ -224,38 +216,6 @@ static int acpi_fan_remove(struct acpi_device *device, int type)
>  	return 0;
>  }
>  
> -static int acpi_fan_suspend(struct acpi_device *device, pm_message_t state)
> -{
> -	if (!device)
> -		return -EINVAL;
> -
> -	acpi_bus_set_power(device->handle, ACPI_STATE_D0);
> -
> -	return AE_OK;
> -}
> -
> -static int acpi_fan_resume(struct acpi_device *device)
> -{
> -	int result = 0;
> -	int power_state = 0;
> -
> -	if (!device)
> -		return -EINVAL;
> -
> -	result = acpi_bus_get_power(device->handle, &power_state);
> -	if (result) {
> -		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
> -				  "Error reading fan power state\n"));
> -		return result;
> -	}
> -
> -	device->flags.force_power_state = 1;
> -	acpi_bus_set_power(device->handle, power_state);
> -	device->flags.force_power_state = 0;
> -
> -	return result;
> -}
> -
>  static int __init acpi_fan_init(void)
>  {
>  	int result = 0;
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 7b74b60..19c3ead 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -168,8 +168,7 @@ struct acpi_device_flags {
>  	u32 power_manageable:1;
>  	u32 performance_manageable:1;
>  	u32 wake_capable:1;	/* Wakeup(_PRW) supported? */
> -	u32 force_power_state:1;
> -	u32 reserved:19;
> +	u32 reserved:20;
>  };
>  
>  /* File System */
> 
> 
> 

-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [PATCH 11/12] ACPI: Fan: fan device does not need own structure

[Rafael J. Wysocki]

On Monday, 22 October 2007 12:19, Alexey Starikovskiy wrote:
> Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
> ---

Acked-by: Rafael J. Wysocki <rjw@sisk.pl>

>  drivers/acpi/fan.c |   32 +++++++-------------------------
>  1 files changed, 7 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/acpi/fan.c b/drivers/acpi/fan.c
> index c81f6bd..a6e149d 100644
> --- a/drivers/acpi/fan.c
> +++ b/drivers/acpi/fan.c
> @@ -68,10 +68,6 @@ static struct acpi_driver acpi_fan_driver = {
>  		},
>  };
>  
> -struct acpi_fan {
> -	struct acpi_device * device;
> -};
> -
>  /* --------------------------------------------------------------------------
>                                FS Interface (/proc)
>     -------------------------------------------------------------------------- */
> @@ -80,12 +76,12 @@ static struct proc_dir_entry *acpi_fan_dir;
>  
>  static int acpi_fan_read_state(struct seq_file *seq, void *offset)
>  {
> -	struct acpi_fan *fan = seq->private;
> +	struct acpi_device *device = seq->private;
>  	int state = 0;
>  
>  
> -	if (fan) {
> -		if (acpi_bus_get_power(fan->device->handle, &state))
> +	if (device) {
> +		if (acpi_bus_get_power(device->handle, &state))
>  			seq_printf(seq, "status:                  ERROR\n");
>  		else
>  			seq_printf(seq, "status:                  %s\n",
> @@ -105,11 +101,10 @@ acpi_fan_write_state(struct file *file, const char __user * buffer,
>  {
>  	int result = 0;
>  	struct seq_file *m = file->private_data;
> -	struct acpi_fan *fan = m->private;
> +	struct acpi_device *device = m->private;
>  	char state_string[12] = { '\0' };
>  
> -
> -	if (!fan || (count > sizeof(state_string) - 1))
> +	if (count > sizeof(state_string) - 1)
>  		return -EINVAL;
>  
>  	if (copy_from_user(state_string, buffer, count))
> @@ -117,7 +112,7 @@ acpi_fan_write_state(struct file *file, const char __user * buffer,
>  
>  	state_string[count] = '\0';
>  
> -	result = acpi_bus_set_power(fan->device->handle,
> +	result = acpi_bus_set_power(device->handle,
>  				    simple_strtoul(state_string, NULL, 0));
>  	if (result)
>  		return result;
> @@ -158,7 +153,7 @@ static int acpi_fan_add_fs(struct acpi_device *device)
>  		return -ENODEV;
>  	else {
>  		entry->proc_fops = &acpi_fan_state_ops;
> -		entry->data = acpi_driver_data(device);
> +		entry->data = device;
>  		entry->owner = THIS_MODULE;
>  	}
>  
> @@ -191,14 +186,8 @@ static int acpi_fan_add(struct acpi_device *device)
>  	if (!device)
>  		return -EINVAL;
>  
> -	fan = kzalloc(sizeof(struct acpi_fan), GFP_KERNEL);
> -	if (!fan)
> -		return -ENOMEM;
> -
> -	fan->device = device;
>  	strcpy(acpi_device_name(device), "Fan");
>  	strcpy(acpi_device_class(device), ACPI_FAN_CLASS);
> -	acpi_driver_data(device) = fan;
>  
>  	result = acpi_bus_get_power(device->handle, &state);
>  	if (result) {
> @@ -227,18 +216,11 @@ static int acpi_fan_add(struct acpi_device *device)
>  
>  static int acpi_fan_remove(struct acpi_device *device, int type)
>  {
> -	struct acpi_fan *fan = NULL;
> -
> -
>  	if (!device || !acpi_driver_data(device))
>  		return -EINVAL;
>  
> -	fan = acpi_driver_data(device);
> -
>  	acpi_fan_remove_fs(device);
>  
> -	kfree(fan);
> -
>  	return 0;
>  }
>  
> 
> 
> 

-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [PATCH 10/12] ACPI: power: don't cache power resource state

[Rafael J. Wysocki]

On Monday, 22 October 2007 12:19, Alexey Starikovskiy wrote:
> ACPI may change power resource state behind our back, so don't
> keep our local copy, which may not be valid.
> 
> Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
> ---
> 
>  drivers/acpi/bus.c   |    6 ++---
>  drivers/acpi/power.c |   63 +++++++++++++++++++-------------------------------
>  2 files changed, 26 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index fb2cff9..fdee82d 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -198,11 +198,9 @@ int acpi_bus_set_power(acpi_handle handle, int state)
>  		return -ENODEV;
>  	}
>  	/*
> -	 * Get device's current power state if it's unknown
> -	 * This means device power state isn't initialized or previous setting failed
> +	 * Get device's current power state
>  	 */
> -	if ((device->power.state == ACPI_STATE_UNKNOWN) || device->flags.force_power_state)
> -		acpi_bus_get_power(device->handle, &device->power.state);
> +	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));
> diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
> index 57b9a29..af1769a 100644
> --- a/drivers/acpi/power.c
> +++ b/drivers/acpi/power.c
> @@ -86,7 +86,6 @@ struct acpi_power_resource {
>  	acpi_bus_id name;
>  	u32 system_level;
>  	u32 order;
> -	int state;
>  	struct mutex resource_lock;
>  	struct list_head reference;
>  };
> @@ -128,33 +127,31 @@ acpi_power_get_context(acpi_handle handle,
>  	return 0;
>  }
>  
> -static int acpi_power_get_state(struct acpi_power_resource *resource)
> +static int acpi_power_get_state(struct acpi_power_resource *resource, int *state)
>  {
>  	acpi_status status = AE_OK;
>  	unsigned long sta = 0;
>  
>  
> -	if (!resource)
> +	if (!resource || !state)
>  		return -EINVAL;
>  
>  	status = acpi_evaluate_integer(resource->device->handle, "_STA", NULL, &sta);
>  	if (ACPI_FAILURE(status))
>  		return -ENODEV;
>  
> -	if (sta & 0x01)
> -		resource->state = ACPI_POWER_RESOURCE_STATE_ON;
> -	else
> -		resource->state = ACPI_POWER_RESOURCE_STATE_OFF;
> +	*state = (sta & 0x01)?ACPI_POWER_RESOURCE_STATE_ON:
> +			      ACPI_POWER_RESOURCE_STATE_OFF;
>  
>  	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Resource [%s] is %s\n",
> -			  resource->name, resource->state ? "on" : "off"));
> +			  resource->name, state ? "on" : "off"));
>  
>  	return 0;
>  }
>  
>  static int acpi_power_get_list_state(struct acpi_handle_list *list, int *state)
>  {
> -	int result = 0;
> +	int result = 0, state1;

This is a tiny bit inconsistent with the instances below where the local
variable is called "state" (ie. without the "1").  I'd just call the argument "state_p"
and the auxiliary variable "state".

>  	struct acpi_power_resource *resource = NULL;
>  	u32 i = 0;
>  
> @@ -168,11 +165,11 @@ static int acpi_power_get_list_state(struct acpi_handle_list *list, int *state)
>  		result = acpi_power_get_context(list->handles[i], &resource);
>  		if (result)
>  			return result;
> -		result = acpi_power_get_state(resource);
> +		result = acpi_power_get_state(resource, &state1);
>  		if (result)
>  			return result;
>  
> -		*state = resource->state;
> +		*state = state1;
>  
>  		if (*state != ACPI_POWER_RESOURCE_STATE_ON)
>  			break;
> @@ -186,7 +183,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;
> +	int result = 0, state;
>  	int found = 0;
>  	acpi_status status = AE_OK;
>  	struct acpi_power_resource *resource = NULL;
> @@ -224,20 +221,14 @@ static int acpi_power_on(acpi_handle handle, struct acpi_device *dev)
>  	}
>  	mutex_unlock(&resource->resource_lock);
>  
> -	if (resource->state == ACPI_POWER_RESOURCE_STATE_ON) {
> -		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Resource [%s] already on\n",
> -				  resource->name));
> -		return 0;
> -	}
> -
>  	status = acpi_evaluate_object(resource->device->handle, "_ON", NULL, NULL);
>  	if (ACPI_FAILURE(status))
>  		return -ENODEV;
>  
> -	result = acpi_power_get_state(resource);
> +	result = acpi_power_get_state(resource, &state);
>  	if (result)
>  		return result;
> -	if (resource->state != ACPI_POWER_RESOURCE_STATE_ON)
> +	if (state != ACPI_POWER_RESOURCE_STATE_ON)
>  		return -ENOEXEC;
>  
>  	/* Update the power resource's _device_ power state */
> @@ -250,7 +241,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;
> +	int result = 0, state;
>  	acpi_status status = AE_OK;
>  	struct acpi_power_resource *resource = NULL;
>  	struct list_head *node, *next;
> @@ -281,20 +272,14 @@ static int acpi_power_off_device(acpi_handle handle, struct acpi_device *dev)
>  	}
>  	mutex_unlock(&resource->resource_lock);
>  
> -	if (resource->state == ACPI_POWER_RESOURCE_STATE_OFF) {
> -		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Resource [%s] already off\n",
> -				  resource->name));
> -		return 0;
> -	}
> -
>  	status = acpi_evaluate_object(resource->device->handle, "_OFF", NULL, NULL);
>  	if (ACPI_FAILURE(status))
>  		return -ENODEV;
>  
> -	result = acpi_power_get_state(resource);
> +	result = acpi_power_get_state(resource, &state);
>  	if (result)
>  		return result;
> -	if (resource->state != ACPI_POWER_RESOURCE_STATE_OFF)
> +	if (state != ACPI_POWER_RESOURCE_STATE_OFF)
>  		return -ENOEXEC;
>  
>  	/* Update the power resource's _device_ power state */
> @@ -494,7 +479,7 @@ static struct proc_dir_entry *acpi_power_dir;
>  static int acpi_power_seq_show(struct seq_file *seq, void *offset)
>  {
>  	int count = 0;
> -	int result = 0;
> +	int result = 0, state;
>  	struct acpi_power_resource *resource = NULL;
>  	struct list_head *node, *next;
>  	struct acpi_power_reference *ref;
> @@ -505,12 +490,12 @@ static int acpi_power_seq_show(struct seq_file *seq, void *offset)
>  	if (!resource)
>  		goto end;
>  
> -	result = acpi_power_get_state(resource);
> +	result = acpi_power_get_state(resource, &state);
>  	if (result)
>  		goto end;
>  
>  	seq_puts(seq, "state:                   ");
> -	switch (resource->state) {
> +	switch (state) {
>  	case ACPI_POWER_RESOURCE_STATE_ON:
>  		seq_puts(seq, "on\n");
>  		break;
> @@ -591,7 +576,7 @@ static int acpi_power_remove_fs(struct acpi_device *device)
>  
>  static int acpi_power_add(struct acpi_device *device)
>  {
> -	int result = 0;
> +	int result = 0, state;
>  	acpi_status status = AE_OK;
>  	struct acpi_power_resource *resource = NULL;
>  	union acpi_object acpi_object;
> @@ -622,11 +607,11 @@ static int acpi_power_add(struct acpi_device *device)
>  	resource->system_level = acpi_object.power_resource.system_level;
>  	resource->order = acpi_object.power_resource.resource_order;
>  
> -	result = acpi_power_get_state(resource);
> +	result = acpi_power_get_state(resource, &state);
>  	if (result)
>  		goto end;
>  
> -	switch (resource->state) {
> +	switch (state) {
>  	case ACPI_POWER_RESOURCE_STATE_ON:
>  		device->power.state = ACPI_STATE_D0;
>  		break;
> @@ -643,7 +628,7 @@ static int acpi_power_add(struct acpi_device *device)
>  		goto end;
>  
>  	printk(KERN_INFO PREFIX "%s [%s] (%s)\n", acpi_device_name(device),
> -	       acpi_device_bid(device), resource->state ? "on" : "off");
> +	       acpi_device_bid(device), state ? "on" : "off");
>  
>        end:
>  	if (result)
> @@ -680,7 +665,7 @@ static int acpi_power_remove(struct acpi_device *device, int type)
>  
>  static int acpi_power_resume(struct acpi_device *device)
>  {
> -	int result = 0;
> +	int result = 0, state;
>  	struct acpi_power_resource *resource = NULL;
>  	struct acpi_power_reference *ref;
>  
> @@ -689,12 +674,12 @@ static int acpi_power_resume(struct acpi_device *device)
>  
>  	resource = (struct acpi_power_resource *)acpi_driver_data(device);
>  
> -	result = acpi_power_get_state(resource);
> +	result = acpi_power_get_state(resource, &state);
>  	if (result)
>  		return result;
>  
>  	mutex_lock(&resource->resource_lock);
> -	if ((resource->state == ACPI_POWER_RESOURCE_STATE_OFF) &&
> +	if (state == ACPI_POWER_RESOURCE_STATE_OFF &&
>  	    !list_empty(&resource->reference)) {
>  		ref = container_of(resource->reference.next, struct acpi_power_reference, node);
>  		mutex_unlock(&resource->resource_lock);
> 
> 
> 

-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [PATCH 09/12] ACPI: EC: Output changes to operational mode

[Rafael J. Wysocki]

On Monday, 22 October 2007 12:19, Alexey Starikovskiy wrote:
> Insert printk() for every change in operational mode.
> 
> Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
> ---

This has been requested by Len, IIRC.

Acked-by: Rafael J. Wysocki <rjw@sisk.pl>

>  drivers/acpi/ec.c |   17 ++++++++++++++---
>  1 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index 202db57..bf60b24 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -173,12 +173,17 @@ static int acpi_ec_wait(struct acpi_ec *ec, enum ec_event event, int force_poll)
>  			return 0;
>  		clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
>  		if (acpi_ec_check_status(ec, event)) {
> -			if (event == ACPI_EC_EVENT_OBF_1)
> +			if (event == ACPI_EC_EVENT_OBF_1) {
>  				/* miss OBF = 1 GPE, don't expect it anymore */
> +				printk(KERN_INFO PREFIX "missing OBF_1 confirmation,"
> +					"switching to degraded mode.\n");
>  				set_bit(EC_FLAGS_ONLY_IBF_GPE, &ec->flags);
> -			else
> +			} else {
>  				/* missing GPEs, switch back to poll mode */
> +				printk(KERN_INFO PREFIX "missing IBF_1 confirmations,"
> +					"switch off interrupt mode.\n");
>  				clear_bit(EC_FLAGS_GPE_MODE, &ec->flags);
> +			}
>  			return 0;
>  		}
>  	} else {
> @@ -491,8 +496,12 @@ static u32 acpi_ec_gpe_handler(void *data)
>  		if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags))
>  			status = acpi_os_execute(OSL_EC_BURST_HANDLER,
>  				acpi_ec_gpe_query, ec);
> -	} else if (unlikely(!test_bit(EC_FLAGS_GPE_MODE, &ec->flags)))
> +	} else if (unlikely(!test_bit(EC_FLAGS_GPE_MODE, &ec->flags))) {
> +		/* this is non-query, must be confirmation */
> +		printk(KERN_INFO PREFIX "non-query interrupt received,"
> +			" switching to interrupt mode\n");
>  		set_bit(EC_FLAGS_GPE_MODE, &ec->flags);
> +	}
>  
>  	return ACPI_SUCCESS(status) ?
>  	    ACPI_INTERRUPT_HANDLED : ACPI_INTERRUPT_NOT_HANDLED;
> @@ -740,6 +749,8 @@ static int acpi_ec_add(struct acpi_device *device)
>  	acpi_ec_add_fs(device);
>  	printk(KERN_INFO PREFIX "GPE = 0x%lx, I/O: command/status = 0x%lx, data = 0x%lx\n",
>  			  ec->gpe, ec->command_addr, ec->data_addr);
> +	printk(KERN_INFO PREFIX "driver started in %s mode\n",
> +		(test_bit(EC_FLAGS_GPE_MODE, &ec->flags))?"interrupt":"poll");
>  	return 0;
>  }
>  
> 
> 
> 

-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [PATCH 07/12] ACPI: EC: Don't re-enable GPE for each transaction.

[Rafael J. Wysocki]

On Monday, 22 October 2007 12:18, Alexey Starikovskiy wrote:
> With the auto selection of operation mode, absence of GPEs does not 
> really degrade performance, so let PM code to handle 
> enabling/disabling GPEs.
> This is a revert of 5d57a6a55ec0bdcb952dbcd3f8ffcde8a3ee9413,
> which was meant to be temporary.
> 
> Reference: http://bugzilla.kernel.org/show_bug.cgi?id=7977
> 
> Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
> ---

Acked-by: Rafael J. Wysocki <rjw@sisk.pl>

[I'm not sure about the resolution of the above bug, though.]
 
>  drivers/acpi/ec.c |    5 -----
>  1 files changed, 0 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index 50d55fe..41a21fc 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -258,9 +258,6 @@ static int acpi_ec_transaction(struct acpi_ec *ec, u8 command,
>  		}
>  	}
>  
> -	/* Make sure GPE is enabled before doing transaction */
> -	acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
> -
>  	status = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, 0);
>  	if (status) {
>  		printk(KERN_ERR PREFIX
> @@ -638,12 +635,10 @@ static struct acpi_ec *make_acpi_ec(void)
>  	struct acpi_ec *ec = kzalloc(sizeof(struct acpi_ec), GFP_KERNEL);
>  	if (!ec)
>  		return NULL;
> -
>  	ec->flags = 1 << EC_FLAGS_QUERY_PENDING;
>  	mutex_init(&ec->lock);
>  	init_waitqueue_head(&ec->wait);
>  	INIT_LIST_HEAD(&ec->list);
> -
>  	return ec;
>  }
>  
> 
> 
> 

-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [PATCH 02/12] ACPI: suspend: Wrong order of GPE restore.

[Rafael J. Wysocki]

On Monday, 22 October 2007 12:18, Alexey Starikovskiy wrote:
> acpi_leave_sleep_state() should have correct list of wake and
> runtime GPEs, which is available only after disable_wakeup_device()
> is called.
> 
> Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
> ---
> 
>  drivers/acpi/sleep/main.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/sleep/main.c b/drivers/acpi/sleep/main.c
> index 2f9d3c1..2c0b663 100644
> --- a/drivers/acpi/sleep/main.c
> +++ b/drivers/acpi/sleep/main.c
> @@ -167,8 +167,8 @@ static void acpi_pm_finish(void)
>  {
>  	u32 acpi_state = acpi_target_sleep_state;
>  
> -	acpi_leave_sleep_state(acpi_state);
>  	acpi_disable_wakeup_device(acpi_state);
> +	acpi_leave_sleep_state(acpi_state);
>  
>  	/* reset firmware waking vector */
>  	acpi_set_firmware_waking_vector((acpi_physical_address) 0);
> @@ -272,8 +272,8 @@ static void acpi_hibernation_finish(void)
>  	 * enable it here.
>  	 */
>  	acpi_enable();
> -	acpi_leave_sleep_state(ACPI_STATE_S4);
>  	acpi_disable_wakeup_device(ACPI_STATE_S4);
> +	acpi_leave_sleep_state(ACPI_STATE_S4);
>  
>  	/* reset firmware waking vector */
>  	acpi_set_firmware_waking_vector((acpi_physical_address) 0);
> 
> -
> 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
> 
> 

-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [PATCH 01/12] ACPI: sleep: Fix GPE suspend cleanup

[Rafael J. Wysocki]

On Monday, 22 October 2007 12:18, Alexey Starikovskiy wrote:
> Commit 9b039330808b83acac3597535da26f47ad1862ce removed 
> acpi_gpe_sleep_prepare(), the only function used at S5 transition
> Add call to generic acpi_enable_wake_device().
> 
> Reference: https://bugzilla.novell.com/show_bug.cgi?id=299882
> 
> Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
> ---

Acked-by: Rafael J. Wysocki <rjw@sisk.pl>

>  drivers/acpi/sleep/main.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/acpi/sleep/main.c b/drivers/acpi/sleep/main.c
> index f3d3867..2f9d3c1 100644
> --- a/drivers/acpi/sleep/main.c
> +++ b/drivers/acpi/sleep/main.c
> @@ -410,6 +410,7 @@ static void acpi_power_off(void)
>  	/* acpi_sleep_prepare(ACPI_STATE_S5) should have already been called */
>  	printk("%s called\n", __FUNCTION__);
>  	local_irq_disable();
> +	acpi_enable_wakeup_device(ACPI_STATE_S5);
>  	acpi_enter_sleep_state(ACPI_STATE_S5);
>  }
>  
> 
> -
> 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
> 
> 

-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [PATCH 03/12] ACPI: button: send initial lid state after add and resume

[Rafael J. Wysocki]

On Monday, 22 October 2007 12:18, Alexey Starikovskiy wrote:
> Input layer should know about initial state of lid switch,
> even before first notify.
> 
> Reference: https://bugzilla.novell.com/show_bug.cgi?id=326814
> 
> Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
> ---

I've already acked this one, IIRC.

>  drivers/acpi/button.c |   37 +++++++++++++++++++++++++++++--------
>  1 files changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index 301e832..24a7865 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -78,6 +78,7 @@ MODULE_DEVICE_TABLE(acpi, button_device_ids);
>  
>  static int acpi_button_add(struct acpi_device *device);
>  static int acpi_button_remove(struct acpi_device *device, int type);
> +static int acpi_button_resume(struct acpi_device *device);
>  static int acpi_button_info_open_fs(struct inode *inode, struct file *file);
>  static int acpi_button_state_open_fs(struct inode *inode, struct file *file);
>  
> @@ -87,6 +88,7 @@ static struct acpi_driver acpi_button_driver = {
>  	.ids = button_device_ids,
>  	.ops = {
>  		.add = acpi_button_add,
> +		.resume = acpi_button_resume,
>  		.remove = acpi_button_remove,
>  	},
>  };
> @@ -253,6 +255,19 @@ static int acpi_button_remove_fs(struct acpi_device *device)
>  /* --------------------------------------------------------------------------
>                                  Driver Interface
>     -------------------------------------------------------------------------- */
> +static int acpi_lid_send_state(struct acpi_button *button)
> +{
> +	unsigned long state;
> +	acpi_status status;
> +
> +	status = acpi_evaluate_integer(button->device->handle, "_LID", NULL,
> +					&state);
> +	if (ACPI_FAILURE(status))
> +		return -ENODEV;
> +	/* input layer checks if event is redundant */
> +	input_report_switch(button->input, SW_LID, !state);
> +	return 0;
> +}
>  
>  static void acpi_button_notify(acpi_handle handle, u32 event, void *data)
>  {
> @@ -265,15 +280,8 @@ static void acpi_button_notify(acpi_handle handle, u32 event, void *data)
>  	switch (event) {
>  	case ACPI_BUTTON_NOTIFY_STATUS:
>  		input = button->input;
> -
>  		if (button->type == ACPI_BUTTON_TYPE_LID) {
> -			struct acpi_handle *handle = button->device->handle;
> -			unsigned long state;
> -
> -			if (!ACPI_FAILURE(acpi_evaluate_integer(handle, "_LID",
> -								NULL, &state)))
> -				input_report_switch(input, SW_LID, !state);
> -
> +			acpi_lid_send_state(button);
>  		} else {
>  			int keycode = test_bit(KEY_SLEEP, input->keybit) ?
>  						KEY_SLEEP : KEY_POWER;
> @@ -336,6 +344,17 @@ static int acpi_button_install_notify_handlers(struct acpi_button *button)
>  	return ACPI_FAILURE(status) ? -ENODEV : 0;
>  }
>  
> +static int acpi_button_resume(struct acpi_device *device)
> +{
> +	struct acpi_button *button;
> +	if (!device)
> +		return -EINVAL;
> +	button = acpi_driver_data(device);
> +	if (button && button->type == ACPI_BUTTON_TYPE_LID)
> +		return acpi_lid_send_state(button);
> +	return 0;
> +}
> +
>  static void acpi_button_remove_notify_handlers(struct acpi_button *button)
>  {
>  	switch (button->type) {
> @@ -453,6 +472,8 @@ static int acpi_button_add(struct acpi_device *device)
>  	error = input_register_device(input);
>  	if (error)
>  		goto err_remove_handlers;
> +	if (button->type == ACPI_BUTTON_TYPE_LID)
> +		acpi_lid_send_state(button);
>  
>  	if (device->wakeup.flags.valid) {
>  		/* Button's GPE is run-wake GPE */
> 
> 
> 

-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [PATCH 04/12] ACPI: EC: Replace atomic variables with bits

[Rafael J. Wysocki]

On Monday, 22 October 2007 12:18, Alexey Starikovskiy wrote:
> Number of flags is about to be increased, so it is better to
> put them all into bits.
> No functional changes.
> 
> Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
> ---

Looks ok.

Acked-by: Rafael J. Wysocki <rjw@sisk.pl>

>  drivers/acpi/ec.c |   79 +++++++++++++++++++++++++----------------------------
>  1 files changed, 38 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index 7b41783..08fbe62 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -65,7 +65,7 @@ enum ec_command {
>  /* EC events */
>  enum ec_event {
>  	ACPI_EC_EVENT_OBF_1 = 1,	/* Output buffer full */
> -	ACPI_EC_EVENT_IBF_0,	/* Input buffer empty */
> +	ACPI_EC_EVENT_IBF_0,		/* Input buffer empty */
>  };
>  
>  #define ACPI_EC_DELAY		500	/* Wait 500ms max. during EC ops */
> @@ -76,6 +76,11 @@ static enum ec_mode {
>  	EC_POLL,		/* Input buffer empty */
>  } acpi_ec_mode = EC_INTR;
>  
> +enum {
> +	EC_FLAGS_WAIT_GPE = 0,		/* Don't check status until GPE arrives */
> +	EC_FLAGS_QUERY_PENDING,		/* Query is pending */
> +};
> +
>  static int acpi_ec_remove(struct acpi_device *device, int type);
>  static int acpi_ec_start(struct acpi_device *device);
>  static int acpi_ec_stop(struct acpi_device *device, int type);
> @@ -116,9 +121,8 @@ static struct acpi_ec {
>  	unsigned long command_addr;
>  	unsigned long data_addr;
>  	unsigned long global_lock;
> +	unsigned long flags;
>  	struct mutex lock;
> -	atomic_t query_pending;
> -	atomic_t event_count;
>  	wait_queue_head_t wait;
>  	struct list_head list;
>  	u8 handlers_installed;
> @@ -148,45 +152,42 @@ static inline void acpi_ec_write_data(struct acpi_ec *ec, u8 data)
>  	outb(data, ec->data_addr);
>  }
>  
> -static inline int acpi_ec_check_status(struct acpi_ec *ec, enum ec_event event,
> -				       unsigned old_count)
> +static inline int acpi_ec_check_status(struct acpi_ec *ec, enum ec_event event)
>  {
> -	u8 status = acpi_ec_read_status(ec);
> -	if (old_count == atomic_read(&ec->event_count))
> +	if (test_bit(EC_FLAGS_WAIT_GPE, &ec->flags))
>  		return 0;
>  	if (event == ACPI_EC_EVENT_OBF_1) {
> -		if (status & ACPI_EC_FLAG_OBF)
> +		if (acpi_ec_read_status(ec) & ACPI_EC_FLAG_OBF)
>  			return 1;
>  	} else if (event == ACPI_EC_EVENT_IBF_0) {
> -		if (!(status & ACPI_EC_FLAG_IBF))
> +		if (!(acpi_ec_read_status(ec) & ACPI_EC_FLAG_IBF))
>  			return 1;
>  	}
>  
>  	return 0;
>  }
>  
> -static int acpi_ec_wait(struct acpi_ec *ec, enum ec_event event,
> -                        unsigned count, int force_poll)
> +static int acpi_ec_wait(struct acpi_ec *ec, enum ec_event event, int force_poll)
>  {
>  	if (unlikely(force_poll) || acpi_ec_mode == EC_POLL) {
>  		unsigned long delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY);
> +		clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
>  		while (time_before(jiffies, delay)) {
> -			if (acpi_ec_check_status(ec, event, 0))
> +			if (acpi_ec_check_status(ec, event))
>  				return 0;
>  		}
>  	} else {
> -		if (wait_event_timeout(ec->wait,
> -				       acpi_ec_check_status(ec, event, count),
> -				       msecs_to_jiffies(ACPI_EC_DELAY)) ||
> -		    acpi_ec_check_status(ec, event, 0)) {
> +		if (wait_event_timeout(ec->wait, acpi_ec_check_status(ec, event),
> +				       msecs_to_jiffies(ACPI_EC_DELAY)))
> +			return 0;
> +		clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
> +		if (acpi_ec_check_status(ec, event)) {
>  			return 0;
> -		} else {
> -			printk(KERN_ERR PREFIX "acpi_ec_wait timeout,"
> -			       " status = %d, expect_event = %d\n",
> -			       acpi_ec_read_status(ec), event);
>  		}
>  	}
> -
> +	printk(KERN_ERR PREFIX "acpi_ec_wait timeout,"
> +			       " status = %d, expect_event = %d\n",
> +			       acpi_ec_read_status(ec), event);
>  	return -ETIME;
>  }
>  
> @@ -196,39 +197,38 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, u8 command,
>  					int force_poll)
>  {
>  	int result = 0;
> -	unsigned count = atomic_read(&ec->event_count);
> +	set_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
>  	acpi_ec_write_cmd(ec, command);
>  
>  	for (; wdata_len > 0; --wdata_len) {
> -		result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, count, force_poll);
> +		result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, force_poll);
>  		if (result) {
>  			printk(KERN_ERR PREFIX
>  			       "write_cmd timeout, command = %d\n", command);
>  			goto end;
>  		}
> -		count = atomic_read(&ec->event_count);
> +		set_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
>  		acpi_ec_write_data(ec, *(wdata++));
>  	}
>  
>  	if (!rdata_len) {
> -		result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, count, force_poll);
> +		result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, force_poll);
>  		if (result) {
>  			printk(KERN_ERR PREFIX
>  			       "finish-write timeout, command = %d\n", command);
>  			goto end;
>  		}
> -	} else if (command == ACPI_EC_COMMAND_QUERY) {
> -		atomic_set(&ec->query_pending, 0);
> -	}
> +	} else if (command == ACPI_EC_COMMAND_QUERY)
> +		clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
>  
>  	for (; rdata_len > 0; --rdata_len) {
> -		result = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF_1, count, force_poll);
> +		result = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF_1, force_poll);
>  		if (result) {
>  			printk(KERN_ERR PREFIX "read timeout, command = %d\n",
>  			       command);
>  			goto end;
>  		}
> -		count = atomic_read(&ec->event_count);
> +		set_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
>  		*(rdata++) = acpi_ec_read_data(ec);
>  	}
>        end:
> @@ -261,7 +261,7 @@ static int acpi_ec_transaction(struct acpi_ec *ec, u8 command,
>  	/* Make sure GPE is enabled before doing transaction */
>  	acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
>  
> -	status = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, 0, 0);
> +	status = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, 0);
>  	if (status) {
>  		printk(KERN_ERR PREFIX
>  		       "input buffer is not empty, aborting transaction\n");
> @@ -476,20 +476,18 @@ static void acpi_ec_gpe_query(void *ec_cxt)
>  static u32 acpi_ec_gpe_handler(void *data)
>  {
>  	acpi_status status = AE_OK;
> -	u8 value;
>  	struct acpi_ec *ec = data;
>  
> -	atomic_inc(&ec->event_count);
> +	clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
>  
>  	if (acpi_ec_mode == EC_INTR) {
>  		wake_up(&ec->wait);
>  	}
>  
> -	value = acpi_ec_read_status(ec);
> -	if ((value & ACPI_EC_FLAG_SCI) && !atomic_read(&ec->query_pending)) {
> -		atomic_set(&ec->query_pending, 1);
> -		status =
> -		    acpi_os_execute(OSL_EC_BURST_HANDLER, acpi_ec_gpe_query, ec);
> +	if (acpi_ec_read_status(ec) & ACPI_EC_FLAG_SCI) {
> +		if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags))
> +			status = acpi_os_execute(OSL_EC_BURST_HANDLER,
> +				acpi_ec_gpe_query, ec);
>  	}
>  
>  	return status == AE_OK ?
> @@ -642,8 +640,7 @@ static struct acpi_ec *make_acpi_ec(void)
>  	if (!ec)
>  		return NULL;
>  
> -	atomic_set(&ec->query_pending, 1);
> -	atomic_set(&ec->event_count, 1);
> +	ec->flags = 1 << EC_FLAGS_QUERY_PENDING;
>  	mutex_init(&ec->lock);
>  	init_waitqueue_head(&ec->wait);
>  	INIT_LIST_HEAD(&ec->list);
> @@ -833,7 +830,7 @@ static int acpi_ec_start(struct acpi_device *device)
>  	ret = ec_install_handlers(ec);
>  
>  	/* EC is fully operational, allow queries */
> -	atomic_set(&ec->query_pending, 0);
> +	clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
>  	return ret;
>  }
>  
> 
> 
> 

-- 
"Premature optimization is the root of all evil." - Donald Knuth