[PATCH] Don't disable interrupts during EC access

[PATCH] Don't disable interrupts during EC access

[Andi Kleen]

On VIA chipset + A64 laptops the EC access can take a long time. This causes
the system to lose timer interrupts while ACPI waits for the event, 
and causes it to always fallback to slower timer sources.

This patch replaces the spinlock with a semaphore and sleeps instead
of busy polling. This should also save power. 

Signed-off-by: Andi Kleen <ak-l3A5Bk7waGM@public.gmane.org>

-Andi

diff -u linux/drivers/acpi/ec.c-o linux/drivers/acpi/ec.c
--- linux/drivers/acpi/ec.c-o	2004-10-26 09:57:32.000000000 -0700
+++ linux/drivers/acpi/ec.c	2004-11-15 12:55:58.000000000 -0800
@@ -29,10 +29,12 @@
 #include <linux/types.h>
 #include <linux/delay.h>
 #include <linux/proc_fs.h>
+#include <linux/interrupt.h>
 #include <asm/io.h>
 #include <acpi/acpi_bus.h>
 #include <acpi/acpi_drivers.h>
 #include <acpi/actypes.h>
+#include <asm/msr.h>
 
 #define _COMPONENT		ACPI_EC_COMPONENT
 ACPI_MODULE_NAME		("acpi_ec")
@@ -53,7 +55,7 @@
 #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_UDELAY_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
@@ -85,7 +87,6 @@
 	struct acpi_generic_address	command_addr;
 	struct acpi_generic_address	data_addr;
 	unsigned long			global_lock;
-	spinlock_t			lock;
 };
 
 /* If we find an EC via the ECDT, we need to keep a ptr to its context */
@@ -116,7 +117,7 @@
 			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(1);
 		} while (--i>0);
 		break;
 	case ACPI_EC_EVENT_IBE:
@@ -124,16 +125,16 @@
 			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(1);
 		} while (--i>0);
 		break;
 	default:
 		return -EINVAL;
 	}
-
 	return -ETIME;
 }
 
+static DECLARE_MUTEX(ec_mutex);
 
 static int
 acpi_ec_read (
@@ -143,7 +144,6 @@
 {
 	acpi_status		status = AE_OK;
 	int			result = 0;
-	unsigned long		flags = 0;
 	u32			glk = 0;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_read");
@@ -158,8 +158,9 @@
 		if (ACPI_FAILURE(status))
 			return_VALUE(-ENODEV);
 	}
-	
-	spin_lock_irqsave(&ec->lock, flags);
+
+	WARN_ON(in_interrupt());
+	down(&ec_mutex);
 
 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_READ, &ec->command_addr);
 	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
@@ -173,16 +174,15 @@
 
 
 	acpi_hw_low_level_read(8, data, &ec->data_addr);
-
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Read [%02x] from address [%02x]\n",
 		*data, address));
 
+		
 end:
-	spin_unlock_irqrestore(&ec->lock, flags);
+	up(&ec_mutex);
 
 	if (ec->global_lock)
 		acpi_release_global_lock(glk);
-
 	return_VALUE(result);
 }
 
@@ -195,7 +195,6 @@
 {
 	int			result = 0;
 	acpi_status		status = AE_OK;
-	unsigned long		flags = 0;
 	u32			glk = 0;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_write");
@@ -209,7 +208,8 @@
 			return_VALUE(-ENODEV);
 	}
 
-	spin_lock_irqsave(&ec->lock, flags);
+	WARN_ON(in_interrupt());
+	down(&ec_mutex);
 
 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_WRITE, &ec->command_addr);
 	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
@@ -230,7 +230,7 @@
 		data, address));
 
 end:
-	spin_unlock_irqrestore(&ec->lock, flags);
+	up(&ec_mutex);
 
 	if (ec->global_lock)
 		acpi_release_global_lock(glk);
@@ -287,7 +287,6 @@
 {
 	int			result = 0;
 	acpi_status		status = AE_OK;
-	unsigned long		flags = 0;
 	u32			glk = 0;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_query");
@@ -308,7 +307,7 @@
 	 * 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);
+	down(&ec_mutex);
 
 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_QUERY, &ec->command_addr);
 	result = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF);
@@ -320,7 +319,7 @@
 		result = -ENODATA;
 
 end:
-	spin_unlock_irqrestore(&ec->lock, flags);
+	up(&ec_mutex);
 
 	if (ec->global_lock)
 		acpi_release_global_lock(glk);
