[PATCH] ACPI : Avoid bogus timeout about SMbus check

[PATCH] ACPI : Avoid bogus timeout about SMbus check

[Zhao Yakui]

Subject: ACPI: Avoid bogus timeout about SMbus check
>From :  Zhao Yakui <yakui.zhao@intel.com>

In the function of wait_transaction_complete when the timeout happens,
OS will try to check the status of SMbus again. If the status is what OS
expected, it will be regarded as the bogus timeout. Otherwise it will be
treated as ETIME.

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

Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
tested-by    : Oldřich Jedlička < <oldium.pro@seznam.cz>

---
 drivers/acpi/sbshc.c |    7 +++++++
 1 file changed, 7 insertions(+)

Index: linux-2.6/drivers/acpi/sbshc.c
===================================================================
--- linux-2.6.orig/drivers/acpi/sbshc.c
+++ linux-2.6/drivers/acpi/sbshc.c
@@ -107,6 +107,13 @@ static int wait_transaction_complete(str
 	if (wait_event_timeout(hc->wait, smb_check_done(hc),
 			       msecs_to_jiffies(timeout)))
 		return 0;
+	/*
+	 * After the timeout happens, OS will try to check the status of SMbus.
+	 * If the status is what OS expected, it will be regarded as the bogus
+	 * timeout.
+	 */
+	if (smb_check_done(hc))
+		return 0;
 	else
 		return -ETIME;
 }


--
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 : Avoid bogus timeout about SMbus check

[Andi Kleen]

On Tue, Aug 26, 2008 at 01:57:34PM +0800, Zhao Yakui wrote:
> Subject: ACPI: Avoid bogus timeout about SMbus check
> >From :  Zhao Yakui <yakui.zhao@intel.com>
> 
> In the function of wait_transaction_complete when the timeout happens,
> OS will try to check the status of SMbus again. If the status is what OS
> expected, it will be regarded as the bogus timeout. Otherwise it will be
> treated as ETIME.
> 
> http://bugzilla.kernel.org/show_bug.cgi?id=10483

I added it for 2.6.27 thanks (since it seems to be a regression) 

-Andi

patch for bugs 9998 and 10724

[Alexey Starikovskiy]

Hi Andy,

I just uploaded patch for bugs 9998 and 10724, which solves the problem of interrupt storm more cleanly.
Could you please add this patch to testing?

Thanks,
Alex.

Re: patch for bugs 9998 and 10724

[Alexey Starikovskiy]

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

Here is the same patch as attachment for your convenience.
Alexey Starikovskiy wrote:
> Hi Andy,
> 
> I just uploaded patch for bugs 9998 and 10724, which solves the problem 
> of interrupt storm more cleanly.
> Could you please add this patch to testing?
> 
> Thanks,
> Alex.
> 
> 


[-- Attachment #2: fast_transaction.patch --]
[-- Type: text/x-diff, Size: 13609 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 |  250 ++++++++++++++++++++++++-----------------------------
 1 files changed, 113 insertions(+), 137 deletions(-)


diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 13593f9..1f96f0e 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>
@@ -65,22 +65,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 +91,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 +109,7 @@ static struct acpi_ec {
 	struct mutex lock;
 	wait_queue_head_t wait;
 	struct list_head list;
-	struct delayed_work work;
+	struct transaction_data t;
 	atomic_t irq_count;
 	u8 handlers_installed;
 } *boot_ec, *first_ec;
@@ -150,13 +154,13 @@ 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)
 {
 	pr_debug(PREFIX "<--- command = 0x%2.2x\n", command);
-	outb(command, ec->command_addr);
+	outb((ec->t.command = command), ec->command_addr);
 }
 
 static inline void acpi_ec_write_data(struct acpi_ec *ec, u8 data)
@@ -165,69 +169,60 @@ 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)
-{
-	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;
-	}
-
+static int ec_transaction_done(struct acpi_ec *ec) {
+	if (!ec->t.command)
+		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->t.command)
+		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 +230,53 @@ 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;
 	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);
 	}
-      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 0;
+}
+
+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,
@@ -299,8 +302,8 @@ static int acpi_ec_transaction(struct acpi_ec *ec, u8 command,
 		}
 	}
 
-	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");
 		goto end;
@@ -437,6 +440,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 +519,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 ((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 +537,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,7 +683,6 @@ 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);
 	return ec;
 }
@@ -736,15 +722,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 +828,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 +864,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: patch for bugs 9998 and 10724

[Andi Kleen]

On Thu, Sep 04, 2008 at 04:35:21PM +0400, Alexey Starikovskiy wrote:
> Here is the same patch as attachment for your convenience.
> Alexey Starikovskiy wrote:
> >Hi Andy,
> >
> >I just uploaded patch for bugs 9998 and 10724, which solves the problem 
> >of interrupt storm more cleanly.
> >Could you please add this patch to testing?

Thanks. Done It had some conflicts with the earlier patch to fix the IO port
access code. Fixed that.

-Andi

-- 
ak@linux.intel.com

Re: patch for bugs 9998 and 10724

[Zhang Rui]

