the errors about two EC patches

the errors about two EC patches

[Zhao Yakui]

Hi, Len
    
    The patch named acpi-ec-dont-degrade-to-poll-mode-at-storm-automatically is 
included by 2.6.27-rc5-mm1 tree. In fact this patch can't fix the problem of 
bug 11089 & 10724. 
    >Not all users of semi-broken EC devices want to degrade to poll mode, so
give them right to choose.
    >This fixes a regression in 2.6.26 (from 2.6.25.3).  Initially reported as
"Asus Eee PC hotkeys stop working if pressed quickly" in bugzilla
<http://bugzilla.kernel.org/show_bug.cgi?id=11089>.

    At the same time the content in the patch is totally discarded by the 
following patch in ACPI test git tree:
>commit 50c0f43ff1199bbea5f0418d40df32f4d4029b13
>Author: Andi Kleen <ak@linux.intel.com>
>Date:   Thu Sep 4 15:13:29 2008 +0200

    >ACPI: EC: do transaction from interrupt context

   In the above patch there exist the following problems:
   a. In the following code the GPE_STORM bit will always be set regardless 
of whether there exists the EC GPE storm.
    >if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) {
    >            /* check if we received SCI during transaction */
    >           ec_check_sci(ec, acpi_ec_read_status(ec));
    >           /* it is safe to enable GPE outside of transaction */
    >            acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
    >    } else if (test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
    >               atomic_read(&ec->irq_count) > ACPI_EC_STORM_THRESHOLD)
    >           pr_debug(PREFIX "GPE storm detected\n");
    >           set_bit(EC_FLAGS_GPE_STORM, &ec->flags);
    b. When EC GPE storm is detected, the EC GPE will be disabled and 
EC will work in polling mode while doing EC transaction. In such case 
maybe EC transaction is not finished. But when timeout happens, it 
will still be regarded as finishing the EC transaction. 
       This error is related with the following source code:
           > {
           >    delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY); 
 //Maybe the preemptible schedule happens here.If the current jiffies
 is greater than the predefined jiffies after the process is returned 
from preempt_schedule, OS will have no opportunity to do the EC transaction.
 It means that EC transaction is not finished. But it is still regarded as 
finishing EC transaction. It is incorrect.
           >     while (time_before(jiffies, delay)) {
            >            gpe_transaction(ec, acpi_ec_read_status(ec));
            >            msleep(1);
//as msleep is realized by using the function of schedule_timeout, 
maybe the current jiffies is already greater than the predefined jiffies 
before finishing EC transaction. In such case the EC transaction is
not finished. But it is also regarded as finishing. It is incorrect.
            >            if (ec_transaction_done(ec))
            >                    goto end;
            >    }
     c. There is no detailed change log although a lot is changed.(
Including the EC work mode, the detect mechanism of EC GPE storm). 
It is not easy to understand.     

     At the same time this commit can't handle the EC notification
event in the following case:
         EC GPE Interrupt storm is detected and EC GPE will be disabled
when doing EC transaction. If EC notification event happens while doing
EC transaction(EC GPE is disabled), the SCI_EVT bit of EC status
register is cleared before OS issuing query command. In such case the EC
notification event will be lost.

  Based on the above analysis ,IMO the two patches are not appropriate.

Best regards.
    Yakui

Re: the errors about two EC patches

[Alexey Starikovskiy]

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

Zhao Yakui wrote:
>
>     >ACPI: EC: do transaction from interrupt context
>
>    In the above patch there exist the following problems:
>    a. In the following code the GPE_STORM bit will always be set regardless 
> of whether there exists the EC GPE storm.
>     >if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) {
>     >            /* check if we received SCI during transaction */
>     >           ec_check_sci(ec, acpi_ec_read_status(ec));
>     >           /* it is safe to enable GPE outside of transaction */
>     >            acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
>     >    } else if (test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
>     >               atomic_read(&ec->irq_count) > ACPI_EC_STORM_THRESHOLD)
>     >           pr_debug(PREFIX "GPE storm detected\n");
>     >           set_bit(EC_FLAGS_GPE_STORM, &ec->flags);
>   
This problem is fixed two days ago, it is not completely fair to mention 
it here.
>     b. When EC GPE storm is detected, the EC GPE will be disabled and 
> EC will work in polling mode while doing EC transaction. In such case 
> maybe EC transaction is not finished. But when timeout happens, it 
> will still be regarded as finishing the EC transaction.
>   
Ok. Fixed, now -ETIME is returned in this case.
>        This error is related with the following source code:
>            > {
>            >    delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY); 
>  //Maybe the preemptible schedule happens here.If the current jiffies
>  is greater than the predefined jiffies after the process is returned 
> from preempt_schedule, OS will have no opportunity to do the EC transaction.
>  It means that EC transaction is not finished. But it is still regarded as 
> finishing EC transaction. It is incorrect.
>            >     while (time_before(jiffies, delay)) {
>             >            gpe_transaction(ec, acpi_ec_read_status(ec));
>             >            msleep(1);
> //as msleep is realized by using the function of schedule_timeout, 
> maybe the current jiffies is already greater than the predefined jiffies 
> before finishing EC transaction. In such case the EC transaction is
> not finished. But it is also regarded as finishing. It is incorrect.
>             >            if (ec_transaction_done(ec))
>             >                    goto end;
>             >    }
>   
ec_transaction_done() returns true only if there is finished transaction 
stored in
acpi_ec. It does not matter if it is late to check for this result from 
timeout point of view.
the same ideology was put into driver before this patch:
"ACPI: Avoid bogus EC timeout when EC is in Polling mode"
>      c. There is no detailed change log although a lot is changed.(
> Including the EC work mode, the detect mechanism of EC GPE storm). 
> It is not easy to understand.     
>
>      At the same time this commit can't handle the EC notification
> event in the following case:
>          EC GPE Interrupt storm is detected and EC GPE will be disabled
> when doing EC transaction. If EC notification event happens while doing
> EC transaction(EC GPE is disabled), the SCI_EVT bit of EC status
> register is cleared before OS issuing query command. In such case the EC
> notification event will be lost.
>   
As I said already, there is no way EC will clear SCI bit before driver 
issues
the Query command to it (thus reading the event).
Even more, this same behavior was in the driver for ages and it did
not broke once.
>   Based on the above analysis ,IMO the two patches are not appropriate.
>
>   
This is the best analysis I've seen so far :) Thanks, it is really 
entertaining.
Please use latest version next time, it is attached for your convenience.