@@ -344,7 +343,6 @@
 {
 	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'};
@@ -354,9 +352,9 @@
 	if (!ec_cxt)
 		goto end;	
 
-	spin_lock_irqsave(&ec->lock, flags);
+	down(&ec_mutex);
 	acpi_hw_low_level_read(8, &value, &ec->command_addr);
-	spin_unlock_irqrestore(&ec->lock, flags);
+	up(&ec_mutex);
 
 	/* TBD: Implement asynch events!
 	 * NOTE: All we care about are EC-SCI's.  Other EC events are
@@ -588,7 +586,6 @@
 
 	ec->handle = device->handle;
 	ec->uid = -1;
-	ec->lock = SPIN_LOCK_UNLOCKED;
 	strcpy(acpi_device_name(device), ACPI_EC_DEVICE_NAME);
 	strcpy(acpi_device_class(device), ACPI_EC_CLASS);
 	acpi_driver_data(device) = ec;
@@ -802,7 +799,6 @@
 	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;
 	/* use the GL just to be safe */
 	ec_ecdt->global_lock = TRUE;
 	ec_ecdt->uid = ecdt_ptr->uid;


-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

RE: [PATCH] Don't disable interrupts during EC access

[Yu, Luming]

Andi,

This patch seems to break the atomic operation.
The original code enclosed by spin_lock_irqsave/spin_unlock_irqresotre is trying to make sure the first operation to EC_COMMAND
register , and the second operation to EC_DATA register belong to one atomic transaction.

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 Andi Kleen
>Sent: 2004年11月16日 6:56
>To: acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>Subject: [ACPI] [PATCH] Don't disable interrupts during EC access
>
>
>On VIA chipset + A64 laptops the EC access can take a long 
>time. This causes
>the system to lose timer interrupts while ACPI waits for the event, 
>and causes it to always fallback to slower timer sources.
>
>This patch replaces the spinlock with a semaphore and sleeps instead
>of busy polling. This should also save power. 
>
>Signed-off-by: Andi Kleen <ak-l3A5Bk7waGM@public.gmane.org>
>
>-Andi
>
>diff -u linux/drivers/acpi/ec.c-o linux/drivers/acpi/ec.c
>--- linux/drivers/acpi/ec.c-o	2004-10-26 09:57:32.000000000 -0700
>+++ linux/drivers/acpi/ec.c	2004-11-15 12:55:58.000000000 -0800
>@@ -29,10 +29,12 @@
> #include <linux/types.h>
> #include <linux/delay.h>
> #include <linux/proc_fs.h>
>+#include <linux/interrupt.h>
> #include <asm/io.h>
> #include <acpi/acpi_bus.h>
> #include <acpi/acpi_drivers.h>
> #include <acpi/actypes.h>
>+#include <asm/msr.h>
> 
> #define _COMPONENT		ACPI_EC_COMPONENT
> ACPI_MODULE_NAME		("acpi_ec")
>@@ -53,7 +55,7 @@
> #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_UDELAY_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
>@@ -85,7 +87,6 @@
> 	struct acpi_generic_address	command_addr;
> 	struct acpi_generic_address	data_addr;
> 	unsigned long			global_lock;
>-	spinlock_t			lock;
> };
> 
> /* If we find an EC via the ECDT, we need to keep a ptr to 
>its context */
>@@ -116,7 +117,7 @@
> 			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(1);
> 		} while (--i>0);
> 		break;
> 	case ACPI_EC_EVENT_IBE:
>@@ -124,16 +125,16 @@
> 			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(1);
> 		} while (--i>0);
> 		break;
> 	default:
> 		return -EINVAL;
> 	}
>-
> 	return -ETIME;
> }
> 
>+static DECLARE_MUTEX(ec_mutex);
> 
> static int
> acpi_ec_read (
>@@ -143,7 +144,6 @@
> {
> 	acpi_status		status = AE_OK;
> 	int			result = 0;
>-	unsigned long		flags = 0;
> 	u32			glk = 0;
> 
> 	ACPI_FUNCTION_TRACE("acpi_ec_read");
>@@ -158,8 +158,9 @@
> 		if (ACPI_FAILURE(status))
> 			return_VALUE(-ENODEV);
> 	}
>-	
>-	spin_lock_irqsave(&ec->lock, flags);
>+
>+	WARN_ON(in_interrupt());
>+	down(&ec_mutex);
> 
> 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_READ, 
>&ec->command_addr);
> 	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
>@@ -173,16 +174,15 @@
> 
> 
> 	acpi_hw_low_level_read(8, data, &ec->data_addr);
>-
> 	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Read [%02x] from 
>address [%02x]\n",
> 		*data, address));
> 
>+		
> end:
>-	spin_unlock_irqrestore(&ec->lock, flags);
>+	up(&ec_mutex);
> 
> 	if (ec->global_lock)
> 		acpi_release_global_lock(glk);
>-
> 	return_VALUE(result);
> }
> 
>@@ -195,7 +195,6 @@
> {
> 	int			result = 0;
> 	acpi_status		status = AE_OK;
>-	unsigned long		flags = 0;
> 	u32			glk = 0;
> 
> 	ACPI_FUNCTION_TRACE("acpi_ec_write");
>@@ -209,7 +208,8 @@
> 			return_VALUE(-ENODEV);
> 	}
> 
>-	spin_lock_irqsave(&ec->lock, flags);
>+	WARN_ON(in_interrupt());
>+	down(&ec_mutex);
> 
> 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_WRITE, 
>&ec->command_addr);
> 	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
>@@ -230,7 +230,7 @@
> 		data, address));
> 
> end:
>-	spin_unlock_irqrestore(&ec->lock, flags);
>+	up(&ec_mutex);
> 
> 	if (ec->global_lock)
> 		acpi_release_global_lock(glk);
>@@ -287,7 +287,6 @@
> {
> 	int			result = 0;
> 	acpi_status		status = AE_OK;
>-	unsigned long		flags = 0;
> 	u32			glk = 0;
> 
> 	ACPI_FUNCTION_TRACE("acpi_ec_query");
>@@ -308,7 +307,7 @@
> 	 * 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);
>+	down(&ec_mutex);
> 
> 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_QUERY, 
>&ec->command_addr);
> 	result = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF);
>@@ -320,7 +319,7 @@
> 		result = -ENODATA;
> 
> end:
>-	spin_unlock_irqrestore(&ec->lock, flags);
>+	up(&ec_mutex);
> 
> 	if (ec->global_lock)
> 		acpi_release_global_lock(glk);
>@@ -344,7 +343,6 @@
> {
> 	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'};
>@@ -354,9 +352,9 @@
> 	if (!ec_cxt)
> 		goto end;	
> 
>-	spin_lock_irqsave(&ec->lock, flags);
>+	down(&ec_mutex);
> 	acpi_hw_low_level_read(8, &value, &ec->command_addr);
>-	spin_unlock_irqrestore(&ec->lock, flags);
>+	up(&ec_mutex);
> 
> 	/* TBD: Implement asynch events!
> 	 * NOTE: All we care about are EC-SCI's.  Other EC events are
>@@ -588,7 +586,6 @@
> 
> 	ec->handle = device->handle;
> 	ec->uid = -1;
>-	ec->lock = SPIN_LOCK_UNLOCKED;
> 	strcpy(acpi_device_name(device), ACPI_EC_DEVICE_NAME);
> 	strcpy(acpi_device_class(device), ACPI_EC_CLASS);
> 	acpi_driver_data(device) = ec;
>@@ -802,7 +799,6 @@
> 	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;
> 	/* use the GL just to be safe */
> 	ec_ecdt->global_lock = TRUE;
> 	ec_ecdt->uid = ecdt_ptr->uid;
>
>
>-------------------------------------------------------
>This SF.Net email is sponsored by: InterSystems CACHE
>FREE OODBMS DOWNLOAD - A multidimensional database that combines
>robust object and relational technologies, making it a perfect match
>for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8
>_______________________________________________
>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: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

Re: [PATCH] Don't disable interrupts during EC access

[Matthew Wilcox]

On Tue, Nov 16, 2004 at 02:15:47PM +0800, Yu, Luming wrote:
> Andi,
> 
> This patch seems to break the atomic operation.
> The original code enclosed by spin_lock_irqsave/spin_unlock_irqresotre is trying to make sure the first operation to EC_COMMAND
> register , and the second operation to EC_DATA register belong to one atomic transaction.

Why would converting from a spinlock to a semaphore break this?

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain


-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

Re: [PATCH] Don't disable interrupts during EC access

[Andi Kleen]

[I didn't see Luming's original mail, so answering to this one]

On Tue, Nov 16, 2004 at 12:16:16PM +0000, Matthew Wilcox wrote:
> On Tue, Nov 16, 2004 at 02:15:47PM +0800, Yu, Luming wrote:
> > Andi,
> > 
> > This patch seems to break the atomic operation.
> > The original code enclosed by spin_lock_irqsave/spin_unlock_irqresotre is trying to make sure the first operation to EC_COMMAND
> > register , and the second operation to EC_DATA register belong to one atomic transaction.
> 
> Why would converting from a spinlock to a semaphore break this?

As Willy said the operation should be still atomic in respect to
the register.

And the event polling can take several ms, there is just no way 
to do this during runtime with interrupts off. Especially since
thermal will do this every few seconds. With my patch at least 
only kacpid is blocked for that long, not the whole system.

-Andi


-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

RE: [PATCH] Don't disable interrupts during EC access

[Yu, Luming]

>On Tue, Nov 16, 2004 at 12:16:16PM +0000, Matthew Wilcox wrote:
>> On Tue, Nov 16, 2004 at 02:15:47PM +0800, Yu, Luming wrote:
>> > Andi,
>> > 
>> > This patch seems to break the atomic operation.
>> > The original code enclosed by 
>spin_lock_irqsave/spin_unlock_irqresotre is trying to make 
>sure the first operation to EC_COMMAND
>> > register , and the second operation to EC_DATA register 
>belong to one atomic transaction.
>> 
>> Why would converting from a spinlock to a semaphore break this?
>
>As Willy said the operation should be still atomic in respect to
>the register.

It is just due to odd hardware behavior.  :-)
For example, if you want to read data from EC.  The first step
is writing 0x80 (read command) to ec command register.
Then, if EC (hardware) write data into the output buffer, the
output buffer flag (OBF) in the status register is set.
(I assume it is within 1ms). According to ACPI spec,
now, EC interrupt will get triggered. And this is
firmware generated edge interrupt. It will hold until OBF get reset.
When the host processor reads this data from the output buffer, 
the OBF can be reset.

