Re: [PATCH] ACPI: EC: do transaction from interrupt context

Re: [PATCH] ACPI: EC: do transaction from interrupt context

[Sitsofe Wheeler]

> > Zhao Yakui wrote:
> > >     I think that the problem on the asus-EEEPC can't be resolved really
> > > by the Alexey's patch. IMO it is only lucky.
> Do you have opportunity to try whether the Asus Eee PC hotkeys on
> windows are very fast?

Not personally. I'd need a USB DVD drive and a legal copy of Windows for that... I guess you might be able to ask in #eeepc on irc.freenode.net though...


      

[PATCH] ACPI: EC: do transaction from interrupt context

[Alexey Starikovskiy]

It is easier and faster to do transaction directly from interrupt context
rather than waking control thread.
Also, cleaner GPE storm avoidance is implemented.
References: 	http://bugzilla.kernel.org/show_bug.cgi?id=9998
		http://bugzilla.kernel.org/show_bug.cgi?id=10724
		http://bugzilla.kernel.org/show_bug.cgi?id=10919
		http://bugzilla.kernel.org/show_bug.cgi?id=11309
		http://bugzilla.kernel.org/show_bug.cgi?id=11549
Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
---

 drivers/acpi/ec.c |  309 ++++++++++++++++++++++++++---------------------------
 1 files changed, 149 insertions(+), 160 deletions(-)


diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 13593f9..7f0d81c 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,21 @@ 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_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 */
+	EC_FLAGS_HANDLERS_INSTALLED	/* Handlers for GPE and
+					 * OpReg are installed */
 };
 
 /* If we find an EC via the ECDT, we need to keep a ptr to its context */
@@ -95,6 +95,14 @@ struct acpi_ec_query_handler {
 	u8 query_bit;
 };
 
+struct transaction_data {
+	const u8 *wdata;
+	u8 *rdata;
+	unsigned short irq_count;
+	u8 wlen;
+	u8 rlen;
+};
+
 static struct acpi_ec {
 	acpi_handle handle;
 	unsigned long gpe;
@@ -105,9 +113,8 @@ static struct acpi_ec {
 	struct mutex lock;
 	wait_queue_head_t wait;
 	struct list_head list;
-	struct delayed_work work;
-	atomic_t irq_count;
-	u8 handlers_installed;
+	struct transaction_data *t;
+	spinlock_t t_lock;
 } *boot_ec, *first_ec;
 
 /* 
@@ -150,7 +157,7 @@ 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)
@@ -165,68 +172,79 @@ 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;
-	}
-
-	return 0;
+	unsigned long flags;
+	int ret = 0;
+	spin_lock_irqsave(&ec->t_lock, flags);
+	if (!ec->t || (!ec->t->wlen && !ec->t->rlen))
+		ret = 1;
+	spin_unlock_irqrestore(&ec->t_lock, flags);
+	return ret;
 }
 
-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));
+	unsigned long flags;
+	spin_lock_irqsave(&ec->t_lock, flags);
+	if (!ec->t)
+		goto unlock;
+	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 */
+			++ec->t->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 */
+			++ec->t->irq_count;
+	}
+unlock:
+	spin_unlock_irqrestore(&ec->t_lock, flags);
 }
 
-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))
+	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);
+	}
+	return 0;
+}
+
+static int ec_poll(struct acpi_ec *ec)
+{
+	unsigned long delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY);
+	msleep(1);
+	while (time_before(jiffies, delay)) {
+		gpe_transaction(ec, acpi_ec_read_status(ec));
+		msleep(1);
+		if (ec_transaction_done(ec))
 			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\"");
 	return -ETIME;
 }
 