Regards,
Alex.


[-- Attachment #2: fast_transaction.patch --]
[-- Type: text/x-diff, Size: 15395 bytes --]

ACPI: EC: do transaction from interrupt context

From: Alexey Starikovskiy <astarikovskiy@suse.de>

It is easier and faster to do transaction directly from interrupt context
rather than waking control thread.
Also, cleaner GPE storm avoidance is implemented.

Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
---

 drivers/acpi/ec.c |  288 ++++++++++++++++++++++++++---------------------------
 1 files changed, 140 insertions(+), 148 deletions(-)


diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 13593f9..504475c 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1,7 +1,7 @@
 /*
- *  ec.c - ACPI Embedded Controller Driver (v2.0)
+ *  ec.c - ACPI Embedded Controller Driver (v2.1)
  *
- *  Copyright (C) 2006, 2007 Alexey Starikovskiy <alexey.y.starikovskiy@intel.com>
+ *  Copyright (C) 2006-2008 Alexey Starikovskiy <astarikovskiy@suse.de>
  *  Copyright (C) 2006 Denis Sadykov <denis.m.sadykov@intel.com>
  *  Copyright (C) 2004 Luming Yu <luming.yu@intel.com>
  *  Copyright (C) 2001, 2002 Andy Grover <andrew.grover@intel.com>
@@ -26,7 +26,7 @@
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  */
 
-/* Uncomment next line to get verbose print outs*/
+/* Uncomment next line to get verbose printout */
 /* #define DEBUG */
 
 #include <linux/kernel.h>
@@ -38,6 +38,7 @@
 #include <linux/seq_file.h>
 #include <linux/interrupt.h>
 #include <linux/list.h>
+#include <linux/spinlock.h>
 #include <asm/io.h>
 #include <acpi/acpi_bus.h>
 #include <acpi/acpi_drivers.h>
@@ -65,22 +66,18 @@ enum ec_command {
 	ACPI_EC_COMMAND_QUERY = 0x84,
 };
 
-/* EC events */
-enum ec_event {
-	ACPI_EC_EVENT_OBF_1 = 1,	/* Output buffer full */
-	ACPI_EC_EVENT_IBF_0,		/* Input buffer empty */
-};
-
 #define ACPI_EC_DELAY		500	/* Wait 500ms max. during EC ops */
 #define ACPI_EC_UDELAY_GLK	1000	/* Wait 1ms max. to get global lock */
 #define ACPI_EC_UDELAY		100	/* Wait 100us before polling EC again */
 
+#define ACPI_EC_STORM_THRESHOLD 20	/* number of false interrupts
+					   per one transaction */
+
 enum {
-	EC_FLAGS_WAIT_GPE = 0,		/* Don't check status until GPE arrives */
 	EC_FLAGS_QUERY_PENDING,		/* Query is pending */
 	EC_FLAGS_GPE_MODE,		/* Expect GPE to be sent for status change */
 	EC_FLAGS_NO_GPE,		/* Don't use GPE mode */
-	EC_FLAGS_RESCHEDULE_POLL	/* Re-schedule poll */
+	EC_FLAGS_GPE_STORM		/* GPE storm detected */
 };
 
 /* If we find an EC via the ECDT, we need to keep a ptr to its context */
@@ -95,6 +92,14 @@ struct acpi_ec_query_handler {
 	u8 query_bit;
 };
 
+struct transaction_data {
+	const u8 *wdata;
+	u8 *rdata;
+	u8 command;
+	u8 wlen;
+	u8 rlen;
+};
+
 static struct acpi_ec {
 	acpi_handle handle;
 	unsigned long gpe;
@@ -105,7 +110,8 @@ static struct acpi_ec {
 	struct mutex lock;
 	wait_queue_head_t wait;
 	struct list_head list;
-	struct delayed_work work;
+	struct transaction_data t;
+	spinlock_t spinlock;
 	atomic_t irq_count;
 	u8 handlers_installed;
 } *boot_ec, *first_ec;
@@ -150,13 +156,26 @@ static inline u8 acpi_ec_read_data(struct acpi_ec *ec)
 {
 	u8 x = inb(ec->data_addr);
 	pr_debug(PREFIX "---> data = 0x%2.2x\n", x);
-	return inb(ec->data_addr);
+	return x;
 }
 
 static inline void acpi_ec_write_cmd(struct acpi_ec *ec, u8 command)
 {
+	unsigned long flags;
 	pr_debug(PREFIX "<--- command = 0x%2.2x\n", command);
-	outb(command, ec->command_addr);
+	spin_lock_irqsave(&ec->spinlock, flags);
+	outb((ec->t.command = command), ec->command_addr);
+	spin_unlock_irqrestore(&ec->spinlock, flags);
+}
+
+static inline u8 ec_read_command(struct acpi_ec *ec)
+{
+	unsigned long flags;
+	u8 cmd;
+	spin_lock_irqsave(&ec->spinlock, flags);
+	cmd = ec->t.command;
+	spin_unlock_irqrestore(&ec->spinlock, flags);
+	return cmd;
 }
 
 static inline void acpi_ec_write_data(struct acpi_ec *ec, u8 data)
@@ -165,69 +184,61 @@ static inline void acpi_ec_write_data(struct acpi_ec *ec, u8 data)
 	outb(data, ec->data_addr);
 }
 
-static inline int acpi_ec_check_status(struct acpi_ec *ec, enum ec_event event)
+static int ec_transaction_done(struct acpi_ec *ec)
 {
-	if (test_bit(EC_FLAGS_WAIT_GPE, &ec->flags))
-		return 0;
-	if (event == ACPI_EC_EVENT_OBF_1) {
-		if (acpi_ec_read_status(ec) & ACPI_EC_FLAG_OBF)
-			return 1;
-	} else if (event == ACPI_EC_EVENT_IBF_0) {
-		if (!(acpi_ec_read_status(ec) & ACPI_EC_FLAG_IBF))
-			return 1;
-	}
-
+	if (!ec_read_command(ec))
+		return 1;
+	if (!ec->t.wlen && !ec->t.rlen)
+		return 1;
 	return 0;
 }
 
-static void ec_schedule_ec_poll(struct acpi_ec *ec)
+static void gpe_transaction(struct acpi_ec *ec, u8 status)
 {
-	if (test_bit(EC_FLAGS_RESCHEDULE_POLL, &ec->flags))
-		schedule_delayed_work(&ec->work,
-				      msecs_to_jiffies(ACPI_EC_DELAY));
+	if (!ec_read_command(ec))
+		return;
+	if (ec->t.wlen > 0) {
+		if ((status & ACPI_EC_FLAG_IBF) == 0) {
+			acpi_ec_write_data(ec, *(ec->t.wdata++));
+			--ec->t.wlen;
+		} else
+			/* false interrupt, state didn't change */
+			atomic_inc(&ec->irq_count);
+
+	} else if (ec->t.rlen > 0) {
+		if ((status & ACPI_EC_FLAG_OBF) == 1) {
+			*(ec->t.rdata++) = acpi_ec_read_data(ec);
+			--ec->t.rlen;
+		} else
+			/* false interrupt, state didn't change */
+			atomic_inc(&ec->irq_count);
+	}
 }
 
-static void ec_switch_to_poll_mode(struct acpi_ec *ec)
+static int acpi_ec_wait(struct acpi_ec *ec)
 {
+	if (wait_event_timeout(ec->wait, ec_transaction_done(ec),
+			       msecs_to_jiffies(ACPI_EC_DELAY)))
+		return 0;
+	/* missing GPEs, switch back to poll mode */
+	if (printk_ratelimit())
+		pr_info(PREFIX "missing confirmations, "
+				"switch off interrupt mode.\n");
 	set_bit(EC_FLAGS_NO_GPE, &ec->flags);
 	clear_bit(EC_FLAGS_GPE_MODE, &ec->flags);
-	acpi_disable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
-	set_bit(EC_FLAGS_RESCHEDULE_POLL, &ec->flags);
+	return 1;
 }
 
-static int acpi_ec_wait(struct acpi_ec *ec, enum ec_event event, int force_poll)
+static void acpi_ec_gpe_query(void *ec_cxt);
+
+static int ec_check_sci(struct acpi_ec *ec, u8 state)
 {
-	atomic_set(&ec->irq_count, 0);
-	if (likely(test_bit(EC_FLAGS_GPE_MODE, &ec->flags)) &&
-	    likely(!force_poll)) {
-		if (wait_event_timeout(ec->wait, acpi_ec_check_status(ec, event),
-				       msecs_to_jiffies(ACPI_EC_DELAY)))
-			return 0;
-		clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
-		if (acpi_ec_check_status(ec, event)) {
-			/* missing GPEs, switch back to poll mode */
-			if (printk_ratelimit())
-				pr_info(PREFIX "missing confirmations, "
-						"switch off interrupt mode.\n");
-			ec_switch_to_poll_mode(ec);
-			ec_schedule_ec_poll(ec);
-			return 0;
-		}
-	} else {
-		unsigned long delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY);
-		clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
-		while (time_before(jiffies, delay)) {
-			if (acpi_ec_check_status(ec, event))
-				return 0;
-			msleep(1);
-		}
-		if (acpi_ec_check_status(ec,event))
-			return 0;
+	if (state & ACPI_EC_FLAG_SCI) {
+		if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags))
+			return acpi_os_execute(OSL_EC_BURST_HANDLER,
+				acpi_ec_gpe_query, ec);
 	}
