Skip to contentSkip to conversation

ACPI/EC: Critical bug fixes related to EC and event handling.

PATCH New Lv Zheng 3 participants 17 messages Jun 15, 2014 – Jul 8, 2014 1 message not in the archive

Subsystems (2)
ACPI ACPI component architecture (acpica)
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Lv Zheng

author
#1 lore raw

v2 cover ACPI/EC: Critical bug fixes related to EC and event handling.

From: Lv Zheng <[email protected]>

History:
v1:
The collection of bug-fixes from kernel bugzilla bug 70891.
v2:
1. Improves the patch descriptions to make the root cause of the 70891 bug more correct.
2. Improves a race fix to make it more robust.

This patchset fixes the following issues:
1. There are 2 race conditions in current EC driver.
2. Cleanup the first command byte write as one of the race condition fix requires additional cleanup for first command byte write. And this also helps us to do improvements in the BURST mode in the future.
3. ACPICA event not handled in the correct way.

This patchset also improves debugging message, register details can be enabled by defining DEBUG. And bug fixes and reports are based on this improvement.

All patches are tested to be valid fixes for the following bug:
https://bugzilla.kernel.org/show_bug.cgi?id=70891

The EC driver version is upgraded to 2.2.

The first 4 patches are useful stable materials. But the culprit of the bug appears so long time ago that patches cannot be applied cleanly any more. Thus 2 commit IDs are listed for the stable reviewers, one for the culprit, the other for the kernel versions that can have these stable materials cleanly applied.

Lv Zheng (7):
ACPI/EC: Fix an issue that advance_transaction() processes stale
hardware status.
ACPI/EC: Add asynchronous command byte write support. ACPI/EC: Remove duplicated ec_wait_ibf0() waiter. ACPI/EC: Fix a race condition in ec_transaction_completed(). ACPI/EC: Update revision due to full asynchrnous command support.
ACPICA: Events: Fix edge-triggered GPE by disabling before
acknowledging it.
ACPI/EC: Add detailed fields debugging support of EC_SC(R).

 drivers/acpi/acpica/evgpe.c |   32 +++++----
 drivers/acpi/ec.c           |  159 ++++++++++++++++++++++---------------------
 2 files changed, 98 insertions(+), 93 deletions(-)
Signature · 1 line
1.7.10
author
#2 lore raw

PATCH v2 1/7 ACPI/EC: Fix an issue that advance_transaction() processes stale hardware status.

From: Lv Zheng <[email protected]>

The advance_transaction() will be invoked from the IRQ context GPE handler and the task context ec_poll(). The handling of this function is locked so that the EC state machine are ensured to be advanced sequentially.

But there is a problem. Before invoking advance_transaction(), EC_SC(R) is read. Then for advance_transaction(), there could be race condition around the lock from both contexts. The first one reading the register could fail this race and when it passes the stale register value to the state machine advancement code, the hardware condition is totally different from when the register is read. And the hardware accesses determined from the wrong hardware status can break the EC state machine. And there could be cases that the functionalities of the platform firmware are seriously affected. For example:
1. When 2 EC_DATA(W) writes compete the IBF=0, the 2nd EC_DATA(W) write may be invalid due to IBF=1 after the 1st EC_DATA(W) write. Then the hardware will either refuse to respond a next EC_SC(W) write of the next command or discard the current WR_EC command when it receives a EC_SC(W) write of the next command.
2. When 1 EC_SC(W) write and 1 EC_DATA(W) write compete the IBF=0, the EC_DATA(W) write may be invalid due to IBF=1 after the EC_SC(W) write. The next EC_DATA(R) could never be responded by the hardware. This is the root cause of the reported issue.

This patch fixes this issue by moving the EC_SC(R) access into the lock so that we can ensure that the state machine is advanced under the right condition.

Reference: https://bugzilla.kernel.org/show_bug.cgi?id=70891

Signed-off-by Lv Zheng <[email protected]>
Reported-and-tested-by Gareth Williams <[email protected]>
Tested-by Steffen Weber <[email protected]>
[zetalog: first affected by:]
Cc <[email protected]> # 2.6.11: 7c6db4e0: ACPI: EC: do transaction from interrupt context
[zetalog: cleanly applying to:]
Cc <[email protected]> # 3.14.x: 42b946bb: ACPI / EC: disable GPE before removing GPE handler
---
 drivers/acpi/ec.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/acpi/ec.c +6 -6
index ad11ba4..762b4cc 100644
@@ -168,12 +168,15 @@ static void start_transaction(struct acpi_ec *ec)
 	acpi_ec_write_cmd(ec, ec->curr->command);
 }
 
-static void advance_transaction(struct acpi_ec *ec, u8 status)
+static void advance_transaction(struct acpi_ec *ec)
 {
 	unsigned long flags;
 	struct transaction *t;
+	u8 status;
 
 	spin_lock_irqsave(&ec->lock, flags);
+	pr_debug("===== %s =====\n", in_interrupt() ? "IRQ" : "TASK");
+	status = acpi_ec_read_status(ec);
 	t = ec->curr;
 	if (!t)
 		goto unlock;

@@ -236,7 +239,7 @@ static int ec_poll(struct acpi_ec *ec)
 						msecs_to_jiffies(1)))
 					return 0;
 			}
-			advance_transaction(ec, acpi_ec_read_status(ec));
+			advance_transaction(ec);
 		} while (time_before(jiffies, delay));
 		pr_debug("controller reset, restart transaction\n");
 		spin_lock_irqsave(&ec->lock, flags);