@@ -235,45 +253,51 @@ 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 tmp;
+	struct transaction_data t = {.wdata = wdata, .rdata = rdata,
+				     .wlen = wdata_len, .rlen = rdata_len,
+				     .irq_count = 0};
+	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)) {
+		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)
+	/* start transaction */
+	spin_lock_irqsave(&ec->t_lock, tmp);
+	/* following two actions should be kept atomic */
+	ec->t = &t;
+	acpi_ec_write_cmd(ec, command);
+	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;
-		}
-		/* 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:
+	spin_unlock_irqrestore(&ec->t_lock, tmp);
+	/* 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))
+		ret = ec_poll(ec);
 	pr_debug(PREFIX "transaction end\n");
-	return result;
+	spin_lock_irqsave(&ec->t_lock, tmp);
+	ec->t = NULL;
+	spin_unlock_irqrestore(&ec->t_lock, tmp);
+	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) &&
+		   t.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 +307,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 +350,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)
@@ -513,46 +533,26 @@ static void acpi_ec_gpe_query(void *ec_cxt)
 
 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 status;
 
 	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))
+	status = acpi_ec_read_status(ec);
+
+	gpe_transaction(ec, status);
+	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) &&
-		   !test_bit(EC_FLAGS_NO_GPE, &ec->flags) &&
-		   in_interrupt()) {
+	ec_check_sci(ec, status);
+	if (!test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
+	    !test_bit(EC_FLAGS_NO_GPE, &ec->flags)) {
 		/* 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);
-	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);
+	return ACPI_INTERRUPT_HANDLED;
 }
 
 /* --------------------------------------------------------------------------
@@ -696,8 +696,7 @@ 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->t_lock);
 	return ec;
 }
 
@@ -736,22 +735,15 @@ 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");
 	if (ACPI_FAILURE(acpi_remove_gpe_handler(NULL, ec->gpe,
 				&acpi_ec_gpe_handler)))
 		pr_err(PREFIX "failed to remove gpe handler\n");
-	ec->handlers_installed = 0;
+	clear_bit(EC_FLAGS_HANDLERS_INSTALLED, &ec->flags);
 }
 
 static int acpi_ec_add(struct acpi_device *device)
@@ -846,17 +838,15 @@ ec_parse_io_ports(struct acpi_resource *resource, void *context)
 static int ec_install_handlers(struct acpi_ec *ec)
 {
 	acpi_status status;
-	if (ec->handlers_installed)
+	if (test_bit(EC_FLAGS_HANDLERS_INSTALLED, &ec->flags))
 		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,
@@ -866,7 +856,7 @@ static int ec_install_handlers(struct acpi_ec *ec)
 		return -ENODEV;
 	}
 
-	ec->handlers_installed = 1;
+	set_bit(EC_FLAGS_HANDLERS_INSTALLED, &ec->flags);
 	return 0;
 }
 
@@ -887,7 +877,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;
 }
 
@@ -906,7 +895,7 @@ static int acpi_ec_stop(struct acpi_device *device, int type)
 
 int __init acpi_boot_ec_enable(void)
 {
-	if (!boot_ec || boot_ec->handlers_installed)
+	if (!boot_ec || test_bit(EC_FLAGS_HANDLERS_INSTALLED, &boot_ec->flags))
 		return 0;
 	if (!ec_install_handlers(boot_ec)) {
 		first_ec = boot_ec;

Re: [PATCH] ACPI: EC: do transaction from interrupt context

[Sitsofe Wheeler]

Alexey Starikovskiy wrote:
> It is easier and faster to do transaction directly from interrupt context
> rather than waking control thread.
> Also, cleaner GPE storm avoidance is implemented.
> References: 	http://bugzilla.kernel.org/show_bug.cgi?id=9998
> 		http://bugzilla.kernel.org/show_bug.cgi?id=10724
> 		http://bugzilla.kernel.org/show_bug.cgi?id=10919
> 		http://bugzilla.kernel.org/show_bug.cgi?id=11309
> 		http://bugzilla.kernel.org/show_bug.cgi?id=11549
> Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
> ---

This resolves the slow hotkeys (e.g. brightness keys) that I see on my 
EeePC 900.

Tested-by: Sitsofe Wheeler <sitsofe@yahoo.com>

-- 
Sitsofe | http://sucs.org/~sits/

Re: [PATCH] ACPI: EC: do transaction from interrupt context

[Zhao Yakui]

On Thu, 2008-09-25 at 18:52 +0100, Sitsofe Wheeler wrote:
> Alexey Starikovskiy wrote:
> > It is easier and faster to do transaction directly from interrupt context
> > rather than waking control thread.
> > Also, cleaner GPE storm avoidance is implemented.
Hi, Sitsofe
    I think that the problem on the asus-EEEPC can't be resolved really
by the Alexey's patch. IMO it is only lucky.
   In fact the main problem on Asus-EEEPC is related with the broken EC.
Before an EC notification event is processed, another EC notification
event arrives again.
   If EC driver check whether the SCI_EVT bit is set after processing
one EC notification event, the problem will be resolved. 
    http://bugzilla.kernel.org/show_bug.cgi?id=11089
    Alan Jenkins already sent a patch about how to fix the issue on the
Asus-EEEPC.
   
   But if the above patch is merged , maybe it will break some laptops.
On my laptop when issuing the query command, a non-zero query event is
returned but it can't be processed.(There is no corresponding ACPI _Qxx
object). At the same time the SCI_EVT bit won't be cleared. In such case
OS can't exit the function of acpi_ec_query_handler, which causes that
the acpid kernel thread can't work well.
   
   
> References: 	http://bugzilla.kernel.org/show_bug.cgi?id=9998
> > 		http://bugzilla.kernel.org/show_bug.cgi?id=10724
> > 		http://bugzilla.kernel.org/show_bug.cgi?id=10919
> > 		http://bugzilla.kernel.org/show_bug.cgi?id=11309
> > 		http://bugzilla.kernel.org/show_bug.cgi?id=11549
> > Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
> > ---
> 
> This resolves the slow hotkeys (e.g. brightness keys) that I see on my 
> EeePC 900.
> 
> Tested-by: Sitsofe Wheeler <sitsofe@yahoo.com>
> 

Re: [PATCH] ACPI: EC: do transaction from interrupt context

[Sitsofe Wheeler]

Zhao Yakui wrote:
>     I think that the problem on the asus-EEEPC can't be resolved really
> by the Alexey's patch. IMO it is only lucky.

How lucky? It really does go from excruciatingly slow to quite fast on 
that laptop. What will happen should my luck fail? What are the odds of 
my luck failing?

>    In fact the main problem on Asus-EEEPC is related with the broken EC.
> Before an EC notification event is processed, another EC notification
> event arrives again.
>    If EC driver check whether the SCI_EVT bit is set after processing
> one EC notification event, the problem will be resolved. 
>     http://bugzilla.kernel.org/show_bug.cgi?id=11089
>     Alan Jenkins already sent a patch about how to fix the issue on the
> Asus-EEEPC.

I'll cc Alan and see what he makes of this patch.

>    But if the above patch is merged , maybe it will break some laptops.

Fair enough but can those people with such laptops test the patch too 
and report back? Can you say in what form the breakages will take? From 
what you are saying it sounds like this patch shouldn't have had any 
affect for me but it does. What happens when they try it? What happens 
when you try it?

-- 
Sitsofe | http://sucs.org/~sits/

Re: [PATCH] ACPI: EC: do transaction from interrupt context

[Alexey Starikovskiy]

Hi Sitsofe,

Alan already tested my patch and it works for him.

Regards,
Alex.

Sitsofe Wheeler wrote:
> Zhao Yakui wrote:
>>     I think that the problem on the asus-EEEPC can't be resolved really
>> by the Alexey's patch. IMO it is only lucky.
> 
> How lucky? It really does go from excruciatingly slow to quite fast on 
> that laptop. What will happen should my luck fail? What are the odds of 
> my luck failing?
> 
>>    In fact the main problem on Asus-EEEPC is related with the broken EC.
>> Before an EC notification event is processed, another EC notification
>> event arrives again.
>>    If EC driver check whether the SCI_EVT bit is set after processing
>> one EC notification event, the problem will be resolved.     
>> http://bugzilla.kernel.org/show_bug.cgi?id=11089
>>     Alan Jenkins already sent a patch about how to fix the issue on the
>> Asus-EEEPC.
> 
> I'll cc Alan and see what he makes of this patch.
> 
>>    But if the above patch is merged , maybe it will break some laptops.
> 
> Fair enough but can those people with such laptops test the patch too 
> and report back? Can you say in what form the breakages will take? From 
> what you are saying it sounds like this patch shouldn't have had any 
> affect for me but it does. What happens when they try it? What happens 
> when you try it?
> 

Re: [PATCH] ACPI: EC: do transaction from interrupt context

[Alan Jenkins]

Alexey Starikovskiy wrote:
> Sitsofe Wheeler wrote:
>> Zhao Yakui wrote:
>>>     I think that the problem on the asus-EEEPC can't be resolved really
>>> by the Alexey's patch. IMO it is only lucky.
>>
>> How lucky? It really does go from excruciatingly slow to quite fast
>> on that laptop. What will happen should my luck fail? What are the
>> odds of my luck failing?
>>
>>>    In fact the main problem on Asus-EEEPC is related with the broken
>>> EC.
>>> Before an EC notification event is processed, another EC notification
>>> event arrives again.
>>>    If EC driver check whether the SCI_EVT bit is set after processing
>>> one EC notification event, the problem will be resolved.    
>>> http://bugzilla.kernel.org/show_bug.cgi?id=11089
>>>     Alan Jenkins already sent a patch about how to fix the issue on the
>>> Asus-EEEPC.

I submitted alternative patches in the past, but they were not up to
scratch.  The first patch stopped the EeePC dropping events, but left
them arriving in bursts every 0.5 seconds - which is still a UI
regression; it looks even more weird when you hold down a brightness
key.  My other patches fixed the EeePC, but caused severe regressions on
other hardware.

So I have stopped pushing any patches of my own.  In the interest of
removing this regression as soon as reasonably possible, I will not
object to changes that actually work.

>> I'll cc Alan and see what he makes of this patch.
>>

> Alan already tested my patch and it works for him.

Yup.  I agree with Zhao, in that it is not crystal clear why this new
patch helps.

The new patch includes a fallback which requires polling-driven
transactions.  That's a different type of polling fallback to the
previous one.  However, from the experience of testing a range of
patches, I believe the new polling fallback would also drop events.  At
face value, the trigger for the polling fallback is the same - 20
"false" (surplus to requirements) interrupts, so it shouldn't work :-).

I can think of two possibilities

1. Doing more in the interrupt handler leaves the interrupt disabled for
longer, hiding some of the false interrupts (which arrive in bursts).

2. Maybe when the device is serviced (e.g. a data byte is read), it
stops the current burst of false interrupts.

>>>    But if the above patch is merged , maybe it will break some laptops.
>>
>> Fair enough but can those people with such laptops test the patch too
>> and report back? Can you say in what form the breakages will take?
>> From what you are saying it sounds like this patch shouldn't have had
>> any affect for me but it does. What happens when they try it? What
>> happens when you try it?
>>
>

Alan

Re: [PATCH] ACPI: EC: do transaction from interrupt context

[Alexey Starikovskiy]

Alan,

Could you please uncomment DEBUG and send dmesg after application of the patch.
I put the "storm detected" message under debug, so it is not visible in normal situation.
I think, that interrupt storm is still detected on your machine, and workaround works.

Regards,
Alex.

Alan Jenkins wrote:
> Alexey Starikovskiy wrote:
>> Sitsofe Wheeler wrote:
>>> Zhao Yakui wrote:
>>>>     I think that the problem on the asus-EEEPC can't be resolved really
>>>> by the Alexey's patch. IMO it is only lucky.
>>> How lucky? It really does go from excruciatingly slow to quite fast
>>> on that laptop. What will happen should my luck fail? What are the
>>> odds of my luck failing?
>>>
>>>>    In fact the main problem on Asus-EEEPC is related with the broken
>>>> EC.
>>>> Before an EC notification event is processed, another EC notification
>>>> event arrives again.
>>>>    If EC driver check whether the SCI_EVT bit is set after processing
>>>> one EC notification event, the problem will be resolved.    
>>>> http://bugzilla.kernel.org/show_bug.cgi?id=11089
>>>>     Alan Jenkins already sent a patch about how to fix the issue on the
>>>> Asus-EEEPC.
> 
> I submitted alternative patches in the past, but they were not up to
> scratch.  The first patch stopped the EeePC dropping events, but left
> them arriving in bursts every 0.5 seconds - which is still a UI
> regression; it looks even more weird when you hold down a brightness
> key.  My other patches fixed the EeePC, but caused severe regressions on
> other hardware.
> 
> So I have stopped pushing any patches of my own.  In the interest of
> removing this regression as soon as reasonably possible, I will not
> object to changes that actually work.
> 
>>> I'll cc Alan and see what he makes of this patch.
>>>
> 
>> Alan already tested my patch and it works for him.

> 
> Yup.  I agree with Zhao, in that it is not crystal clear why this new
> patch helps.
> 
> The new patch includes a fallback which requires polling-driven
> transactions.  That's a different type of polling fallback to the
> previous one.  However, from the experience of testing a range of
> patches, I believe the new polling fallback would also drop events.  At
> face value, the trigger for the polling fallback is the same - 20
> "false" (surplus to requirements) interrupts, so it shouldn't work :-).
> 
> I can think of two possibilities
> 
> 1. Doing more in the interrupt handler leaves the interrupt disabled for
> longer, hiding some of the false interrupts (which arrive in bursts).
> 
> 2. Maybe when the device is serviced (e.g. a data byte is read), it
> stops the current burst of false interrupts.
> 
>>>>    But if the above patch is merged , maybe it will break some laptops.
>>> Fair enough but can those people with such laptops test the patch too
>>> and report back? Can you say in what form the breakages will take?
>>> From what you are saying it sounds like this patch shouldn't have had
>>> any affect for me but it does. What happens when they try it? What
>>> happens when you try it?
>>>
> 
> Alan

Re: [PATCH] ACPI: EC: do transaction from interrupt context

[Alan Jenkins]

Alexey Starikovskiy wrote:
> Alan,
>
> Could you please uncomment DEBUG and send dmesg after application of
> the patch.
> I put the "storm detected" message under debug, so it is not visible
> in normal situation.
> I think, that interrupt storm is still detected on your machine, and
> workaround works.
>
> Regards,
> Alex.
>
Nope.  IIRC, the first version of this patch I tested uncommented DEBUG
anyway.

"dmesg|grep -i storm" doesn't show anything.

Since the entire boot log weighs 100k, I'm sending it to Alex privately.

Alan

Re: [PATCH] ACPI: EC: do transaction from interrupt context

[Zhao Yakui]

On Fri, 2008-09-26 at 06:42 +0100, Sitsofe Wheeler wrote:
> Zhao Yakui wrote:
> >     I think that the problem on the asus-EEEPC can't be resolved really
> > by the Alexey's patch. IMO it is only lucky.
Do you have opportunity to try whether the Asus Eee PC hotkeys on
windows are very fast?
> How lucky? It really does go from excruciatingly slow to quite fast on 
> that laptop. What will happen should my luck fail? What are the odds of 
> my luck failing?
> 
> >    In fact the main problem on Asus-EEEPC is related with the broken EC.
> > Before an EC notification event is processed, another EC notification
> > event arrives again.
> >    If EC driver check whether the SCI_EVT bit is set after processing
> > one EC notification event, the problem will be resolved. 
> >     http://bugzilla.kernel.org/show_bug.cgi?id=11089
> >     Alan Jenkins already sent a patch about how to fix the issue on the
> > Asus-EEEPC.
> 
> I'll cc Alan and see what he makes of this patch.
> 
> >    But if the above patch is merged , maybe it will break some laptops.
> 
> Fair enough but can those people with such laptops test the patch too 
> and report back? Can you say in what form the breakages will take? From 
> what you are saying it sounds like this patch shouldn't have had any 
> affect for me but it does. What happens when they try it? What happens 
> when you try it?
> 

Re: [PATCH] ACPI: EC: do transaction from interrupt context

[Rafael J. Wysocki]

On Friday, 26 of September 2008, Zhao Yakui wrote:
> On Thu, 2008-09-25 at 18:52 +0100, Sitsofe Wheeler wrote:
> > Alexey Starikovskiy wrote:
> > > It is easier and faster to do transaction directly from interrupt context
> > > rather than waking control thread.
> > > Also, cleaner GPE storm avoidance is implemented.
> Hi, Sitsofe
>     I think that the problem on the asus-EEEPC can't be resolved really
> by the Alexey's patch. IMO it is only lucky.
>    In fact the main problem on Asus-EEEPC is related with the broken EC.
> Before an EC notification event is processed, another EC notification
> event arrives again.
>    If EC driver check whether the SCI_EVT bit is set after processing
> one EC notification event, the problem will be resolved. 
>     http://bugzilla.kernel.org/show_bug.cgi?id=11089
>     Alan Jenkins already sent a patch about how to fix the issue on the
> Asus-EEEPC.
>    
>    But if the above patch is merged , maybe it will break some laptops.
> On my laptop when issuing the query command, a non-zero query event is
> returned but it can't be processed.(There is no corresponding ACPI _Qxx
> object). At the same time the SCI_EVT bit won't be cleared. In such case
> OS can't exit the function of acpi_ec_query_handler, which causes that
> the acpid kernel thread can't work well.

So does the patch break your box as a result?

Rafael

RE: [PATCH] ACPI: EC: do transaction from interrupt context

[Zhao, Yakui]

Alan gives a suggestion that after processing the EC notification event, EC driver
should continue to issue the query command and process the EC notification event until the query command returns zero.
But this suggestion will break my laptop. On my laptop when issuing the query command, a non-zero query event is  returned but it can't be processed.(There is no corresponding ACPI _Qxx object). At the same time the SCI_EVT bit won't be cleared. In such case OS can't exit the function of acpi_ec_query_handler, which causes that the acpid kernel thread can't work well.

In fact according to my analysis IMO Alexey's patch will break my laptop. But now the laptop is not in my hand and I can't do the test. After I own the laptop again, I will do the test and attach the corresponding info.

At the same time there are two generic issues in this patch:
   a. spin_lock is overkill. When 2000 EC I/O read/write are executed, the CPU interrupt will be disabled for about 1.5ms. This will affect the normal laptops.
   b. the local variable that resides in process stack space is used in interrupt-context. 

Best regards.

-----Original Message-----
From: Rafael J. Wysocki [mailto:rjw@sisk.pl] 
Sent: 2008年9月26日 20:34
To: Zhao, Yakui
Cc: Sitsofe Wheeler; Alexey Starikovskiy; LenBrown; Linux-acpi@vger.kernel.org
Subject: Re: [PATCH] ACPI: EC: do transaction from interrupt context

On Friday, 26 of September 2008, Zhao Yakui wrote:
> On Thu, 2008-09-25 at 18:52 +0100, Sitsofe Wheeler wrote:
> > Alexey Starikovskiy wrote:
> > > It is easier and faster to do transaction directly from interrupt context
> > > rather than waking control thread.
> > > Also, cleaner GPE storm avoidance is implemented.
> Hi, Sitsofe
>     I think that the problem on the asus-EEEPC can't be resolved really
> by the Alexey's patch. IMO it is only lucky.
>    In fact the main problem on Asus-EEEPC is related with the broken EC.
> Before an EC notification event is processed, another EC notification
> event arrives again.
>    If EC driver check whether the SCI_EVT bit is set after processing
> one EC notification event, the problem will be resolved. 
>     http://bugzilla.kernel.org/show_bug.cgi?id=11089
>     Alan Jenkins already sent a patch about how to fix the issue on the
> Asus-EEEPC.
>    
>    But if the above patch is merged , maybe it will break some laptops.
> On my laptop when issuing the query command, a non-zero query event is
> returned but it can't be processed.(There is no corresponding ACPI _Qxx
> object). At the same time the SCI_EVT bit won't be cleared. In such case
> OS can't exit the function of acpi_ec_query_handler, which causes that
> the acpid kernel thread can't work well.

So does the patch break your box as a result?

Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ACPI: EC: do transaction from interrupt context

[Rafael J. Wysocki]

On Friday, 26 of September 2008, Zhao, Yakui wrote:
> Alan gives a suggestion that after processing the EC notification event, EC driver
> should continue to issue the query command and process the EC notification
> event until the query command returns zero. 
> But this suggestion will break my laptop. On my laptop when issuing the query
> command, a non-zero query event is  returned but it can't be processed.(There
> is no corresponding ACPI _Qxx object). At the same time the SCI_EVT bit won't
> be cleared. In such case OS can't exit the function of acpi_ec_query_handler,
> which causes that the acpid kernel thread can't work well.

Well, perhaps we should exit acpi_ec_query_handler() if SCI_EVT is clear _or_
the processing of an event fails due to the lack of a _Qxx object?  Alex?

> In fact according to my analysis IMO Alexey's patch will break my laptop.

That obviously would be a regression.

> But now the laptop is not in my hand and I can't do the test. After I own the
> laptop again, I will do the test and attach the corresponding info.

OK, how much time is it going to take, approximately?

> At the same time there are two generic issues in this patch:
>    a. spin_lock is overkill. When 2000 EC I/O read/write are executed, the
>    CPU interrupt will be disabled for about 1.5ms. This will affect the normal
>    laptops.

OTOH, I think we need _some_ locking in there in order to prevent races from
happening.

>    b. the local variable that resides in process stack space is used in
>    interrupt-context.

Which one?

Rafael

Re: [PATCH] ACPI: EC: do transaction from interrupt context

[Alexey Starikovskiy]

Rafael J. Wysocki wrote:
> On Friday, 26 of September 2008, Zhao, Yakui wrote:
>   
>> Alan gives a suggestion that after processing the EC notification event, EC driver
>> should continue to issue the query command and process the EC notification
>> event until the query command returns zero. 
>> But this suggestion will break my laptop. On my laptop when issuing the query
>> command, a non-zero query event is  returned but it can't be processed.(There
>> is no corresponding ACPI _Qxx object). At the same time the SCI_EVT bit won't
>> be cleared. In such case OS can't exit the function of acpi_ec_query_handler,
>> which causes that the acpid kernel thread can't work well.
>>     
>
> Well, perhaps we should exit acpi_ec_query_handler() if SCI_EVT is clear _or_
> the processing of an event fails due to the lack of a _Qxx object?  Alex?
>
>   
1. There is no such function acpi_ec_query_handler().
2. acpi_ec_gpe_query() does two things:
    a) checks if EC returns non-zero query index. If EC has nothing to
    query -- it return 0 and we stop.
    b) we check index with stored available handlers, and if there is
    such handler, we execute it.
This function is scheduled to run in response to raised SCI_EVT, and
as I know no EC which would clear SCI_EVT by itself (and it's against 
spec),
there is no reason to check if it's raised inside the function.
SCI_EVT is cleared by EC in response to QUERY command issued by driver,
and this is always done in acpi_ec_gpe_query().
We (Alan and me) already found that machines in 9998 would break if you try
to do several QUERY commands for single raise of SCI_EVT in a hope to "clean
the EC query buffer" -- EC would just return _same_ event over and over, 
thus
driver does not try to do this. Please not mix patch from Alan (with 
above query loop)
with my patch(it never had above query loop).
>> In fact according to my analysis IMO Alexey's patch will break my laptop.
>>     
>
> That obviously would be a regression.
>   
I highly suspect quality of such "analysis" based on 2 weeks of 
conversation with
Yakui. Feel free to make your own judgment though...
>   
>> But now the laptop is not in my hand and I can't do the test. After I own the
>> laptop again, I will do the test and attach the corresponding info.
>>     
>
> OK, how much time is it going to take, approximately?
>
>   
>> At the same time there are two generic issues in this patch:
>>    a. spin_lock is overkill. When 2000 EC I/O read/write are executed, the
>>    CPU interrupt will be disabled for about 1.5ms. This will affect the normal
>>    laptops.
>>     
>
> OTOH, I think we need _some_ locking in there in order to prevent races from
> happening.
>
>   
He used 1000 EC I/O per second before, and now it's raised to 2000
just to make a good number. One full read of battery with SBS (most 
irq-intensive)
is in order of 100 GPEs. You need to update your battery state 20 times 
per second
to get this number.
I will not try to doubt 0.7usec of inb/outb operation to EC, but my 
understanding
is that you don't wait while LPC transaction is over, so it is inb/outb 
to south bridge
which needs to be counted.
Irq handler itself will disable interrupts as well, it will read several 
ACPI GPE registers
in order to find GPE bit, which caused interrupt (first delay).
second, even the most dumb GPE EC handler needs to read status register 
of EC
(second delay).
Now, my patch adds either read or write of EC data register to this 
(third delay).

So, in the very worst case of 0.7usec access to EC register (and free 
GPE register access),
 we will disable interrupt for 1.5usec instead of 0.7usec.
Notice, we enable interrupt between each of these 1.5usec intervals.
Notice also, spinlock does not add anything to these timings.
>>    b. the local variable that resides in process stack space is used in
>>    interrupt-context.
>>     
>
> Which one?
>
>   
He refers to the ec->curr (former ec->t).


Regards,
Alex.

Re: [PATCH] ACPI: EC: do transaction from interrupt context

[Rafael J. Wysocki]

On Friday, 26 of September 2008, Alexey Starikovskiy wrote:
> Rafael J. Wysocki wrote:
> > On Friday, 26 of September 2008, Zhao, Yakui wrote:
> >   
> >> Alan gives a suggestion that after processing the EC notification event, EC driver
> >> should continue to issue the query command and process the EC notification
> >> event until the query command returns zero. 
> >> But this suggestion will break my laptop. On my laptop when issuing the query
> >> command, a non-zero query event is  returned but it can't be processed.(There
> >> is no corresponding ACPI _Qxx object). At the same time the SCI_EVT bit won't
> >> be cleared. In such case OS can't exit the function of acpi_ec_query_handler,
> >> which causes that the acpid kernel thread can't work well.
> >>     
> >
> > Well, perhaps we should exit acpi_ec_query_handler() if SCI_EVT is clear _or_
> > the processing of an event fails due to the lack of a _Qxx object?  Alex?
> >
> >   
> 1. There is no such function acpi_ec_query_handler().
> 2. acpi_ec_gpe_query() does two things:
>     a) checks if EC returns non-zero query index. If EC has nothing to
>     query -- it return 0 and we stop.
>     b) we check index with stored available handlers, and if there is
>     such handler, we execute it.
> This function is scheduled to run in response to raised SCI_EVT, and
> as I know no EC which would clear SCI_EVT by itself (and it's against 
> spec),
> there is no reason to check if it's raised inside the function.
> SCI_EVT is cleared by EC in response to QUERY command issued by driver,
> and this is always done in acpi_ec_gpe_query().

OK

> We (Alan and me) already found that machines in 9998 would break if you try
> to do several QUERY commands for single raise of SCI_EVT in a hope to "clean
> the EC query buffer" -- EC would just return _same_ event over and over, 
> thus
> driver does not try to do this. Please not mix patch from Alan (with 
> above query loop)
> with my patch(it never had above query loop).
> >> In fact according to my analysis IMO Alexey's patch will break my laptop.
> >>     
> >
> > That obviously would be a regression.
> >   
> I highly suspect quality of such "analysis" based on 2 weeks of 
> conversation with
> Yakui. Feel free to make your own judgment though...

Well, I think your patch is correct.

However, I'd like it to be tested on the Zhao Yakui's box anyway.

> >> But now the laptop is not in my hand and I can't do the test. After I own the
> >> laptop again, I will do the test and attach the corresponding info.
> >>     
> >
> > OK, how much time is it going to take, approximately?
> >
> >   
> >> At the same time there are two generic issues in this patch:
> >>    a. spin_lock is overkill. When 2000 EC I/O read/write are executed, the
> >>    CPU interrupt will be disabled for about 1.5ms. This will affect the normal
> >>    laptops.
> >>     
> >
> > OTOH, I think we need _some_ locking in there in order to prevent races from
> > happening.
> >
> >   
> He used 1000 EC I/O per second before, and now it's raised to 2000
> just to make a good number. One full read of battery with SBS (most 
> irq-intensive)
> is in order of 100 GPEs. You need to update your battery state 20 times 
> per second
> to get this number.
> I will not try to doubt 0.7usec of inb/outb operation to EC, but my 
> understanding
> is that you don't wait while LPC transaction is over, so it is inb/outb 
> to south bridge
> which needs to be counted.
> Irq handler itself will disable interrupts as well, it will read several 
> ACPI GPE registers
> in order to find GPE bit, which caused interrupt (first delay).
> second, even the most dumb GPE EC handler needs to read status register 
> of EC
> (second delay).
> Now, my patch adds either read or write of EC data register to this 
> (third delay).
> 
> So, in the very worst case of 0.7usec access to EC register (and free 
> GPE register access),
>  we will disable interrupt for 1.5usec instead of 0.7usec.
> Notice, we enable interrupt between each of these 1.5usec intervals.
> Notice also, spinlock does not add anything to these timings.

OK, so the 2000 EC I/O operations per second are unrealistic and there's really
nothing to worry about (in theory).

> >>    b. the local variable that resides in process stack space is used in
> >>    interrupt-context.
> >>     
> >
> > Which one?
> >
> >   
> He refers to the ec->curr (former ec->t).

Hm, I'm not sure what the failing scenario in this case would be.

Thanks,
Rafael

Re: [PATCH] ACPI: EC: do transaction from interrupt context

[Alexey Starikovskiy]

Rafael J. Wysocki wrote:
>> He refers to the ec->curr (former ec->t).
> 
> Hm, I'm not sure what the failing scenario in this case would be.
Last one was the thread of the stack is killed during transaction,
thus stack vanishes and interrupt goes over freed memory.

Re: [PATCH] ACPI: EC: do transaction from interrupt context

[Rafael J. Wysocki]

On Friday, 26 of September 2008, Alexey Starikovskiy wrote:
> Rafael J. Wysocki wrote:
> >> He refers to the ec->curr (former ec->t).
> > 
> > Hm, I'm not sure what the failing scenario in this case would be.
> Last one was the thread of the stack is killed during transaction,
> thus stack vanishes and interrupt goes over freed memory.

This isn't possible IMO.  The thread is uninterruptible while waiting for the
transaction to finish (if I'm not mistaken).

That said, we can still embed 'curr' in 'struct acpi_ec' and do
'ec.curr = t' instead of 'ec->curr = &t' in acpi_ec_transaction_unlocked().
Now, if 'command' is embedded in 'curr', it will be sufficient to do
'if (curr.command)' instead of doing 'if (ec->curr)', wherever applicable.

Re: [PATCH] ACPI: EC: do transaction from interrupt context

[Alexey Starikovskiy]

Rafael J. Wysocki wrote:
> On Friday, 26 of September 2008, Alexey Starikovskiy wrote:
>   
>> Rafael J. Wysocki wrote:
>>     
>>>> He refers to the ec->curr (former ec->t).
>>>>         
>>> Hm, I'm not sure what the failing scenario in this case would be.
>>>       
>> Last one was the thread of the stack is killed during transaction,
>> thus stack vanishes and interrupt goes over freed memory.
>>     
>
> This isn't possible IMO.  The thread is uninterruptible while waiting for the
> transaction to finish (if I'm not mistaken).
>   
Right, I already explained it to Yakui, this is why he only claims 
"ugly" now.
> That said, we can still embed 'curr' in 'struct acpi_ec' and do
> 'ec.curr = t' instead of 'ec->curr = &t' in acpi_ec_transaction_unlocked().
> Now, if 'command' is embedded in 'curr', it will be sufficient to do
> 'if (curr.command)' instead of doing 'if (ec->curr)', wherever applicable.
>   
No, I like current way more. Now it is not possible to use transaction 
data if
acpi_ec_transaction is not on the stack.

Re: [PATCH] ACPI: EC: do transaction from interrupt context

[Rafael J. Wysocki]

On Friday, 26 of September 2008, Alexey Starikovskiy wrote:
> Rafael J. Wysocki wrote:
> > On Friday, 26 of September 2008, Alexey Starikovskiy wrote:
> >   
> >> Rafael J. Wysocki wrote:
> >>     
> >>>> He refers to the ec->curr (former ec->t).
> >>>>         
> >>> Hm, I'm not sure what the failing scenario in this case would be.
> >>>       
> >> Last one was the thread of the stack is killed during transaction,
> >> thus stack vanishes and interrupt goes over freed memory.
> >>     
> >
> > This isn't possible IMO.  The thread is uninterruptible while waiting for the
> > transaction to finish (if I'm not mistaken).
> >   
> Right, I already explained it to Yakui, this is why he only claims 
> "ugly" now.
> > That said, we can still embed 'curr' in 'struct acpi_ec' and do
> > 'ec.curr = t' instead of 'ec->curr = &t' in acpi_ec_transaction_unlocked().
> > Now, if 'command' is embedded in 'curr', it will be sufficient to do
> > 'if (curr.command)' instead of doing 'if (ec->curr)', wherever applicable.
> >   
> No, I like current way more. Now it is not possible to use transaction 
> data if
> acpi_ec_transaction is not on the stack.

OK

Re: [PATCH] ACPI: EC: do transaction from interrupt context

[Zhao Yakui]

On Fri, 2008-09-26 at 16:15 +0200, Rafael J. Wysocki wrote:
> On Friday, 26 of September 2008, Zhao, Yakui wrote:
> > Alan gives a suggestion that after processing the EC notification event, EC driver
> > should continue to issue the query command and process the EC notification
> > event until the query command returns zero. 
> > But this suggestion will break my laptop. On my laptop when issuing the query
> > command, a non-zero query event is  returned but it can't be processed.(There
> > is no corresponding ACPI _Qxx object). At the same time the SCI_EVT bit won't
> > be cleared. In such case OS can't exit the function of acpi_ec_query_handler,
> > which causes that the acpid kernel thread can't work well.
> 
> Well, perhaps we should exit acpi_ec_query_handler() if SCI_EVT is clear _or_
> the processing of an event fails due to the lack of a _Qxx object?  Alex?
> 
> > In fact according to my analysis IMO Alexey's patch will break my laptop.
> 
> That obviously would be a regression.
> 
> > But now the laptop is not in my hand and I can't do the test. After I own the
> > laptop again, I will do the test and attach the corresponding info.
> 
> OK, how much time is it going to take, approximately?
Maybe I can own the laptop again after two days. And I will try to do
the test as early as possible and attach the test result. 
> 
> > At the same time there are two generic issues in this patch:
> >    a. spin_lock is overkill. When 2000 EC I/O read/write are executed, the
> >    CPU interrupt will be disabled for about 1.5ms. This will affect the normal
> >    laptops.
> 
> OTOH, I think we need _some_ locking in there in order to prevent races from
> happening.
Agree with what you said. The locking mechanism is needed to prevent the
races from happening.  
But in current source code the EC transaction is done in control thread.
In such case there is no need to use the spin_lock.(Mutex lock is
already used).
If we add the spin_lock for the broken BIOS/laptops , I agree with this
method and regard it very reasonable. But if this mechanism also affects
the normal laptops,  IMO this is not unreasonable.
> >    b. the local variable that resides in process stack space is used in
> >    interrupt-context.
       The pointer of local variable in the function of
acpi_ec_transaction_unlocked is assigned to ec->t(this is regarded as
the global variable). And ec->t is accessed in interrupt-context.
Although it won't break anything, IMO this looks so ugly.
> Which one?
> 
> Rafael

Re: [PATCH] ACPI: EC: do transaction from interrupt context

[Rafael J. Wysocki]

On Friday, 26 of September 2008, Zhao Yakui wrote:
> On Fri, 2008-09-26 at 16:15 +0200, Rafael J. Wysocki wrote:
> > On Friday, 26 of September 2008, Zhao, Yakui wrote:
> > > Alan gives a suggestion that after processing the EC notification event, EC driver
> > > should continue to issue the query command and process the EC notification
> > > event until the query command returns zero. 
> > > But this suggestion will break my laptop. On my laptop when issuing the query
> > > command, a non-zero query event is  returned but it can't be processed.(There
> > > is no corresponding ACPI _Qxx object). At the same time the SCI_EVT bit won't
> > > be cleared. In such case OS can't exit the function of acpi_ec_query_handler,
> > > which causes that the acpid kernel thread can't work well.
> > 
> > Well, perhaps we should exit acpi_ec_query_handler() if SCI_EVT is clear _or_
> > the processing of an event fails due to the lack of a _Qxx object?  Alex?
> > 
> > > In fact according to my analysis IMO Alexey's patch will break my laptop.
> > 
> > That obviously would be a regression.
> > 
> > > But now the laptop is not in my hand and I can't do the test. After I own the
> > > laptop again, I will do the test and attach the corresponding info.
> > 
> > OK, how much time is it going to take, approximately?
> Maybe I can own the laptop again after two days. And I will try to do
> the test as early as possible and attach the test result. 
> > 
> > > At the same time there are two generic issues in this patch:
> > >    a. spin_lock is overkill. When 2000 EC I/O read/write are executed, the
> > >    CPU interrupt will be disabled for about 1.5ms. This will affect the normal
> > >    laptops.
> > 
> > OTOH, I think we need _some_ locking in there in order to prevent races from
> > happening.
> Agree with what you said. The locking mechanism is needed to prevent the
> races from happening.  
> But in current source code the EC transaction is done in control thread.
> In such case there is no need to use the spin_lock.(Mutex lock is
> already used).

IOW, it's done synchronously.

> If we add the spin_lock for the broken BIOS/laptops , I agree with this
> method and regard it very reasonable. But if this mechanism also affects
> the normal laptops,  IMO this is not unreasonable.

That really depends on how the "normal" laptops are affected and to what
extent.  Also, I'm not sure what the meaning of "normal" is in that case.
Namely, if you define "normal" as "working with our current code well" and
everything else as "broken", then the entire discussion doesn't make sense
from that point on.

The question is whether there are machines that _should_ work correctly and
they don't because _we_ try to handle them in a wrong way.  If the answer is
"yes", then our current code is inadequate and we have to change it, this way
or another.  I personally think that the Alex's patch goes in the right
direction.

> > >    b. the local variable that resides in process stack space is used in
> > >    interrupt-context.
>        The pointer of local variable in the function of
> acpi_ec_transaction_unlocked is assigned to ec->t(this is regarded as
> the global variable). And ec->t is accessed in interrupt-context.
> Although it won't break anything, IMO this looks so ugly.

Well, I can agree with that.  We can embed the 'struct transaction_data'
(or whatever it's going to be called) in 'struct acpi_ec' and copy the values
into it instead of using the pointer (ISTR, this is how it was done in the
first iterations of the Alex's patch and the pointer was my idea).

Thanks,
Rafael

Re: [PATCH] ACPI: EC: do transaction from interrupt context

[Zhao Yakui]

On Fri, 2008-09-26 at 17:39 +0200, Rafael J. Wysocki wrote:

> > > OK, how much time is it going to take, approximately?
> > Maybe I can own the laptop again after two days. And I will try to do
> > the test as early as possible and attach the test result. 
I test the patch on my box. And it won't break the box. I am sorry for
that I give the incorrect judgement before doing the test.
     On my box sometimes the SCI_EVT bit of EC status is cleared in
course of issuing Query command. But after the query command is
finished, the SCI_EVT is always set. (If the SCI_EVT bit is set in the
course of issuing query command, my laptop will be broken.).
> > > > At the same time there are two generic issues in this patch:
> > > >    a. spin_lock is overkill. When 2000 EC I/O read/write are executed, the
> > > >    CPU interrupt will be disabled for about 1.5ms. This will affect the normal
> > > >    laptops.
> > > 
> > > OTOH, I think we need _some_ locking in there in order to prevent races from
> > > happening.
> > Agree with what you said. The locking mechanism is needed to prevent the
> > races from happening.  
> > But in current source code the EC transaction is done in control thread.
> > In such case there is no need to use the spin_lock.(Mutex lock is
> > already used).
> 
> IOW, it's done synchronously.
> 
> > If we add the spin_lock for the broken BIOS/laptops , I agree with this
> > method and regard it very reasonable. But if this mechanism also affects
> > the normal laptops,  IMO this is not unreasonable.
"Normal" laptops means that : EC follows the ACPI spec, there is no EC
GPE storm, EC GPE interrupt can be triggered normally for every EC
transaction, the SCI_EVT bit won't be cleared before issuing query
command.   Of course it can work in the current source code. 
     Of course such laptops can still work well except that CPU
interrupt will be disabled for some extra time after Alexey's patch is
applied .(The some extra time depends on how many EC Inb/out operations
are executed).

In my email the purpose that I point out 2000 EC I/O read/write
operation doesn't indicate that 2000 EC I/O read/write will be executed
per second. What I want to say is that EC I/O read/write is slow
operation. If quite a lot of EC I/O read/write are accessed for some
reasons in some time, the CPU interrupt will be disabled for a long
time.  In such case the normal laptop will be affected by this. (Of
course maybe the affect will be very tiny).
   In fact in kernel source code if the race can be resolved by other
synchronization protection mechanism, the spin_lock had better be
avoided. 
   


> That really depends on how the "normal" laptops are affected and to what
> extent.  Also, I'm not sure what the meaning of "normal" is in that case.
> Namely, if you define "normal" as "working with our current code well" and
> everything else as "broken", then the entire discussion doesn't make sense
> from that point on.
> 
> The question is whether there are machines that _should_ work correctly and
> they don't because _we_ try to handle them in a wrong way.  If the answer is
> "yes", then our current code is inadequate and we have to change it, this way
> or another.  I personally think that the Alex's patch goes in the right
> direction.
> 
> > > >    b. the local variable that resides in process stack space is used in
> > > >    interrupt-context.
> >        The pointer of local variable in the function of
> > acpi_ec_transaction_unlocked is assigned to ec->t(this is regarded as
> > the global variable). And ec->t is accessed in interrupt-context.
> > Although it won't break anything, IMO this looks so ugly.
> 
> Well, I can agree with that.  We can embed the 'struct transaction_data'
> (or whatever it's going to be called) in 'struct acpi_ec' and copy the values
> into it instead of using the pointer (ISTR, this is how it was done in the
> first iterations of the Alex's patch and the pointer was my idea).
> 
> Thanks,
> Rafael

Re: [PATCH] ACPI: EC: do transaction from interrupt context

[Alexey Starikovskiy]

Zhao Yakui wrote:
>      Of course such laptops can still work well except that CPU
> interrupt will be disabled for some extra time after Alexey's patch is
> applied .(The some extra time depends on how many EC Inb/out operations
> are executed).
>
>   
spinlock is released after each I/O, so by your own calculation
it is disabled for 1.5 usec.
You can then multiply this number by lifetime of the universe,
it does not mean anything.

Regards,
Alex.
> In my email the purpose that I point out 2000 EC I/O read/write
> operation doesn't indicate that 2000 EC I/O read/write will be executed
> per second. What I want to say is that EC I/O read/write is slow
> operation. If quite a lot of EC I/O read/write are accessed for some
> reasons in some time, the CPU interrupt will be disabled for a long
> time.  In such case the normal laptop will be affected by this. (Of
> course maybe the affect will be very tiny).
>    In fact in kernel source code if the race can be resolved by other
> synchronization protection mechanism, the spin_lock had better be
> avoided. 
>   
Please explain, I'd like to make poster of it :)

Re: [PATCH] ACPI: EC: do transaction from interrupt context

[Zhao Yakui]

On Sat, 2008-09-27 at 09:37 +0400, Alexey Starikovskiy wrote:
> Zhao Yakui wrote:
> >      Of course such laptops can still work well except that CPU
> > interrupt will be disabled for some extra time after Alexey's patch is
> > applied .(The some extra time depends on how many EC Inb/out operations
> > are executed).
> >
> >   
> spinlock is released after each I/O, so by your own calculation
> it is disabled for 1.5 usec.
Yes. The CPU interrupt is disabled for about 0.7usec while doing every
EC I/O access.(Inb/outb).
This affect is very tiny. But when the time is accumulated for many EC
I/O access, it seems that the CPU will be disabled for longer time.

> You can then multiply this number by lifetime of the universe,
> it does not mean anything.
> 
> Regards,
> Alex.
> > In my email the purpose that I point out 2000 EC I/O read/write
> > operation doesn't indicate that 2000 EC I/O read/write will be executed
> > per second. What I want to say is that EC I/O read/write is slow
> > operation. If quite a lot of EC I/O read/write are accessed for some
> > reasons in some time, the CPU interrupt will be disabled for a long
> > time.  In such case the normal laptop will be affected by this. (Of
> > course maybe the affect will be very tiny).
> >    In fact in kernel source code if the race can be resolved by other
> > synchronization protection mechanism, the spin_lock had better be
> > avoided. 
> >   
> Please explain, I'd like to make poster of it :)
As in the current patch the gpe_transaction is accessed in the two
different routines(Control thread, Interrupt-context), spin_lock is
necessary to prevent the race. 
I agree with what you have done.  

> 

Re: [PATCH] ACPI: EC: do transaction from interrupt context

[Alexey Starikovskiy]

Zhao Yakui wrote:
> I agree with what you have done.  
what a relief!

Re: [PATCH] ACPI: EC: do transaction from interrupt context

[Len Brown]

applied to acpi-test

thanks,
-Len

On Thu, 25 Sep 2008, Alexey Starikovskiy wrote:

> It is easier and faster to do transaction directly from interrupt context
> rather than waking control thread.
> Also, cleaner GPE storm avoidance is implemented.
> References: 	http://bugzilla.kernel.org/show_bug.cgi?id=9998
> 		http://bugzilla.kernel.org/show_bug.cgi?id=10724
> 		http://bugzilla.kernel.org/show_bug.cgi?id=10919
> 		http://bugzilla.kernel.org/show_bug.cgi?id=11309
> 		http://bugzilla.kernel.org/show_bug.cgi?id=11549
> Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
> ---
> 
>  drivers/acpi/ec.c |  309 ++++++++++++++++++++++++++---------------------------
>  1 files changed, 149 insertions(+), 160 deletions(-)
> 
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index 13593f9..7f0d81c 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)
>   *

[PATCH] ACPI: EC: do transaction from interrupt context

[Alexey Starikovskiy]

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>

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

 drivers/acpi/ec.c |  311 ++++++++++++++++++++++++++---------------------------
 1 files changed, 153 insertions(+), 158 deletions(-)


diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 13593f9..e01235b 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,19 @@ 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 */
+	EC_FLAGS_HANDLERS_INSTALLED	/* Handlers for GPE and OpReg are installed */
 };
 
 /* If we find an EC via the ECDT, we need to keep a ptr to its context */