-	pr_err(PREFIX "acpi_ec_wait timeout, status = 0x%2.2x, event = %s\n",
-		acpi_ec_read_status(ec),
-		(event == ACPI_EC_EVENT_OBF_1) ? "\"b0=1\"" : "\"b1=0\"");
-	return -ETIME;
+	return 0;
 }
 
 static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, u8 command,
@@ -235,45 +246,56 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, u8 command,
 					u8 * rdata, unsigned rdata_len,
 					int force_poll)
 {
-	int result = 0;
-	set_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
+	unsigned long delay;
+	int ret = 0;
 	pr_debug(PREFIX "transaction start\n");
-	acpi_ec_write_cmd(ec, command);
-	for (; wdata_len > 0; --wdata_len) {
-		result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, force_poll);
-		if (result) {
-			pr_err(PREFIX
-			       "write_cmd timeout, command = %d\n", command);
-			goto end;
-		}
-		set_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
-		acpi_ec_write_data(ec, *(wdata++));
+	/* disable GPE during transaction if storm is detected */
+	if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) {
+		set_bit(EC_FLAGS_NO_GPE, &ec->flags);
+		clear_bit(EC_FLAGS_GPE_MODE, &ec->flags);
+		acpi_disable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
 	}
-
-	if (!rdata_len) {
-		result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, force_poll);
-		if (result) {
-			pr_err(PREFIX
-			       "finish-write timeout, command = %d\n", command);
-			goto end;
-		}
-	} else if (command == ACPI_EC_COMMAND_QUERY)
-		clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
-
-	for (; rdata_len > 0; --rdata_len) {
-		result = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF_1, force_poll);
-		if (result) {
-			pr_err(PREFIX "read timeout, command = %d\n", command);
-			goto end;
+	atomic_set(&ec->irq_count, 0);
+	/* fill in transaction structure */
+	ec->t.wdata = wdata;
+	ec->t.wlen = wdata_len;
+	ec->t.rdata = rdata;
+	ec->t.rlen = rdata_len;
+	/* start transaction */
+	acpi_ec_write_cmd(ec, command);
+	/* if we selected poll mode or failed in GPE-mode do a poll loop */
+	if (force_poll ||
+	    !test_bit(EC_FLAGS_GPE_MODE, &ec->flags) ||
+	    acpi_ec_wait(ec)) {
+		delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY);
+		while (time_before(jiffies, delay)) {
+			gpe_transaction(ec, acpi_ec_read_status(ec));
+			msleep(1);
+			if (ec_transaction_done(ec))
+				goto end;
 		}
-		/* Don't expect GPE after last read */
-		if (rdata_len > 1)
-			set_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
-		*(rdata++) = acpi_ec_read_data(ec);
+		ret = -ETIME;
 	}