On Thu, 2008-09-04 at 16:35 +0400, Alexey Starikovskiy wrote:
> Here is the same patch as attachment for your convenience.
> Alexey Starikovskiy wrote:
> > Hi Andy,
> > 
> > I just uploaded patch for bugs 9998 and 10724, which solves the problem 
> > of interrupt storm more cleanly.
> > Could you please add this patch to testing?
> > 
> > Thanks,
> > Alex.
> > 
> > 
> 
+       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 0;
+}

you will set the EC_FLAGS_GPE_STORM flag in every ec transaction.
I don't think that's what you/we want. :)

thanks,
rui

--
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 for bugs 9998 and 10724

[Zhao Yakui]

On Fri, 2008-09-05 at 10:26 +0800, Zhang Rui wrote:
> On Thu, 2008-09-04 at 16:35 +0400, Alexey Starikovskiy wrote:
> > Here is the same patch as attachment for your convenience.
> > Alexey Starikovskiy wrote:
> > > Hi Andy,
> > > 
> > > I just uploaded patch for bugs 9998 and 10724, which solves the problem 
> > > of interrupt storm more cleanly.
> > > Could you please add this patch to testing?
> > > 
> > > Thanks,
> > > Alex.
> > > 
> > > 
> > 
> +       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 0;
> +}
> 
> you will set the EC_FLAGS_GPE_STORM flag in every ec transaction.
> I don't think that's what you/we want. :)
What Rui said is right.
If the system is already switched to GPE mode, how to continue EC
transaction if there is no GPE interrupt confirmation in some EC
transaction? For example: on the laptops of bug 11428 & 8459. In fact
sometimes there is no EC GPE interrupt confirmation only for the last
step of EC transaction(OBF bit already indicates that data is ready but
there is no EC interrupt).

Thanks.
> 
> thanks,
> rui
> 
> --
> 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

--
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 for bugs 9998 and 10724

[Alexey Starikovskiy]

Zhao Yakui wrote:
> If the system is already switched to GPE mode, how to continue EC
> transaction if there is no GPE interrupt confirmation in some EC
> transaction? For example: on the laptops of bug 11428 & 8459. In fact
> sometimes there is no EC GPE interrupt confirmation only for the last
> step of EC transaction(OBF bit already indicates that data is ready but
> there is no EC interrupt).
You can't read, do you?