@@ -95,6 +93,14 @@ struct acpi_ec_query_handler {
 	u8 query_bit;
 };
 
+struct transaction_data {
+	const u8 *wdata;
+	u8 *rdata;
+	unsigned short irq_count;
+	u8 wlen;
+	u8 rlen;
+};
+
 static struct acpi_ec {
 	acpi_handle handle;
 	unsigned long gpe;
@@ -105,9 +111,8 @@ static struct acpi_ec {
 	struct mutex lock;
 	wait_queue_head_t wait;
 	struct list_head list;
-	struct delayed_work work;
-	atomic_t irq_count;
-	u8 handlers_installed;
+	struct transaction_data *t;
+	spinlock_t t_lock;
 } *boot_ec, *first_ec;
 
 /* 
@@ -150,7 +155,7 @@ 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)
@@ -165,68 +170,86 @@ 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;
-	}
-
-	return 0;
+	unsigned long flags;
+	int ret = 0;
+	spin_lock_irqsave(&ec->t_lock, flags);
+	if (!ec->t || (!ec->t->wlen && !ec->t->rlen))
+		ret = 1;
+	spin_unlock_irqrestore(&ec->t_lock, flags);
+	return ret;
 }
 
-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));
+	unsigned long flags;
+	spin_lock_irqsave(&ec->t_lock, flags);
+	if (!ec->t)
+		goto unlock;
+	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 */
+			++ec->t->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 */
+			++ec->t->irq_count;
+	}
+unlock:
+	spin_unlock_irqrestore(&ec->t_lock, flags);
 }
 