So, if you remove the spin_lock_irqsave, when OBF is set
you could get interrupt storm.  

For detail, please refer to ACPI spec :
12   ACPI Embedded Controller Interface Specification.

>
>And the event polling can take several ms, there is just no way 
>to do this during runtime with interrupts off. Especially since
>thermal will do this every few seconds. With my patch at least 
>only kacpid is blocked for that long, not the whole system.
>
>-Andi
>

As for thermal event, AFAIK,  not all laptops use EC to generate thermal
event. And I'm thinking of a algorithm.

  There is a issue of kacpid consuming a lot of CPU time, which should 
be due to GPEs keep firing without any control. Current GPE handling
just
re-enable GPEs after handling GPEs without considering any semantics 
conveyed by that GPE.  For example,  some laptop could keep generating 
thermal GPE interrupt, if temperature is above a threshold. All these
thermal events
is just telling OS one thing, that is "temperature is above a specific
threshold,
 please turn on cooling methods ...." .  Maybe this is platform issue,
 but driver  should handle this case smoothly.
 
http://bugme.osdl.org/show_bug.cgi?id=3686
 

Thanks
Luming



-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

Re: [PATCH] Don't disable interrupts during EC access

[Andi Kleen]

On Tue, Nov 16, 2004 at 11:21:03PM +0800, Yu, Luming wrote:
> It is just due to odd hardware behavior.  :-)
> For example, if you want to read data from EC.  The first step
> is writing 0x80 (read command) to ec command register.
> Then, if EC (hardware) write data into the output buffer, the
> output buffer flag (OBF) in the status register is set.
> (I assume it is within 1ms). According to ACPI spec,
> now, EC interrupt will get triggered. And this is
> firmware generated edge interrupt. It will hold until OBF get reset.
> When the host processor reads this data from the output buffer, 
> the OBF can be reset.
> 
> So, if you remove the spin_lock_irqsave, when OBF is set
> you could get interrupt storm.  