@@ -635,11 +638,8 @@ static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
 	u32 gpe_number, void *data)
 {
 	struct acpi_ec *ec = data;
-	u8 status = acpi_ec_read_status(ec);
-
-	pr_debug("~~~> interrupt, status:0x%02x\n", status);
 
-	advance_transaction(ec, status);
+	advance_transaction(ec);
 	if (ec_transaction_done(ec) &&
 	    (acpi_ec_read_status(ec) & ACPI_EC_FLAG_IBF) == 0) {
 		wake_up(&ec->wait);
Signature · 1 line
1.7.10
author
#3 lore raw

PATCH v2 2/7 ACPI/EC: Add asynchronous command byte write support.

In reply to Lv Zheng

From: Lv Zheng <[email protected]>

This patch moves the first command byte write into advance_transaction() so that all EC register accesses that can affect the command processing state machine can happen in this asynchronous state machine advancement function.

The advance_transaction() function then can be a complete implementation of an asyncrhonous transaction for a single command so that:
1. The first command byte can be written in the interrupt context;
2. The command completion waiter can also be used to wait the first command byte's timeout;
3. In BURST mode, the follow-up command bytes can be written in the interrupt context directly, so that it doesn't need to return to the task context. Returning to the task context reduces the throughput of the BURST mode and in the worst cases where the system workload is very high, this leads to the hardware driven automatic BURST mode exit.

In order not to increase memory consumptions, this patch converts 'done' into 'flags' to contain multiple indications:
1. ACPI_EC_COMMAND_COMPLETE: converting from original 'done' condition, indicating the completion of the command transaction.
2. ACPI_EC_COMMAND_POLL: indicating the availability of writing the first command byte. A new command can utilize this flag to compete for the right of accessing the underlying hardware. There is a follow-up bug fix that has utilized this new flag.

The 2 flags are important because it also reflects a key concept of IO programs' design used in the system softwares. Normally an IO program running in the kernel should first be implemented in the asynchronous way. And the 2 flags are the most common way to implement its synchronous operations on top of the asynchronous operations:
1. POLL: This flag can be used to block until the asynchronous operations can happen.
2. COMPLETE: This flag can be used to block until the asynchronous operations have completed.
By constructing code cleanly in this way, many difficult problems can be solved smoothly.

[zetalog: changelog updated]
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=70891

Signed-off-by Lv Zheng <[email protected]>
Reported-and-tested-by Gareth Williams <[email protected]>
Tested-by Steffen Weber <[email protected]>
[zetalog: first affected by:]
Cc <[email protected]> # 2.6.11: 7c6db4e0: ACPI: EC: do transaction from interrupt context
[zetalog: cleanly applying to:]
Cc <[email protected]> # 3.14.x: 42b946bb: ACPI / EC: disable GPE before removing GPE handler
---
 drivers/acpi/ec.c |   83 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 48 insertions(+), 35 deletions(-)
drivers/acpi/ec.c +48 -35
index 762b4cc..f09386e 100644
@@ -78,6 +78,9 @@ enum {
 	EC_FLAGS_BLOCKED,		/* Transactions are blocked */
 };
 
+#define ACPI_EC_COMMAND_POLL		0x01 /* Available for command byte */
+#define ACPI_EC_COMMAND_COMPLETE	0x02 /* Completed last byte */
+
 /* ec.c is compiled in acpi namespace so this shows up as acpi.ec_delay param */
 static unsigned int ec_delay __read_mostly = ACPI_EC_DELAY;
 module_param(ec_delay, uint, 0644);

@@ -109,7 +112,7 @@ struct transaction {
 	u8 ri;
 	u8 wlen;
 	u8 rlen;
-	bool done;
+	u8 flags;
 };
 
 struct acpi_ec *boot_ec, *first_ec;

@@ -150,63 +153,68 @@ static inline void acpi_ec_write_data(struct acpi_ec *ec, u8 data)
 	outb(data, ec->data_addr);
 }
 
-static int ec_transaction_done(struct acpi_ec *ec)
+static int ec_transaction_completed(struct acpi_ec *ec)
 {
 	unsigned long flags;
 	int ret = 0;
 	spin_lock_irqsave(&ec->lock, flags);
-	if (!ec->curr || ec->curr->done)
+	if (!ec->curr || (ec->curr->flags & ACPI_EC_COMMAND_COMPLETE))
 		ret = 1;
 	spin_unlock_irqrestore(&ec->lock, flags);
 	return ret;
 }
 
-static void start_transaction(struct acpi_ec *ec)
-{
-	ec->curr->irq_count = ec->curr->wi = ec->curr->ri = 0;
-	ec->curr->done = false;
-	acpi_ec_write_cmd(ec, ec->curr->command);
-}
-
 static void advance_transaction(struct acpi_ec *ec)
 {
-	unsigned long flags;
 	struct transaction *t;
 	u8 status;
 
-	spin_lock_irqsave(&ec->lock, flags);
 	pr_debug("===== %s =====\n", in_interrupt() ? "IRQ" : "TASK");
 	status = acpi_ec_read_status(ec);
 	t = ec->curr;
 	if (!t)
-		goto unlock;
-	if (t->wlen > t->wi) {
-		if ((status & ACPI_EC_FLAG_IBF) == 0)
-			acpi_ec_write_data(ec,
-				t->wdata[t->wi++]);
-		else
-			goto err;
-	} else if (t->rlen > t->ri) {
-		if ((status & ACPI_EC_FLAG_OBF) == 1) {
-			t->rdata[t->ri++] = acpi_ec_read_data(ec);
-			if (t->rlen == t->ri)
-				t->done = true;
+		goto err;
+	if (t->flags & ACPI_EC_COMMAND_POLL) {
+		if (t->wlen > t->wi) {
+			if ((status & ACPI_EC_FLAG_IBF) == 0)
+				acpi_ec_write_data(ec, t->wdata[t->wi++]);
+			else
+				goto err;
+		} else if (t->rlen > t->ri) {
+			if ((status & ACPI_EC_FLAG_OBF) == 1) {
+				t->rdata[t->ri++] = acpi_ec_read_data(ec);
+				if (t->rlen == t->ri)
+					t->flags |= ACPI_EC_COMMAND_COMPLETE;
+			} else
+				goto err;
+		} else if (t->wlen == t->wi &&
+			   (status & ACPI_EC_FLAG_IBF) == 0)
+			t->flags |= ACPI_EC_COMMAND_COMPLETE;
+		return;
+	} else {
+		if ((status & ACPI_EC_FLAG_IBF) == 0) {
+			acpi_ec_write_cmd(ec, t->command);
+			t->flags |= ACPI_EC_COMMAND_POLL;
 		} else
 			goto err;
-	} else if (t->wlen == t->wi &&
-		   (status & ACPI_EC_FLAG_IBF) == 0)
-		t->done = true;
-	goto unlock;
+		return;
+	}
 err:
 	/*
 	 * If SCI bit is set, then don't think it's a false IRQ
 	 * otherwise will take a not handled IRQ as a false one.
 	 */
-	if (in_interrupt() && !(status & ACPI_EC_FLAG_SCI))
-		++t->irq_count;
+	if (!(status & ACPI_EC_FLAG_SCI)) {
+		if (in_interrupt() && t)
+			++t->irq_count;
+	}
+}
 
-unlock:
-	spin_unlock_irqrestore(&ec->lock, flags);
+static void start_transaction(struct acpi_ec *ec)
+{
+	ec->curr->irq_count = ec->curr->wi = ec->curr->ri = 0;
+	ec->curr->flags = 0;
+	advance_transaction(ec);
 }
 
 static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data);

@@ -231,15 +239,17 @@ static int ec_poll(struct acpi_ec *ec)
 			/* don't sleep with disabled interrupts */
 			if (EC_FLAGS_MSI || irqs_disabled()) {
 				udelay(ACPI_EC_MSI_UDELAY);
-				if (ec_transaction_done(ec))
+				if (ec_transaction_completed(ec))
 					return 0;
 			} else {
 				if (wait_event_timeout(ec->wait,
-						ec_transaction_done(ec),
+						ec_transaction_completed(ec),
 						msecs_to_jiffies(1)))
 					return 0;
 			}
+			spin_lock_irqsave(&ec->lock, flags);
 			advance_transaction(ec);
+			spin_unlock_irqrestore(&ec->lock, flags);
 		} while (time_before(jiffies, delay));
 		pr_debug("controller reset, restart transaction\n");
 		spin_lock_irqsave(&ec->lock, flags);

@@ -637,10 +647,13 @@ static int ec_check_sci(struct acpi_ec *ec, u8 state)
 static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
 	u32 gpe_number, void *data)
 {
+	unsigned long flags;
 	struct acpi_ec *ec = data;
 
+	spin_lock_irqsave(&ec->lock, flags);
 	advance_transaction(ec);
-	if (ec_transaction_done(ec) &&
+	spin_unlock_irqrestore(&ec->lock, flags);
+	if (ec_transaction_completed(ec) &&
 	    (acpi_ec_read_status(ec) & ACPI_EC_FLAG_IBF) == 0) {
 		wake_up(&ec->wait);
 		ec_check_sci(ec, acpi_ec_read_status(ec));
Signature · 1 line
1.7.10
author
#4 lore raw

PATCH v2 3/7 ACPI/EC: Remove duplicated ec_wait_ibf0() waiter.

In reply to Lv Zheng

From: Lv Zheng <[email protected]>

After we've added the first command byte write into advance_transaction(), the IBF=0 waiter is duplicated with the command completion waiter implemented in the ec_poll() because:
If IBF=1 blocked the first command byte write invoked in the task context ec_poll(), it would be kicked off upon IBF=0 interrupt or timed out and retried again in the task context.
This patch removes this seperate and duplicate IBF=0 waiter, by doing so, we can reduce the overall number of times to access the EC_SC(R) status register.

Reference: https://bugzilla.kernel.org/show_bug.cgi?id=70891

Signed-off-by Lv Zheng <[email protected]>
Reported-and-tested-by Gareth Williams <[email protected]>
Tested-by Steffen Weber <[email protected]>
[zetalog: first affected by:]
Cc <[email protected]> # 2.6.11: 7c6db4e0: ACPI: EC: do transaction from interrupt context
[zetalog: cleanly applying to:]
Cc <[email protected]> # 3.14.x: 42b946bb: ACPI / EC: disable GPE before removing GPE handler
---
 drivers/acpi/ec.c |   27 +--------------------------
 1 file changed, 1 insertion(+), 26 deletions(-)
drivers/acpi/ec.c +1 -26
index f09386e..d016ea3 100644
@@ -281,23 +281,6 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
 	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 ec_wait_ibf0(struct acpi_ec *ec)
-{
-	unsigned long delay = jiffies + msecs_to_jiffies(ec_delay);
-	/* interrupt wait manually if GPE mode is not active */
-	while (time_before(jiffies, delay))
-		if (wait_event_timeout(ec->wait, ec_check_ibf0(ec),
-					msecs_to_jiffies(1)))
-			return 0;
-	return -ETIME;
-}
-
 static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t)
 {
 	int status;

@@ -318,12 +301,6 @@ static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t)
 			goto unlock;
 		}
 	}