-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 (state & ACPI_EC_FLAG_SCI) {
+		if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
+			/* disable GPE until next transaction */
+			if (in_interrupt() &&
+			    test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) {
+				clear_bit(EC_FLAGS_GPE_MODE, &ec->flags);
+				acpi_disable_gpe(NULL, ec->gpe, ACPI_ISR);
+			}
+			return acpi_os_execute(OSL_EC_BURST_HANDLER,
+				acpi_ec_gpe_query, ec);
 		}
-		if (acpi_ec_check_status(ec,event))
+	}
+	return 0;
+}
+
+static int ec_poll(struct acpi_ec *ec)
+{
+	unsigned long delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY);
+	msleep(1);
+	while (time_before(jiffies, delay)) {
+		gpe_transaction(ec, acpi_ec_read_status(ec));
+		msleep(1);
+		if (ec_transaction_done(ec))
 			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\"");
 	return -ETIME;
 }
 
@@ -235,45 +258,52 @@ 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 tmp;
+	struct transaction_data t = {.wdata = wdata, .rdata = rdata,
+				     .wlen = wdata_len, .rlen = rdata_len,
+				     .irq_count = 0};
+	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)) {
+		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)
+	/* start transaction */
+	spin_lock_irqsave(&ec->t_lock, tmp);
+	/* following two actions should be kept atomic */
+	ec->t = &t;
+	acpi_ec_write_cmd(ec, command);
+	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;
-		}
-		/* 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:
+	spin_unlock_irqrestore(&ec->t_lock, tmp);
+	/* 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))
+		ret = ec_poll(ec);
 	pr_debug(PREFIX "transaction end\n");
-	return result;
+	spin_lock_irqsave(&ec->t_lock, tmp);
+	ec->t = NULL;
+	spin_unlock_irqrestore(&ec->t_lock, tmp);
+	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));
+		/* we expect query, enable GPE back */
+		if (!test_bit(EC_FLAGS_QUERY_PENDING, &ec->flags))
+			acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
+	} else if (test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
+		   t.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 +313,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 +356,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)
@@ -513,46 +539,26 @@ static void acpi_ec_gpe_query(void *ec_cxt)
 
 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 status;
 
 	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))