> 
> For detail, please refer to ACPI spec :
> 12   ACPI Embedded Controller Interface Specification.

Ok. But the problem is not the actual write, but the waiting 
for the event which takes extremly long on these machines
(it's not a single broken machine, but has been observed
on several VIA based laptops). Would it work to only disable
interrupts during the register write/read, but not during the
polling delay? 

Another possible way would be to only disable the ACPI interrupt
in the local APIC and keep the others alive  (but what to do in PIC mode?) 
> 
> >
> >And the event polling can take several ms, there is just no way 
> >to do this during runtime with interrupts off. Especially since
> >thermal will do this every few seconds. With my patch at least 
> >only kacpid is blocked for that long, not the whole system.
> >
> >-Andi
> >
> 
> As for thermal event, AFAIK,  not all laptops use EC to generate thermal
> event. And I'm thinking of a algorithm.

A better algorithm to do this would be fine, but even if you do
the EC access only very infrequent it's imho still not really 
appropiate to lose timer interrupts and create long scheduling bubbles
durign this. So some solution for the broken EC reads needs to be found even
independent from the thermal algorithm. 


-Andi


-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

RE: [PATCH] Don't disable interrupts during EC access

[Yu, Luming]

  I forget one thing that could make me feel frustrated.
  There is a user outside of ACPI driver, that is sonypi_ec_read,
which can be invoked by sonypi_misc_ioctl, which haven't
disable EC-GPE. :-)
  If everything is handled by kacpid. I even think semaphore
is also unnecessary. Because every thing is run in the order
of a queue. :-)

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 Yu, Luming
>Sent: 2004年11月16日 23:21
>To: Andi Kleen; Matthew Wilcox
>Cc: acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; Fu, Michael; Brown, Len; 
>Moore, Robert
>Subject: RE: [ACPI] [PATCH] Don't disable interrupts during EC access
>
>>On Tue, Nov 16, 2004 at 12:16:16PM +0000, Matthew Wilcox wrote:
>>> On Tue, Nov 16, 2004 at 02:15:47PM +0800, Yu, Luming wrote:
>>> > Andi,
>>> > 
>>> > This patch seems to break the atomic operation.
>>> > The original code enclosed by 
>>spin_lock_irqsave/spin_unlock_irqresotre is trying to make 
>>sure the first operation to EC_COMMAND
>>> > register , and the second operation to EC_DATA register 
>>belong to one atomic transaction.
>>> 
>>> Why would converting from a spinlock to a semaphore break this?
>>
>>As Willy said the operation should be still atomic in respect to
>>the register.
>
>It is just due to odd hardware behavior.  :-)
>For example, if you want to read data from EC.  The first step
>is writing 0x80 (read command) to ec command register.
>Then, if EC (hardware) write data into the output buffer, the
>output buffer flag (OBF) in the status register is set.
>(I assume it is within 1ms). According to ACPI spec,
>now, EC interrupt will get triggered. And this is
>firmware generated edge interrupt. It will hold until OBF get reset.
>When the host processor reads this data from the output buffer, 
>the OBF can be reset.
>
>So, if you remove the spin_lock_irqsave, when OBF is set
>you could get interrupt storm.  
>
>For detail, please refer to ACPI spec :
>12   ACPI Embedded Controller Interface Specification.
>
>>
>>And the event polling can take several ms, there is just no way 
>>to do this during runtime with interrupts off. Especially since
>>thermal will do this every few seconds. With my patch at least 
>>only kacpid is blocked for that long, not the whole system.
>>
>>-Andi
>>
>
>As for thermal event, AFAIK,  not all laptops use EC to 
>generate thermal
>event. And I'm thinking of a algorithm.
>
>  There is a issue of kacpid consuming a lot of CPU time, which should 
>be due to GPEs keep firing without any control. Current GPE handling
>just
>re-enable GPEs after handling GPEs without considering any semantics 
>conveyed by that GPE.  For example,  some laptop could keep generating 
>thermal GPE interrupt, if temperature is above a threshold. All these
>thermal events
>is just telling OS one thing, that is "temperature is above a specific
>threshold,
> please turn on cooling methods ...." .  Maybe this is platform issue,
> but driver  should handle this case smoothly.
> 
>http://bugme.osdl.org/show_bug.cgi?id=3686
> 
>
>Thanks
>Luming
>
>
>
>-------------------------------------------------------
>This SF.Net email is sponsored by: InterSystems CACHE
>FREE OODBMS DOWNLOAD - A multidimensional database that combines
>robust object and relational technologies, making it a perfect match
>for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8
>_______________________________________________
>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: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