-      end:
+end:
 	pr_debug(PREFIX "transaction end\n");
-	return result;
+	ec->t.command = 0;
+	if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) {
+		/* check if we received SCI during transaction */
+		ec_check_sci(ec, acpi_ec_read_status(ec));
+		/* it is safe to enable GPE outside of transaction */
+		acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
+	} else if (test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
+		   atomic_read(&ec->irq_count) > ACPI_EC_STORM_THRESHOLD) {
+		pr_debug(PREFIX "GPE storm detected\n");
+		set_bit(EC_FLAGS_GPE_STORM, &ec->flags);
+	}
+	return ret;
+}
+
+static int ec_check_ibf0(struct acpi_ec *ec)
+{
+	u8 status = acpi_ec_read_status(ec);
+	return (status & ACPI_EC_FLAG_IBF) == 0;
 }
 
 static int acpi_ec_transaction(struct acpi_ec *ec, u8 command,
@@ -283,40 +305,34 @@ static int acpi_ec_transaction(struct acpi_ec *ec, u8 command,
 {
 	int status;
 	u32 glk;
-
 	if (!ec || (wdata_len && !wdata) || (rdata_len && !rdata))
 		return -EINVAL;
-
 	if (rdata)
 		memset(rdata, 0, rdata_len);
-
 	mutex_lock(&ec->lock);
 	if (ec->global_lock) {
 		status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk);
 		if (ACPI_FAILURE(status)) {
-			mutex_unlock(&ec->lock);
-			return -ENODEV;
+			status = -ENODEV;
+			goto unlock;
 		}
 	}
-
-	status = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, 0);
-	if (status) {
+	if (!wait_event_timeout(ec->wait, ec_check_ibf0(ec),
+				msecs_to_jiffies(ACPI_EC_DELAY))) {
 		pr_err(PREFIX "input buffer is not empty, "
 				"aborting transaction\n");
+		status = -ETIME;
 		goto end;
 	}
-
 	status = acpi_ec_transaction_unlocked(ec, command,
 					      wdata, wdata_len,
 					      rdata, rdata_len,
 					      force_poll);
-
-      end:
-
+end:
 	if (ec->global_lock)
 		acpi_release_global_lock(glk);
+unlock:
 	mutex_unlock(&ec->lock);
-
 	return status;
 }
 
@@ -332,7 +348,9 @@ int acpi_ec_burst_enable(struct acpi_ec *ec)
 
 int acpi_ec_burst_disable(struct acpi_ec *ec)
 {
-	return acpi_ec_transaction(ec, ACPI_EC_BURST_DISABLE, NULL, 0, NULL, 0, 0);
+	return (acpi_ec_read_status(ec) & ACPI_EC_FLAG_BURST)?
+		acpi_ec_transaction(ec, ACPI_EC_BURST_DISABLE,
+			NULL, 0, NULL, 0, 0) : 0;
 }
 
 static int acpi_ec_read(struct acpi_ec *ec, u8 address, u8 * data)
@@ -437,6 +455,7 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 * data)
 	 */
 
 	result = acpi_ec_transaction(ec, ACPI_EC_COMMAND_QUERY, NULL, 0, &d, 1, 0);
+	clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
 	if (result)
 		return result;
 
@@ -515,24 +534,17 @@ static u32 acpi_ec_gpe_handler(void *data)
 {
 	acpi_status status = AE_OK;
 	struct acpi_ec *ec = data;
-	u8 state = acpi_ec_read_status(ec);
+	u8 state;
 
 	pr_debug(PREFIX "~~~> interrupt\n");
-	atomic_inc(&ec->irq_count);
-	if (atomic_read(&ec->irq_count) > 5) {
-		pr_err(PREFIX "GPE storm detected, disabling EC GPE\n");
-		ec_switch_to_poll_mode(ec);
-		goto end;
-	}
-	clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
-	if (test_bit(EC_FLAGS_GPE_MODE, &ec->flags))
+	state = acpi_ec_read_status(ec);
+
+	gpe_transaction(ec, state);
+	if (ec_transaction_done(ec) && (status & ACPI_EC_FLAG_IBF) == 0)
 		wake_up(&ec->wait);
 
-	if (state & ACPI_EC_FLAG_SCI) {
-		if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags))
-			status = acpi_os_execute(OSL_EC_BURST_HANDLER,
-				acpi_ec_gpe_query, ec);
-	} else if (!test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
+	status = ec_check_sci(ec, state);
+	if (!test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
 		   !test_bit(EC_FLAGS_NO_GPE, &ec->flags) &&
 		   in_interrupt()) {
 		/* this is non-query, must be confirmation */
@@ -540,21 +552,11 @@ static u32 acpi_ec_gpe_handler(void *data)
 			pr_info(PREFIX "non-query interrupt received,"
 				" switching to interrupt mode\n");
 		set_bit(EC_FLAGS_GPE_MODE, &ec->flags);
-		clear_bit(EC_FLAGS_RESCHEDULE_POLL, &ec->flags);
 	}
-end:
-	ec_schedule_ec_poll(ec);
 	return ACPI_SUCCESS(status) ?
 	    ACPI_INTERRUPT_HANDLED : ACPI_INTERRUPT_NOT_HANDLED;
 }
 
