RE: [PATCH] Change spinlock mutex to semaphor in ec.c

RE: [PATCH] Change spinlock mutex to semaphor in ec.c

[Yu, Luming]

I have patch set for this issue filed at
 http://bugzilla.kernel.org/show_bug.cgi?id=3851#c28 .

The first patch is from you.

I think that is the right solution, but the patch itself needs wider test, and solve
any possible regression.


Thanks
Luming 

>-----Original Message-----
>From: acpi-devel-admin-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org 
>[mailto:acpi-devel-admin-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org] On Behalf Of 
>Dmitry Torokhov
>Sent: 2005年2月4日 13:56
>To: acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>Cc: Rich Townsend
>Subject: Re: [ACPI] [PATCH] Change spinlock mutex to semaphor in ec.c
>
>On Thursday 03 February 2005 19:24, Rich Townsend wrote:
>> Hi all --
>> 
>> Attached is a patch against the 2.6.10 kernel, to convert 
>the mutexing 
>> in the embdedded controller driver from a spinlock to a 
>semaphore. The 
>> rationale behind this is to permit sleeping inside the 
>acpi_ec_wait() 
>> polling routine.
>> 
>
>Hi,
>
>Last time such patch was proposed there were some concerns regarding
>possible interrupt storm, see the follwing thread:
>
>http://marc.theaimsgroup.com/?l=acpi4linux&m=110055992814866&w=2
>
>I have an alternative patch that tries to address these concerns,
>unfortunately I do not have a box with EC controller so it is
>completely untested.
>
>-- 
>Dmitry
>
>
>===================================================================
>
>
>ChangeSet@1.1997, 2005-02-04 00:43:27-05:00, dtor_core-yWtbtysYrB+LZ21kGMrzwg@public.gmane.org
>  ACPI: EC - switch from busy-polling to event-based completion
>        signalling. EC operations take quite a long time and busy
>        polling with interrupts disabled causes big latencies or
>        even losing timer interrupts.
>        EC IO timeout has been increased from 10 to 50ms.
>        Also get rid of unneeded variable initializations.
>  
>        Based on patch by Andi Kleen <ak-l3A5Bk7waGM@public.gmane.org>
>  
>  Signed-off-by: Dmitry Torokhov <dtor-JGs/UdohzUI@public.gmane.org>
>
>
> ec.c |  264 
>+++++++++++++++++++++++++++++++++++--------------------------------
> 1 files changed, 141 insertions(+), 123 deletions(-)
>
>
>===================================================================
>
>
>
>diff -Nru a/drivers/acpi/ec.c b/drivers/acpi/ec.c
>--- a/drivers/acpi/ec.c	2005-02-04 00:47:16 -05:00
>+++ b/drivers/acpi/ec.c	2005-02-04 00:47:16 -05:00
>@@ -31,6 +31,7 @@
> #include <linux/delay.h>
> #include <linux/proc_fs.h>
> #include <linux/seq_file.h>
>+#include <linux/interrupt.h>
> #include <asm/io.h>
> #include <acpi/acpi_bus.h>
> #include <acpi/acpi_drivers.h>
>@@ -51,11 +52,7 @@
> #define ACPI_EC_FLAG_IBF	0x02	/* Input buffer full */
> #define ACPI_EC_FLAG_SCI	0x20	/* EC-SCI occurred */
> 
>-#define ACPI_EC_EVENT_OBF	0x01	/* Output buffer full */
>-#define ACPI_EC_EVENT_IBE	0x02	/* Input buffer empty */
>-
>-#define ACPI_EC_UDELAY		100	/* Poll @ 100us 
>increments */
>-#define ACPI_EC_UDELAY_COUNT	1000	/* Wait 10ms max. 
>during EC ops */
>+#define ACPI_EC_DELAY		50	/* Wait 50ms max. 
>during EC ops */
> #define ACPI_EC_UDELAY_GLK	1000	/* Wait 1ms max. to get 
>global lock */
> 
> #define ACPI_EC_COMMAND_READ	0x80
>@@ -86,8 +83,11 @@
> 	struct acpi_generic_address	status_addr;
> 	struct acpi_generic_address	command_addr;
> 	struct acpi_generic_address	data_addr;
>+	u32				data;
> 	unsigned long			global_lock;
>-	spinlock_t			lock;
>+	unsigned int			rw_in_progress;
>+	struct semaphore		sem;
>+	wait_queue_head_t		wait;
> };
> 
> /* If we find an EC via the ECDT, we need to keep a ptr to 
>its context */
>@@ -100,53 +100,40 @@
>                              Transaction Management
>    
>---------------------------------------------------------------
>----------- */
> 
>-static int
>-acpi_ec_wait (
>-	struct acpi_ec		*ec,
>-	u8			event)
>+static inline u32 acpi_ec_read_status(struct acpi_ec *ec)
> {
>-	u32			acpi_ec_status = 0;
>-	u32			i = ACPI_EC_UDELAY_COUNT;
>+	u32	status = 0;
> 
>-	if (!ec)
>-		return -EINVAL;
>+	acpi_hw_low_level_read(8, &status, &ec->status_addr);
>+	return status;
>+}
> 
>-	/* Poll the EC status register waiting for the event to 
>occur. */
>-	switch (event) {
>-	case ACPI_EC_EVENT_OBF:
>-		do {
>-			acpi_hw_low_level_read(8, 
>&acpi_ec_status, &ec->status_addr);
>-			if (acpi_ec_status & ACPI_EC_FLAG_OBF)
>-				return 0;
>-			udelay(ACPI_EC_UDELAY);
>-		} while (--i>0);
>-		break;
>-	case ACPI_EC_EVENT_IBE:
>-		do {
>-			acpi_hw_low_level_read(8, 
>&acpi_ec_status, &ec->status_addr);
>-			if (!(acpi_ec_status & ACPI_EC_FLAG_IBF))
>-				return 0;
>-			udelay(ACPI_EC_UDELAY);
>-		} while (--i>0);
>-		break;
>-	default:
>-		return -EINVAL;
>+static inline void acpi_ec_prepare_for_wait(struct acpi_ec *ec)
>+{
>+	ec->rw_in_progress = 1;
>+	mb();
>+}
>+
>+static int acpi_ec_wait(struct acpi_ec *ec)
>+{
>+	if (wait_event_timeout(ec->wait, !ec->rw_in_progress,
>+				msecs_to_jiffies(ACPI_EC_DELAY)) == 0) {
>+		ec->rw_in_progress = 0;
>+		return -ETIME;
> 	}
> 
>-	return -ETIME;
>+	return 0;
> }
> 
>-
> static int
> acpi_ec_read (
> 	struct acpi_ec		*ec,
> 	u8			address,
> 	u32			*data)
> {
>-	acpi_status		status = AE_OK;
>-	int			result = 0;
>-	unsigned long		flags = 0;
>-	u32			glk = 0;
>+	acpi_status		status;
>+	int			result;
>+	u32			glk;
> 
> 	ACPI_FUNCTION_TRACE("acpi_ec_read");
> 
>@@ -160,27 +147,29 @@
> 		if (ACPI_FAILURE(status))
> 			return_VALUE(-ENODEV);
> 	}
>-	
>-	spin_lock_irqsave(&ec->lock, flags);
> 
>+	WARN_ON(in_interrupt());
>+	down(&ec->sem);
>+
>+	acpi_ec_prepare_for_wait(ec);
> 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_READ, 
>&ec->command_addr);
>-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
>+	result = acpi_ec_wait(ec);
> 	if (result)
> 		goto end;
> 
>+	acpi_ec_prepare_for_wait(ec);
> 	acpi_hw_low_level_write(8, address, &ec->data_addr);
>-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF);
>+	result = acpi_ec_wait(ec);
> 	if (result)
> 		goto end;
> 
>-
>-	acpi_hw_low_level_read(8, data, &ec->data_addr);
>+	*data = ec->data;
> 
> 	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Read [%02x] from 
>address [%02x]\n",
> 		*data, address));
> 
> end:
>-	spin_unlock_irqrestore(&ec->lock, flags);
>+	up(&ec->sem);
> 
> 	if (ec->global_lock)
> 		acpi_release_global_lock(glk);
>@@ -195,10 +184,9 @@
> 	u8			address,
> 	u8			data)
> {
>-	int			result = 0;
>-	acpi_status		status = AE_OK;
>-	unsigned long		flags = 0;
>-	u32			glk = 0;
>+	int			result;
>+	acpi_status		status;
>+	u32			glk;
> 
> 	ACPI_FUNCTION_TRACE("acpi_ec_write");
> 
>@@ -211,20 +199,24 @@
> 			return_VALUE(-ENODEV);
> 	}
> 
>-	spin_lock_irqsave(&ec->lock, flags);
>+	WARN_ON(in_interrupt());
>+	down(&ec->sem);
> 
>+	acpi_ec_prepare_for_wait(ec);
> 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_WRITE, 
>&ec->command_addr);
>-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
>+	result = acpi_ec_wait(ec);
> 	if (result)
> 		goto end;
> 
>+	acpi_ec_prepare_for_wait(ec);
> 	acpi_hw_low_level_write(8, address, &ec->data_addr);
>-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
>+	result = acpi_ec_wait(ec);
> 	if (result)
> 		goto end;
> 
>+	acpi_ec_prepare_for_wait(ec);
> 	acpi_hw_low_level_write(8, data, &ec->data_addr);
>-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
>+	result = acpi_ec_wait(ec);
> 	if (result)
> 		goto end;
> 
>@@ -232,7 +224,7 @@
> 		data, address));
> 
> end:
>-	spin_unlock_irqrestore(&ec->lock, flags);
>+	up(&ec->sem);
> 
> 	if (ec->global_lock)
> 		acpi_release_global_lock(glk);
>@@ -289,10 +281,9 @@
> 	struct acpi_ec		*ec,
> 	u32			*data)
> {
>-	int			result = 0;
>-	acpi_status		status = AE_OK;
>-	unsigned long		flags = 0;
>-	u32			glk = 0;
>+	int			result;
>+	acpi_status		status;
>+	u32			glk;
> 
> 	ACPI_FUNCTION_TRACE("acpi_ec_query");
> 
>@@ -312,20 +303,18 @@
> 	 * Note that successful completion of the query causes 
>the ACPI_EC_SCI
> 	 * bit to be cleared (and thus clearing the interrupt source).
> 	 */
>-	spin_lock_irqsave(&ec->lock, flags);
>-
>+	acpi_ec_prepare_for_wait(ec);
> 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_QUERY, 
>&ec->command_addr);
>-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF);
>+	result = acpi_ec_wait(ec);
> 	if (result)
> 		goto end;
>-	
>-	acpi_hw_low_level_read(8, data, &ec->data_addr);
>+
>+	*data = ec->data;
>+
> 	if (!*data)
> 		result = -ENODATA;
> 
> end:
>-	spin_unlock_irqrestore(&ec->lock, flags);
>-
> 	if (ec->global_lock)
> 		acpi_release_global_lock(glk);
> 
>@@ -342,38 +331,63 @@
> 	u8			data;
> };
> 
>+static void acpi_ec_check_rw_complete(struct acpi_ec *ec, u32 status)
>+{
>+	u32		data = 0;
>+
>+	if (status & ACPI_EC_FLAG_OBF)
>+		acpi_hw_low_level_read(8, &data, &ec->data_addr);
>+
>+	if (ec->rw_in_progress &&
>+	    ((status & ACPI_EC_FLAG_OBF) || (~status & 
>ACPI_EC_FLAG_IBF))) {
>+		ec->data = data;
>+		ec->rw_in_progress = 0;
>+		wake_up(&ec->wait);
>+	}
>+}
>+
> static void
> acpi_ec_gpe_query (
> 	void			*ec_cxt)
> {
> 	struct acpi_ec		*ec = (struct acpi_ec *) ec_cxt;
>-	u32			value = 0;
>-	unsigned long		flags = 0;
>+	u32			value;
>+	int			result = -ENODATA;
> 	static char		object_name[5] = {'_','Q','0','0','\0'};
> 	const char		hex[] = 
>{'0','1','2','3','4','5','6','7',
> 				         
>'8','9','A','B','C','D','E','F'};
> 
> 	ACPI_FUNCTION_TRACE("acpi_ec_gpe_query");
> 
>-	if (!ec_cxt)
>-		goto end;	
>-
>-	spin_lock_irqsave(&ec->lock, flags);
>-	acpi_hw_low_level_read(8, &value, &ec->command_addr);
>-	spin_unlock_irqrestore(&ec->lock, flags);
>-
>-	/* TBD: Implement asynch events!
>-	 * NOTE: All we care about are EC-SCI's.  Other EC events are
>-	 * handled via polling (yuck!).  This is because some systems
>-	 * treat EC-SCIs as level (versus EDGE!) triggered, preventing
>-	 *  a purely interrupt-driven approach (grumble, grumble).
>+	/*
>+	 * Because some systems treat EC-SCIs as level (versus EDGE!)
>+	 * triggered we can not just check SCI flag here. If there is
>+	 * a reader/writer waiting on response we need to wait till
>+	 * data is available and wake it up. Only when there is no
>+	 * outstanding requests we can finally service SCI.
> 	 */
>-	if (!(value & ACPI_EC_FLAG_SCI))
>-		goto end;
> 
>-	if (acpi_ec_query(ec, &value))
>+	while (unlikely(down_trylock(&ec->sem))) {
>+
>+		/* Reader or writer is waiting holding ec->sem */
>+		value = acpi_ec_read_status(ec);
>+		acpi_ec_check_rw_complete(ec, value);
>+
>+		/*
>+		 * Even if we never get IBE/OBF condition client
>+		 * will eventually time out allowing us proceed.
>+		 */
>+		msleep(1);
>+	}
>+
>+	if (acpi_ec_read_status(ec) & ACPI_EC_FLAG_SCI)
>+		result = acpi_ec_query(ec, &value);
>+
>+	up(&ec->sem);
>+
>+	if (result)
> 		goto end;
>-	
>+
> 	object_name[2] = hex[((value >> 4) & 0x0F)];
> 	object_name[3] = hex[(value & 0x0F)];
> 
>@@ -390,6 +404,7 @@
> 	void			*data)
> {
> 	acpi_status		status = AE_OK;
>+	u32			value;
> 	struct acpi_ec		*ec = (struct acpi_ec *) data;
> 
> 	if (!ec)
>@@ -397,13 +412,15 @@
> 
> 	acpi_disable_gpe(NULL, ec->gpe_bit, ACPI_ISR);
> 
>-	status = acpi_os_queue_for_execution(OSD_PRIORITY_GPE,
>-		acpi_ec_gpe_query, ec);
>+	value = acpi_ec_read_status(ec);
>+	acpi_ec_check_rw_complete(ec, value);
> 
>-	if (status == AE_OK)
>-		return ACPI_INTERRUPT_HANDLED;
>-	else
>-		return ACPI_INTERRUPT_NOT_HANDLED;
>+	if (value & ACPI_EC_FLAG_SCI)
>+		status = acpi_os_queue_for_execution(OSD_PRIORITY_GPE,
>+						acpi_ec_gpe_query, ec);
>+
>+	return status == AE_OK ?
>+		ACPI_INTERRUPT_HANDLED : ACPI_INTERRUPT_NOT_HANDLED;
> }
> 
> /* 
>---------------------------------------------------------------
>-----------
>@@ -421,10 +438,8 @@
> 	 * The EC object is in the handler context and is needed
> 	 * when calling the acpi_ec_space_handler.
> 	 */
>-	if(function == ACPI_REGION_DEACTIVATE) 
>-		*return_context = NULL;
>-	else 
>-		*return_context = handler_context;
>+	*return_context  = (function != ACPI_REGION_DEACTIVATE) ?
>+						handler_context : NULL;
> 
> 	return AE_OK;
> }
>@@ -439,9 +454,9 @@
> 	void			*handler_context,
> 	void			*region_context)
> {
>-	int			result = 0;
>-	struct acpi_ec		*ec = NULL;
>-	u32			temp = 0;
>+	int			result;
>+	struct acpi_ec		*ec;
>+	u32			temp;
> 	acpi_integer		f_v = 0;
> 	int 			i = 0;
> 
>@@ -450,7 +465,7 @@
> 	if ((address > 0xFF) || !value || !handler_context)
> 		return_VALUE(AE_BAD_PARAMETER);
> 
>-	if(bit_width != 8) {
>+	if (bit_width != 8) {
> 		printk(KERN_WARNING PREFIX 
>"acpi_ec_space_handler: bit_width should be 8\n");
> 		if (acpi_strict)
> 			return_VALUE(AE_BAD_PARAMETER);
>@@ -474,23 +489,23 @@
> 	}
> 
> 	bit_width -= 8;
>-	if(bit_width){
>+	if (bit_width) {
> 
>-		if(function == ACPI_READ)
>+		if (function == ACPI_READ)
> 			f_v |= (acpi_integer) (*value) << 8*i;
>-		if(function == ACPI_WRITE)
>-			(*value) >>=8; 
>+		if (function == ACPI_WRITE)
>+			(*value) >>=8;
> 		i++;
> 		goto next_byte;
> 	}
> 
> 
>-	if(function == ACPI_READ){
>+	if (function == ACPI_READ) {
> 		f_v |= (acpi_integer) (*value) << 8*i;
> 		*value = f_v;
> 	}
> 
>-		
>+
> out:
> 	switch (result) {
> 	case -EINVAL:
>@@ -505,8 +520,6 @@
> 	default:
> 		return_VALUE(AE_OK);
> 	}
>-	
>-
> }
> 
> 
>@@ -532,7 +545,7 @@
> 	seq_printf(seq, "ports:                   0x%02x, 0x%02x\n",
> 		(u32) ec->status_addr.address, (u32) 
>ec->data_addr.address);
> 	seq_printf(seq, "use global lock:         %s\n",
>-		ec->global_lock?"yes":"no");
>+		ec->global_lock ? "yes" : "no");
> 
> end:
> 	return_VALUE(0);
>@@ -555,7 +568,7 @@
> acpi_ec_add_fs (
> 	struct acpi_device	*device)
> {
>-	struct proc_dir_entry	*entry = NULL;
>+	struct proc_dir_entry	*entry;
> 
> 	ACPI_FUNCTION_TRACE("acpi_ec_add_fs");
> 
>@@ -606,9 +619,9 @@
> acpi_ec_add (
> 	struct acpi_device	*device)
> {
>-	int			result = 0;
>-	acpi_status		status = AE_OK;
>-	struct acpi_ec		*ec = NULL;
>+	int			result;
>+	acpi_status		status;
>+	struct acpi_ec		*ec;
> 	unsigned long		uid;
> 
> 	ACPI_FUNCTION_TRACE("acpi_ec_add");
>@@ -623,7 +636,8 @@
> 
> 	ec->handle = device->handle;
> 	ec->uid = -1;
>-	spin_lock_init(&ec->lock);
>+	init_MUTEX(&ec->sem);
>+	init_waitqueue_head(&ec->wait);
> 	strcpy(acpi_device_name(device), ACPI_EC_DEVICE_NAME);
> 	strcpy(acpi_device_class(device), ACPI_EC_CLASS);
> 	acpi_driver_data(device) = ec;
>@@ -637,7 +651,7 @@
> 	if (ec_ecdt && ec_ecdt->uid == uid) {
> 		acpi_remove_address_space_handler(ACPI_ROOT_OBJECT,
> 			ACPI_ADR_SPACE_EC, &acpi_ec_space_handler);
>-	
>+
> 		acpi_remove_gpe_handler(NULL, ec_ecdt->gpe_bit, 
>&acpi_ec_gpe_handler);
> 
> 		kfree(ec_ecdt);
>@@ -677,7 +691,7 @@
> 	struct acpi_device	*device,
> 	int			type)
> {
>-	struct acpi_ec		*ec = NULL;
>+	struct acpi_ec		*ec;
> 
> 	ACPI_FUNCTION_TRACE("acpi_ec_remove");
> 
>@@ -732,8 +746,8 @@
> acpi_ec_start (
> 	struct acpi_device	*device)
> {
>-	acpi_status		status = AE_OK;
>-	struct acpi_ec		*ec = NULL;
>+	acpi_status		status;
>+	struct acpi_ec		*ec;
> 
> 	ACPI_FUNCTION_TRACE("acpi_ec_start");
> 
>@@ -789,8 +803,8 @@
> 	struct acpi_device	*device,
> 	int			type)
> {
>-	acpi_status		status = AE_OK;
>-	struct acpi_ec		*ec = NULL;
>+	acpi_status		status;
>+	struct acpi_ec		*ec;
> 
> 	ACPI_FUNCTION_TRACE("acpi_ec_stop");
> 
>@@ -824,6 +838,7 @@
> 		acpi_ec_io_ports, ec_ecdt);
> 	if (ACPI_FAILURE(status))
> 		return status;
>+
> 	ec_ecdt->status_addr = ec_ecdt->command_addr;
> 
> 	ec_ecdt->uid = -1;
>@@ -832,7 +847,9 @@
> 	status = acpi_evaluate_integer(handle, "_GPE", NULL, 
>&ec_ecdt->gpe_bit);
> 	if (ACPI_FAILURE(status))
> 		return status;
>-	spin_lock_init(&ec_ecdt->lock);
>+
>+	init_MUTEX(&ec_ecdt->sem);
>+	init_waitqueue_head(&ec_ecdt->wait);
> 	ec_ecdt->global_lock = TRUE;
> 	ec_ecdt->handle = handle;
> 
>@@ -890,7 +907,7 @@
> 	acpi_status		status;
> 	struct acpi_table_ecdt 	*ecdt_ptr;
> 
>-	status = acpi_get_firmware_table("ECDT", 1, 
>ACPI_LOGICAL_ADDRESSING, 
>+	status = acpi_get_firmware_table("ECDT", 1, 
>ACPI_LOGICAL_ADDRESSING,
> 		(struct acpi_table_header **) &ecdt_ptr);
> 	if (ACPI_FAILURE(status))
> 		return -ENODEV;
>@@ -905,11 +922,12 @@
> 		return -ENOMEM;
> 	memset(ec_ecdt, 0, sizeof(struct acpi_ec));
> 
>+	init_MUTEX(&ec_ecdt->sem);
>+	init_waitqueue_head(&ec_ecdt->wait);
> 	ec_ecdt->command_addr = ecdt_ptr->ec_control;
> 	ec_ecdt->status_addr = ecdt_ptr->ec_control;
> 	ec_ecdt->data_addr = ecdt_ptr->ec_data;
> 	ec_ecdt->gpe_bit = ecdt_ptr->gpe_bit;
>-	spin_lock_init(&ec_ecdt->lock);
> 	/* use the GL just to be safe */
> 	ec_ecdt->global_lock = TRUE;
> 	ec_ecdt->uid = ecdt_ptr->uid;
>@@ -978,7 +996,7 @@
> 
> static int __init acpi_ec_init (void)
> {
>-	int			result = 0;
>+	int			result;
> 
> 	ACPI_FUNCTION_TRACE("acpi_ec_init");
> 
>
>
>-------------------------------------------------------
>This SF.Net email is sponsored by: IntelliVIEW -- Interactive Reporting
>Tool for open source databases. Create drag-&-drop reports. Save time
>by over 75%! Publish reports on the web. Export to DOC, XLS, RTF, etc.
>Download a FREE copy at http://www.intelliview.com/go/osdn_nl
>_______________________________________________
>Acpi-devel mailing list
>Acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>https://lists.sourceforge.net/lists/listinfo/acpi-devel
>


-------------------------------------------------------
This SF.Net email is sponsored by: IntelliVIEW -- Interactive Reporting
Tool for open source databases. Create drag-&-drop reports. Save time
by over 75%! Publish reports on the web. Export to DOC, XLS, RTF, etc.
Download a FREE copy at http://www.intelliview.com/go/osdn_nl

Re: [PATCH] Change spinlock mutex to semaphor in ec.c

[Dmitry Torokhov]

On Friday 04 February 2005 01:31, Yu, Luming wrote:
> I have patch set for this issue filed at
>  http://bugzilla.kernel.org/show_bug.cgi?id=3851#c28 .
> 
> The first patch is from you.
> 

Oh, I see... this is a newer version though, simplified a bit (I think).

-- 
Dmitry


-------------------------------------------------------
This SF.Net email is sponsored by: IntelliVIEW -- Interactive Reporting
Tool for open source databases. Create drag-&-drop reports. Save time
by over 75%! Publish reports on the web. Export to DOC, XLS, RTF, etc.
Download a FREE copy at http://www.intelliview.com/go/osdn_nl

Re: [PATCH] Change spinlock mutex to semaphor in ec.c

[Rich Townsend]

Dmitry Torokhov wrote:
> On Friday 04 February 2005 01:31, Yu, Luming wrote:
> 
>>I have patch set for this issue filed at
>> http://bugzilla.kernel.org/show_bug.cgi?id=3851#c28 .
>>
>>The first patch is from you.
>>
> 
> 
> Oh, I see... this is a newer version though, simplified a bit (I think).
> 

Hi Dmitry & Luming --

I'm glad to see that better minds than mine have been working on this 
problem!

However, I can't get either of the patches on the above Bugzilla page to 
apply cleanly to 2.6.10. Are there any up-to-date versions of the patch?

cheers,

Rich



-------------------------------------------------------
This SF.Net email is sponsored by: IntelliVIEW -- Interactive Reporting
Tool for open source databases. Create drag-&-drop reports. Save time
by over 75%! Publish reports on the web. Export to DOC, XLS, RTF, etc.
Download a FREE copy at http://www.intelliview.com/go/osdn_nl

Re: [PATCH] Change spinlock mutex to semaphor in ec.c

[Dmitry Torokhov]

On Fri, 04 Feb 2005 07:05:59 -0500, Rich Townsend <rhdt-OBnUx95tOyn10jlvfTC4gA@public.gmane.org> wrote:
> Dmitry Torokhov wrote:
> > On Friday 04 February 2005 01:31, Yu, Luming wrote:
> >
> >>I have patch set for this issue filed at
> >> http://bugzilla.kernel.org/show_bug.cgi?id=3851#c28 .
> >>
> >>The first patch is from you.
> >>
> >
> >
> > Oh, I see... this is a newer version though, simplified a bit (I think).
> >
> 
> Hi Dmitry & Luming --
> 
> I'm glad to see that better minds than mine have been working on this
> problem!
> 
> However, I can't get either of the patches on the above Bugzilla page to
> apply cleanly to 2.6.10. Are there any up-to-date versions of the patch?
> 

The patch I sent in e-mail is against post 2.6.11-rc2 pull from Linus
tree. Not sure if it will apply to 2.6.10 though.

-- 
Dmitry


-------------------------------------------------------
This SF.Net email is sponsored by: IntelliVIEW -- Interactive Reporting
Tool for open source databases. Create drag-&-drop reports. Save time
by over 75%! Publish reports on the web. Export to DOC, XLS, RTF, etc.
Download a FREE copy at http://www.intelliview.com/go/osdn_nl

[PATCH] Change spinlock mutex to semaphor in ec.c

[Rich Townsend]

[-- Attachment #1: Type: text/plain, Size: 1487 bytes --]

Hi all --

Attached is a patch against the 2.6.10 kernel, to convert the mutexing 
in the embdedded controller driver from a spinlock to a semaphore. The 
rationale behind this is to permit sleeping inside the acpi_ec_wait() 
polling routine.

The way things are done at the moment, the pause between each poll of 
the EC is done using udelay(). Unfortunately, a typical SMBus access via 
the EC takes around 3ms, during which interrupts cannot be serviced. In 
my smart battery driver, each read of the battery status takes 5 SMBus 
transactions, so we have a total 15ms of interrupt deadtime. Considering 
that typical monitoring tools (e.g., gkrellm, wmacpi) will read the 
battery every few seconds, we can see that we are almost certain to 
start losing interrupts. And we do; using my most recent version of the 
smart battery driver, I get lost keystrokes all over the place.

This patch changes the spinlock mutexes into semaphores, which in turn 
allows msleep() calls to replace the udelay() calls in acpi_ec_wait. 
I've found that this seems to work great; no more lost keystrokes (or at 
least, a reduction to the point where I can't distinguish them from my 
own bad typing), and my system hasn't exploded yet.

I'll be including this patch when I release the new DSDT stuff for smart 
batteries (should be ready by the weekend, just doing final testing); 
but I'd appreciate some input as to whether this patch is OK, or whether 
I've done a Very Bad Thing.

cheers,

Rich

[-- Attachment #2: acpi-ec-nospinlock-2.6.10.diff --]
[-- Type: text/plain, Size: 5777 bytes --]

diff -Nurp linux-2.6.10/drivers/acpi/ec.c linux-2.6.10-ec-nospinlock/drivers/acpi/ec.c
--- linux-2.6.10/drivers/acpi/ec.c	2004-12-24 16:34:58.000000000 -0500
+++ linux-2.6.10-ec-nospinlock/drivers/acpi/ec.c	2005-02-03 19:01:25.490539512 -0500
@@ -31,6 +31,7 @@
 #include <linux/delay.h>
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
+#include <asm/semaphore.h>
 #include <asm/io.h>
 #include <acpi/acpi_bus.h>
 #include <acpi/acpi_drivers.h>
@@ -54,8 +55,8 @@ ACPI_MODULE_NAME		("acpi_ec")
 #define ACPI_EC_EVENT_OBF	0x01	/* Output buffer full */
 #define ACPI_EC_EVENT_IBE	0x02	/* Input buffer empty */
 
-#define ACPI_EC_UDELAY		100	/* Poll @ 100us increments */
-#define ACPI_EC_UDELAY_COUNT	1000	/* Wait 10ms max. during EC ops */
+#define ACPI_EC_MSLEEP		1	/* Sleep 1ms between polling */
+#define ACPI_EC_MSLEEP_COUNT	10	/* Wait 10ms max. during EC ops */
 #define ACPI_EC_UDELAY_GLK	1000	/* Wait 1ms max. to get global lock */
 
 #define ACPI_EC_COMMAND_READ	0x80
@@ -87,7 +88,7 @@ struct acpi_ec {
 	struct acpi_generic_address	command_addr;
 	struct acpi_generic_address	data_addr;
 	unsigned long			global_lock;
-	spinlock_t			lock;
+	struct semaphore		sem;
 };
 
 /* If we find an EC via the ECDT, we need to keep a ptr to its context */
@@ -106,7 +107,7 @@ acpi_ec_wait (
 	u8			event)
 {
 	u32			acpi_ec_status = 0;
-	u32			i = ACPI_EC_UDELAY_COUNT;
+	u32			i = ACPI_EC_MSLEEP_COUNT;
 
 	if (!ec)
 		return -EINVAL;
@@ -118,7 +119,7 @@ acpi_ec_wait (
 			acpi_hw_low_level_read(8, &acpi_ec_status, &ec->status_addr);
 			if (acpi_ec_status & ACPI_EC_FLAG_OBF)
 				return 0;
-			udelay(ACPI_EC_UDELAY);
+			msleep(ACPI_EC_MSLEEP);
 		} while (--i>0);
 		break;
 	case ACPI_EC_EVENT_IBE:
@@ -126,7 +127,7 @@ acpi_ec_wait (
 			acpi_hw_low_level_read(8, &acpi_ec_status, &ec->status_addr);
 			if (!(acpi_ec_status & ACPI_EC_FLAG_IBF))
 				return 0;
-			udelay(ACPI_EC_UDELAY);
+			msleep(ACPI_EC_MSLEEP);
 		} while (--i>0);
 		break;
 	default:
@@ -137,7 +138,7 @@ acpi_ec_wait (
 }
 
 
-static int
+int
 acpi_ec_read (
 	struct acpi_ec		*ec,
 	u8			address,
@@ -145,7 +146,6 @@ acpi_ec_read (
 {
 	acpi_status		status = AE_OK;
 	int			result = 0;
-	unsigned long		flags = 0;
 	u32			glk = 0;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_read");
@@ -160,9 +160,12 @@ acpi_ec_read (
 		if (ACPI_FAILURE(status))
 			return_VALUE(-ENODEV);
 	}
-	
-	spin_lock_irqsave(&ec->lock, flags);
 
+	if (down_interruptible (&ec->sem)) {
+		result = -ERESTARTSYS;
+		goto end_nosem;
+	}
+	
 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_READ, &ec->command_addr);
 	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
 	if (result)
@@ -180,16 +183,15 @@ acpi_ec_read (
 		*data, address));
 
 end:
-	spin_unlock_irqrestore(&ec->lock, flags);
-
+	up (&ec->sem);
+end_nosem:
 	if (ec->global_lock)
 		acpi_release_global_lock(glk);
 
 	return_VALUE(result);
 }
 
-
-static int
+int
 acpi_ec_write (
 	struct acpi_ec		*ec,
 	u8			address,
@@ -197,7 +199,6 @@ acpi_ec_write (
 {
 	int			result = 0;
 	acpi_status		status = AE_OK;
-	unsigned long		flags = 0;
 	u32			glk = 0;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_write");
@@ -211,8 +212,11 @@ acpi_ec_write (
 			return_VALUE(-ENODEV);
 	}
 
-	spin_lock_irqsave(&ec->lock, flags);
-
+	if (down_interruptible (&ec->sem)) {
+		result = -ERESTARTSYS;
+		goto end_nosem;
+	}
+	
 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_WRITE, &ec->command_addr);
 	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
 	if (result)
@@ -232,8 +236,8 @@ acpi_ec_write (
 		data, address));
 
 end:
-	spin_unlock_irqrestore(&ec->lock, flags);
-
+	up (&ec->sem);
+end_nosem:
 	if (ec->global_lock)
 		acpi_release_global_lock(glk);
 
@@ -291,7 +295,6 @@ acpi_ec_query (
 {
 	int			result = 0;
 	acpi_status		status = AE_OK;
-	unsigned long		flags = 0;
 	u32			glk = 0;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_query");
@@ -312,8 +315,11 @@ acpi_ec_query (
 	 * Note that successful completion of the query causes the ACPI_EC_SCI
 	 * bit to be cleared (and thus clearing the interrupt source).
 	 */
-	spin_lock_irqsave(&ec->lock, flags);
-
+	if (down_interruptible (&ec->sem)) {
+		result = -ERESTARTSYS;
+		goto end_nosem;
+	}
+	
 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_QUERY, &ec->command_addr);
 	result = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF);
 	if (result)
@@ -324,8 +330,8 @@ acpi_ec_query (
 		result = -ENODATA;
 
 end:
-	spin_unlock_irqrestore(&ec->lock, flags);
-
+	up (&ec->sem);
+end_nosem:
 	if (ec->global_lock)
 		acpi_release_global_lock(glk);
 
@@ -348,7 +354,6 @@ acpi_ec_gpe_query (
 {
 	struct acpi_ec		*ec = (struct acpi_ec *) ec_cxt;
 	u32			value = 0;
-	unsigned long		flags = 0;
 	static char		object_name[5] = {'_','Q','0','0','\0'};
 	const char		hex[] = {'0','1','2','3','4','5','6','7',
 				         '8','9','A','B','C','D','E','F'};
@@ -358,9 +363,11 @@ acpi_ec_gpe_query (
 	if (!ec_cxt)
 		goto end;	
 
-	spin_lock_irqsave(&ec->lock, flags);
+	if (down_interruptible (&ec->sem)) {
+		return_VOID;
+	}
 	acpi_hw_low_level_read(8, &value, &ec->command_addr);
-	spin_unlock_irqrestore(&ec->lock, flags);
+	up (&ec->sem);
 
 	/* TBD: Implement asynch events!
 	 * NOTE: All we care about are EC-SCI's.  Other EC events are
@@ -599,7 +606,7 @@ acpi_ec_add (
 
 	ec->handle = device->handle;
 	ec->uid = -1;
-	ec->lock = SPIN_LOCK_UNLOCKED;
+	sema_init(&ec->sem, 1);
 	strcpy(acpi_device_name(device), ACPI_EC_DEVICE_NAME);
 	strcpy(acpi_device_class(device), ACPI_EC_CLASS);
 	acpi_driver_data(device) = ec;
@@ -813,7 +820,7 @@ acpi_ec_ecdt_probe (void)
 	ec_ecdt->status_addr = ecdt_ptr->ec_control;
 	ec_ecdt->data_addr = ecdt_ptr->ec_data;
 	ec_ecdt->gpe_bit = ecdt_ptr->gpe_bit;
-	ec_ecdt->lock = SPIN_LOCK_UNLOCKED;
+	sema_init(&ec_ecdt->sem, 1);
 	/* use the GL just to be safe */
 	ec_ecdt->global_lock = TRUE;
 	ec_ecdt->uid = ecdt_ptr->uid;

Re: [PATCH] Change spinlock mutex to semaphor in ec.c

[Dmitry Torokhov]

On Thursday 03 February 2005 19:24, Rich Townsend wrote:
> Hi all --
> 
> Attached is a patch against the 2.6.10 kernel, to convert the mutexing 
> in the embdedded controller driver from a spinlock to a semaphore. The 
> rationale behind this is to permit sleeping inside the acpi_ec_wait() 
> polling routine.
> 

Hi,

Last time such patch was proposed there were some concerns regarding
possible interrupt storm, see the follwing thread:

http://marc.theaimsgroup.com/?l=acpi4linux&m=110055992814866&w=2

I have an alternative patch that tries to address these concerns,
unfortunately I do not have a box with EC controller so it is
completely untested.

-- 
Dmitry


===================================================================


ChangeSet@1.1997, 2005-02-04 00:43:27-05:00, dtor_core-yWtbtysYrB+LZ21kGMrzwg@public.gmane.org
  ACPI: EC - switch from busy-polling to event-based completion
        signalling. EC operations take quite a long time and busy
        polling with interrupts disabled causes big latencies or
        even losing timer interrupts.
        EC IO timeout has been increased from 10 to 50ms.
        Also get rid of unneeded variable initializations.
  
        Based on patch by Andi Kleen <ak-l3A5Bk7waGM@public.gmane.org>
  
  Signed-off-by: Dmitry Torokhov <dtor-JGs/UdohzUI@public.gmane.org>


 ec.c |  264 +++++++++++++++++++++++++++++++++++--------------------------------
 1 files changed, 141 insertions(+), 123 deletions(-)


===================================================================



diff -Nru a/drivers/acpi/ec.c b/drivers/acpi/ec.c
--- a/drivers/acpi/ec.c	2005-02-04 00:47:16 -05:00
+++ b/drivers/acpi/ec.c	2005-02-04 00:47:16 -05:00
@@ -31,6 +31,7 @@
 #include <linux/delay.h>
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
+#include <linux/interrupt.h>
 #include <asm/io.h>
 #include <acpi/acpi_bus.h>
 #include <acpi/acpi_drivers.h>
@@ -51,11 +52,7 @@
 #define ACPI_EC_FLAG_IBF	0x02	/* Input buffer full */
 #define ACPI_EC_FLAG_SCI	0x20	/* EC-SCI occurred */
 
-#define ACPI_EC_EVENT_OBF	0x01	/* Output buffer full */
-#define ACPI_EC_EVENT_IBE	0x02	/* Input buffer empty */
-
-#define ACPI_EC_UDELAY		100	/* Poll @ 100us increments */
-#define ACPI_EC_UDELAY_COUNT	1000	/* Wait 10ms max. during EC ops */
+#define ACPI_EC_DELAY		50	/* Wait 50ms max. during EC ops */
 #define ACPI_EC_UDELAY_GLK	1000	/* Wait 1ms max. to get global lock */
 
 #define ACPI_EC_COMMAND_READ	0x80
@@ -86,8 +83,11 @@
 	struct acpi_generic_address	status_addr;
 	struct acpi_generic_address	command_addr;
 	struct acpi_generic_address	data_addr;
+	u32				data;
 	unsigned long			global_lock;
-	spinlock_t			lock;
+	unsigned int			rw_in_progress;
+	struct semaphore		sem;
+	wait_queue_head_t		wait;
 };
 
 /* If we find an EC via the ECDT, we need to keep a ptr to its context */
@@ -100,53 +100,40 @@
                              Transaction Management
    -------------------------------------------------------------------------- */
 
-static int
-acpi_ec_wait (
-	struct acpi_ec		*ec,
-	u8			event)
+static inline u32 acpi_ec_read_status(struct acpi_ec *ec)
 {
-	u32			acpi_ec_status = 0;
-	u32			i = ACPI_EC_UDELAY_COUNT;
+	u32	status = 0;
 
-	if (!ec)
-		return -EINVAL;
+	acpi_hw_low_level_read(8, &status, &ec->status_addr);
+	return status;
+}
 
-	/* Poll the EC status register waiting for the event to occur. */
-	switch (event) {
-	case ACPI_EC_EVENT_OBF:
-		do {
-			acpi_hw_low_level_read(8, &acpi_ec_status, &ec->status_addr);
-			if (acpi_ec_status & ACPI_EC_FLAG_OBF)
-				return 0;
-			udelay(ACPI_EC_UDELAY);
-		} while (--i>0);
-		break;
-	case ACPI_EC_EVENT_IBE:
-		do {
-			acpi_hw_low_level_read(8, &acpi_ec_status, &ec->status_addr);
-			if (!(acpi_ec_status & ACPI_EC_FLAG_IBF))
-				return 0;
-			udelay(ACPI_EC_UDELAY);
-		} while (--i>0);
-		break;
-	default:
-		return -EINVAL;
+static inline void acpi_ec_prepare_for_wait(struct acpi_ec *ec)
+{
+	ec->rw_in_progress = 1;
+	mb();
+}
+
+static int acpi_ec_wait(struct acpi_ec *ec)
+{
+	if (wait_event_timeout(ec->wait, !ec->rw_in_progress,
+				msecs_to_jiffies(ACPI_EC_DELAY)) == 0) {
+		ec->rw_in_progress = 0;
+		return -ETIME;
 	}
 
-	return -ETIME;
+	return 0;
 }
 
-
 static int
 acpi_ec_read (
 	struct acpi_ec		*ec,
 	u8			address,
 	u32			*data)
 {
-	acpi_status		status = AE_OK;
-	int			result = 0;
-	unsigned long		flags = 0;
-	u32			glk = 0;
+	acpi_status		status;
+	int			result;
+	u32			glk;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_read");
 
@@ -160,27 +147,29 @@
 		if (ACPI_FAILURE(status))
 			return_VALUE(-ENODEV);
 	}
-	
-	spin_lock_irqsave(&ec->lock, flags);
 
+	WARN_ON(in_interrupt());
+	down(&ec->sem);
+
+	acpi_ec_prepare_for_wait(ec);
 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_READ, &ec->command_addr);
-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
+	result = acpi_ec_wait(ec);
 	if (result)
 		goto end;
 
+	acpi_ec_prepare_for_wait(ec);
 	acpi_hw_low_level_write(8, address, &ec->data_addr);
-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF);
+	result = acpi_ec_wait(ec);
 	if (result)
 		goto end;
 
-
-	acpi_hw_low_level_read(8, data, &ec->data_addr);
+	*data = ec->data;
 
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Read [%02x] from address [%02x]\n",
 		*data, address));
 
 end:
-	spin_unlock_irqrestore(&ec->lock, flags);
+	up(&ec->sem);
 
 	if (ec->global_lock)
 		acpi_release_global_lock(glk);
@@ -195,10 +184,9 @@
 	u8			address,
 	u8			data)
 {
-	int			result = 0;
-	acpi_status		status = AE_OK;
-	unsigned long		flags = 0;
-	u32			glk = 0;
+	int			result;
+	acpi_status		status;
+	u32			glk;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_write");
 
@@ -211,20 +199,24 @@
 			return_VALUE(-ENODEV);
 	}
 
-	spin_lock_irqsave(&ec->lock, flags);
+	WARN_ON(in_interrupt());
+	down(&ec->sem);
 
+	acpi_ec_prepare_for_wait(ec);
 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_WRITE, &ec->command_addr);
-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
+	result = acpi_ec_wait(ec);
 	if (result)
 		goto end;
 
+	acpi_ec_prepare_for_wait(ec);
 	acpi_hw_low_level_write(8, address, &ec->data_addr);
-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
+	result = acpi_ec_wait(ec);
 	if (result)
 		goto end;
 
+	acpi_ec_prepare_for_wait(ec);
 	acpi_hw_low_level_write(8, data, &ec->data_addr);
-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
+	result = acpi_ec_wait(ec);
 	if (result)
 		goto end;
 
@@ -232,7 +224,7 @@
 		data, address));
 
 end:
-	spin_unlock_irqrestore(&ec->lock, flags);
+	up(&ec->sem);
 
 	if (ec->global_lock)
 		acpi_release_global_lock(glk);
@@ -289,10 +281,9 @@
 	struct acpi_ec		*ec,
 	u32			*data)
 {
-	int			result = 0;
-	acpi_status		status = AE_OK;
-	unsigned long		flags = 0;
-	u32			glk = 0;
+	int			result;
+	acpi_status		status;
+	u32			glk;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_query");
 
@@ -312,20 +303,18 @@
 	 * Note that successful completion of the query causes the ACPI_EC_SCI
 	 * bit to be cleared (and thus clearing the interrupt source).
 	 */
-	spin_lock_irqsave(&ec->lock, flags);
-
+	acpi_ec_prepare_for_wait(ec);
 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_QUERY, &ec->command_addr);
-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF);
+	result = acpi_ec_wait(ec);
 	if (result)
 		goto end;
-	
-	acpi_hw_low_level_read(8, data, &ec->data_addr);
+
+	*data = ec->data;
+
 	if (!*data)
 		result = -ENODATA;
 
 end:
-	spin_unlock_irqrestore(&ec->lock, flags);
-
 	if (ec->global_lock)
 		acpi_release_global_lock(glk);
 
@@ -342,38 +331,63 @@
 	u8			data;
 };
 
+static void acpi_ec_check_rw_complete(struct acpi_ec *ec, u32 status)
+{
+	u32		data = 0;
+
+	if (status & ACPI_EC_FLAG_OBF)
+		acpi_hw_low_level_read(8, &data, &ec->data_addr);
+
+	if (ec->rw_in_progress &&
+	    ((status & ACPI_EC_FLAG_OBF) || (~status & ACPI_EC_FLAG_IBF))) {
+		ec->data = data;
+		ec->rw_in_progress = 0;
+		wake_up(&ec->wait);
+	}
+}
+
 static void
 acpi_ec_gpe_query (
 	void			*ec_cxt)
 {
 	struct acpi_ec		*ec = (struct acpi_ec *) ec_cxt;
-	u32			value = 0;
-	unsigned long		flags = 0;
+	u32			value;
+	int			result = -ENODATA;
 	static char		object_name[5] = {'_','Q','0','0','\0'};
 	const char		hex[] = {'0','1','2','3','4','5','6','7',
 				         '8','9','A','B','C','D','E','F'};
 
 	ACPI_FUNCTION_TRACE("acpi_ec_gpe_query");
 
-	if (!ec_cxt)
-		goto end;	
-
-	spin_lock_irqsave(&ec->lock, flags);
-	acpi_hw_low_level_read(8, &value, &ec->command_addr);
-	spin_unlock_irqrestore(&ec->lock, flags);
-
-	/* TBD: Implement asynch events!
-	 * NOTE: All we care about are EC-SCI's.  Other EC events are
-	 * handled via polling (yuck!).  This is because some systems
-	 * treat EC-SCIs as level (versus EDGE!) triggered, preventing
-	 *  a purely interrupt-driven approach (grumble, grumble).
+	/*
+	 * Because some systems treat EC-SCIs as level (versus EDGE!)
+	 * triggered we can not just check SCI flag here. If there is
+	 * a reader/writer waiting on response we need to wait till
+	 * data is available and wake it up. Only when there is no
+	 * outstanding requests we can finally service SCI.
 	 */
-	if (!(value & ACPI_EC_FLAG_SCI))
-		goto end;
 
-	if (acpi_ec_query(ec, &value))
+	while (unlikely(down_trylock(&ec->sem))) {
+
+		/* Reader or writer is waiting holding ec->sem */
+		value = acpi_ec_read_status(ec);
+		acpi_ec_check_rw_complete(ec, value);
+
+		/*
+		 * Even if we never get IBE/OBF condition client
+		 * will eventually time out allowing us proceed.
+		 */
+		msleep(1);
+	}
+
+	if (acpi_ec_read_status(ec) & ACPI_EC_FLAG_SCI)
+		result = acpi_ec_query(ec, &value);
+
+	up(&ec->sem);
+
+	if (result)
 		goto end;
-	
+
 	object_name[2] = hex[((value >> 4) & 0x0F)];
 	object_name[3] = hex[(value & 0x0F)];
 
@@ -390,6 +404,7 @@
 	void			*data)
 {
 	acpi_status		status = AE_OK;
+	u32			value;
 	struct acpi_ec		*ec = (struct acpi_ec *) data;
 
 	if (!ec)
@@ -397,13 +412,15 @@
 
 	acpi_disable_gpe(NULL, ec->gpe_bit, ACPI_ISR);
 
-	status = acpi_os_queue_for_execution(OSD_PRIORITY_GPE,
-		acpi_ec_gpe_query, ec);
+	value = acpi_ec_read_status(ec);
+	acpi_ec_check_rw_complete(ec, value);
 
-	if (status == AE_OK)
-		return ACPI_INTERRUPT_HANDLED;
-	else
-		return ACPI_INTERRUPT_NOT_HANDLED;
+	if (value & ACPI_EC_FLAG_SCI)
+		status = acpi_os_queue_for_execution(OSD_PRIORITY_GPE,
+						acpi_ec_gpe_query, ec);
+
+	return status == AE_OK ?
+		ACPI_INTERRUPT_HANDLED : ACPI_INTERRUPT_NOT_HANDLED;
 }
 
 /* --------------------------------------------------------------------------
@@ -421,10 +438,8 @@
 	 * The EC object is in the handler context and is needed
 	 * when calling the acpi_ec_space_handler.
 	 */
-	if(function == ACPI_REGION_DEACTIVATE) 
-		*return_context = NULL;
-	else 
-		*return_context = handler_context;
+	*return_context  = (function != ACPI_REGION_DEACTIVATE) ?
+						handler_context : NULL;
 
 	return AE_OK;
 }
@@ -439,9 +454,9 @@
 	void			*handler_context,
 	void			*region_context)
 {
-	int			result = 0;
-	struct acpi_ec		*ec = NULL;
-	u32			temp = 0;
+	int			result;
+	struct acpi_ec		*ec;
+	u32			temp;
 	acpi_integer		f_v = 0;
 	int 			i = 0;
 
@@ -450,7 +465,7 @@
 	if ((address > 0xFF) || !value || !handler_context)
 		return_VALUE(AE_BAD_PARAMETER);
 
-	if(bit_width != 8) {
+	if (bit_width != 8) {
 		printk(KERN_WARNING PREFIX "acpi_ec_space_handler: bit_width should be 8\n");
 		if (acpi_strict)
 			return_VALUE(AE_BAD_PARAMETER);
@@ -474,23 +489,23 @@
 	}
 
 	bit_width -= 8;
-	if(bit_width){
+	if (bit_width) {
 
-		if(function == ACPI_READ)
+		if (function == ACPI_READ)
 			f_v |= (acpi_integer) (*value) << 8*i;
-		if(function == ACPI_WRITE)
-			(*value) >>=8; 
+		if (function == ACPI_WRITE)
+			(*value) >>=8;
 		i++;
 		goto next_byte;
 	}
 
 
-	if(function == ACPI_READ){
+	if (function == ACPI_READ) {
 		f_v |= (acpi_integer) (*value) << 8*i;
 		*value = f_v;
 	}
 
-		
+
 out:
 	switch (result) {
 	case -EINVAL:
@@ -505,8 +520,6 @@
 	default:
 		return_VALUE(AE_OK);
 	}
-	
-
 }
 
 
@@ -532,7 +545,7 @@
 	seq_printf(seq, "ports:                   0x%02x, 0x%02x\n",
 		(u32) ec->status_addr.address, (u32) ec->data_addr.address);
 	seq_printf(seq, "use global lock:         %s\n",
-		ec->global_lock?"yes":"no");
+		ec->global_lock ? "yes" : "no");
 
 end:
 	return_VALUE(0);
@@ -555,7 +568,7 @@
 acpi_ec_add_fs (
 	struct acpi_device	*device)
 {
-	struct proc_dir_entry	*entry = NULL;
+	struct proc_dir_entry	*entry;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_add_fs");
 
@@ -606,9 +619,9 @@
 acpi_ec_add (
 	struct acpi_device	*device)
 {
-	int			result = 0;
-	acpi_status		status = AE_OK;
-	struct acpi_ec		*ec = NULL;
+	int			result;
+	acpi_status		status;
+	struct acpi_ec		*ec;
 	unsigned long		uid;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_add");
@@ -623,7 +636,8 @@
 
 	ec->handle = device->handle;
 	ec->uid = -1;
-	spin_lock_init(&ec->lock);
+	init_MUTEX(&ec->sem);
+	init_waitqueue_head(&ec->wait);
 	strcpy(acpi_device_name(device), ACPI_EC_DEVICE_NAME);
 	strcpy(acpi_device_class(device), ACPI_EC_CLASS);
 	acpi_driver_data(device) = ec;
@@ -637,7 +651,7 @@
 	if (ec_ecdt && ec_ecdt->uid == uid) {
 		acpi_remove_address_space_handler(ACPI_ROOT_OBJECT,
 			ACPI_ADR_SPACE_EC, &acpi_ec_space_handler);
-	
+
 		acpi_remove_gpe_handler(NULL, ec_ecdt->gpe_bit, &acpi_ec_gpe_handler);
 
 		kfree(ec_ecdt);
@@ -677,7 +691,7 @@
 	struct acpi_device	*device,
 	int			type)
 {
-	struct acpi_ec		*ec = NULL;
+	struct acpi_ec		*ec;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_remove");
 
@@ -732,8 +746,8 @@
 acpi_ec_start (
 	struct acpi_device	*device)
 {
-	acpi_status		status = AE_OK;
-	struct acpi_ec		*ec = NULL;
+	acpi_status		status;
+	struct acpi_ec		*ec;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_start");
 
@@ -789,8 +803,8 @@
 	struct acpi_device	*device,
 	int			type)
 {
-	acpi_status		status = AE_OK;
-	struct acpi_ec		*ec = NULL;
+	acpi_status		status;
+	struct acpi_ec		*ec;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_stop");
 
@@ -824,6 +838,7 @@
 		acpi_ec_io_ports, ec_ecdt);
 	if (ACPI_FAILURE(status))
 		return status;
+
 	ec_ecdt->status_addr = ec_ecdt->command_addr;
 
 	ec_ecdt->uid = -1;
@@ -832,7 +847,9 @@
 	status = acpi_evaluate_integer(handle, "_GPE", NULL, &ec_ecdt->gpe_bit);
 	if (ACPI_FAILURE(status))
 		return status;
-	spin_lock_init(&ec_ecdt->lock);
+
+	init_MUTEX(&ec_ecdt->sem);
+	init_waitqueue_head(&ec_ecdt->wait);
 	ec_ecdt->global_lock = TRUE;
 	ec_ecdt->handle = handle;
 
@@ -890,7 +907,7 @@
 	acpi_status		status;
 	struct acpi_table_ecdt 	*ecdt_ptr;
 
-	status = acpi_get_firmware_table("ECDT", 1, ACPI_LOGICAL_ADDRESSING, 
+	status = acpi_get_firmware_table("ECDT", 1, ACPI_LOGICAL_ADDRESSING,
 		(struct acpi_table_header **) &ecdt_ptr);
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
@@ -905,11 +922,12 @@
 		return -ENOMEM;
 	memset(ec_ecdt, 0, sizeof(struct acpi_ec));
 
+	init_MUTEX(&ec_ecdt->sem);
+	init_waitqueue_head(&ec_ecdt->wait);
 	ec_ecdt->command_addr = ecdt_ptr->ec_control;
 	ec_ecdt->status_addr = ecdt_ptr->ec_control;
 	ec_ecdt->data_addr = ecdt_ptr->ec_data;
 	ec_ecdt->gpe_bit = ecdt_ptr->gpe_bit;
-	spin_lock_init(&ec_ecdt->lock);
 	/* use the GL just to be safe */
 	ec_ecdt->global_lock = TRUE;
 	ec_ecdt->uid = ecdt_ptr->uid;
@@ -978,7 +996,7 @@
 
 static int __init acpi_ec_init (void)
 {
-	int			result = 0;
+	int			result;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_init");
 


-------------------------------------------------------
This SF.Net email is sponsored by: IntelliVIEW -- Interactive Reporting
Tool for open source databases. Create drag-&-drop reports. Save time
by over 75%! Publish reports on the web. Export to DOC, XLS, RTF, etc.
Download a FREE copy at http://www.intelliview.com/go/osdn_nl

Re: [PATCH] Change spinlock mutex to semaphor in ec.c

[Rich Townsend]

Dmitry Torokhov wrote:
> On Thursday 03 February 2005 19:24, Rich Townsend wrote:
> 
>>Hi all --
>>
>>Attached is a patch against the 2.6.10 kernel, to convert the mutexing 
>>in the embdedded controller driver from a spinlock to a semaphore. The 
>>rationale behind this is to permit sleeping inside the acpi_ec_wait() 
>>polling routine.
>>
> 
> 
> Hi,
> 
> Last time such patch was proposed there were some concerns regarding
> possible interrupt storm, see the follwing thread:
> 
> http://marc.theaimsgroup.com/?l=acpi4linux&m=110055992814866&w=2
> 
> I have an alternative patch that tries to address these concerns,
> unfortunately I do not have a box with EC controller so it is
> completely untested.
> 

I've just tried your patch out on 2.6.11-rc3 (with acpi-dsdt-initrd and 
swsusp2 patches), and it causes a kernel oops when the EC is accessed at 
boot time. Unfortunately, the backtrace is too long to see where the 
oops arises; I'm sure there's a way of capturing the oops output, but I 
don't know it yet (suggestions welcome).

cheers,

Rich


-------------------------------------------------------
This SF.Net email is sponsored by: IntelliVIEW -- Interactive Reporting
Tool for open source databases. Create drag-&-drop reports. Save time
by over 75%! Publish reports on the web. Export to DOC, XLS, RTF, etc.
Download a FREE copy at http://www.intelliview.com/go/osdn_nl

Re: [PATCH] Change spinlock mutex to semaphor in ec.c

[Dmitry Torokhov]

On Friday 04 February 2005 13:26, Rich Townsend wrote:
> 
> I've just tried your patch out on 2.6.11-rc3 (with acpi-dsdt-initrd and 
> swsusp2 patches), and it causes a kernel oops when the EC is accessed at 
> boot time.

Well, I said it was completely untested ;) But I see couple of problems
with it. Could you please try the patch below and see if it boots?

Thanks!

-- 
Dmitry

===== drivers/acpi/ec.c 1.46 vs edited =====
--- 1.46/drivers/acpi/ec.c	2005-02-07 01:09:52 -05:00
+++ edited/drivers/acpi/ec.c	2005-02-07 02:14:51 -05:00
@@ -31,6 +31,7 @@
 #include <linux/delay.h>
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
+#include <linux/interrupt.h>
 #include <asm/io.h>
 #include <acpi/acpi_bus.h>
 #include <acpi/acpi_drivers.h>
@@ -54,8 +55,7 @@
 #define ACPI_EC_EVENT_OBF	0x01	/* Output buffer full */
 #define ACPI_EC_EVENT_IBE	0x02	/* Input buffer empty */
 
-#define ACPI_EC_UDELAY		100	/* Poll @ 100us increments */
-#define ACPI_EC_UDELAY_COUNT	1000	/* Wait 10ms max. during EC ops */
+#define ACPI_EC_DELAY		50	/* Wait 50ms max. during EC ops */
 #define ACPI_EC_UDELAY_GLK	1000	/* Wait 1ms max. to get global lock */
 
 #define ACPI_EC_COMMAND_READ	0x80
@@ -86,8 +86,11 @@
 	struct acpi_generic_address	status_addr;
 	struct acpi_generic_address	command_addr;
 	struct acpi_generic_address	data_addr;
+	u32				data;
 	unsigned long			global_lock;
-	spinlock_t			lock;
+	unsigned int			expect_event;
+	struct semaphore		sem;
+	wait_queue_head_t		wait;
 };
 
 /* If we find an EC via the ECDT, we need to keep a ptr to its context */
@@ -100,53 +103,39 @@
                              Transaction Management
    -------------------------------------------------------------------------- */
 
-static int
-acpi_ec_wait (
-	struct acpi_ec		*ec,
-	u8			event)
+static inline u32 acpi_ec_read_status(struct acpi_ec *ec)
 {
-	u32			acpi_ec_status = 0;
-	u32			i = ACPI_EC_UDELAY_COUNT;
+	u32	status = 0;
 
-	if (!ec)
-		return -EINVAL;
+	acpi_hw_low_level_read(8, &status, &ec->status_addr);
+	return status;
+}
 
-	/* Poll the EC status register waiting for the event to occur. */
-	switch (event) {
-	case ACPI_EC_EVENT_OBF:
-		do {
-			acpi_hw_low_level_read(8, &acpi_ec_status, &ec->status_addr);
-			if (acpi_ec_status & ACPI_EC_FLAG_OBF)
-				return 0;
-			udelay(ACPI_EC_UDELAY);
-		} while (--i>0);
-		break;
-	case ACPI_EC_EVENT_IBE:
-		do {
-			acpi_hw_low_level_read(8, &acpi_ec_status, &ec->status_addr);
-			if (!(acpi_ec_status & ACPI_EC_FLAG_IBF))
-				return 0;
-			udelay(ACPI_EC_UDELAY);
-		} while (--i>0);
-		break;
-	default:
-		return -EINVAL;
-	}
+static inline void acpi_ec_prepare_for(struct acpi_ec *ec, unsigned int event)
+{
+	ec->expect_event = event;
+	mb();
+}
 
+static int acpi_ec_wait(struct acpi_ec *ec)
+{
+	if (wait_event_timeout(ec->wait, !ec->expect_event,
+				msecs_to_jiffies(ACPI_EC_DELAY)))
+		return 0;
+
+	ec->expect_event = 0;
 	return -ETIME;
 }
 
-
 static int
 acpi_ec_read (
 	struct acpi_ec		*ec,
 	u8			address,
 	u32			*data)
 {
-	acpi_status		status = AE_OK;
-	int			result = 0;
-	unsigned long		flags = 0;
-	u32			glk = 0;
+	acpi_status		status;
+	int			result;
+	u32			glk;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_read");
 
@@ -160,27 +149,29 @@
 		if (ACPI_FAILURE(status))
 			return_VALUE(-ENODEV);
 	}
-	
-	spin_lock_irqsave(&ec->lock, flags);
 
+	WARN_ON(in_interrupt());
+	down(&ec->sem);
+
+	acpi_ec_prepare_for(ec, ACPI_EC_EVENT_IBE);
 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_READ, &ec->command_addr);
-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
+	result = acpi_ec_wait(ec);
 	if (result)
 		goto end;
 
+	acpi_ec_prepare_for(ec, ACPI_EC_EVENT_OBF);
 	acpi_hw_low_level_write(8, address, &ec->data_addr);
-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF);
+	result = acpi_ec_wait(ec);
 	if (result)
 		goto end;
 
-
-	acpi_hw_low_level_read(8, data, &ec->data_addr);
+	*data = ec->data;
 
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Read [%02x] from address [%02x]\n",
 		*data, address));
 
 end:
-	spin_unlock_irqrestore(&ec->lock, flags);
+	up(&ec->sem);
 
 	if (ec->global_lock)
 		acpi_release_global_lock(glk);
@@ -195,10 +186,9 @@
 	u8			address,
 	u8			data)
 {
-	int			result = 0;
-	acpi_status		status = AE_OK;
-	unsigned long		flags = 0;
-	u32			glk = 0;
+	int			result;
+	acpi_status		status;
+	u32			glk;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_write");
 
@@ -211,20 +201,24 @@
 			return_VALUE(-ENODEV);
 	}
 