Re: [PATCH] Don't disable interrupts during EC access

[Stefan Seyfried]

On Tue, Nov 16, 2004 at 01:29:46PM +0100, Andi Kleen wrote:
 
> And the event polling can take several ms, there is just no way 
> to do this during runtime with interrupts off. Especially since
> thermal will do this every few seconds. With my patch at least 
> only kacpid is blocked for that long, not the whole system.

may this also help the awfully slow battery polling on some machines
(e.g. Dell D600) or is this a completely different case?
-- 
Stefan Seyfried



-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

RE: [PATCH] Don't disable interrupts during EC access

[Yu, Luming]

>Another possible way would be to only disable the ACPI interrupt
>in the local APIC and keep the others alive  (but what to do 
>in PIC mode?) 

Disable EC GPE in your patch could achieve the goal.

Thanks,
Luming


-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

Re: [PATCH] Don't disable interrupts during EC access

[Dmitry Torokhov]

On Tuesday 16 November 2004 10:37 am, Andi Kleen wrote:
> > For detail, please refer to ACPI spec :
> > 12 ? ACPI Embedded Controller Interface Specification.
> 
> Ok. But the problem is not the actual write, but the waiting 
> for the event which takes extremly long on these machines
> (it's not a single broken machine, but has been observed
> on several VIA based laptops). Would it work to only disable
> interrupts during the register write/read, but not during the
> polling delay? 

Actually, the proper solution would be to get rid of polling entirely,
like in the patch below. Could anyone with a box with EC verify that
it works?

Btw, what's up with initializing every variable in ACPI code? It is
not free after all...
 
-- 
Dmitry


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


ChangeSet@1.1998, 2004-11-17 01:44:59-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.
        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 |  217 ++++++++++++++++++++++++++++++++++---------------------------------
 1 files changed, 113 insertions(+), 104 deletions(-)


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



diff -Nru a/drivers/acpi/ec.c b/drivers/acpi/ec.c
--- a/drivers/acpi/ec.c	2004-11-17 01:45:43 -05:00
+++ b/drivers/acpi/ec.c	2004-11-17 01:45:43 -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		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 +84,9 @@
 	struct acpi_generic_address	command_addr;
 	struct acpi_generic_address	data_addr;
 	unsigned long			global_lock;
-	spinlock_t			lock;
+	unsigned int			expect_ibe;
+	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 +99,56 @@
                              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_expect_ibe(struct acpi_ec *ec)
+{
+	ec->expect_ibe = 1;
+	mb();
+}
+
+static int acpi_ec_wait_ibe(struct acpi_ec *ec)
+{
+	int	result;
+
+	result = wait_event_interruptible_timeout(ec->wait,
+				!(acpi_ec_read_status(ec) & ACPI_EC_FLAG_IBF),
+				msecs_to_jiffies(ACPI_EC_DELAY));
+	if (result == 0)
+		result = -ETIME;
 
-	return -ETIME;
+	return result;
 }
 
 
+static int acpi_ec_wait_obf(struct acpi_ec *ec)
+{
+	int	result;
+
+	result = wait_event_interruptible_timeout(ec->wait,
+				(acpi_ec_read_status(ec) & ACPI_EC_FLAG_OBF),
+				msecs_to_jiffies(ACPI_EC_DELAY));
+	if (result == 0)
+		result = -ETIME;
+
+	return result;
+}
+
 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 +162,28 @@
 		if (ACPI_FAILURE(status))
 			return_VALUE(-ENODEV);
 	}
-	
-	spin_lock_irqsave(&ec->lock, flags);
 
+	WARN_ON(in_interrupt());
+	down(&ec->sem);
+
+	acpi_ec_expect_ibe(ec);
 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_READ, &ec->command_addr);
-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
-	if (result)
+	result = acpi_ec_wait_ibe(ec);
+	if (result < 0)
 		goto end;
 
 	acpi_hw_low_level_write(8, address, &ec->data_addr);
-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF);
-	if (result)
+	result = acpi_ec_wait_obf(ec);
+	if (result < 0)
 		goto end;
 
-
 	acpi_hw_low_level_read(8, data, &ec->data_addr);
 
 	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 +198,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,28 +213,32 @@
 			return_VALUE(-ENODEV);
 	}
 
-	spin_lock_irqsave(&ec->lock, flags);
+	WARN_ON(in_interrupt());
+	down(&ec->sem);
 
+	acpi_ec_expect_ibe(ec);
 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_WRITE, &ec->command_addr);
-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
-	if (result)
+	result = acpi_ec_wait_ibe(ec);
+	if (result < 0)
 		goto end;
 
+	acpi_ec_expect_ibe(ec);
 	acpi_hw_low_level_write(8, address, &ec->data_addr);
-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
-	if (result)
+	result = acpi_ec_wait_ibe(ec);
+	if (result < 0)
 		goto end;
 
+	acpi_ec_expect_ibe(ec);
 	acpi_hw_low_level_write(8, data, &ec->data_addr);