-static void do_ec_poll(struct work_struct *work)
-{
-	struct acpi_ec *ec = container_of(work, struct acpi_ec, work.work);
-	atomic_set(&ec->irq_count, 0);
-	(void)acpi_ec_gpe_handler(ec);
-}
-
 /* --------------------------------------------------------------------------
                              Address Space Management
    -------------------------------------------------------------------------- */
@@ -696,8 +698,8 @@ static struct acpi_ec *make_acpi_ec(void)
 	mutex_init(&ec->lock);
 	init_waitqueue_head(&ec->wait);
 	INIT_LIST_HEAD(&ec->list);
-	INIT_DELAYED_WORK_DEFERRABLE(&ec->work, do_ec_poll);
 	atomic_set(&ec->irq_count, 0);
+	spin_lock_init(&ec->spinlock);
 	return ec;
 }
 
@@ -736,15 +738,8 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
 	return AE_CTRL_TERMINATE;
 }
 
-static void ec_poll_stop(struct acpi_ec *ec)
-{
-	clear_bit(EC_FLAGS_RESCHEDULE_POLL, &ec->flags);
-	cancel_delayed_work(&ec->work);
-}
-
 static void ec_remove_handlers(struct acpi_ec *ec)
 {
-	ec_poll_stop(ec);
 	if (ACPI_FAILURE(acpi_remove_address_space_handler(ec->handle,
 				ACPI_ADR_SPACE_EC, &acpi_ec_space_handler)))
 		pr_err(PREFIX "failed to remove space handler\n");
@@ -849,14 +844,12 @@ static int ec_install_handlers(struct acpi_ec *ec)
 	if (ec->handlers_installed)
 		return 0;
 	status = acpi_install_gpe_handler(NULL, ec->gpe,
-					  ACPI_GPE_EDGE_TRIGGERED,
-					  &acpi_ec_gpe_handler, ec);
+				  ACPI_GPE_EDGE_TRIGGERED,
+				  &acpi_ec_gpe_handler, ec);
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
-
 	acpi_set_gpe_type(NULL, ec->gpe, ACPI_GPE_TYPE_RUNTIME);
 	acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
-
 	status = acpi_install_address_space_handler(ec->handle,
 						    ACPI_ADR_SPACE_EC,
 						    &acpi_ec_space_handler,
@@ -887,7 +880,6 @@ static int acpi_ec_start(struct acpi_device *device)
 
 	/* EC is fully operational, allow queries */
 	clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
-	ec_schedule_ec_poll(ec);
 	return ret;
 }
 

Re: the errors about two EC patches

[Rafael J. Wysocki]

Hi Alex,

On Wednesday, 10 of September 2008, Alexey Starikovskiy wrote:
> Zhao Yakui wrote:
> >
> >     >ACPI: EC: do transaction from interrupt context
> >
> >    In the above patch there exist the following problems:
> >    a. In the following code the GPE_STORM bit will always be set regardless 
> > of whether there exists the EC GPE storm.
> >     >if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) {
> >     >            /* check if we received SCI during transaction */
> >     >           ec_check_sci(ec, acpi_ec_read_status(ec));
> >     >           /* it is safe to enable GPE outside of transaction */
> >     >            acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
> >     >    } else if (test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
> >     >               atomic_read(&ec->irq_count) > ACPI_EC_STORM_THRESHOLD)
> >     >           pr_debug(PREFIX "GPE storm detected\n");
> >     >           set_bit(EC_FLAGS_GPE_STORM, &ec->flags);
> >   
> This problem is fixed two days ago, it is not completely fair to mention 
> it here.
> >     b. When EC GPE storm is detected, the EC GPE will be disabled and 
> > EC will work in polling mode while doing EC transaction. In such case 
> > maybe EC transaction is not finished. But when timeout happens, it 
> > will still be regarded as finishing the EC transaction.
> >   
> Ok. Fixed, now -ETIME is returned in this case.
> >        This error is related with the following source code:
> >            > {
> >            >    delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY); 
> >  //Maybe the preemptible schedule happens here.If the current jiffies
> >  is greater than the predefined jiffies after the process is returned 
> > from preempt_schedule, OS will have no opportunity to do the EC transaction.
> >  It means that EC transaction is not finished. But it is still regarded as 
> > finishing EC transaction. It is incorrect.
> >            >     while (time_before(jiffies, delay)) {
> >             >            gpe_transaction(ec, acpi_ec_read_status(ec));
> >             >            msleep(1);
> > //as msleep is realized by using the function of schedule_timeout, 
> > maybe the current jiffies is already greater than the predefined jiffies 
> > before finishing EC transaction. In such case the EC transaction is
> > not finished. But it is also regarded as finishing. It is incorrect.
> >             >            if (ec_transaction_done(ec))
> >             >                    goto end;
> >             >    }
> >   
> ec_transaction_done() returns true only if there is finished transaction 
> stored in
> acpi_ec. It does not matter if it is late to check for this result from 
> timeout point of view.
> the same ideology was put into driver before this patch:
> "ACPI: Avoid bogus EC timeout when EC is in Polling mode"
> >      c. There is no detailed change log although a lot is changed.(
> > Including the EC work mode, the detect mechanism of EC GPE storm). 
> > It is not easy to understand.     
> >
> >      At the same time this commit can't handle the EC notification
> > event in the following case:
> >          EC GPE Interrupt storm is detected and EC GPE will be disabled
> > when doing EC transaction. If EC notification event happens while doing
> > EC transaction(EC GPE is disabled), the SCI_EVT bit of EC status
> > register is cleared before OS issuing query command. In such case the EC
> > notification event will be lost.
> >   
> As I said already, there is no way EC will clear SCI bit before driver 
> issues
> the Query command to it (thus reading the event).
> Even more, this same behavior was in the driver for ages and it did
> not broke once.
> >   Based on the above analysis ,IMO the two patches are not appropriate.
> >
> >   
> This is the best analysis I've seen so far :) Thanks, it is really 
> entertaining.
> Please use latest version next time, it is attached for your convenience.