-	if (ec_wait_ibf0(ec)) {
-		pr_err("input buffer is not empty, "
-				"aborting transaction\n");
-		status = -ETIME;
-		goto end;
-	}
 	pr_debug("transaction start (cmd=0x%02x, addr=0x%02x)\n",
 			t->command, t->wdata ? t->wdata[0] : 0);
 	/* disable GPE during transaction if storm is detected */

@@ -347,7 +324,6 @@ static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t)
 		set_bit(EC_FLAGS_GPE_STORM, &ec->flags);
 	}
 	pr_debug("transaction end\n");
-end:
 	if (ec->global_lock)
 		acpi_release_global_lock(glk);
 unlock:

@@ -653,8 +629,7 @@ static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
 	spin_lock_irqsave(&ec->lock, flags);
 	advance_transaction(ec);
 	spin_unlock_irqrestore(&ec->lock, flags);
-	if (ec_transaction_completed(ec) &&
-	    (acpi_ec_read_status(ec) & ACPI_EC_FLAG_IBF) == 0) {
+	if (ec_transaction_completed(ec)) {
 		wake_up(&ec->wait);
 		ec_check_sci(ec, acpi_ec_read_status(ec));
 	}
Signature · 1 line
1.7.10
author
#5 lore raw

PATCH v2 4/7 ACPI/EC: Fix a race condition in ec_transaction_completed().

In reply to Lv Zheng

From: Lv Zheng <[email protected]>

There is a race condition in ec_transaction_completed().

When ec_transaction_completed() is called in the GPE handler, it could return true because of (ec->curr == NULL). Then the wake_up() invocation could complete the next command unexpectedly since there is no lock between the 2 invocations. With the previous cleanup, we now needn't handle IBF=0 waiter race. It's now safe for us to return a flag from advance_condition() to indicate the requirement of wakeup, the flag is returned from a locked context.

The ec_transaction_completed() now is only invoked by the ec_poll() where the ec->curr is ensured to be !NULL.

After cleaning up, the EVT_SCI=1 check should be moved out of the wakeup condition so that an EVT_SCI raised with (ec->curr == NULL) can trigger a QR_SC command.

Reference: https://bugzilla.kernel.org/show_bug.cgi?id=70891

Signed-off-by Lv Zheng <[email protected]>
Reported-and-tested-by Gareth Williams <[email protected]>
Tested-by Steffen Weber <[email protected]>
[zetalog: first affected by:]
Cc <[email protected]> # 2.6.11: 7c6db4e0: ACPI: EC: do transaction from interrupt context
[zetalog: cleanly applying to:]
Cc <[email protected]> # 3.14.x: 42b946bb: ACPI / EC: disable GPE before removing GPE handler
---
 drivers/acpi/ec.c |   30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)
drivers/acpi/ec.c +17 -13
index d016ea3..49d8990 100644
@@ -158,16 +158,17 @@ static int ec_transaction_completed(struct acpi_ec *ec)
 	unsigned long flags;
 	int ret = 0;
 	spin_lock_irqsave(&ec->lock, flags);
-	if (!ec->curr || (ec->curr->flags & ACPI_EC_COMMAND_COMPLETE))
+	if (ec->curr && (ec->curr->flags & ACPI_EC_COMMAND_COMPLETE))
 		ret = 1;
 	spin_unlock_irqrestore(&ec->lock, flags);
 	return ret;
 }
 