-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
-	if (result)
+	result = acpi_ec_wait_ibe(ec);
+	if (result < 0)
 		goto end;
 
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Wrote [%02x] to address [%02x]\n",
 		data, address));
 
 end:
-	spin_unlock_irqrestore(&ec->lock, flags);
+	up(&ec->sem);
 
 	if (ec->global_lock)
 		acpi_release_global_lock(glk);
@@ -287,10 +293,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");
 
@@ -310,19 +315,20 @@
 	 * 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);
+	WARN_ON(in_interrupt());
+	down(&ec->sem);
 
 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_QUERY, &ec->command_addr);
-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF);
-	if (result)
+	result = acpi_ec_wait_obf(ec);
+	if (result < 0)
 		goto end;
-	
+
 	acpi_hw_low_level_read(8, data, &ec->data_addr);
 	if (!*data)
 		result = -ENODATA;
 
 end:
-	spin_unlock_irqrestore(&ec->lock, flags);
+	up(&ec->sem);
 
 	if (ec->global_lock)
 		acpi_release_global_lock(glk);
@@ -345,8 +351,7 @@
 	void			*ec_cxt)
 {
 	struct acpi_ec		*ec = (struct acpi_ec *) ec_cxt;
-	u32			value = 0;
-	unsigned long		flags = 0;
+	u32			value;
 	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'};
@@ -354,11 +359,7 @@
 	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);
+		goto end;
 
 	/* TBD: Implement asynch events!
 	 * NOTE: All we care about are EC-SCI's.  Other EC events are
@@ -366,12 +367,12 @@
 	 * treat EC-SCIs as level (versus EDGE!) triggered, preventing
 	 *  a purely interrupt-driven approach (grumble, grumble).
 	 */
-	if (!(value & ACPI_EC_FLAG_SCI))
+	if (!(acpi_ec_read_status(ec) & ACPI_EC_FLAG_SCI))
 		goto end;
 
 	if (acpi_ec_query(ec, &value))
 		goto end;
-	
+
 	object_name[2] = hex[((value >> 4) & 0x0F)];
 	object_name[3] = hex[(value & 0x0F)];
 
@@ -388,6 +389,7 @@
 	void			*data)
 {
 	acpi_status		status = AE_OK;
+	u32			value;
 	struct acpi_ec		*ec = (struct acpi_ec *) data;
 
 	if (!ec)
@@ -395,13 +397,20 @@
 
 	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);
 
-	if (status == AE_OK)
-		return ACPI_INTERRUPT_HANDLED;
-	else
-		return ACPI_INTERRUPT_NOT_HANDLED;
+	if ((value & ACPI_EC_FLAG_OBF) ||
+	    (ec->expect_ibe && !(value & ACPI_EC_FLAG_IBF))) {
+		ec->expect_ibe = 0;
+		wake_up(&ec->wait);
+	}
+
+	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;
 }
 
 /* --------------------------------------------------------------------------
@@ -419,10 +428,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;
 }
@@ -437,9 +444,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_FUNCTION_TRACE("acpi_ec_space_handler");
 
@@ -529,7 +536,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");
 
@@ -580,9 +587,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");
@@ -597,7 +604,8 @@
 
 	ec->handle = device->handle;
 	ec->uid = -1;
-	ec->lock = SPIN_LOCK_UNLOCKED;
+	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;
@@ -611,7 +619,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);
@@ -651,7 +659,7 @@
 	struct acpi_device	*device,
 	int			type)
 {
-	struct acpi_ec		*ec = NULL;
+	struct acpi_ec		*ec;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_remove");
 
@@ -706,8 +714,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");
 
@@ -763,8 +771,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");
 
@@ -792,7 +800,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 0;
@@ -807,11 +815,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;
-	ec_ecdt->lock = SPIN_LOCK_UNLOCKED;
 	/* use the GL just to be safe */
 	ec_ecdt->global_lock = TRUE;
 	ec_ecdt->uid = ecdt_ptr->uid;
@@ -855,7 +864,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: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

RE: [PATCH] Don't disable interrupts during EC access

[Li, Shaohua]

>On Tuesday 16 November 2004 10:37 am, Andi Kleen wrote:
>> > For detail, please refer to ACPI spec :
>> > 12 ? ACPI Embedded Controller Interface Specification.
>>
>> Ok. But the problem is not the actual write, but the waiting
>> for the event which takes extremly long on these machines
>> (it's not a single broken machine, but has been observed
>> on several VIA based laptops). Would it work to only disable
>> interrupts during the register write/read, but not during the
>> polling delay?
>
>Actually, the proper solution would be to get rid of polling entirely,
>like in the patch below. Could anyone with a box with EC verify that
>it works?
>
>Btw, what's up with initializing every variable in ACPI code? It is
>not free after all...
>
I agree the right solution should be to stop polling, but did you look
at the comments in ec.c (line 361)? It said some systems can't work with
asynchronous mode.

Thanks,
Shaohua


-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

RE: [PATCH] Don't disable interrupts during EC access

[Yu, Luming]

Spec defined EC-SCI is edge-trigged.
So, I think that is solution sticking with Spec.
Not sure, how many laptop break this rule.

If  wait_event_interruptible_timeout  returns due to timeout,
Can we think it of polling timeout?

So, I suggest Dmitry to complete  that to include
level-trigged situation.

Thanks
Luming 