Hm, the attached patch conflicts with
acpi-ec-dont-degrade-to-poll-mode-at-storm-automatically.patch currently in
-mm.  Should we Andrew to drop that patch?

Rafael

Re: the errors about two EC patches

[Alexey Starikovskiy]

Rafael J. Wysocki wrote:
> Hi Alex,
>
> [...]
>   
> Hm, the attached patch conflicts with
> acpi-ec-dont-degrade-to-poll-mode-at-storm-automatically.patch currently in
> -mm.  Should we Andrew to drop that patch?
>
>   
Yes,  acpi-ec-dont-degrade-to-poll-mode-at-storm-automatically.patch 
should be dropped.

Thanks,
Alex.

Re: the errors about two EC patches

[Zhao Yakui]

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

On Wed, 2008-09-10 at 16:37 +0400, Alexey Starikovskiy wrote:
thanks for the so quick response. 

In the update patch it seems that spin_lock is used in the function of
ec_read_command/acpi_ec_write_cmd.
   IMO this is not reasonable. 
   Maybe the spin_lock is already called before OS evaluates some ACPI
object, in which the EC internal register will be accessed. In such case
there will give the following warning message.
      >2 locks held by swapper/1

>  
> Ok. Fixed, now -ETIME is returned in this case.
Yes. The code flow seems logic and reasonable after the -Etime is added.
If the timeout happens when EC transaction is not finished, it is
regarded as timeout.

But we should investigate why EC transaction is not finished when
timeout happens. Is it related with EC hardware or EC driver?
If EC can't update the status register in time after issuing
command/address, it is reasonable that it is regarded as timeout. If
not, maybe it is related with the EC driver.

In fact when timeout happens in the above cases, OS has no opportunity
to issue the EC command set completely.
    For example: Read Command: 
      a. Maybe OS has no opportunity to issue the accessed EC register
address.
      b. Maybe OS has no opportunity to read the returned data from EC
controller
In such case it is not appropriate that it is still regarded as timeout.

Maybe we should use the several phases to issue the EC transaction. And
In every phase OS should check whether the EC status is what OS expected
in the predefined time. If in some phase EC status is not what OS
expected, it can be regarded as timeout.
For example: EC Read transaction:
      a. Issuing the read command. (Write the 0x80 to EC Cmd port)
      b. Checking whether the input buffer is empty(IBF bit) and write
the accessed address to EC Data port.
      c. Checking wheter the data is already(OBF bit) and read the
returned data from EC controller

Based on the above analysis IMO the flow of EC transaction is quite
reasonable. Of course what should be improved is how to make it
reasonable when waiting whether EC status is what OS expected.

Please check whether the attached is reasonable.

Of course in the attached patch there is no detect mechanism of EC GPE
interrupt storm.


[-- Attachment #2: ec_work_mode.patch --]
[-- Type: text/x-patch, Size: 6374 bytes --]

Subject: ACPI: Unify the EC work modes of polling and GPE interrupt
>From : Zhao Yakui <yakui.zhao@intel.com>

The flow of EC transaction is still used. But the function of acpi_ec_wait
is changed. At the same time it is unnecessary to switch EC work mode.

While OS is waiting whether the EC status is what OS expected, it will work in
interrupt-driven mode if there is EC GPE confirmation. Otherwise it will still
work in polling mode. And there is no need to switch the work mode.

At the same time the acpi_ec_gpe_handler is also simplified. Only two tasks
are reserved. One is to process the EC notification event. Another is to 
wake up the process in ec waitqueue.

Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
---
 drivers/acpi/ec.c |   80 ++++++++++++++----------------------------------------
 1 file changed, 22 insertions(+), 58 deletions(-)

Index: linux-2.6/drivers/acpi/ec.c
===================================================================
--- linux-2.6.orig/drivers/acpi/ec.c	2008-09-11 09:54:51.000000000 +0800
+++ linux-2.6/drivers/acpi/ec.c	2008-09-11 10:09:46.000000000 +0800
@@ -167,8 +167,6 @@
 
 static inline int acpi_ec_check_status(struct acpi_ec *ec, enum ec_event event)
 {
-	if (test_bit(EC_FLAGS_WAIT_GPE, &ec->flags))
-		return 0;
 	if (event == ACPI_EC_EVENT_OBF_1) {
 		if (acpi_ec_read_status(ec) & ACPI_EC_FLAG_OBF)
 			return 1;
@@ -197,33 +195,28 @@
 
 static int acpi_ec_wait(struct acpi_ec *ec, enum ec_event event, int force_poll)
 {
+	unsigned long delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY);
 	atomic_set(&ec->irq_count, 0);
-	if (likely(test_bit(EC_FLAGS_GPE_MODE, &ec->flags)) &&
-	    likely(!force_poll)) {
-		if (wait_event_timeout(ec->wait, acpi_ec_check_status(ec, event),
-				       msecs_to_jiffies(ACPI_EC_DELAY)))
-			return 0;
-		clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
-		if (acpi_ec_check_status(ec, event)) {
-			/* missing GPEs, switch back to poll mode */
-			if (printk_ratelimit())
-				pr_info(PREFIX "missing confirmations, "
-						"switch off interrupt mode.\n");
-			ec_switch_to_poll_mode(ec);
-			ec_schedule_ec_poll(ec);
-			return 0;
-		}
-	} else {
-		unsigned long delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY);
-		clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
-		while (time_before(jiffies, delay)) {
-			if (acpi_ec_check_status(ec, event))
-				return 0;
-			msleep(1);
-		}
-		if (acpi_ec_check_status(ec,event))
+	clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
+	while (time_before(jiffies, delay)) {
+		/*
+		 * Checking whether the EC staus is what OS expected by calling
+		 * wait_event_timeout. This process can be waked up by two
+		 * methods: One: timeout;  Two: EC GPE interrupt.
+		 * If it returns not zero, it means that it is waked up by EC
+		 * GPE interrupt and EC status is already what OS expected.
+		 * If it returns zero, it will continue to check the EC status.
+		 * That is to say, if there is EC GPE confirmation, it will
+		 * work in interrupt-driven mode. Otherwise it will work in
+		 * polling mode.
+		 */
+		if (wait_event_timeout(ec->wait,
+			acpi_ec_check_status(ec, event), 1))
 			return 0;
 	}
+	if (acpi_ec_check_status(ec, event))
+		return 0;
+
 	pr_err(PREFIX "acpi_ec_wait timeout, status = 0x%2.2x, event = %s\n",
 		acpi_ec_read_status(ec),
 		(event == ACPI_EC_EVENT_OBF_1) ? "\"b0=1\"" : "\"b1=0\"");
@@ -236,7 +229,6 @@
 					int force_poll)
 {
 	int result = 0;
-	set_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
 	pr_debug(PREFIX "transaction start\n");
 	acpi_ec_write_cmd(ec, command);
 	for (; wdata_len > 0; --wdata_len) {
@@ -246,7 +238,6 @@
 			       "write_cmd timeout, command = %d\n", command);
 			goto end;
 		}
-		set_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
 		acpi_ec_write_data(ec, *(wdata++));
 	}
 
@@ -265,9 +256,6 @@
 			pr_err(PREFIX "read timeout, command = %d\n", command);
 			goto end;
 		}