-static void advance_transaction(struct acpi_ec *ec)
+static bool advance_transaction(struct acpi_ec *ec)
 {
 	struct transaction *t;
 	u8 status;
+	bool wakeup = false;
 
 	pr_debug("===== %s =====\n", in_interrupt() ? "IRQ" : "TASK");
 	status = acpi_ec_read_status(ec);

@@ -183,21 +184,25 @@ static void advance_transaction(struct acpi_ec *ec)
 		} else if (t->rlen > t->ri) {
 			if ((status & ACPI_EC_FLAG_OBF) == 1) {
 				t->rdata[t->ri++] = acpi_ec_read_data(ec);
-				if (t->rlen == t->ri)
+				if (t->rlen == t->ri) {
 					t->flags |= ACPI_EC_COMMAND_COMPLETE;
+					wakeup = true;
+				}
 			} else
 				goto err;
 		} else if (t->wlen == t->wi &&
-			   (status & ACPI_EC_FLAG_IBF) == 0)
+			   (status & ACPI_EC_FLAG_IBF) == 0) {
 			t->flags |= ACPI_EC_COMMAND_COMPLETE;
-		return;
+			wakeup = true;
+		}
+		return wakeup;
 	} else {
 		if ((status & ACPI_EC_FLAG_IBF) == 0) {
 			acpi_ec_write_cmd(ec, t->command);
 			t->flags |= ACPI_EC_COMMAND_POLL;
 		} else
 			goto err;
-		return;
+		return wakeup;
 	}
 err:
 	/*

@@ -208,13 +213,14 @@ err:
 		if (in_interrupt() && t)
 			++t->irq_count;
 	}
+	return wakeup;
 }
 
 static void start_transaction(struct acpi_ec *ec)
 {
 	ec->curr->irq_count = ec->curr->wi = ec->curr->ri = 0;
 	ec->curr->flags = 0;
-	advance_transaction(ec);
+	(void)advance_transaction(ec);
 }
 
 static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data);

@@ -248,7 +254,7 @@ static int ec_poll(struct acpi_ec *ec)
 					return 0;
 			}
 			spin_lock_irqsave(&ec->lock, flags);
-			advance_transaction(ec);
+			(void)advance_transaction(ec);
 			spin_unlock_irqrestore(&ec->lock, flags);
 		} while (time_before(jiffies, delay));
 		pr_debug("controller reset, restart transaction\n");

@@ -627,12 +633,10 @@ static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
 	struct acpi_ec *ec = data;
 
 	spin_lock_irqsave(&ec->lock, flags);
-	advance_transaction(ec);
-	spin_unlock_irqrestore(&ec->lock, flags);
-	if (ec_transaction_completed(ec)) {
+	if (advance_transaction(ec))
 		wake_up(&ec->wait);
-		ec_check_sci(ec, acpi_ec_read_status(ec));
-	}
+	spin_unlock_irqrestore(&ec->lock, flags);
+	ec_check_sci(ec, acpi_ec_read_status(ec));
 	return ACPI_INTERRUPT_HANDLED | ACPI_REENABLE_GPE;
 }
Signature · 1 line
1.7.10
author
#6 lore raw

PATCH v2 5/7 ACPI/EC: Update revision due to full asynchrnous command support.

In reply to Lv Zheng

From: Lv Zheng <[email protected]>

The bug fixes and asynchronous improvements have been done to the EC driver by the previous commits. This patch increases the revision to 2.2 to indicate the behavior differences between the old and the new drivers. The copyright/authorship notices are also updated.

Authorship of Alexey is updated according to the following diff block:

  - *  ec.c - ACPI Embedded Controller Driver (v2.0)
  + *  ec.c - ACPI Embedded Controller Driver (v2.1)
  - *  Copyright (C) 2006, 2007 Alexey Starikovskiy <[email protected]>
  + *  Copyright (C) 2006-2008 Alexey Starikovskiy <[email protected]>

This change is made by the following commit:
Commit: 7c6db4e050601f359081fde418ca6dc4fc2d0011
Subject: ACPI: EC: do transaction from interrupt context

Signed-off-by Lv Zheng <[email protected]>
---
 drivers/acpi/ec.c |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)
drivers/acpi/ec.c +9 -6
index 49d8990..051ac47 100644
@@ -1,11 +1,14 @@
 /*
- *  ec.c - ACPI Embedded Controller Driver (v2.1)
+ *  ec.c - ACPI Embedded Controller Driver (v2.2)
  *
- *  Copyright (C) 2006-2008 Alexey Starikovskiy <[email protected]>
- *  Copyright (C) 2006 Denis Sadykov <[email protected]>
- *  Copyright (C) 2004 Luming Yu <[email protected]>
- *  Copyright (C) 2001, 2002 Andy Grover <[email protected]>
- *  Copyright (C) 2001, 2002 Paul Diefenbaugh <[email protected]>
+ *  Copyright (C) 2001-2014 Intel Corporation
+ *    Auther: 2014       Lv Zheng <[email protected]>
+ *            2006, 2007 Alexey Starikovskiy <[email protected]>
+ *            2006       Denis Sadykov <[email protected]>
+ *            2004       Luming Yu <[email protected]>
+ *            2001, 2002 Andy Grover <[email protected]>
+ *            2001, 2002 Paul Diefenbaugh <[email protected]>
+ *  Copyright (C) 2008      Alexey Starikovskiy <[email protected]>
  *
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  *
Signature · 1 line
1.7.10
author
#7 lore raw

PATCH v2 6/7 ACPICA: Events: Fix edge-triggered GPE by disabling before acknowledging it.

In reply to Lv Zheng

From: Lv Zheng <[email protected]>

Due to ACPI specificiation 5, chapter 5.6.4 General-Purpose Event Hnadling, OSPMs need to disable GPE before clearing the status bit for edge-triggered GPEs.

Signed-off-by Lv Zheng <[email protected]>
Tested-by Gareth Williams <[email protected]>
Tested-by Steffen Weber <[email protected]>
---
 drivers/acpi/acpica/evgpe.c |   32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)
drivers/acpi/acpica/evgpe.c +17 -15
index 48f7001..e4ba4de 100644
@@ -698,21 +698,6 @@ acpi_ev_gpe_dispatch(struct acpi_namespace_node *gpe_device,
 	}
 
 	/*
-	 * If edge-triggered, clear the GPE status bit now. Note that
-	 * level-triggered events are cleared after the GPE is serviced.
-	 */
-	if ((gpe_event_info->flags & ACPI_GPE_XRUPT_TYPE_MASK) ==
-	    ACPI_GPE_EDGE_TRIGGERED) {
-		status = acpi_hw_clear_gpe(gpe_event_info);
-		if (ACPI_FAILURE(status)) {
-			ACPI_EXCEPTION((AE_INFO, status,
-					"Unable to clear GPE %02X",
-					gpe_number));
-			return_UINT32(ACPI_INTERRUPT_NOT_HANDLED);
-		}
-	}
-
-	/*
 	 * Always disable the GPE so that it does not keep firing before
 	 * any asynchronous activity completes (either from the execution
 	 * of a GPE method or an asynchronous GPE handler.)

@@ -729,6 +714,23 @@ acpi_ev_gpe_dispatch(struct acpi_namespace_node *gpe_device,
 	}
 
 	/*
+	 * If edge-triggered, clear the GPE status bit now. Note that
+	 * level-triggered events are cleared after the GPE is serviced.
+	 */
+	if ((gpe_event_info->flags & ACPI_GPE_XRUPT_TYPE_MASK) ==
+	    ACPI_GPE_EDGE_TRIGGERED) {
+		status = acpi_hw_clear_gpe(gpe_event_info);
+		if (ACPI_FAILURE(status)) {
+			ACPI_EXCEPTION((AE_INFO, status,
+					"Unable to clear GPE %02X",
+					gpe_number));
+			(void)acpi_hw_low_set_gpe(gpe_event_info,
+						  ACPI_GPE_CONDITIONAL_ENABLE);
+			return_UINT32(ACPI_INTERRUPT_NOT_HANDLED);
+		}
+	}
+
+	/*
 	 * Dispatch the GPE to either an installed handler or the control
 	 * method associated with this GPE (_Lxx or _Exx). If a handler
 	 * exists, we invoke it and do not attempt to run the method.
Signature · 1 line
1.7.10
author
#8 lore raw

PATCH v2 7/7 ACPI/EC: Add detailed fields debugging support of EC_SC(R).

In reply to Lv Zheng

From: Lv Zheng <[email protected]>

Developers really don't need to translate EC_SC(R) in mind as long as the field details are decoded in the debugging message.

Signed-off-by Lv Zheng <[email protected]>
Tested-by Gareth Williams <[email protected]>
Tested-by Steffen Weber <[email protected]>
---
 drivers/acpi/ec.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)
drivers/acpi/ec.c +12 -4
index 051ac47..c3299f6 100644
@@ -55,6 +55,7 @@
 /* EC status register */
 #define ACPI_EC_FLAG_OBF	0x01	/* Output buffer full */
 #define ACPI_EC_FLAG_IBF	0x02	/* Input buffer full */
+#define ACPI_EC_FLAG_CMD	0x08	/* Input buffer contains a command */
 #define ACPI_EC_FLAG_BURST	0x10	/* burst mode */
 #define ACPI_EC_FLAG_SCI	0x20	/* EC-SCI occurred */
 

@@ -133,26 +134,33 @@ static int EC_FLAGS_CLEAR_ON_RESUME; /* Needs acpi_ec_clear() on boot/resume */
 static inline u8 acpi_ec_read_status(struct acpi_ec *ec)
 {
 	u8 x = inb(ec->command_addr);
-	pr_debug("---> status = 0x%2.2x\n", x);
+	pr_debug("EC_SC(R) = 0x%2.2x "
+		 "SCI_EVT=%d BURST=%d CMD=%d IBF=%d OBF=%d\n",
+		 x,
+		 !!(x & ACPI_EC_FLAG_SCI),
+		 !!(x & ACPI_EC_FLAG_BURST),
+		 !!(x & ACPI_EC_FLAG_CMD),
+		 !!(x & ACPI_EC_FLAG_IBF),
+		 !!(x & ACPI_EC_FLAG_OBF));
 	return x;
 }
 
 static inline u8 acpi_ec_read_data(struct acpi_ec *ec)
 {
 	u8 x = inb(ec->data_addr);
-	pr_debug("---> data = 0x%2.2x\n", x);
+	pr_debug("EC_DATA(R) = 0x%2.2x\n", x);
 	return x;
 }
 
 static inline void acpi_ec_write_cmd(struct acpi_ec *ec, u8 command)
 {
-	pr_debug("<--- command = 0x%2.2x\n", command);
+	pr_debug("EC_SC(W) = 0x%2.2x\n", command);
 	outb(command, ec->command_addr);
 }
 
 static inline void acpi_ec_write_data(struct acpi_ec *ec, u8 data)
 {
-	pr_debug("<--- data = 0x%2.2x\n", data);
+	pr_debug("EC_DATA(W) = 0x%2.2x\n", data);
 	outb(data, ec->data_addr);
 }