-	spin_lock_irqsave(&ec->lock, flags);
+	WARN_ON(in_interrupt());
+	down(&ec->sem);
 
+	acpi_ec_prepare_for(ec, ACPI_EC_EVENT_IBE);
 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_WRITE, &ec->command_addr);
-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
+	result = acpi_ec_wait(ec);
 	if (result)
 		goto end;
 
+	acpi_ec_prepare_for(ec, ACPI_EC_EVENT_IBE);
 	acpi_hw_low_level_write(8, address, &ec->data_addr);
-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
+	result = acpi_ec_wait(ec);
 	if (result)
 		goto end;
 
+	acpi_ec_prepare_for(ec, ACPI_EC_EVENT_IBE);
 	acpi_hw_low_level_write(8, data, &ec->data_addr);
-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
+	result = acpi_ec_wait(ec);
 	if (result)
 		goto end;
 
@@ -232,7 +226,7 @@
 		data, address));
 
 end:
-	spin_unlock_irqrestore(&ec->lock, flags);
+	up(&ec->sem);
 
 	if (ec->global_lock)
 		acpi_release_global_lock(glk);
@@ -289,16 +283,13 @@
 	struct acpi_ec		*ec,
 	u32			*data)
 {
-	int			result = 0;
-	acpi_status		status = AE_OK;
-	unsigned long		flags = 0;
-	u32			glk = 0;
+	acpi_status		status;
+	u32			glk;
+	int			result = -ETIME;
+	int			i = ACPI_EC_DELAY;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_query");
 
-	if (!ec || !data)
-		return_VALUE(-EINVAL);
-
 	*data = 0;
 
 	if (ec->global_lock) {
@@ -311,20 +302,18 @@
 	 * Query the EC to find out which _Qxx method we need to evaluate.
 	 * Note that successful completion of the query causes the ACPI_EC_SCI
 	 * bit to be cleared (and thus clearing the interrupt source).
+	 * Here we do polling for command completion because GPE is disabled.
 	 */
-	spin_lock_irqsave(&ec->lock, flags);
-
 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_QUERY, &ec->command_addr);
-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF);
-	if (result)
-		goto end;
-	
-	acpi_hw_low_level_read(8, data, &ec->data_addr);
-	if (!*data)
-		result = -ENODATA;
 