+	status = acpi_ec_read_status(ec);
+
+	gpe_transaction(ec, status);
+	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) &&
-		   !test_bit(EC_FLAGS_NO_GPE, &ec->flags) &&
-		   in_interrupt()) {
+	ec_check_sci(ec, status);
+	if (!test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
+	    !test_bit(EC_FLAGS_NO_GPE, &ec->flags)) {
 		/* 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);
-	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);
+	return ACPI_INTERRUPT_HANDLED;
 }
 
 /* --------------------------------------------------------------------------
@@ -696,8 +702,7 @@ 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->t_lock);
 	return ec;
 }
 
@@ -736,22 +741,15 @@ 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");
 	if (ACPI_FAILURE(acpi_remove_gpe_handler(NULL, ec->gpe,
 				&acpi_ec_gpe_handler)))
 		pr_err(PREFIX "failed to remove gpe handler\n");
-	ec->handlers_installed = 0;
+	clear_bit(EC_FLAGS_HANDLERS_INSTALLED, &ec->flags);
 }
 
 static int acpi_ec_add(struct acpi_device *device)
@@ -846,17 +844,15 @@ ec_parse_io_ports(struct acpi_resource *resource, void *context)
 static int ec_install_handlers(struct acpi_ec *ec)
 {
 	acpi_status status;
-	if (ec->handlers_installed)
+	if (test_bit(EC_FLAGS_HANDLERS_INSTALLED, &ec->flags))
 		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,
@@ -866,7 +862,7 @@ static int ec_install_handlers(struct acpi_ec *ec)
 		return -ENODEV;
 	}
 
-	ec->handlers_installed = 1;
+	set_bit(EC_FLAGS_HANDLERS_INSTALLED, &ec->flags);
 	return 0;
 }
 
@@ -887,7 +883,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;
 }
 
@@ -906,7 +901,7 @@ static int acpi_ec_stop(struct acpi_device *device, int type)
 
 int __init acpi_boot_ec_enable(void)
 {
-	if (!boot_ec || boot_ec->handlers_installed)
+	if (!boot_ec || test_bit(EC_FLAGS_HANDLERS_INSTALLED, &boot_ec->flags))
 		return 0;
 	if (!ec_install_handlers(boot_ec)) {
 		first_ec = boot_ec;