Signature · 1 line
1.7.10
#9 lore raw

v2 cover ACPI/EC: Critical race fixes.

In reply to Lv Zheng

History:
v1:
The collection of bug-fixes from kernel bugzilla bug 70891.
v2:
1. Improves the patch descriptions to make the root cause of the 70891 bug more correct.
2. Improves a race fix to make it more robust.
v2 UPDATE:
Added "Reported-by" and "Tested-by" from kernel bugzilla bug 59911/63931 as a result of EC bug triage. Only patch descriptions are updated. Removed the GPE fix, it is included by another GPE API fix series.

This patchset includes the following improvements of the ACPI/EC driver:
1. Fixes 2 race conditions in the current EC driver.
2. Cleans up the first command byte write as one of the race condition fix requires additional cleanup for the first command byte write. And this also helps us to do improvements in the BURST mode in the future.
3. Improves debugging message, register details can be enabled by defining DEBUG. And bug fixes and reports are based on this improvement.
The EC driver is upgraded to v2.2 to indicate the difference from the previous buggy one, so that the future bug reports can be correctly categorized.

All patches are tested to be valid fixes for the following bugs:
https://bugzilla.kernel.org/show_bug.cgi?id=59911
https://bugzilla.kernel.org/show_bug.cgi?id=63931
https://bugzilla.kernel.org/show_bug.cgi?id=70891

Lv Zheng (6):
ACPI/EC: Fix an issue that advance_transaction() processes stale
hardware status.
ACPI/EC: Add asynchronous command byte write support. ACPI/EC: Remove duplicated ec_wait_ibf0() waiter. ACPI/EC: Fix a race condition in ec_transaction_completed(). ACPI/EC: Update revision due to full asynchrnous command support. ACPI/EC: Add detailed fields debugging support of EC_SC(R).

 drivers/acpi/ec.c |  159 +++++++++++++++++++++++++++--------------------------
 1 file changed, 81 insertions(+), 78 deletions(-)
Signature · 1 line
1.7.10
#10 lore raw

PATCH v2 1/6 ACPI/EC: Fix an issue that advance_transaction() processes stale hardware status.

The advance_transaction() will be invoked from the IRQ context GPE handler and the task context ec_poll(). The handling of this function is locked so that the EC state machine are ensured to be advanced sequentially.

But there is a problem. Before invoking advance_transaction(), EC_SC(R) is read. Then for advance_transaction(), there could be race condition around the lock from both contexts. The first one reading the register could fail this race and when it passes the stale register value to the state machine advancement code, the hardware condition is totally different from when the register is read. And the hardware accesses determined from the wrong hardware status can break the EC state machine. And there could be cases that the functionalities of the platform firmware are seriously affected. For example:
1. When 2 EC_DATA(W) writes compete the IBF=0, the 2nd EC_DATA(W) write may be invalid due to IBF=1 after the 1st EC_DATA(W) write. Then the hardware will either refuse to respond a next EC_SC(W) write of the next command or discard the current WR_EC command when it receives a EC_SC(W) write of the next command.
2. When 1 EC_SC(W) write and 1 EC_DATA(W) write compete the IBF=0, the EC_DATA(W) write may be invalid due to IBF=1 after the EC_SC(W) write. The next EC_DATA(R) could never be responded by the hardware. This is the root cause of the reported issue in 70891.

This patch fixes this issue by moving the EC_SC(R) access into the lock so that we can ensure that the state machine is advanced under the right condition.

Reference: https://bugzilla.kernel.org/show_bug.cgi?id=70891
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=63931
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=59911

Signed-off-by Lv Zheng <[email protected]>
Reported-and-tested-by Gareth Williams <[email protected]>
Reported-and-tested-by Hans de Goede <[email protected]>
Reported-by Barton Xu <[email protected]>
Tested-by Arthur Chen <[email protected]>
Tested-by Steffen Weber <[email protected]>
[zetalog: first affected by:]
Cc <[email protected]> # 2.6.11: 7c6db4e0: ACPI: EC: do transaction from interrupt context
[zetalog: cleanly applying to:]
Cc <[email protected]> # 3.14.x: 42b946bb: ACPI / EC: disable GPE before removing GPE handler
---
 drivers/acpi/ec.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/acpi/ec.c +6 -6
index ad11ba4..762b4cc 100644
@@ -168,12 +168,15 @@ static void start_transaction(struct acpi_ec *ec)
 	acpi_ec_write_cmd(ec, ec->curr->command);
 }
 
-static void advance_transaction(struct acpi_ec *ec, u8 status)
+static void advance_transaction(struct acpi_ec *ec)
 {
 	unsigned long flags;
 	struct transaction *t;
+	u8 status;
 
 	spin_lock_irqsave(&ec->lock, flags);
+	pr_debug("===== %s =====\n", in_interrupt() ? "IRQ" : "TASK");
+	status = acpi_ec_read_status(ec);
 	t = ec->curr;
 	if (!t)
 		goto unlock;

@@ -236,7 +239,7 @@ static int ec_poll(struct acpi_ec *ec)
 						msecs_to_jiffies(1)))
 					return 0;
 			}
-			advance_transaction(ec, acpi_ec_read_status(ec));
+			advance_transaction(ec);
 		} while (time_before(jiffies, delay));
 		pr_debug("controller reset, restart transaction\n");
 		spin_lock_irqsave(&ec->lock, flags);