-end:
-	spin_unlock_irqrestore(&ec->lock, flags);
+	do {
+		if (acpi_ec_read_status(ec) & ACPI_EC_FLAG_OBF) {
+			acpi_hw_low_level_read(8, data, &ec->data_addr);
+			result = *data ? 0 : -ENODATA;
+			break;
+		}
+		msleep(1);
+	} while (--i > 0);
 
 	if (ec->global_lock)
 		acpi_release_global_lock(glk);
@@ -342,38 +331,66 @@
 	u8			data;
 };
 
+static void acpi_ec_check_rw_complete(struct acpi_ec *ec, u32 status)
+{
+	if (ec->expect_event == ACPI_EC_EVENT_OBF) {
+
+		if (status & ACPI_EC_FLAG_OBF) {
+			acpi_hw_low_level_read(8, &ec->data, &ec->data_addr);
+			ec->expect_event = 0;
+			wake_up(&ec->wait);
+		}
+	} else if (ec->expect_event == ACPI_EC_EVENT_IBE) {
+
+	 	if (~status & ACPI_EC_FLAG_IBF) {
+			ec->expect_event = 0;
+			wake_up(&ec->wait);
+		}
+	}
+}
+
 static void
 acpi_ec_gpe_query (
 	void			*ec_cxt)
 {
 	struct acpi_ec		*ec = (struct acpi_ec *) ec_cxt;
-	u32			value = 0;
-	unsigned long		flags = 0;
+	u32			value;
+	int			result = -ENODATA;
 	static char		object_name[5] = {'_','Q','0','0','\0'};
 	const char		hex[] = {'0','1','2','3','4','5','6','7',
 				         '8','9','A','B','C','D','E','F'};
 
 	ACPI_FUNCTION_TRACE("acpi_ec_gpe_query");
 
-	if (!ec_cxt)
-		goto end;	
-
-	spin_lock_irqsave(&ec->lock, flags);
-	acpi_hw_low_level_read(8, &value, &ec->command_addr);
-	spin_unlock_irqrestore(&ec->lock, flags);
-
-	/* TBD: Implement asynch events!
-	 * NOTE: All we care about are EC-SCI's.  Other EC events are
-	 * handled via polling (yuck!).  This is because some systems
-	 * treat EC-SCIs as level (versus EDGE!) triggered, preventing
-	 *  a purely interrupt-driven approach (grumble, grumble).
+	/*
+	 * Because some systems treat EC-SCIs as level (versus EDGE!)
+	 * triggered we can not just check SCI flag here. If there is
+	 * a reader/writer waiting on response we need to wait till
+	 * data is available and wake it up. Only when there is no
+	 * outstanding requests we can finally service SCI.
 	 */
-	if (!(value & ACPI_EC_FLAG_SCI))
-		goto end;
 
-	if (acpi_ec_query(ec, &value))
+	while (unlikely(down_trylock(&ec->sem))) {
+
+		/* Reader or writer is waiting holding ec->sem */
+		value = acpi_ec_read_status(ec);
+		acpi_ec_check_rw_complete(ec, value);
+
+		/*
+		 * Even if we never get IBE/OBF condition client
+		 * will eventually time out allowing us to proceed.
+		 */
+		msleep(1);
+	}
+
+	if (acpi_ec_read_status(ec) & ACPI_EC_FLAG_SCI)
+		result = acpi_ec_query(ec, &value);
+
+	up(&ec->sem);
+
+	if (result)
 		goto end;
-	
+
 	object_name[2] = hex[((value >> 4) & 0x0F)];
 	object_name[3] = hex[(value & 0x0F)];
 
@@ -390,6 +407,7 @@
 	void			*data)
 {
 	acpi_status		status = AE_OK;
+	u32			value;
 	struct acpi_ec		*ec = (struct acpi_ec *) data;
 
 	if (!ec)
@@ -397,13 +415,17 @@
 
 	acpi_disable_gpe(NULL, ec->gpe_bit, ACPI_ISR);
 
-	status = acpi_os_queue_for_execution(OSD_PRIORITY_GPE,
-		acpi_ec_gpe_query, ec);
+	value = acpi_ec_read_status(ec);
+	acpi_ec_check_rw_complete(ec, value);
 
-	if (status == AE_OK)
-		return ACPI_INTERRUPT_HANDLED;
+	if (value & ACPI_EC_FLAG_SCI)
+		status = acpi_os_queue_for_execution(OSD_PRIORITY_GPE,
+						acpi_ec_gpe_query, ec);
 	else
-		return ACPI_INTERRUPT_NOT_HANDLED;
+		acpi_enable_gpe(NULL, ec->gpe_bit, ACPI_ISR);
+
+	return status == AE_OK ?
+		ACPI_INTERRUPT_HANDLED : ACPI_INTERRUPT_NOT_HANDLED;
 }
 
 /* --------------------------------------------------------------------------
@@ -421,10 +443,8 @@
 	 * The EC object is in the handler context and is needed
 	 * when calling the acpi_ec_space_handler.
 	 */