-		/* Don't expect GPE after last read */
-		if (rdata_len > 1)
-			set_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
 		*(rdata++) = acpi_ec_read_data(ec);
 	}
       end:
@@ -535,32 +523,15 @@
 	u8 state = acpi_ec_read_status(ec);
 
 	pr_debug(PREFIX "~~~> interrupt\n");
-	atomic_inc(&ec->irq_count);
-	if (atomic_read(&ec->irq_count) > 5) {
-		pr_err(PREFIX "GPE storm detected, disabling EC GPE\n");
-		ec_switch_to_poll_mode(ec);
-		goto end;
-	}
-	clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
-	if (test_bit(EC_FLAGS_GPE_MODE, &ec->flags))
-		wake_up(&ec->wait);
 
 	if (state & ACPI_EC_FLAG_SCI) {
 		if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags))
 			status = acpi_os_execute(OSL_EC_BURST_HANDLER,
 				acpi_ec_gpe_query, ec);
-	} else if (!test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
-		   !test_bit(EC_FLAGS_NO_GPE, &ec->flags) &&
-		   in_interrupt()) {
-		/* this is non-query, must be confirmation */
-		if (printk_ratelimit())
-			pr_info(PREFIX "non-query interrupt received,"
-				" switching to interrupt mode\n");
-		set_bit(EC_FLAGS_GPE_MODE, &ec->flags);
-		clear_bit(EC_FLAGS_RESCHEDULE_POLL, &ec->flags);
 	}
-end:
-	ec_schedule_ec_poll(ec);
+
+	if ((state & ACPI_EC_FLAG_OBF) || !(state & ACPI_EC_FLAG_IBF))
+		wake_up(&ec->wait);
 	return ACPI_SUCCESS(status) ?
 	    ACPI_INTERRUPT_HANDLED : ACPI_INTERRUPT_NOT_HANDLED;
 }
@@ -761,7 +732,6 @@
 
 static void ec_remove_handlers(struct acpi_ec *ec)
 {
-	ec_poll_stop(ec);
 	if (ACPI_FAILURE(acpi_remove_address_space_handler(ec->handle,
 				ACPI_ADR_SPACE_EC, &acpi_ec_space_handler)))
 		pr_err(PREFIX "failed to remove space handler\n");
@@ -809,8 +779,6 @@
 	acpi_ec_add_fs(device);
 	pr_info(PREFIX "GPE = 0x%lx, I/O: command/status = 0x%lx, data = 0x%lx\n",
 			  ec->gpe, ec->command_addr, ec->data_addr);
-	pr_info(PREFIX "driver started in %s mode\n",
-		(test_bit(EC_FLAGS_GPE_MODE, &ec->flags))?"interrupt":"poll");
 	return 0;
 }
 
@@ -904,7 +872,6 @@
 
 	/* EC is fully operational, allow queries */
 	clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
-	ec_schedule_ec_poll(ec);
 	return ret;
 }
 
@@ -1000,8 +967,6 @@
 {
 	struct acpi_ec *ec = acpi_driver_data(device);
 	/* Stop using GPE */
-	set_bit(EC_FLAGS_NO_GPE, &ec->flags);
-	clear_bit(EC_FLAGS_GPE_MODE, &ec->flags);
 	acpi_disable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
 	return 0;
 }
@@ -1010,7 +975,6 @@
 {
 	struct acpi_ec *ec = acpi_driver_data(device);
 	/* Enable use of GPE back */
-	clear_bit(EC_FLAGS_NO_GPE, &ec->flags);
 	acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
 	return 0;
 }

Re: the errors about two EC patches

[Alexey Starikovskiy]