@@ -635,11 +638,8 @@ static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
 	u32 gpe_number, void *data)
 {
 	struct acpi_ec *ec = data;
-	u8 status = acpi_ec_read_status(ec);
-
-	pr_debug("~~~> interrupt, status:0x%02x\n", status);
 
-	advance_transaction(ec, status);
+	advance_transaction(ec);
 	if (ec_transaction_done(ec) &&
 	    (acpi_ec_read_status(ec) & ACPI_EC_FLAG_IBF) == 0) {
 		wake_up(&ec->wait);
Signature · 1 line
1.7.10
#11 lore raw

PATCH v2 2/6 ACPI/EC: Add asynchronous command byte write support.

In reply to Lv Zheng

This patch moves the first command byte write into advance_transaction() so that all EC register accesses that can affect the command processing state machine can happen in this asynchronous state machine advancement function.

The advance_transaction() function then can be a complete implementation of an asyncrhonous transaction for a single command so that:
1. The first command byte can be written in the interrupt context;
2. The command completion waiter can also be used to wait the first command byte's timeout;
3. In BURST mode, the follow-up command bytes can be written in the interrupt context directly, so that it doesn't need to return to the task context. Returning to the task context reduces the throughput of the BURST mode and in the worst cases where the system workload is very high, this leads to the hardware driven automatic BURST mode exit.

In order not to increase memory consumptions, this patch converts 'done' into 'flags' to contain multiple indications:
1. ACPI_EC_COMMAND_COMPLETE: converting from original 'done' condition, indicating the completion of the command transaction.
2. ACPI_EC_COMMAND_POLL: indicating the availability of writing the first command byte. A new command can utilize this flag to compete for the right of accessing the underlying hardware. There is a follow-up bug fix that has utilized this new flag.

The 2 flags are important because it also reflects a key concept of IO programs' design used in the system softwares. Normally an IO program running in the kernel should first be implemented in the asynchronous way. And the 2 flags are the most common way to implement its synchronous operations on top of the asynchronous operations:
1. POLL: This flag can be used to block until the asynchronous operations can happen.
2. COMPLETE: This flag can be used to block until the asynchronous operations have completed.
By constructing code cleanly in this way, many difficult problems can be solved smoothly.

[zetalog: changelog updated]
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=70891
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=63931
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=59911

Signed-off-by Lv Zheng <[email protected]>
Reported-and-tested-by Gareth Williams <[email protected]>
Reported-and-tested-by Hans de Goede <[email protected]>
Reported-by Barton Xu <[email protected]>
Tested-by Arthur Chen <[email protected]>
Tested-by Steffen Weber <[email protected]>
[zetalog: first affected by:]
Cc <[email protected]> # 2.6.11: 7c6db4e0: ACPI: EC: do transaction from interrupt context
[zetalog: cleanly applying to:]
Cc <[email protected]> # 3.14.x: 42b946bb: ACPI / EC: disable GPE before removing GPE handler
---
 drivers/acpi/ec.c |   83 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 48 insertions(+), 35 deletions(-)
drivers/acpi/ec.c +48 -35
index 762b4cc..f09386e 100644
@@ -78,6 +78,9 @@ enum {
 	EC_FLAGS_BLOCKED,		/* Transactions are blocked */
 };
 
+#define ACPI_EC_COMMAND_POLL		0x01 /* Available for command byte */
+#define ACPI_EC_COMMAND_COMPLETE	0x02 /* Completed last byte */
+
 /* ec.c is compiled in acpi namespace so this shows up as acpi.ec_delay param */
 static unsigned int ec_delay __read_mostly = ACPI_EC_DELAY;
 module_param(ec_delay, uint, 0644);

@@ -109,7 +112,7 @@ struct transaction {
 	u8 ri;
 	u8 wlen;
 	u8 rlen;
-	bool done;
+	u8 flags;
 };
 
 struct acpi_ec *boot_ec, *first_ec;

@@ -150,63 +153,68 @@ static inline void acpi_ec_write_data(struct acpi_ec *ec, u8 data)
 	outb(data, ec->data_addr);
 }
 
-static int ec_transaction_done(struct acpi_ec *ec)
+static int ec_transaction_completed(struct acpi_ec *ec)
 {
 	unsigned long flags;
 	int ret = 0;
 	spin_lock_irqsave(&ec->lock, flags);
-	if (!ec->curr || ec->curr->done)
+	if (!ec->curr || (ec->curr->flags & ACPI_EC_COMMAND_COMPLETE))
 		ret = 1;
 	spin_unlock_irqrestore(&ec->lock, flags);
 	return ret;
 }
 
-static void start_transaction(struct acpi_ec *ec)
-{
-	ec->curr->irq_count = ec->curr->wi = ec->curr->ri = 0;
-	ec->curr->done = false;
-	acpi_ec_write_cmd(ec, ec->curr->command);
-}
-
 static void advance_transaction(struct acpi_ec *ec)
 {
-	unsigned long flags;
 	struct transaction *t;
 	u8 status;
 
-	spin_lock_irqsave(&ec->lock, flags);
 	pr_debug("===== %s =====\n", in_interrupt() ? "IRQ" : "TASK");
 	status = acpi_ec_read_status(ec);
 	t = ec->curr;
 	if (!t)
-		goto unlock;
-	if (t->wlen > t->wi) {
-		if ((status & ACPI_EC_FLAG_IBF) == 0)
-			acpi_ec_write_data(ec,
-				t->wdata[t->wi++]);
-		else
-			goto err;
-	} else if (t->rlen > t->ri) {
-		if ((status & ACPI_EC_FLAG_OBF) == 1) {
-			t->rdata[t->ri++] = acpi_ec_read_data(ec);
-			if (t->rlen == t->ri)
-				t->done = true;
+		goto err;
+	if (t->flags & ACPI_EC_COMMAND_POLL) {
+		if (t->wlen > t->wi) {
+			if ((status & ACPI_EC_FLAG_IBF) == 0)
+				acpi_ec_write_data(ec, t->wdata[t->wi++]);
+			else
+				goto err;
+		} else if (t->rlen > t->ri) {
+			if ((status & ACPI_EC_FLAG_OBF) == 1) {
+				t->rdata[t->ri++] = acpi_ec_read_data(ec);
+				if (t->rlen == t->ri)
+					t->flags |= ACPI_EC_COMMAND_COMPLETE;
+			} else
+				goto err;
+		} else if (t->wlen == t->wi &&
+			   (status & ACPI_EC_FLAG_IBF) == 0)
+			t->flags |= ACPI_EC_COMMAND_COMPLETE;
+		return;
+	} else {
+		if ((status & ACPI_EC_FLAG_IBF) == 0) {
+			acpi_ec_write_cmd(ec, t->command);
+			t->flags |= ACPI_EC_COMMAND_POLL;
 		} else
 			goto err;
-	} else if (t->wlen == t->wi &&
-		   (status & ACPI_EC_FLAG_IBF) == 0)
-		t->done = true;
-	goto unlock;
+		return;
+	}
 err:
 	/*
 	 * If SCI bit is set, then don't think it's a false IRQ
 	 * otherwise will take a not handled IRQ as a false one.
 	 */
-	if (in_interrupt() && !(status & ACPI_EC_FLAG_SCI))
-		++t->irq_count;
+	if (!(status & ACPI_EC_FLAG_SCI)) {
+		if (in_interrupt() && t)
+			++t->irq_count;
+	}
+}
 
-unlock:
-	spin_unlock_irqrestore(&ec->lock, flags);
+static void start_transaction(struct acpi_ec *ec)
+{
+	ec->curr->irq_count = ec->curr->wi = ec->curr->ri = 0;
+	ec->curr->flags = 0;
+	advance_transaction(ec);
 }
 
 static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data);

@@ -231,15 +239,17 @@ static int ec_poll(struct acpi_ec *ec)
 			/* don't sleep with disabled interrupts */
 			if (EC_FLAGS_MSI || irqs_disabled()) {
 				udelay(ACPI_EC_MSI_UDELAY);
-				if (ec_transaction_done(ec))
+				if (ec_transaction_completed(ec))
 					return 0;
 			} else {
 				if (wait_event_timeout(ec->wait,
-						ec_transaction_done(ec),
+						ec_transaction_completed(ec),
 						msecs_to_jiffies(1)))
 					return 0;
 			}
+			spin_lock_irqsave(&ec->lock, flags);
 			advance_transaction(ec);
+			spin_unlock_irqrestore(&ec->lock, flags);
 		} while (time_before(jiffies, delay));
 		pr_debug("controller reset, restart transaction\n");
 		spin_lock_irqsave(&ec->lock, flags);