>-----Original Message-----
>From: Li, Shaohua 
>Sent: 2004年11月17日 15:10
>To: Dmitry Torokhov; acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>Cc: Andi Kleen; Yu, Luming; Matthew Wilcox; Fu, Michael; 
>Brown, Len; Moore, Robert
>Subject: RE: [ACPI] [PATCH] Don't disable interrupts during EC access
>
>>On Tuesday 16 November 2004 10:37 am, Andi Kleen wrote:
>>> > For detail, please refer to ACPI spec :
>>> > 12 ? ACPI Embedded Controller Interface Specification.
>>>
>>> Ok. But the problem is not the actual write, but the waiting
>>> for the event which takes extremly long on these machines
>>> (it's not a single broken machine, but has been observed
>>> on several VIA based laptops). Would it work to only disable
>>> interrupts during the register write/read, but not during the
>>> polling delay?
>>
>>Actually, the proper solution would be to get rid of polling entirely,
>>like in the patch below. Could anyone with a box with EC verify that
>>it works?
>>
>>Btw, what's up with initializing every variable in ACPI code? It is
>>not free after all...
>>
>I agree the right solution should be to stop polling, but did 
>you look at the comments in ec.c (line 361)? It said some 
>systems can't work with asynchronous mode.
>
>Thanks,
>Shaohua
>


-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

Re: [PATCH] Don't disable interrupts during EC access

[Dmitry Torokhov]

On Wednesday 17 November 2004 02:10 am, Li, Shaohua wrote:
> >On Tuesday 16 November 2004 10:37 am, Andi Kleen wrote:
> >> > For detail, please refer to ACPI spec :
> >> > 12 ? ACPI Embedded Controller Interface Specification.
> >>
> >> Ok. But the problem is not the actual write, but the waiting
> >> for the event which takes extremly long on these machines
> >> (it's not a single broken machine, but has been observed
> >> on several VIA based laptops). Would it work to only disable
> >> interrupts during the register write/read, but not during the
> >> polling delay?
> >
> >Actually, the proper solution would be to get rid of polling entirely,
> >like in the patch below. Could anyone with a box with EC verify that
> >it works?
> >
> >Btw, what's up with initializing every variable in ACPI code? It is
> >not free after all...
> >
> I agree the right solution should be to stop polling, but did you look
> at the comments in ec.c (line 361)? It said some systems can't work with
> asynchronous mode.
> 

Hmm... Let's see. We can only have 1 reader/writer active at the same time
per EC becuase they are serialized via ec->sem. So the only thing we can
race with is SCI generated by firmware in response to external factors.
So I guess we still need sleep-polling in acpi_ec_query_data, something
like this:

	while (unlikely(down_trylock(&ec->sem))) {
		/* reader or writer is waiting for completion */
		value = acpi_ec_read_status(ec);

        	if ((value & ACPI_EC_FLAG_OBF) ||
            	    (ec->expect_ibe && !(value & ACPI_EC_FLAG_IBF))) {
                	ec->expect_ibe = 0;
                	wake_up(&ec->wait);
        	}
		
		msleep(1);
	}

        acpi_hw_low_level_write(8, ACPI_EC_COMMAND_QUERY, &ec->command_addr);
        result = acpi_ec_wait_obf(ec);
        if (result < 0)
                goto end;
	.....

Will this work?

-- 
Dmitry


-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

Re: [PATCH] Don't disable interrupts during EC access

[Dmitry Torokhov]

On Wednesday 17 November 2004 02:46 am, Yu, Luming wrote:
> Spec defined EC-SCI is edge-trigged.
> So, I think that is solution sticking with Spec.
> Not sure, how many laptop break this rule.
> 
> If  wait_event_interruptible_timeout  returns due to timeout,
> Can we think it of polling timeout?
> 

Yes, I think this is a great idea - if we time out check the
status register one more time to verify that we did not miss
interrupt because SCI was raised earlier.

-- 
Dmitry


-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

Re: [PATCH] Don't disable interrupts during EC access

[Len Brown]

just to add some fuel to this fire,
here is an example of where we don't wait long enough for the EC:

http://bugzilla.kernel.org/show_bug.cgi?id=2845



-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

Re: [PATCH] Don't disable interrupts during EC access

[Dmitry Torokhov]

On Wednesday 17 November 2004 04:53 pm, Len Brown wrote:
> just to add some fuel to this fire,
> here is an example of where we don't wait long enough for the EC:
> 
> http://bugzilla.kernel.org/show_bug.cgi?id=2845
> 
> 

Ok, so here is the new version that tries to hanle level-triggered SCIs
while using event signalling instead of polling.

Since we serialize access to EC via ec->sem semaphore there can be only
one reader/writer at one time so they can not race with each other.
acpi_ec_gpe_query() also holds ec->sem until EC is queried which should
reset SCI condition. The only poblem window that I see is when there is
an SCI was raised and acpi_ec_gpe_handler() scheduled acpi_ec_gpe_query()
and there comes a reader or writer. To take care of it instead of doing
normal "down" in acpi_ec_gpe_query() we will do down_trylock(). If it is 
unsuccessful that means that other thread is waiting on IO completion and
we will do the soft polling (interrupts enabled) for it and wake it up
when IO completes.

Am I missing somethig?

Please give it a try - it compiles but since I don't have EC I am not
able to test it. 

-- 
Dmitry


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