Zhao Yakui wrote:
> On Wed, 2008-09-10 at 16:37 +0400, Alexey Starikovskiy wrote:
> thanks for the so quick response. 
>
> In the update patch it seems that spin_lock is used in the function of
> ec_read_command/acpi_ec_write_cmd.
>    IMO this is not reasonable. 
>    Maybe the spin_lock is already called before OS evaluates some ACPI
> object, in which the EC internal register will be accessed. In such case
> there will give the following warning message.
>       >2 locks held by swapper/1
>
>   
I've just consulted with Greg KH about the non-sense you tell about.
spinlock will always be acquired while already holding a mutex, and
released while still holding mutex. This is perfectly fine use of mutexes
and spinlocks. You should never use reverse order, obviously.
>>  
>> Ok. Fixed, now -ETIME is returned in this case.
>>     
> Yes. The code flow seems logic and reasonable after the -Etime is added.
> If the timeout happens when EC transaction is not finished, it is
> regarded as timeout.
>
> But we should investigate why EC transaction is not finished when
> timeout happens. Is it related with EC hardware or EC driver?
> If EC can't update the status register in time after issuing
> command/address, it is reasonable that it is regarded as timeout. If
> not, maybe it is related with the EC driver.
>
>   
We should investigate that only if we receive such timeouts.
This is clearly only needed for debug purposes, as no user
of EC driver is interested in why exactly it did not made a
transaction, only that it failed temporarily (as opposed to
-EFAIL or -ENODEV), which indicate permanent failure.
Thus, there is no reason to make flowchart (your favorite)
any more complex.
> In fact when timeout happens in the above cases, OS has no opportunity
> to issue the EC command set completely.
>     For example: Read Command: 
>       a. Maybe OS has no opportunity to issue the accessed EC register
> address.
>       b. Maybe OS has no opportunity to read the returned data from EC
> controller
> In such case it is not appropriate that it is still regarded as timeout.
>
> Maybe we should use the several phases to issue the EC transaction. And
> In every phase OS should check whether the EC status is what OS expected
> in the predefined time. If in some phase EC status is not what OS
> expected, it can be regarded as timeout.
> For example: EC Read transaction:
>       a. Issuing the read command. (Write the 0x80 to EC Cmd port)
>       b. Checking whether the input buffer is empty(IBF bit) and write
> the accessed address to EC Data port.
>       c. Checking wheter the data is already(OBF bit) and read the
> returned data from EC controller
>
> Based on the above analysis IMO the flow of EC transaction is quite
> reasonable. Of course what should be improved is how to make it
> reasonable when waiting whether EC status is what OS expected.
>
> Please check whether the attached is reasonable.
>
> Of course in the attached patch there is no detect mechanism of EC GPE
> interrupt storm.
>
>   
Regards,
Alex.

Re: the errors about two EC patches

[Zhao Yakui]

On Thu, 2008-09-11 at 12:14 +0400, Alexey Starikovskiy wrote:
> >
> > But we should investigate why EC transaction is not finished when
> > timeout happens. Is it related with EC hardware or EC driver?
> > If EC can't update the status register in time after issuing
> > command/address, it is reasonable that it is regarded as timeout. If
> > not, maybe it is related with the EC driver.
> >
> >   
> We should investigate that only if we receive such timeouts.
> This is clearly only needed for debug purposes, as no user
> of EC driver is interested in why exactly it did not made a
> transaction, only that it failed temporarily (as opposed to
> -EFAIL or -ENODEV), which indicate permanent failure.
> Thus, there is no reason to make flowchart (your favorite)
> any more complex.
    If the timeout is caused by that EC controller can't return response
in time, it is reasonable that it can be regarded as -ETIME. It
indicates that EC can't work well. 
    But if the timeout is caused by that OS has no opportunity to issue
the EC command set completely, IMO it is not reasonable that it is
regarded as -ETIME. If such an error happens while turning on the FAN
device through EC, maybe the catastrophic accident will happen.

   Maybe it is faster to do transaction directly from interrupt context.
But in some case EC transaction can't be processed correctly although EC
controller works well.
   IMO it is not very stable that OS does transaction directly from
interrupt context.
   
   
> > Based on the above analysis IMO the flow of EC transaction is quite
> > reasonable. Of course what should be improved is how to make it
> > reasonable when waiting whether EC status is what OS expected.
> >
> > Please check whether the attached is reasonable.
> >
> > Of course in the attached patch there is no detect mechanism of EC GPE
> > interrupt storm.
> >
> >   
> Regards,
> Alex.

the errors about the EC patch from Alexey

[Zhao Yakui]

Hi, All
    A patch is attached in kernel bugzilla(bug number is  11549), which is to
fix the issue of EC GPE storm.(This patch is from Alexey Starikovskiy)
    The patch is attached in http://bugzilla.kernel.org/show_bug.cgi?id=11549#c1
    Although the bug can be fixed by the patch, there exists the following error about
the synchronization:
     Synchronization about the content of transaction_data(ec->t): 
       The function of gpe_transaction is reenterable. It will be called in 
the two different routines. One is called in ec_gpe_handler(Interrupt context) and
another is called in the function of ec_poll(This is called in acpi_ec_transaction_unlocked).
       The content of transaction_data will be changed/read in the function of gpe_transaction.
             >if (t->wlen > 0) {
              >  if ((status & ACPI_EC_FLAG_IBF) == 0) {
               >         acpi_ec_write_data(ec, *(t->wdata++));
               >         --t->wlen;
       But unfortunately there is no synchronization between the two routines.
       IMO this is incorrect.   
      As there are no protection and synchronization about the content of ec->t, maybe the ec->t 
is assigned to zero while OS is accessing the content of ec->t. In such case maybe the system 
will be crashed. Even when there is no crash, it is still unreasonable as OS will try to access the 
invalid memory.

     At the same time the following source code looks so ugly.
     The address of local variable is assigned to the global pointer variable. Although it won't
break the system, it looks very ugly. 
      >struct transaction_data t = {.wdata = wdata, .rdata = rdata,
                                     .wlen = wdata_len, .rlen = rdata_len};
      >ec->t = &t;
      And the ec->t will be accessed in the function of acpi_ec_gpe_handler(Interrupt context).
      
     
Best regards.
    Yakui