-	if(function == ACPI_REGION_DEACTIVATE) 
-		*return_context = NULL;
-	else 
-		*return_context = handler_context;
+	*return_context  = (function != ACPI_REGION_DEACTIVATE) ?
+						handler_context : NULL;
 
 	return AE_OK;
 }
@@ -439,9 +459,9 @@
 	void			*handler_context,
 	void			*region_context)
 {
-	int			result = 0;
-	struct acpi_ec		*ec = NULL;
-	u32			temp = 0;
+	int			result;
+	struct acpi_ec		*ec;
+	u32			temp;
 	acpi_integer		f_v = 0;
 	int 			i = 0;
 
@@ -450,7 +470,7 @@
 	if ((address > 0xFF) || !value || !handler_context)
 		return_VALUE(AE_BAD_PARAMETER);
 
-	if(bit_width != 8) {
+	if (bit_width != 8) {
 		printk(KERN_WARNING PREFIX "acpi_ec_space_handler: bit_width should be 8\n");
 		if (acpi_strict)
 			return_VALUE(AE_BAD_PARAMETER);
@@ -474,23 +494,23 @@
 	}
 
 	bit_width -= 8;
-	if(bit_width){
+	if (bit_width) {
 
-		if(function == ACPI_READ)
+		if (function == ACPI_READ)
 			f_v |= (acpi_integer) (*value) << 8*i;
-		if(function == ACPI_WRITE)
-			(*value) >>=8; 
+		if (function == ACPI_WRITE)
+			(*value) >>=8;
 		i++;
 		goto next_byte;
 	}
 
 
-	if(function == ACPI_READ){
+	if (function == ACPI_READ) {
 		f_v |= (acpi_integer) (*value) << 8*i;
 		*value = f_v;
 	}
 
-		
+
 out:
 	switch (result) {
 	case -EINVAL:
@@ -505,8 +525,6 @@
 	default:
 		return_VALUE(AE_OK);
 	}
-	
-
 }
 
 
@@ -532,7 +550,7 @@
 	seq_printf(seq, "ports:                   0x%02x, 0x%02x\n",
 		(u32) ec->status_addr.address, (u32) ec->data_addr.address);
 	seq_printf(seq, "use global lock:         %s\n",
-		ec->global_lock?"yes":"no");
+		ec->global_lock ? "yes" : "no");
 
 end:
 	return_VALUE(0);
@@ -555,7 +573,7 @@
 acpi_ec_add_fs (
 	struct acpi_device	*device)
 {
-	struct proc_dir_entry	*entry = NULL;
+	struct proc_dir_entry	*entry;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_add_fs");
 
@@ -606,9 +624,9 @@
 acpi_ec_add (
 	struct acpi_device	*device)
 {
-	int			result = 0;
-	acpi_status		status = AE_OK;
-	struct acpi_ec		*ec = NULL;
+	int			result;
+	acpi_status		status;
+	struct acpi_ec		*ec;
 	unsigned long		uid;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_add");
@@ -623,7 +641,8 @@
 
 	ec->handle = device->handle;
 	ec->uid = -1;
-	spin_lock_init(&ec->lock);
+	init_MUTEX(&ec->sem);
+	init_waitqueue_head(&ec->wait);
 	strcpy(acpi_device_name(device), ACPI_EC_DEVICE_NAME);
 	strcpy(acpi_device_class(device), ACPI_EC_CLASS);
 	acpi_driver_data(device) = ec;
@@ -637,7 +656,7 @@
 	if (ec_ecdt && ec_ecdt->uid == uid) {
 		acpi_remove_address_space_handler(ACPI_ROOT_OBJECT,
 			ACPI_ADR_SPACE_EC, &acpi_ec_space_handler);
-	
+
 		acpi_remove_gpe_handler(NULL, ec_ecdt->gpe_bit, &acpi_ec_gpe_handler);
 
 		kfree(ec_ecdt);
@@ -677,7 +696,7 @@
 	struct acpi_device	*device,
 	int			type)
 {
-	struct acpi_ec		*ec = NULL;
+	struct acpi_ec		*ec;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_remove");
 
@@ -732,8 +751,8 @@
 acpi_ec_start (
 	struct acpi_device	*device)
 {
-	acpi_status		status = AE_OK;
-	struct acpi_ec		*ec = NULL;
+	acpi_status		status;
+	struct acpi_ec		*ec;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_start");
 
@@ -789,8 +808,8 @@
 	struct acpi_device	*device,
 	int			type)
 {
-	acpi_status		status = AE_OK;
-	struct acpi_ec		*ec = NULL;
+	acpi_status		status;
+	struct acpi_ec		*ec;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_stop");
 
@@ -824,6 +843,7 @@
 		acpi_ec_io_ports, ec_ecdt);
 	if (ACPI_FAILURE(status))
 		return status;
+
 	ec_ecdt->status_addr = ec_ecdt->command_addr;
 
 	ec_ecdt->uid = -1;
@@ -832,7 +852,9 @@
 	status = acpi_evaluate_integer(handle, "_GPE", NULL, &ec_ecdt->gpe_bit);
 	if (ACPI_FAILURE(status))
 		return status;
-	spin_lock_init(&ec_ecdt->lock);
+
+	init_MUTEX(&ec_ecdt->sem);
+	init_waitqueue_head(&ec_ecdt->wait);
 	ec_ecdt->global_lock = TRUE;
 	ec_ecdt->handle = handle;
 
@@ -890,7 +912,7 @@
 	acpi_status		status;
 	struct acpi_table_ecdt 	*ecdt_ptr;
 
-	status = acpi_get_firmware_table("ECDT", 1, ACPI_LOGICAL_ADDRESSING, 
+	status = acpi_get_firmware_table("ECDT", 1, ACPI_LOGICAL_ADDRESSING,
 		(struct acpi_table_header **) &ecdt_ptr);
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
@@ -905,11 +927,12 @@
 		return -ENOMEM;
 	memset(ec_ecdt, 0, sizeof(struct acpi_ec));
 
+	init_MUTEX(&ec_ecdt->sem);
+	init_waitqueue_head(&ec_ecdt->wait);
 	ec_ecdt->command_addr = ecdt_ptr->ec_control;
 	ec_ecdt->status_addr = ecdt_ptr->ec_control;
 	ec_ecdt->data_addr = ecdt_ptr->ec_data;
 	ec_ecdt->gpe_bit = ecdt_ptr->gpe_bit;
-	spin_lock_init(&ec_ecdt->lock);
 	/* use the GL just to be safe */
 	ec_ecdt->global_lock = TRUE;
 	ec_ecdt->uid = ecdt_ptr->uid;
@@ -978,7 +1001,7 @@
 
 static int __init acpi_ec_init (void)
 {
-	int			result = 0;
+	int			result;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_init");
 



-------------------------------------------------------
This SF.Net email is sponsored by: IntelliVIEW -- Interactive Reporting
Tool for open source databases. Create drag-&-drop reports. Save time
by over 75%! Publish reports on the web. Export to DOC, XLS, RTF, etc.
Download a FREE copy at http://www.intelliview.com/go/osdn_nl