@@ -637,10 +647,13 @@ static int ec_check_sci(struct acpi_ec *ec, u8 state)
 static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
 	u32 gpe_number, void *data)
 {
+	unsigned long flags;
 	struct acpi_ec *ec = data;
 
+	spin_lock_irqsave(&ec->lock, flags);
 	advance_transaction(ec);
-	if (ec_transaction_done(ec) &&
+	spin_unlock_irqrestore(&ec->lock, flags);
+	if (ec_transaction_completed(ec) &&
 	    (acpi_ec_read_status(ec) & ACPI_EC_FLAG_IBF) == 0) {
 		wake_up(&ec->wait);
 		ec_check_sci(ec, acpi_ec_read_status(ec));
Signature · 1 line
1.7.10
#12 lore raw

PATCH v2 3/6 ACPI/EC: Remove duplicated ec_wait_ibf0() waiter.

In reply to Lv Zheng

After we've added the first command byte write into advance_transaction(), the IBF=0 waiter is duplicated with the command completion waiter implemented in the ec_poll() because:
If IBF=1 blocked the first command byte write invoked in the task context ec_poll(), it would be kicked off upon IBF=0 interrupt or timed out and retried again in the task context.
This patch removes this seperate and duplicate IBF=0 waiter, by doing so, we can reduce the overall number of times to access the EC_SC(R) status register.

Reference: https://bugzilla.kernel.org/show_bug.cgi?id=70891
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=63931
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=59911

Signed-off-by Lv Zheng <[email protected]>
Reported-and-tested-by Gareth Williams <[email protected]>
Reported-and-tested-by Hans de Goede <[email protected]>
Reported-by Barton Xu <[email protected]>
Tested-by Arthur Chen <[email protected]>
Tested-by Steffen Weber <[email protected]>
[zetalog: first affected by:]
Cc <[email protected]> # 2.6.11: 7c6db4e0: ACPI: EC: do transaction from interrupt context
[zetalog: cleanly applying to:]
Cc <[email protected]> # 3.14.x: 42b946bb: ACPI / EC: disable GPE before removing GPE handler
---
 drivers/acpi/ec.c |   27 +--------------------------
 1 file changed, 1 insertion(+), 26 deletions(-)
drivers/acpi/ec.c +1 -26
index f09386e..d016ea3 100644
@@ -281,23 +281,6 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
 	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 ec_wait_ibf0(struct acpi_ec *ec)
-{
-	unsigned long delay = jiffies + msecs_to_jiffies(ec_delay);
-	/* interrupt wait manually if GPE mode is not active */
-	while (time_before(jiffies, delay))
-		if (wait_event_timeout(ec->wait, ec_check_ibf0(ec),
-					msecs_to_jiffies(1)))
-			return 0;
-	return -ETIME;
-}
-
 static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t)
 {
 	int status;

@@ -318,12 +301,6 @@ static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t)
 			goto unlock;
 		}
 	}
-	if (ec_wait_ibf0(ec)) {
-		pr_err("input buffer is not empty, "
-				"aborting transaction\n");
-		status = -ETIME;
-		goto end;
-	}
 	pr_debug("transaction start (cmd=0x%02x, addr=0x%02x)\n",
 			t->command, t->wdata ? t->wdata[0] : 0);
 	/* disable GPE during transaction if storm is detected */

@@ -347,7 +324,6 @@ static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t)
 		set_bit(EC_FLAGS_GPE_STORM, &ec->flags);
 	}
 	pr_debug("transaction end\n");
-end:
 	if (ec->global_lock)
 		acpi_release_global_lock(glk);
 unlock:

@@ -653,8 +629,7 @@ static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
 	spin_lock_irqsave(&ec->lock, flags);
 	advance_transaction(ec);
 	spin_unlock_irqrestore(&ec->lock, flags);
-	if (ec_transaction_completed(ec) &&
-	    (acpi_ec_read_status(ec) & ACPI_EC_FLAG_IBF) == 0) {
+	if (ec_transaction_completed(ec)) {
 		wake_up(&ec->wait);
 		ec_check_sci(ec, acpi_ec_read_status(ec));
 	}
Signature · 1 line
1.7.10
#13 lore raw

PATCH v2 4/6 ACPI/EC: Fix a race condition in ec_transaction_completed().

In reply to Lv Zheng

There is a race condition in ec_transaction_completed().

When ec_transaction_completed() is called in the GPE handler, it could return true because of (ec->curr == NULL). Then the wake_up() invocation could complete the next command unexpectedly since there is no lock between the 2 invocations. This is the root cause of the reported issue in 63931.

With the previous cleanup, we now needn't handle IBF=0 waiter race. It's now safe for us to return a flag from advance_condition() to indicate the requirement of wakeup, the flag is returned from a locked context.

The ec_transaction_completed() now is only invoked by the ec_poll() where the ec->curr is ensured to be !NULL.

After cleaning up, the EVT_SCI=1 check should be moved out of the wakeup condition so that an EVT_SCI raised with (ec->curr == NULL) can trigger a QR_SC command.

Reference: https://bugzilla.kernel.org/show_bug.cgi?id=70891
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=63931
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=59911

Signed-off-by Lv Zheng <[email protected]>
Reported-and-tested-by Gareth Williams <[email protected]>
Reported-and-tested-by Hans de Goede <[email protected]>
Reported-by Barton Xu <[email protected]>
Tested-by Arthur Chen <[email protected]>
Tested-by Steffen Weber <[email protected]>
[zetalog: first affected by:]
Cc <[email protected]> # 2.6.11: 7c6db4e0: ACPI: EC: do transaction from interrupt context
[zetalog: cleanly applying to:]
Cc <[email protected]> # 3.14.x: 42b946bb: ACPI / EC: disable GPE before removing GPE handler
---
 drivers/acpi/ec.c |   30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)
drivers/acpi/ec.c +17 -13
index d016ea3..49d8990 100644
@@ -158,16 +158,17 @@ static int ec_transaction_completed(struct acpi_ec *ec)
 	unsigned long flags;
 	int ret = 0;
 	spin_lock_irqsave(&ec->lock, flags);
-	if (!ec->curr || (ec->curr->flags & ACPI_EC_COMMAND_COMPLETE))
+	if (ec->curr && (ec->curr->flags & ACPI_EC_COMMAND_COMPLETE))
 		ret = 1;
 	spin_unlock_irqrestore(&ec->lock, flags);
 	return ret;
 }
 