If the interrupt does not arrive for OBF in the middle of transaction,
transaction will be completed after the GPE_MODE is cleared in acpi_wait()
in a poll loop below, there is even a comment for that.

	/* 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;
		}
	}

Re: patch for bugs 9998 and 10724

[Zhao Yakui]

On Fri, 2008-09-05 at 16:58 +0400, Alexey Starikovskiy wrote:
> Zhao Yakui wrote:
> > If the system is already switched to GPE mode, how to continue EC
> > transaction if there is no GPE interrupt confirmation in some EC
> > transaction? For example: on the laptops of bug 11428 & 8459. In fact
> > sometimes there is no EC GPE interrupt confirmation only for the last
> > step of EC transaction(OBF bit already indicates that data is ready but
> > there is no EC interrupt).
> You can't read, do you?
> 
> If the interrupt does not arrive for OBF in the middle of transaction,
> transaction will be completed after the GPE_MODE is cleared in acpi_wait()
> in a poll loop below, there is even a comment for that.
Oh, I misunderstand what you have done in the patch.
But from the patch it seems that the definition of acpi_ec_wait in the
attached patch is totally different with that in upstream kernel. Right?
In the attached patch it indicates whether the transaction is finished.
But in the upstream kernel it indicates whether the EC status is already
what OS expected.

If the function definition is totally different, you had better add some
comments. Otherwise it is too difficult to understand.

Maybe your patch is right and can work well on your laptop. But the
basic working flowchart is changed a lot, maybe it had better be tested
on more laptops. At the same time you should add more comments and
descriptions. Otherwise when the regression happens, no one can make any
improvements about the EC driver.

Best regards.
   Yakui.
> 	/* 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;
> 		}
> 	}
> 

Re: patch for bugs 9998 and 10724

[Alexey Starikovskiy]

Zhang Rui wrote:
> On Thu, 2008-09-04 at 16:35 +0400, Alexey Starikovskiy wrote:
>> Here is the same patch as attachment for your convenience.
>> Alexey Starikovskiy wrote:
>>> Hi Andy,
>>>
>>> I just uploaded patch for bugs 9998 and 10724, which solves the problem 
>>> of interrupt storm more cleanly.
>>> Could you please add this patch to testing?
>>>
>>> Thanks,
>>> Alex.
>>>
>>>
> +       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 0;
> +}
> 
> you will set the EC_FLAGS_GPE_STORM flag in every ec transaction.
> I don't think that's what you/we want. :)
please elaborate...
If EC_FLAGS_GPE_STORM is set once, I will not get there (else).
Do I miss something?
> 
> thanks,
> rui
> 

--
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 for bugs 9998 and 10724

[Zhang Rui]

On Fri, 2008-09-05 at 16:45 +0400, Alexey Starikovskiy wrote:
> Zhang Rui wrote:
> > On Thu, 2008-09-04 at 16:35 +0400, Alexey Starikovskiy wrote:
> >> Here is the same patch as attachment for your convenience.
> >> Alexey Starikovskiy wrote:
> >>> Hi Andy,
> >>>
> >>> I just uploaded patch for bugs 9998 and 10724, which solves the problem 
> >>> of interrupt storm more cleanly.
> >>> Could you please add this patch to testing?
> >>>
> >>> Thanks,
> >>> Alex.
> >>>
> >>>
> > +       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);
+	}


thanks,
rui

> > +       return 0;
> > +}
> > 
> > you will set the EC_FLAGS_GPE_STORM flag in every ec transaction.
> > I don't think that's what you/we want. :)
> please elaborate...
> If EC_FLAGS_GPE_STORM is set once, I will not get there (else).


--
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 for bugs 9998 and 10724

[Alexey Starikovskiy]

Zhang Rui wrote:
> On Fri, 2008-09-05 at 16:45 +0400, Alexey Starikovskiy wrote:
>> Zhang Rui wrote:
>>> On Thu, 2008-09-04 at 16:35 +0400, Alexey Starikovskiy wrote:
>>>> Here is the same patch as attachment for your convenience.
>>>> Alexey Starikovskiy wrote:
>>>>> Hi Andy,
>>>>>
>>>>> I just uploaded patch for bugs 9998 and 10724, which solves the problem 
>>>>> of interrupt storm more cleanly.
>>>>> Could you please add this patch to testing?
>>>>>
>>>>> Thanks,
>>>>> Alex.
>>>>>
>>>>>
>>> +       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);
> +	}
Thanks!
> 
> 
> thanks,
> rui
> 
>>> +       return 0;
>>> +}
>>>
>>> you will set the EC_FLAGS_GPE_STORM flag in every ec transaction.
>>> I don't think that's what you/we want. :)
>> please elaborate...
>> If EC_FLAGS_GPE_STORM is set once, I will not get there (else).
> 
> 

--
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 for bugs 9998 and 10724

[Zhao Yakui]

Hi, Alexey
    You attached the updated patch on the bug 10724/9998.
    Can the following situation be handled by your patch?(For example:
bug 10724)
       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
automatically before we can handle it. 
       Can the EC notification event be handled by OS?

 Thanks
     Yakui.
> Zhang Rui wrote:
> > On Fri, 2008-09-05 at 16:45 +0400, Alexey Starikovskiy wrote:
> >> Zhang Rui wrote:
> >>> On Thu, 2008-09-04 at 16:35 +0400, Alexey Starikovskiy wrote:
> >>>> Here is the same patch as attachment for your convenience.
> >>>> Alexey Starikovskiy wrote:
> >>>>> Hi Andy,
> >>>>>
> >>>>> I just uploaded patch for bugs 9998 and 10724, which solves the problem 
> >>>>> of interrupt storm more cleanly.
> >>>>> Could you please add this patch to testing?
> >>>>>
> >>>>> Thanks,
> >>>>> Alex.
> >>>>>
> >>>>>
> >>> +       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);
> > +	}
> Thanks!
> > 
> > 
> > thanks,
> > rui
> > 
> >>> +       return 0;
> >>> +}
> >>>
> >>> you will set the EC_FLAGS_GPE_STORM flag in every ec transaction.
> >>> I don't think that's what you/we want. :)
> >> please elaborate...
> >> If EC_FLAGS_GPE_STORM is set once, I will not get there (else).
> > 
> > 
> 
> --
> 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

--
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 for bugs 9998 and 10724

[Alexey Starikovskiy]

Zhao Yakui wrote:
> Hi, Alexey
>     You attached the updated patch on the bug 10724/9998.
>     Can the following situation be handled by your patch?(For example:
> bug 10724)
>        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
> automatically before we can handle it.
SCI_EVT is cleared only as a response to query command sent to EC, there is no 
timeout on it.
Transaction will take at most 2 ms (1 write/1 read or 2 writes in poll mode), 
and at the end of the transaction there is a check of the SCI bit.
With the un-patched EC SCI bit needs to stay for period while the QUERY_PENDING bit is set,
which could be quite more than 2 ms, and it seems to comply with that even on most broken
hardware.
>        Can the EC notification event be handled by OS?
What do you mean? 
> 
>  Thanks
>      Yakui.
Please strip unneeded tails from your mails.

Re: patch for bugs 9998 and 10724

[Alexey Starikovskiy]

One more thought. One of your patches increase the time of
QUERY_PENDING bit significantly (by including whole notify handling into it),
so you need to worry more about it, and not my patch :)

Your patch actually fails to address it's purpose, as the main
consumer of queries on these broken machines is SBS HC, which does it's
own deferred execution right now. This fact saves you at the moment, as
QUERY_PENDING needs to be clear (and be able to send more queries) 
for execution of SBS HC callback to complete.

Regards,
Alex.

Alexey Starikovskiy wrote:
> Zhao Yakui wrote:
>> Hi, Alexey
>>     You attached the updated patch on the bug 10724/9998.
>>     Can the following situation be handled by your patch?(For example:
>> bug 10724)
>>        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
>> automatically before we can handle it.
> SCI_EVT is cleared only as a response to query command sent to EC, there 
> is no timeout on it.
> Transaction will take at most 2 ms (1 write/1 read or 2 writes in poll 
> mode), and at the end of the transaction there is a check of the SCI bit.
> With the un-patched EC SCI bit needs to stay for period while the 
> QUERY_PENDING bit is set,
> which could be quite more than 2 ms, and it seems to comply with that 
> even on most broken
> hardware.
>>        Can the EC notification event be handled by OS?
> What do you mean?
>>
>>  Thanks
>>      Yakui.
> Please strip unneeded tails from your mails.
> 

Re: patch for bugs 9998 and 10724

[Zhao Yakui]

On Tue, 2008-09-09 at 13:12 +0400, Alexey Starikovskiy wrote:
> Zhao Yakui wrote:
> > Hi, Alexey
> >     You attached the updated patch on the bug 10724/9998.
> >     Can the following situation be handled by your patch?(For example:
> > bug 10724)
> >        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
> > automatically before we can handle it.
> SCI_EVT is cleared only as a response to query command sent to EC, there is no 
> timeout on it.
If the SCI_EVT is cleared only after issuing query the command, in
theory the poll timer should also handle it. But on the laptop of bug
10724 it seems that the poll timer can't handle such notification event.
And on the laptops of bug 11428 and 8459 the poll timer can't handle the
notification event when the system is switched from GPE mode to polling
mode.
> Transaction will take at most 2 ms (1 write/1 read or 2 writes in poll mode), 
> and at the end of the transaction there is a check of the SCI bit.
> With the un-patched EC SCI bit needs to stay for period while the QUERY_PENDING bit is set,
> which could be quite more than 2 ms, and it seems to comply with that even on most broken
> hardware.
> >        Can the EC notification event be handled by OS?
Maybe it will take very short time to do the EC transaction. Myabe in
most cases it is about 2ms. But in some worse situation it will take a
long time(For example: 100ms). If the EC notification happens in such
case, how to handle the event? (the SCI_EVT bit will be cleared before
we can detect it).

Best regards.
> What do you mean? 
> > 
> >  Thanks
> >      Yakui.
> Please strip unneeded tails from your mails.

Re: patch for bugs 9998 and 10724

[Alexey Starikovskiy]

Zhao Yakui wrote:
> On Tue, 2008-09-09 at 13:12 +0400, Alexey Starikovskiy wrote:
>> Zhao Yakui wrote:
>>> Hi, Alexey
>>>     You attached the updated patch on the bug 10724/9998.
>>>     Can the following situation be handled by your patch?(For example:
>>> bug 10724)
>>>        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
>>> automatically before we can handle it.
>> SCI_EVT is cleared only as a response to query command sent to EC, there is no 
>> timeout on it.
> If the SCI_EVT is cleared only after issuing query the command, in
> theory the poll timer should also handle it. But on the laptop of bug
> 10724 it seems that the poll timer can't handle such notification event.
> And on the laptops of bug 11428 and 8459 the poll timer can't handle the
> notification event when the system is switched from GPE mode to polling
> mode.
The problem here is with auto-repeat of function keys, thus the need to _distinguish_
one event from another (SCI bit will be set, just the event will be either different 
or next of the same kind). Poll timer does it's job every 500ms, thus auto-repeated keys are
lost. With the fastest auto-repeat being 30/s or 33ms we have plenty of time.
>> Transaction will take at most 2 ms (1 write/1 read or 2 writes in poll mode), 
>> and at the end of the transaction there is a check of the SCI bit.
>> With the un-patched EC SCI bit needs to stay for period while the QUERY_PENDING bit is set,
>> which could be quite more than 2 ms, and it seems to comply with that even on most broken
>> hardware.
>>>        Can the EC notification event be handled by OS?
> Maybe it will take very short time to do the EC transaction. Myabe in
> most cases it is about 2ms. But in some worse situation it will take a
> long time(For example: 100ms). If the EC notification happens in such
> case, how to handle the event? (the SCI_EVT bit will be cleared before
> we can detect it).
Where do you get this 100ms number? 

Re: patch for bugs 9998 and 10724

[Zhao Yakui]

On Tue, 2008-09-09 at 13:36 +0400, Alexey Starikovskiy wrote:
> Zhao Yakui wrote:
> > On Tue, 2008-09-09 at 13:12 +0400, Alexey Starikovskiy wrote:
> >> Zhao Yakui wrote:
> >>> Hi, Alexey
> >>>     You attached the updated patch on the bug 10724/9998.
> >>>     Can the following situation be handled by your patch?(For example:
> >>> bug 10724)
> >>>        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
> >>> automatically before we can handle it.
> >> SCI_EVT is cleared only as a response to query command sent to EC, there is no 
> >> timeout on it.
> > If the SCI_EVT is cleared only after issuing query the command, in
> > theory the poll timer should also handle it. But on the laptop of bug
> > 10724 it seems that the poll timer can't handle such notification event.
> > And on the laptops of bug 11428 and 8459 the poll timer can't handle the
> > notification event when the system is switched from GPE mode to polling
> > mode.
What we cared only is whether the EC notification event can be detected
by OS when the following case happens? (For example: On the laptop of
bug 10724)
    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 we
can handle it.
    
> The problem here is with auto-repeat of function keys, thus the need to _distinguish_
> one event from another (SCI bit will be set, just the event will be either different 
> or next of the same kind). Poll timer does it's job every 500ms, thus auto-repeated keys are
> lost. With the fastest auto-repeat being 30/s or 33ms we have plenty of time.
> >> Transaction will take at most 2 ms (1 write/1 read or 2 writes in poll mode), 
> >> and at the end of the transaction there is a check of the SCI bit.
> >> With the un-patched EC SCI bit needs to stay for period while the QUERY_PENDING bit is set,
> >> which could be quite more than 2 ms, and it seems to comply with that even on most broken
> >> hardware.
> >>>        Can the EC notification event be handled by OS?
> > Maybe it will take very short time to do the EC transaction. Myabe in
> > most cases it is about 2ms. But in some worse situation it will take a
> > long time(For example: 100ms). If the EC notification happens in such
> > case, how to handle the event? (the SCI_EVT bit will be cleared before
> > we can detect it).
> Where do you get this 100ms number? 
As I state in the previous email, the msleep is realized by the function
of schedule_timeout. The input parameter of msleep will be translated to
jiffies unit. In such case the time of EC transaction will be related
with the definition of HZ.(If the HZ is 100, msleep(1) will cost
10ms).At the same time when one process is waked up, maybe it won't be
scheduled immediately. If so, the time of EC transaction will be
longer.Maybe in some worse case the time will exceed 500ms.
    The detailed info log can be found in 
     http://bugzilla.kernel.org/show_bug.cgi?id=11141

Re: patch for bugs 9998 and 10724

[Alexey Starikovskiy]

Zhao Yakui wrote:
> On Tue, 2008-09-09 at 13:36 +0400, Alexey Starikovskiy wrote:
>> Zhao Yakui wrote:
>>> On Tue, 2008-09-09 at 13:12 +0400, Alexey Starikovskiy wrote:
>>>> Zhao Yakui wrote:
>>>>> Hi, Alexey
>>>>>     You attached the updated patch on the bug 10724/9998.
>>>>>     Can the following situation be handled by your patch?(For example:
>>>>> bug 10724)
>>>>>        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
>>>>> automatically before we can handle it.
>>>> SCI_EVT is cleared only as a response to query command sent to EC, there is no 
>>>> timeout on it.
>>> If the SCI_EVT is cleared only after issuing query the command, in
>>> theory the poll timer should also handle it. But on the laptop of bug
>>> 10724 it seems that the poll timer can't handle such notification event.
>>> And on the laptops of bug 11428 and 8459 the poll timer can't handle the
>>> notification event when the system is switched from GPE mode to polling
>>> mode.
> What we cared only is whether the EC notification event can be detected
> by OS when the following case happens? (For example: On the laptop of
> bug 10724)
>     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 we
> can handle it.
In such case, event is lost. 
>     
>> The problem here is with auto-repeat of function keys, thus the need to _distinguish_
>> one event from another (SCI bit will be set, just the event will be either different 
>> or next of the same kind). Poll timer does it's job every 500ms, thus auto-repeated keys are
>> lost. With the fastest auto-repeat being 30/s or 33ms we have plenty of time.
>>>> Transaction will take at most 2 ms (1 write/1 read or 2 writes in poll mode), 
>>>> and at the end of the transaction there is a check of the SCI bit.
>>>> With the un-patched EC SCI bit needs to stay for period while the QUERY_PENDING bit is set,
>>>> which could be quite more than 2 ms, and it seems to comply with that even on most broken
>>>> hardware.
>>>>>        Can the EC notification event be handled by OS?
>>> Maybe it will take very short time to do the EC transaction. Myabe in
>>> most cases it is about 2ms. But in some worse situation it will take a
>>> long time(For example: 100ms). If the EC notification happens in such
>>> case, how to handle the event? (the SCI_EVT bit will be cleared before
>>> we can detect it).
>> Where do you get this 100ms number? 
> As I state in the previous email, the msleep is realized by the function
> of schedule_timeout. The input parameter of msleep will be translated to
> jiffies unit. In such case the time of EC transaction will be related
> with the definition of HZ.(If the HZ is 100, msleep(1) will cost
> 10ms).At the same time when one process is waked up, maybe it won't be
> scheduled immediately. If so, the time of EC transaction will be
> longer.Maybe in some worse case the time will exceed 500ms.
>     The detailed info log can be found in 
>      http://bugzilla.kernel.org/show_bug.cgi?id=11141
> 
> 
Ok.

Re: [PATCH] ACPI : Avoid bogus timeout about SMbus check

[Oldrich Jedlicka]

On Thursday 04 of September 2008 at 14:18:20, Andi Kleen wrote:
> On Tue, Aug 26, 2008 at 01:57:34PM +0800, Zhao Yakui wrote:
> > Subject: ACPI: Avoid bogus timeout about SMbus check
> >
> > >From :  Zhao Yakui <yakui.zhao@intel.com>
> >
> > In the function of wait_transaction_complete when the timeout happens,
> > OS will try to check the status of SMbus again. If the status is what OS
> > expected, it will be regarded as the bogus timeout. Otherwise it will be
> > treated as ETIME.
> >
> > http://bugzilla.kernel.org/show_bug.cgi?id=10483
>
> I added it for 2.6.27 thanks (since it seems to be a regression)

Hi all,

the problem in bug 10483 isn't solved, unfortunately the patch applied to 
2.6.27 isn't the one I tested.

The problem I face is that wait_transaction_complete() in sbshc.c doesn't take 
at most "timeout" time (=one second), but it rather often takes very long 
time to complete on my system (several minutes) - notebook Acer TravelMate 
4502WLMi.

To describe the problem I need to go into wait_event_timeout() macro as used 
in wait_transaction_complete(): it first calls the user method 
smb_check_done() to check if the loop should end and if not, it waits for at 
most "timeout" time. The wait can be woken-up by a wake_up() method and if 
so, it remembers the remaining "timeout", repeats the call to user method 
smb_check_done() and possibly continues with wait (with the 
remaining "timeout"). This is done in a loop.

Now my problem: the wait_event_timeout() loop takes several minutes instead of 
one second (as specified by the parameter "timeout"). The reason for that - as 
I think - is this:

  1. The smb_check_done() method called each loop takes some time to execute, 
but this time is not included into the overall timeout.
  2. The wait_event_timeout() is woken-up by smbus_alarm() very early each 
loop, so the remaining "timeout" gets lower very slow (I think it is so, but 
I haven't verified this hypothesis yet).

So my question is what I shoud do now? If you want to see some logs (and the 
original patch), please go to my bug report:

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

Thanks.

Note: I'm building my house after my normal work, so I don't have much time 
left, but I will do my best to try whatever you suggest. And sorry for my 
english :-)

Regards,
Oldrich.

Re: [PATCH] ACPI : Avoid bogus timeout about SMbus check

[Alexey Starikovskiy]

Hi Oldrich,

It might happen that you too have GPE storm on your machine. The patch
to workaround it was placed in 9998 and 10724 bug reports.
So, for a start you could check if the last patch in 9998 makes any difference.
We probably could drop wait_event_timeout() as the last resort.

Regards,
Alex.

Oldrich Jedlicka wrote:
> On Thursday 04 of September 2008 at 14:18:20, Andi Kleen wrote:
>> On Tue, Aug 26, 2008 at 01:57:34PM +0800, Zhao Yakui wrote:
>>> Subject: ACPI: Avoid bogus timeout about SMbus check
>>>
>>> >From :  Zhao Yakui <yakui.zhao@intel.com>
>>>
>>> In the function of wait_transaction_complete when the timeout happens,
>>> OS will try to check the status of SMbus again. If the status is what OS
>>> expected, it will be regarded as the bogus timeout. Otherwise it will be
>>> treated as ETIME.
>>>
>>> http://bugzilla.kernel.org/show_bug.cgi?id=10483
>> I added it for 2.6.27 thanks (since it seems to be a regression)
> 
> Hi all,
> 
> the problem in bug 10483 isn't solved, unfortunately the patch applied to 
> 2.6.27 isn't the one I tested.
> 
> The problem I face is that wait_transaction_complete() in sbshc.c doesn't take 
> at most "timeout" time (=one second), but it rather often takes very long 
> time to complete on my system (several minutes) - notebook Acer TravelMate 
> 4502WLMi.
> 
> To describe the problem I need to go into wait_event_timeout() macro as used 
> in wait_transaction_complete(): it first calls the user method 
> smb_check_done() to check if the loop should end and if not, it waits for at 
> most "timeout" time. The wait can be woken-up by a wake_up() method and if 
> so, it remembers the remaining "timeout", repeats the call to user method 
> smb_check_done() and possibly continues with wait (with the 
> remaining "timeout"). This is done in a loop.
> 
> Now my problem: the wait_event_timeout() loop takes several minutes instead of 
> one second (as specified by the parameter "timeout"). The reason for that - as 
> I think - is this:
> 
>   1. The smb_check_done() method called each loop takes some time to execute, 
> but this time is not included into the overall timeout.
>   2. The wait_event_timeout() is woken-up by smbus_alarm() very early each 
> loop, so the remaining "timeout" gets lower very slow (I think it is so, but 
> I haven't verified this hypothesis yet).
> 
> So my question is what I shoud do now? If you want to see some logs (and the 
> original patch), please go to my bug report:
> 
>   http://bugzilla.kernel.org/show_bug.cgi?id=10483
> 
> Thanks.
> 
> Note: I'm building my house after my normal work, so I don't have much time 
> left, but I will do my best to try whatever you suggest. And sorry for my 
> english :-)
> 
> Regards,
> Oldrich.
> 

Re: [PATCH] ACPI : Avoid bogus timeout about SMbus check

[Oldrich Jedlicka]

On Thursday 11 of September 2008 at 23:13:12, Alexey Starikovskiy wrote:
> Hi Oldrich,
>
> It might happen that you too have GPE storm on your machine. The patch
> to workaround it was placed in 9998 and 10724 bug reports.
> So, for a start you could check if the last patch in 9998 makes any
> difference. We probably could drop wait_event_timeout() as the last resort.
>
> Regards,
> Alex.

Hi Alex,

the patch from 9998 ("updated fast transaction patch") works for me. Sometimes 
it looks like the reading goes into 1 second timeout, but this is no longer 
problem - no "endless" states, values in /proc/acpi/battery/BAT0/state change 
between multiple reads. And that is generally good :-)

Are you interrested in some logs? And if yes, what kernel switch would you 
like to add to boot to show debug output in dmesg?

Thanks,
Oldrich.

> Oldrich Jedlicka wrote:
> > On Thursday 04 of September 2008 at 14:18:20, Andi Kleen wrote:
> >> On Tue, Aug 26, 2008 at 01:57:34PM +0800, Zhao Yakui wrote:
> >>> Subject: ACPI: Avoid bogus timeout about SMbus check
> >>>
> >>> >From :  Zhao Yakui <yakui.zhao@intel.com>
> >>>
> >>> In the function of wait_transaction_complete when the timeout happens,
> >>> OS will try to check the status of SMbus again. If the status is what
> >>> OS expected, it will be regarded as the bogus timeout. Otherwise it
> >>> will be treated as ETIME.
> >>>
> >>> http://bugzilla.kernel.org/show_bug.cgi?id=10483
> >>
> >> I added it for 2.6.27 thanks (since it seems to be a regression)
> >
> > Hi all,
> >
> > the problem in bug 10483 isn't solved, unfortunately the patch applied to
> > 2.6.27 isn't the one I tested.
> >
> > The problem I face is that wait_transaction_complete() in sbshc.c doesn't
> > take at most "timeout" time (=one second), but it rather often takes very
> > long time to complete on my system (several minutes) - notebook Acer
> > TravelMate 4502WLMi.
> >
> > To describe the problem I need to go into wait_event_timeout() macro as
> > used in wait_transaction_complete(): it first calls the user method
> > smb_check_done() to check if the loop should end and if not, it waits for
> > at most "timeout" time. The wait can be woken-up by a wake_up() method
> > and if so, it remembers the remaining "timeout", repeats the call to user
> > method smb_check_done() and possibly continues with wait (with the
> > remaining "timeout"). This is done in a loop.
> >
> > Now my problem: the wait_event_timeout() loop takes several minutes
> > instead of one second (as specified by the parameter "timeout"). The
> > reason for that - as I think - is this:
> >
> >   1. The smb_check_done() method called each loop takes some time to
> > execute, but this time is not included into the overall timeout.
> >   2. The wait_event_timeout() is woken-up by smbus_alarm() very early
> > each loop, so the remaining "timeout" gets lower very slow (I think it is
> > so, but I haven't verified this hypothesis yet).
> >
> > So my question is what I shoud do now? If you want to see some logs (and
> > the original patch), please go to my bug report:
> >
> >   http://bugzilla.kernel.org/show_bug.cgi?id=10483
> >
> > Thanks.
> >
> > Note: I'm building my house after my normal work, so I don't have much
> > time left, but I will do my best to try whatever you suggest. And sorry
> > for my english :-)
> >
> > Regards,
> > Oldrich.
>
> --
> 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 : Avoid bogus timeout about SMbus check

[Zhao Yakui]

On Thu, 2008-09-11 at 21:49 +0200, Oldrich Jedlicka wrote:
> On Thursday 04 of September 2008 at 14:18:20, Andi Kleen wrote:
> > On Tue, Aug 26, 2008 at 01:57:34PM +0800, Zhao Yakui wrote:
> > > Subject: ACPI: Avoid bogus timeout about SMbus check
> > >
> > > >From :  Zhao Yakui <yakui.zhao@intel.com>
> > >
> > > In the function of wait_transaction_complete when the timeout happens,
> > > OS will try to check the status of SMbus again. If the status is what OS
> > > expected, it will be regarded as the bogus timeout. Otherwise it will be
> > > treated as ETIME.
> > >
> > > http://bugzilla.kernel.org/show_bug.cgi?id=10483
> >
> > I added it for 2.6.27 thanks (since it seems to be a regression)
> 
> Hi all,
> 
> the problem in bug 10483 isn't solved, unfortunately the patch applied to 
> 2.6.27 isn't the one I tested.
> 
> The problem I face is that wait_transaction_complete() in sbshc.c doesn't take 
> at most "timeout" time (=one second), but it rather often takes very long 
> time to complete on my system (several minutes) - notebook Acer TravelMate 
> 4502WLMi.
> To describe the problem I need to go into wait_event_timeout() macro as used 
> in wait_transaction_complete(): it first calls the user method 
> smb_check_done() to check if the loop should end and if not, it waits for at 
> most "timeout" time. The wait can be woken-up by a wake_up() method and if 
> so, it remembers the remaining "timeout", repeats the call to user method 
> smb_check_done() and possibly continues with wait (with the 
> remaining "timeout"). This is done in a loop.
> 
> Now my problem: the wait_event_timeout() loop takes several minutes instead of 
> one second (as specified by the parameter "timeout"). The reason for that - as 
> I think - is this:
> 
>   1. The smb_check_done() method called each loop takes some time to execute, 
> but this time is not included into the overall timeout.
What you said is right. It will take some time to execute the function
of smb_check_done. When the ec_read is called in smb_check_done, maybe
it will also sleep for some time in ec_read. But this time is not
accounted into the overall timeout.
Please use the following mechanism to replace the wait_event_timeout.

	unsigned long delay = jiffies + msecs_to_jiffies(timeout);

	while (time_before(jiffies, delay)) {
		if (smb_check_done(hc))
			return 0;
		msleep(1);
	}
>   2. The wait_event_timeout() is woken-up by smbus_alarm() very early each 
> loop, so the remaining "timeout" gets lower very slow (I think it is so, but 
> I haven't verified this hypothesis yet).

> 
> So my question is what I shoud do now? If you want to see some logs (and the 
> original patch), please go to my bug report:
> 
>   http://bugzilla.kernel.org/show_bug.cgi?id=10483
> 
> Thanks.
> 
> Note: I'm building my house after my normal work, so I don't have much time 
> left, but I will do my best to try whatever you suggest. And sorry for my 
> english :-)
> 
> Regards,
> Oldrich.
> 

Re: [PATCH] ACPI : Avoid bogus timeout about SMbus check

[Oldrich Jedlicka]

Hi Yakui,

On Friday 12 of September 2008 at 03:38:49, Zhao Yakui wrote:
> On Thu, 2008-09-11 at 21:49 +0200, Oldrich Jedlicka wrote:
> > On Thursday 04 of September 2008 at 14:18:20, Andi Kleen wrote:
> > > On Tue, Aug 26, 2008 at 01:57:34PM +0800, Zhao Yakui wrote:
> > > > Subject: ACPI: Avoid bogus timeout about SMbus check
> > > >
> > > > >From :  Zhao Yakui <yakui.zhao@intel.com>
> > > >
> > > > In the function of wait_transaction_complete when the timeout
> > > > happens, OS will try to check the status of SMbus again. If the
> > > > status is what OS expected, it will be regarded as the bogus timeout.
> > > > Otherwise it will be treated as ETIME.
> > > >
> > > > http://bugzilla.kernel.org/show_bug.cgi?id=10483
> > >
> > > I added it for 2.6.27 thanks (since it seems to be a regression)
> >
> > Hi all,
> >
> > the problem in bug 10483 isn't solved, unfortunately the patch applied to
> > 2.6.27 isn't the one I tested.
> >
> > The problem I face is that wait_transaction_complete() in sbshc.c doesn't
> > take at most "timeout" time (=one second), but it rather often takes very
> > long time to complete on my system (several minutes) - notebook Acer
> > TravelMate 4502WLMi.
> > To describe the problem I need to go into wait_event_timeout() macro as
> > used in wait_transaction_complete(): it first calls the user method
> > smb_check_done() to check if the loop should end and if not, it waits for
> > at most "timeout" time. The wait can be woken-up by a wake_up() method
> > and if so, it remembers the remaining "timeout", repeats the call to user
> > method smb_check_done() and possibly continues with wait (with the
> > remaining "timeout"). This is done in a loop.
> >
> > Now my problem: the wait_event_timeout() loop takes several minutes
> > instead of one second (as specified by the parameter "timeout"). The
> > reason for that - as I think - is this:
> >
> >   1. The smb_check_done() method called each loop takes some time to
> > execute, but this time is not included into the overall timeout.
>
> What you said is right. It will take some time to execute the function
> of smb_check_done. When the ec_read is called in smb_check_done, maybe
> it will also sleep for some time in ec_read. But this time is not
> accounted into the overall timeout.
> Please use the following mechanism to replace the wait_event_timeout.
>
> 	unsigned long delay = jiffies + msecs_to_jiffies(timeout);
>
> 	while (time_before(jiffies, delay)) {
> 		if (smb_check_done(hc))
> 			return 0;
> 		msleep(1);
> 	}
>

This is what I tried and it works for me. The working patch of 
wait_transaction_complete() is attached to bug 10483, but actually the patch 
from bug 9998 works for me too.

For me any of those two patches fix the problem. The big advantage is that 
I've got rid of the "endless" state now.

Regards,
Oldrich.

> >   2. The wait_event_timeout() is woken-up by smbus_alarm() very early
> > each loop, so the remaining "timeout" gets lower very slow (I think it is
> > so, but I haven't verified this hypothesis yet).
> >
> >
> > So my question is what I shoud do now? If you want to see some logs (and
> > the original patch), please go to my bug report:
> >
> >   http://bugzilla.kernel.org/show_bug.cgi?id=10483
> >
> > Thanks.
> >
> > Note: I'm building my house after my normal work, so I don't have much
> > time left, but I will do my best to try whatever you suggest. And sorry
> > for my english :-)
> >
> > Regards,
> > Oldrich.
>
> --
> 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