ChangeSet@1.2001, 2004-11-18 01:11:46-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 |  222 ++++++++++++++++++++++++++++++++++++++-----------------------------
 1 files changed, 126 insertions(+), 96 deletions(-)


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



diff -Nru a/drivers/acpi/ec.c b/drivers/acpi/ec.c
--- a/drivers/acpi/ec.c	2004-11-18 01:13:03 -05:00
+++ b/drivers/acpi/ec.c	2004-11-18 01:13:03 -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
@@ -87,7 +87,9 @@
 	struct acpi_generic_address	command_addr;
 	struct acpi_generic_address	data_addr;
 	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 +102,59 @@
                              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;
+}
+
+static int acpi_ec_wait(struct acpi_ec *ec, unsigned int event)
+{
+	int	result;
 
-	/* Poll the EC status register waiting for the event to occur. */
+	ec->expect_event = event;
+	smp_mb();
+
+	result = wait_event_interruptible_timeout(ec->wait,
+					!ec->expect_event,
+					msecs_to_jiffies(ACPI_EC_DELAY));
+	ec->expect_event = 0;
+	smp_mb();
+
+	if (result < 0)
+		return result;
+
+	/*
+	 * Verify that the event in question has actually happened by
+	 * querying EC status. Do the check even if operation timed-out
+	 * to make sure that we did not miss interrupt.
+	 */
 	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);
+		if (acpi_ec_read_status(ec) & ACPI_EC_FLAG_OBF)
+			return 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);
+		if (~acpi_ec_read_status(ec) & ACPI_EC_FLAG_IBF)
+			return 0;
 		break;
-	default:
-		return -EINVAL;
 	}
 
 	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,8 +168,9 @@
 		if (ACPI_FAILURE(status))
 			return_VALUE(-ENODEV);
 	}
-	
-	spin_lock_irqsave(&ec->lock, flags);
+
+	WARN_ON(in_interrupt());
+	down(&ec->sem);
 
 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_READ, &ec->command_addr);
 	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
@@ -173,14 +182,13 @@
 	if (result)
 		goto end;
 
-
 	acpi_hw_low_level_read(8, data, &ec->data_addr);
 
 	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 +203,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,7 +218,8 @@
 			return_VALUE(-ENODEV);
 	}
 
-	spin_lock_irqsave(&ec->lock, flags);
+	WARN_ON(in_interrupt());
+	down(&ec->sem);
 
 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_WRITE, &ec->command_addr);
 	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
@@ -232,7 +240,7 @@
 		data, address));
 
 end:
-	spin_unlock_irqrestore(&ec->lock, flags);
+	up(&ec->sem);
 
 	if (ec->global_lock)
 		acpi_release_global_lock(glk);
@@ -287,10 +295,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");
 
@@ -310,20 +317,16 @@
 	 * 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_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);
-
 	if (ec->global_lock)
 		acpi_release_global_lock(glk);
 
@@ -345,33 +348,50 @@
 	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);
+
+		if ((ec->expect_event == ACPI_EC_EVENT_OBF &&
+					(value & ACPI_EC_FLAG_OBF)) ||
+		    (ec->expect_event == ACPI_EC_EVENT_IBE &&
+					!(value & ACPI_EC_FLAG_IBF))) {
+			ec->expect_event = 0;
+			wake_up(&ec->wait);
+		}
+
+		/*
+		 * 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)];
 
@@ -388,6 +408,7 @@
 	void			*data)
 {
 	acpi_status		status = AE_OK;
+	u32			value;
 	struct acpi_ec		*ec = (struct acpi_ec *) data;
 
 	if (!ec)
@@ -395,13 +416,22 @@
 
 	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);
 
-	if (status == AE_OK)
-		return ACPI_INTERRUPT_HANDLED;
-	else
-		return ACPI_INTERRUPT_NOT_HANDLED;
+	if ((ec->expect_event == ACPI_EC_EVENT_OBF &&
+				(value & ACPI_EC_FLAG_OBF)) ||
+	    (ec->expect_event == ACPI_EC_EVENT_IBE &&
+				!(value & ACPI_EC_FLAG_IBF))) {
+		ec->expect_event = 0;
+		wake_up(&ec->wait);
+	}
+
+	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;
 }
 
 /* --------------------------------------------------------------------------
@@ -419,10 +449,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;
 }
@@ -437,9 +465,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_FUNCTION_TRACE("acpi_ec_space_handler");
 
@@ -506,7 +534,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);
@@ -529,7 +557,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");
 
@@ -580,9 +608,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");
@@ -597,7 +625,8 @@
 
 	ec->handle = device->handle;
 	ec->uid = -1;
-	ec->lock = SPIN_LOCK_UNLOCKED;
+	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;
@@ -611,7 +640,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);
@@ -651,7 +680,7 @@
 	struct acpi_device	*device,
 	int			type)
 {
-	struct acpi_ec		*ec = NULL;
+	struct acpi_ec		*ec;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_remove");
 
@@ -706,8 +735,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");
 
@@ -763,8 +792,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");
 
@@ -792,7 +821,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 0;
@@ -807,11 +836,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;
-	ec_ecdt->lock = SPIN_LOCK_UNLOCKED;
 	/* use the GL just to be safe */
 	ec_ecdt->global_lock = TRUE;
 	ec_ecdt->uid = ecdt_ptr->uid;
@@ -855,7 +885,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: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8