-static void advance_transaction(struct acpi_ec *ec)
+static bool advance_transaction(struct acpi_ec *ec)
 {
 	struct transaction *t;
 	u8 status;
+	bool wakeup = false;
 
 	pr_debug("===== %s =====\n", in_interrupt() ? "IRQ" : "TASK");
 	status = acpi_ec_read_status(ec);

@@ -183,21 +184,25 @@ static void advance_transaction(struct acpi_ec *ec)
 		} else if (t->rlen > t->ri) {
 			if ((status & ACPI_EC_FLAG_OBF) == 1) {
 				t->rdata[t->ri++] = acpi_ec_read_data(ec);
-				if (t->rlen == t->ri)
+				if (t->rlen == t->ri) {
 					t->flags |= ACPI_EC_COMMAND_COMPLETE;
+					wakeup = true;
+				}
 			} else
 				goto err;
 		} else if (t->wlen == t->wi &&
-			   (status & ACPI_EC_FLAG_IBF) == 0)
+			   (status & ACPI_EC_FLAG_IBF) == 0) {
 			t->flags |= ACPI_EC_COMMAND_COMPLETE;
-		return;
+			wakeup = true;
+		}
+		return wakeup;
 	} else {
 		if ((status & ACPI_EC_FLAG_IBF) == 0) {
 			acpi_ec_write_cmd(ec, t->command);
 			t->flags |= ACPI_EC_COMMAND_POLL;
 		} else
 			goto err;
-		return;
+		return wakeup;
 	}
 err:
 	/*

@@ -208,13 +213,14 @@ err:
 		if (in_interrupt() && t)
 			++t->irq_count;
 	}
+	return wakeup;
 }
 
 static void start_transaction(struct acpi_ec *ec)
 {
 	ec->curr->irq_count = ec->curr->wi = ec->curr->ri = 0;
 	ec->curr->flags = 0;
-	advance_transaction(ec);
+	(void)advance_transaction(ec);
 }
 
 static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data);

@@ -248,7 +254,7 @@ static int ec_poll(struct acpi_ec *ec)
 					return 0;
 			}
 			spin_lock_irqsave(&ec->lock, flags);
-			advance_transaction(ec);
+			(void)advance_transaction(ec);
 			spin_unlock_irqrestore(&ec->lock, flags);
 		} while (time_before(jiffies, delay));
 		pr_debug("controller reset, restart transaction\n");

@@ -627,12 +633,10 @@ static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
 	struct acpi_ec *ec = data;
 
 	spin_lock_irqsave(&ec->lock, flags);
-	advance_transaction(ec);
-	spin_unlock_irqrestore(&ec->lock, flags);
-	if (ec_transaction_completed(ec)) {
+	if (advance_transaction(ec))
 		wake_up(&ec->wait);
-		ec_check_sci(ec, acpi_ec_read_status(ec));
-	}
+	spin_unlock_irqrestore(&ec->lock, flags);
+	ec_check_sci(ec, acpi_ec_read_status(ec));
 	return ACPI_INTERRUPT_HANDLED | ACPI_REENABLE_GPE;
 }
Signature · 1 line
1.7.10
#14 lore raw

PATCH v2 5/6 ACPI/EC: Update revision due to full asynchrnous command support.

In reply to Lv Zheng

The bug fixes and asynchronous improvements have been done to the EC driver by the previous commits. This patch increases the revision to 2.2 to indicate the behavior differences between the old and the new drivers. The copyright/authorship notices are also updated.

Authorship of Alexey is updated according to the following diff block:

  - *  ec.c - ACPI Embedded Controller Driver (v2.0)
  + *  ec.c - ACPI Embedded Controller Driver (v2.1)
  - *  Copyright (C) 2006, 2007 Alexey Starikovskiy <[email protected]>
  + *  Copyright (C) 2006-2008 Alexey Starikovskiy <[email protected]>

This change is made by the following commit:
Commit: 7c6db4e050601f359081fde418ca6dc4fc2d0011
Subject: ACPI: EC: do transaction from interrupt context

Signed-off-by Lv Zheng <[email protected]>
---
 drivers/acpi/ec.c |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)
drivers/acpi/ec.c +9 -6
index 49d8990..051ac47 100644
@@ -1,11 +1,14 @@
 /*
- *  ec.c - ACPI Embedded Controller Driver (v2.1)
+ *  ec.c - ACPI Embedded Controller Driver (v2.2)
  *
- *  Copyright (C) 2006-2008 Alexey Starikovskiy <[email protected]>
- *  Copyright (C) 2006 Denis Sadykov <[email protected]>
- *  Copyright (C) 2004 Luming Yu <[email protected]>
- *  Copyright (C) 2001, 2002 Andy Grover <[email protected]>
- *  Copyright (C) 2001, 2002 Paul Diefenbaugh <[email protected]>
+ *  Copyright (C) 2001-2014 Intel Corporation
+ *    Auther: 2014       Lv Zheng <[email protected]>
+ *            2006, 2007 Alexey Starikovskiy <[email protected]>
+ *            2006       Denis Sadykov <[email protected]>
+ *            2004       Luming Yu <[email protected]>
+ *            2001, 2002 Andy Grover <[email protected]>
+ *            2001, 2002 Paul Diefenbaugh <[email protected]>
+ *  Copyright (C) 2008      Alexey Starikovskiy <[email protected]>
  *
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  *
Signature · 1 line
1.7.10
#15 lore raw

PATCH v2 6/6 ACPI/EC: Add detailed fields debugging support of EC_SC(R).

In reply to Lv Zheng

Developers really don't need to translate EC_SC(R) in mind as long as the field details are decoded in the debugging message.

Signed-off-by Lv Zheng <[email protected]>
Tested-by Gareth Williams <[email protected]>
Tested-by Hans de Goede <[email protected]>
Tested-by Steffen Weber <[email protected]>
Tested-by Arthur Chen <[email protected]>
---
 drivers/acpi/ec.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)
drivers/acpi/ec.c +12 -4
index 051ac47..c3299f6 100644
@@ -55,6 +55,7 @@
 /* EC status register */
 #define ACPI_EC_FLAG_OBF	0x01	/* Output buffer full */
 #define ACPI_EC_FLAG_IBF	0x02	/* Input buffer full */
+#define ACPI_EC_FLAG_CMD	0x08	/* Input buffer contains a command */
 #define ACPI_EC_FLAG_BURST	0x10	/* burst mode */
 #define ACPI_EC_FLAG_SCI	0x20	/* EC-SCI occurred */
 

@@ -133,26 +134,33 @@ static int EC_FLAGS_CLEAR_ON_RESUME; /* Needs acpi_ec_clear() on boot/resume */
 static inline u8 acpi_ec_read_status(struct acpi_ec *ec)
 {
 	u8 x = inb(ec->command_addr);
-	pr_debug("---> status = 0x%2.2x\n", x);
+	pr_debug("EC_SC(R) = 0x%2.2x "
+		 "SCI_EVT=%d BURST=%d CMD=%d IBF=%d OBF=%d\n",
+		 x,
+		 !!(x & ACPI_EC_FLAG_SCI),
+		 !!(x & ACPI_EC_FLAG_BURST),
+		 !!(x & ACPI_EC_FLAG_CMD),
+		 !!(x & ACPI_EC_FLAG_IBF),
+		 !!(x & ACPI_EC_FLAG_OBF));
 	return x;
 }
 
 static inline u8 acpi_ec_read_data(struct acpi_ec *ec)
 {
 	u8 x = inb(ec->data_addr);
-	pr_debug("---> data = 0x%2.2x\n", x);
+	pr_debug("EC_DATA(R) = 0x%2.2x\n", x);
 	return x;
 }
 
 static inline void acpi_ec_write_cmd(struct acpi_ec *ec, u8 command)
 {
-	pr_debug("<--- command = 0x%2.2x\n", command);
+	pr_debug("EC_SC(W) = 0x%2.2x\n", command);
 	outb(command, ec->command_addr);
 }
 
 static inline void acpi_ec_write_data(struct acpi_ec *ec, u8 data)
 {
-	pr_debug("<--- data = 0x%2.2x\n", data);
+	pr_debug("EC_DATA(W) = 0x%2.2x\n", data);
 	outb(data, ec->data_addr);
 }
Signature · 1 line
1.7.10
#16 lore raw

Re: [UPDATE PATCH v2 0/6] ACPI/EC: Critical race fixes.

In reply to Lv Zheng
Quoted 10 lines · Lv Zheng

History:
v1:
The collection of bug-fixes from kernel bugzilla bug 70891.
v2:
1. Improves the patch descriptions to make the root cause of the 70891 bug more correct.
2. Improves a race fix to make it more robust.
v2 UPDATE:
Added "Reported-by" and "Tested-by" from kernel bugzilla bug 59911/63931 as a result of EC bug triage. Only patch descriptions are updated.

OK, I've updated the commits in my tree in accordance with this.

Quoted 1 line

Removed the GPE fix, it is included by another GPE API fix series.

I've applied it separately already, so please don't resend it.

Thanks,
Rafael

#17 lore raw

RE: [UPDATE PATCH v2 0/6] ACPI/EC: Critical race fixes.

Hi,

Quoted 15 lines

From: Rafael J. Wysocki [mailto:[email protected]]
Sent: Monday, July 07, 2014 7:18 PM

Quoted 10 lines · Lv Zheng

History:
v1:
The collection of bug-fixes from kernel bugzilla bug 70891.
v2:
1. Improves the patch descriptions to make the root cause of the 70891 bug more correct.
2. Improves a race fix to make it more robust.
v2 UPDATE:
Added "Reported-by" and "Tested-by" from kernel bugzilla bug 59911/63931 as a result of EC bug triage. Only patch descriptions are updated.

Quoted 5 lines

OK, I've updated the commits in my tree in accordance with this.

Quoted 1 line

Removed the GPE fix, it is included by another GPE API fix series.

I've applied it separately already, so please don't resend it.

OK.

Thanks and best regards
-Lv

Quoted 3 lines

Thanks,
Rafael