[PATCH 0/6] ACPI / EC: Fix issues related to the event handling.

[PATCH 0/6] ACPI / EC: Fix issues related to the event handling.

[Lv Zheng]

ACPI specification doesn't define the SCI_EVT clearing timing for the EC
firmware. There could be 4 possible positions the firmware may use to clear
the SCI_EVT indication:
 STATUS: After indicating SCI_EVT to the host via the status register
         (EC_SC), the target can clear SCI_EVT at any time.
 QUERY: After seeing the query request (QR_EC) written to the command
        register (EC_CMD) by the host and having prepared the responding
        event value in the data register (EC_DATA), the target can safely
        clear SCI_EVT.
 EVENT: After seeing the event response read from the data register
        (EC_DATA) by the host, the target can clear SCI_EVT.
 METHOD: After the event has been handled via the host _Qxx evaluation,
         the target can clear SCI_EVT.

The old EC driver re-checked SCI_EVT using the 3rd position, while during
the race fixes, the new EC driver was switched to re-check SCI_EVT using
the 1st position. Using the earliest position may help to reduce issues
when the other races are not fully fixed and there is an ACPI specification
statement around an expected behavior:
 The query value of zero is reserved for a spurious query result and
 indicates "no outstanding events."
This statement was the motivation of this choice.

But there are platforms do not follow this definition, since there is also
a coverity issue left in the new EC driver, the unmatched SCI_EVT clearing
timing finally was trapped by this coverity issue, and users can see that
the new EC driver stops processing further SCI_EVTs. The new EC driver then
implemented EC_FLAGS_QUERY_HANDSHAKE quirk as a workaround for such
platforms.
Now more and more reports can be seen for this SCI_EVT clearing timing
unmatched problem, we need to consider a better approach to fix all of them
instead of adding new EC_FLAGS_QUERY_HANDSHAKE quirk users.

This patchset thus finally implements 4 modes to handle the SCI_EVT
clearing timing variations. And according to the tests, chooses the next
earlier position (the 2rd position) as the default behavior.
This patchset also fixes the coverity problem so that even though the
default behavior still cannot match the Windows behavior, it is possible
for the new EC driver to proceed to handle further SCI_EVT indications.

Related bug reports are as follows:
Link: https://bugzilla.kernel.org/show_bug.cgi?id=44161
Link: https://bugzilla.kernel.org/show_bug.cgi?id=82611
Link: https://bugzilla.kernel.org/show_bug.cgi?id=94411
Link: https://bugzilla.kernel.org/show_bug.cgi?id=97381
Link: https://bugzilla.kernel.org/show_bug.cgi?id=98111

Lv Zheng (6):
  ACPI / EC: Cleanup transaction state transition.
  ACPI / EC: Cleanup _Qxx evaluation work item.
  ACPI / EC: Convert event handling work queue into loop style.
  ACPI / EC: Add event clearing variation support.
  ACPI / EC: Fix EC_FLAGS_QUERY_HANDSHAKE platforms using new event
    clearing timing.
  ACPI / EC: Fix a code coverity issue when QR_EC transactions are
    failed.

 drivers/acpi/ec.c       |  272 ++++++++++++++++++++++++++++++++++++++---------
 drivers/acpi/internal.h |    1 +
 2 files changed, 223 insertions(+), 50 deletions(-)

-- 
1.7.10

[PATCH 1/6] ACPI / EC: Cleanup transaction state transition.

[Lv Zheng]

This patch collects transaction state transition code into one function. We
then could have a single function to maintain transaction related
behaviors. No functional changes.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
Tested-by: Tigran Gabrielyan <tigrangab@gmail.com>
Tested-by: Adrien D <ghbdtn@openmailbox.org>
---
 drivers/acpi/ec.c |   23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 149b5e7..0ce8b6e8 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -391,7 +391,7 @@ static void acpi_ec_submit_query(struct acpi_ec *ec)
 
 static void acpi_ec_complete_query(struct acpi_ec *ec)
 {
-	if (ec->curr->command == ACPI_EC_COMMAND_QUERY) {
+	if (test_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
 		clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
 		ec_dbg_req("Event stopped");
 	}
@@ -421,6 +421,15 @@ static int ec_transaction_completed(struct acpi_ec *ec)
 	return ret;
 }
 
+static inline void ec_transaction_transition(struct acpi_ec *ec, unsigned long flag)
+{
+	ec->curr->flags |= flag;
+	if (ec->curr->command == ACPI_EC_COMMAND_QUERY) {
+		if (flag == ACPI_EC_COMMAND_POLL)
+			acpi_ec_complete_query(ec);
+	}
+}
+
 static void advance_transaction(struct acpi_ec *ec)
 {
 	struct transaction *t;
@@ -449,7 +458,7 @@ static void advance_transaction(struct acpi_ec *ec)
 			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;
+					ec_transaction_transition(ec, ACPI_EC_COMMAND_COMPLETE);
 					if (t->command == ACPI_EC_COMMAND_QUERY)
 						ec_dbg_req("Command(%s) hardware completion",
 							   acpi_ec_cmd_string(t->command));
@@ -459,7 +468,7 @@ static void advance_transaction(struct acpi_ec *ec)
 				goto err;
 		} else if (t->wlen == t->wi &&
 			   (status & ACPI_EC_FLAG_IBF) == 0) {
-			t->flags |= ACPI_EC_COMMAND_COMPLETE;
+			ec_transaction_transition(ec, ACPI_EC_COMMAND_COMPLETE);
 			wakeup = true;
 		}
 		goto out;
@@ -467,17 +476,15 @@ static void advance_transaction(struct acpi_ec *ec)
 		if (EC_FLAGS_QUERY_HANDSHAKE &&
 		    !(status & ACPI_EC_FLAG_SCI) &&
 		    (t->command == ACPI_EC_COMMAND_QUERY)) {
-			t->flags |= ACPI_EC_COMMAND_POLL;
-			acpi_ec_complete_query(ec);
+			ec_transaction_transition(ec, ACPI_EC_COMMAND_POLL);
 			t->rdata[t->ri++] = 0x00;
-			t->flags |= ACPI_EC_COMMAND_COMPLETE;
+			ec_transaction_transition(ec, ACPI_EC_COMMAND_COMPLETE);
 			ec_dbg_req("Command(%s) software completion",
 				   acpi_ec_cmd_string(t->command));
 			wakeup = true;
 		} else if ((status & ACPI_EC_FLAG_IBF) == 0) {
 			acpi_ec_write_cmd(ec, t->command);
-			t->flags |= ACPI_EC_COMMAND_POLL;
-			acpi_ec_complete_query(ec);
+			ec_transaction_transition(ec, ACPI_EC_COMMAND_POLL);
 		} else
 			goto err;
 		goto out;
-- 
1.7.10

[PATCH 2/6] ACPI / EC: Cleanup _Qxx evaluation work item.

[Lv Zheng]

The _Qxx evaluation work item can be eliminated and _Qxx can be evaluated
right in the same work item as the QR_EC transaction. This patch cleans up
the code to achieve this.

Originally, QR_EC transaction and _Qxx evaluation were all done in the same
work queue flushed by acpi_os_wait_events_complete(). Though the QR_EC
transaction (previous commit) and _Qxx evaluation (this commmit) are now
moved to the work queue that is not covered by
acpi_os_wait_events_complete(), there is no issue can be seen in the
suspend/resume tests.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   35 ++++++++++++-----------------------
 1 file changed, 12 insertions(+), 23 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 0ce8b6e8..b956dbc 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -904,27 +904,12 @@ void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
 }
 EXPORT_SYMBOL_GPL(acpi_ec_remove_query_handler);
 
-static void acpi_ec_run(void *cxt)
-{
-	struct acpi_ec_query_handler *handler = cxt;
-
-	if (!handler)
-		return;
-	ec_dbg_evt("Query(0x%02x) started", handler->query_bit);
-	if (handler->func)
-		handler->func(handler->data);
-	else if (handler->handle)
-		acpi_evaluate_object(handler->handle, NULL, NULL, NULL);
-	ec_dbg_evt("Query(0x%02x) stopped", handler->query_bit);
-	acpi_ec_put_query_handler(handler);
-}
-
 static int acpi_ec_query(struct acpi_ec *ec, u8 *data)
 {
 	u8 value = 0;
 	int result;
-	acpi_status status;
 	struct acpi_ec_query_handler *handler;
+	bool handler_found = false;
 	struct transaction t = {.command = ACPI_EC_COMMAND_QUERY,
 				.wdata = NULL, .rdata = &value,
 				.wlen = 0, .rlen = 1};
@@ -947,17 +932,21 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 *data)
 		if (value == handler->query_bit) {
 			/* have custom handler for this bit */
 			handler = acpi_ec_get_query_handler(handler);
-			ec_dbg_evt("Query(0x%02x) scheduled",
-				   handler->query_bit);
-			status = acpi_os_execute((handler->func) ?
-				OSL_NOTIFY_HANDLER : OSL_GPE_HANDLER,
-				acpi_ec_run, handler);
-			if (ACPI_FAILURE(status))
-				result = -EBUSY;
+			handler_found = true;
 			break;
 		}
 	}
 	mutex_unlock(&ec->mutex);
+
+	if (handler_found) {
+		ec_dbg_evt("Query(0x%02x) started", handler->query_bit);
+		if (handler->func)
+			handler->func(handler->data);
+		else if (handler->handle)
+			acpi_evaluate_object(handler->handle, NULL, NULL, NULL);
+		ec_dbg_evt("Query(0x%02x) stopped", handler->query_bit);
+		acpi_ec_put_query_handler(handler);
+	}
 	return result;
 }
 
-- 
1.7.10

[PATCH 3/6] ACPI / EC: Convert event handling work queue into loop style.

[Lv Zheng]

During the period that a work queue is scheduled (queued up for run) but
hasn't been run, second schedule_work() could fail. This may not lead to
the loss of queries because QR_EC is always ensured to be submitted after
the work queue has been in the running state.

Thus except the logging message changes, this patch is just a no-op. But
the event handling work queue can be changed into the loop style to allow
us to control the code in a more flexible way (for example, adding
event=0x00 termination condition in the loop).

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
Tested-by: Tigran Gabrielyan <tigrangab@gmail.com>
Tested-by: Adrien D <ghbdtn@openmailbox.org>
---
 drivers/acpi/ec.c       |   33 ++++++++++++++++++++++++---------
 drivers/acpi/internal.h |    1 +
 2 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index b956dbc..7a30f59 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -384,7 +384,9 @@ static bool acpi_ec_submit_flushable_request(struct acpi_ec *ec)
 static void acpi_ec_submit_query(struct acpi_ec *ec)
 {
 	if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
-		ec_dbg_req("Event started");
+		ec_dbg_evt("Command(%s) submitted/blocked",
+			   acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
+		ec->nr_pending_queries++;
 		schedule_work(&ec->work);
 	}
 }
@@ -393,7 +395,8 @@ static void acpi_ec_complete_query(struct acpi_ec *ec)
 {
 	if (test_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
 		clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
-		ec_dbg_req("Event stopped");
+		ec_dbg_evt("Command(%s) unblocked",
+			   acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
 	}
 }
 
@@ -460,8 +463,8 @@ static void advance_transaction(struct acpi_ec *ec)
 				if (t->rlen == t->ri) {
 					ec_transaction_transition(ec, ACPI_EC_COMMAND_COMPLETE);
 					if (t->command == ACPI_EC_COMMAND_QUERY)
-						ec_dbg_req("Command(%s) hardware completion",
-							   acpi_ec_cmd_string(t->command));
+						ec_dbg_evt("Command(%s) completed by hardware",
+							   acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
 					wakeup = true;
 				}
 			} else
@@ -479,8 +482,8 @@ static void advance_transaction(struct acpi_ec *ec)
 			ec_transaction_transition(ec, ACPI_EC_COMMAND_POLL);
 			t->rdata[t->ri++] = 0x00;
 			ec_transaction_transition(ec, ACPI_EC_COMMAND_COMPLETE);
-			ec_dbg_req("Command(%s) software completion",
-				   acpi_ec_cmd_string(t->command));
+			ec_dbg_evt("Command(%s) completed by software",
+				   acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
 			wakeup = true;
 		} else if ((status & ACPI_EC_FLAG_IBF) == 0) {
 			acpi_ec_write_cmd(ec, t->command);
@@ -950,11 +953,23 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 *data)
 	return result;
 }
 
-static void acpi_ec_gpe_poller(struct work_struct *work)
+static void acpi_ec_event_handler(struct work_struct *work)
 {
+	unsigned long flags;
 	struct acpi_ec *ec = container_of(work, struct acpi_ec, work);
 
-	acpi_ec_query(ec, NULL);
+	ec_dbg_evt("Event started");
+
+	spin_lock_irqsave(&ec->lock, flags);
+	while (ec->nr_pending_queries) {
+		spin_unlock_irqrestore(&ec->lock, flags);
+		(void)acpi_ec_query(ec, NULL);
+		spin_lock_irqsave(&ec->lock, flags);
+		ec->nr_pending_queries--;
+	}
+	spin_unlock_irqrestore(&ec->lock, flags);
+
+	ec_dbg_evt("Event stopped");
 }
 
 static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
@@ -1029,7 +1044,7 @@ static struct acpi_ec *make_acpi_ec(void)
 	init_waitqueue_head(&ec->wait);
 	INIT_LIST_HEAD(&ec->list);
 	spin_lock_init(&ec->lock);
-	INIT_WORK(&ec->work, acpi_ec_gpe_poller);
+	INIT_WORK(&ec->work, acpi_ec_event_handler);
 	ec->timestamp = jiffies;
 	return ec;
 }
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index cf358d4..ae919b5 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -139,6 +139,7 @@ struct acpi_ec {
 	spinlock_t lock;
 	struct work_struct work;
 	unsigned long timestamp;
+	unsigned long nr_pending_queries;
 };
 
 extern struct acpi_ec *first_ec;
-- 
1.7.10

[PATCH 4/6] ACPI / EC: Add event clearing variation support.

[Lv Zheng]

We've been suffering from the uncertainty of the SCI_EVT clearing timing.
This patch implements 4 possible modes to handle SCI_EVT clearing
variations. The old behavior is kept in this patch.

Status: QR_EC is re-checked as early as possible after checking previous
        SCI_EVT. This always leads to 2 QR_EC transactions per SCI_EVT
        indication and the target may implement event queue which returns
        0x00 indicating "no outstanding event".
Query: QR_EC is re-checked after the target has handled the QR_EC query
       request command pushed by the host.
Event: QR_EC is re-checked after the target has noticed the query event
       response data pulled by the host.
Method: QR_EC is re-checked as late as possible after completing the _Qxx
        evaluation. The target may implement SCI_EVT like a level triggered
        interrupt.

Note that, according to the reports, there are platforms that cannot be
handled using the "Status" mode without enabling the
EC_FLAGS_QUERY_HANDSHAKE quirk. But they can be handled with the other 3
modes according to the tests. See Lenovo Z50 test result.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=94411
Link: https://bugzilla.kernel.org/show_bug.cgi?id=97381
Link: https://bugzilla.kernel.org/show_bug.cgi?id=98111
Reported-and-tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
Reported-and-tested-by: Tigran Gabrielyan <tigrangab@gmail.com>
Reported-and-tested-by: Adrien D <ghbdtn@openmailbox.org>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |  157 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 152 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 7a30f59..8477626 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -59,6 +59,49 @@
 #define ACPI_EC_FLAG_BURST	0x10	/* burst mode */
 #define ACPI_EC_FLAG_SCI	0x20	/* EC-SCI occurred */
 
+/*
+ * The SCI_EVT clearing timing is not defined by the ACPI specification.
+ * This leads to lots of practical timing issues for the host EC driver.
+ * The following variations are defined (from the target EC firmware's
+ * perspective):
+ * STATUS: After indicating SCI_EVT edge triggered IRQ to the host, the
+ *         target can clear SCI_EVT at any time so long as the host can see
+ *         the indication by reading the status register (EC_SC). So the
+ *         host should re-check SCI_EVT after the first time the SCI_EVT
+ *         indication is seen, which is the same time the query request
+ *         (QR_EC) is written to the command register (EC_CMD). SCI_EVT set
+ *         at any later time could indicate another event. Normally such
+ *         kind of EC firmware has implemented an event queue and will
+ *         return 0x00 to indicate "no outstanding event".
+ * QUERY: After seeing the query request (QR_EC) written to the command
+ *        register (EC_CMD) by the host and having prepared the responding
+ *        event value in the data register (EC_DATA), the target can safely
+ *        clear SCI_EVT because the target can confirm that the current
+ *        event is being handled by the host. The host then should check
+ *        SCI_EVT right after reading the event response from the data
+ *        register (EC_DATA).
+ * EVENT: After seeing the event response read from the data register
+ *        (EC_DATA) by the host, the target can clear SCI_EVT. As the
+ *        target requires time to notice the change in the data register
+ *        (EC_DATA), the host may be required to wait additional guarding
+ *        time before checking the SCI_EVT again. Such guarding may not be
+ *        necessary if the host is notified via another IRQ.
+ * METHOD: After the event has been handled via the host _Qxx evaluation,
+ *         the target can clear SCI_EVT. Normally such kind of EC firmware
+ *         flags SCI_EVT in a level triggered way, so the host can wait
+ *         another IRQ instead of checking SCI_EVT again right after _Qxx
+ *         evaluation completes. This case can thus be covered by any of
+ *         the above modes except the overheads caused by the unwanted
+ *         queries. But if the firmware doesn't keep on raising IRQs to the
+ *         host, the host have to wait an additional guarding time and
+ *         check the SCI_EVT again. Such guarding may not be necessary
+ *         if the host is notified via another IRQ.
+ */
+#define ACPI_EC_EVT_TIMING_STATUS	0x00
+#define ACPI_EC_EVT_TIMING_QUERY	0x01
+#define ACPI_EC_EVT_TIMING_EVENT	0x02
+#define ACPI_EC_EVT_TIMING_METHOD	0x03
+
 /* EC commands */
 enum ec_command {
 	ACPI_EC_COMMAND_READ = 0x80,
@@ -76,6 +119,7 @@ enum ec_command {
 
 enum {
 	EC_FLAGS_QUERY_PENDING,		/* Query is pending */
+	EC_FLAGS_QUERY_GUARDING,	/* Guard for SCI_EVT check */
 	EC_FLAGS_HANDLERS_INSTALLED,	/* Handlers for GPE and
 					 * OpReg are installed */
 	EC_FLAGS_STARTED,		/* Driver is started */
@@ -100,6 +144,8 @@ static unsigned int ec_polling_guard __read_mostly = ACPI_EC_UDELAY_POLL;
 module_param(ec_polling_guard, uint, 0644);
 MODULE_PARM_DESC(ec_polling_guard, "Guard time(us) between EC accesses in polling modes");
 
+static unsigned int ec_event_clearing __read_mostly = ACPI_EC_EVT_TIMING_STATUS;
+
 /*
  * If the number of false interrupts per one transaction exceeds
  * this threshold, will think there is a GPE storm happened and
@@ -400,6 +446,21 @@ static void acpi_ec_complete_query(struct acpi_ec *ec)
 	}
 }
 
+static bool acpi_ec_guard_event(struct acpi_ec *ec)
+{
+	if (ec_event_clearing == ACPI_EC_EVT_TIMING_STATUS ||
+	    ec_event_clearing == ACPI_EC_EVT_TIMING_QUERY ||
+	    !test_bit(EC_FLAGS_QUERY_PENDING, &ec->flags) ||
+	    (ec->curr && ec->curr->command == ACPI_EC_COMMAND_QUERY))
+		return false;
+
+	/*
+	 * Postpone the query submission to allow the firmware to proceed,
+	 * we shouldn't check SCI_EVT before the firmware reflagging it.
+	 */
+	return true;
+}
+
 static int ec_transaction_polled(struct acpi_ec *ec)
 {
 	unsigned long flags;
@@ -428,8 +489,15 @@ static inline void ec_transaction_transition(struct acpi_ec *ec, unsigned long f
 {
 	ec->curr->flags |= flag;
 	if (ec->curr->command == ACPI_EC_COMMAND_QUERY) {
-		if (flag == ACPI_EC_COMMAND_POLL)
+		if (ec_event_clearing == ACPI_EC_EVT_TIMING_STATUS &&
+		    flag == ACPI_EC_COMMAND_POLL)
+			acpi_ec_complete_query(ec);
+		if (ec_event_clearing == ACPI_EC_EVT_TIMING_QUERY &&
+		    flag == ACPI_EC_COMMAND_COMPLETE)
 			acpi_ec_complete_query(ec);
+		if (ec_event_clearing == ACPI_EC_EVT_TIMING_EVENT &&
+		    flag == ACPI_EC_COMMAND_COMPLETE)
+			set_bit(EC_FLAGS_QUERY_GUARDING, &ec->flags);
 	}
 }
 
@@ -449,6 +517,20 @@ static void advance_transaction(struct acpi_ec *ec)
 	acpi_ec_clear_gpe(ec);
 	status = acpi_ec_read_status(ec);
 	t = ec->curr;
+	/*
+	 * Another IRQ or a guarded polling mode advancement is detected,
+	 * the next QR_EC submission is then allowed.
+	 */
+	if (!t || !(t->flags & ACPI_EC_COMMAND_POLL)) {
+		if (ec_event_clearing == ACPI_EC_EVT_TIMING_EVENT &&
+		    test_bit(EC_FLAGS_QUERY_GUARDING, &ec->flags)) {
+			clear_bit(EC_FLAGS_QUERY_GUARDING, &ec->flags);
+			acpi_ec_complete_query(ec);
+		}
+		if (ec_event_clearing == ACPI_EC_EVT_TIMING_METHOD &&
+		    !ec->nr_pending_queries)
+			acpi_ec_complete_query(ec);
+	}
 	if (!t)
 		goto err;
 	if (t->flags & ACPI_EC_COMMAND_POLL) {
@@ -534,11 +616,13 @@ static int ec_guard(struct acpi_ec *ec)
 			/*
 			 * Perform wait polling
 			 *
-			 * The following check is there to keep the old
-			 * logic - no inter-transaction guarding for the
-			 * wait polling mode.
+			 * For SCI_EVT clearing timing of "event",
+			 * performing guarding before re-checking the
+			 * SCI_EVT. Otherwise, such guarding is not needed
+			 * due to the old practices.
 			 */
-			if (!ec_transaction_polled(ec))
+			if (!ec_transaction_polled(ec) &&
+			    !acpi_ec_guard_event(ec))
 				break;
 			if (wait_event_timeout(ec->wait,
 					       ec_transaction_completed(ec),
@@ -953,6 +1037,25 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 *data)
 	return result;
 }
 
+static void acpi_ec_check_event(struct acpi_ec *ec)
+{
+	unsigned long flags;
+
+	if (ec_event_clearing == ACPI_EC_EVT_TIMING_EVENT ||
+	    ec_event_clearing == ACPI_EC_EVT_TIMING_METHOD) {
+		if (ec_guard(ec)) {
+			spin_lock_irqsave(&ec->lock, flags);
+			/*
+			 * Take care of the SCI_EVT unless no one else is
+			 * taking care of it.
+			 */
+			if (!ec->curr)
+				advance_transaction(ec);
+			spin_unlock_irqrestore(&ec->lock, flags);
+		}
+	}
+}
+
 static void acpi_ec_event_handler(struct work_struct *work)
 {
 	unsigned long flags;
@@ -970,6 +1073,8 @@ static void acpi_ec_event_handler(struct work_struct *work)
 	spin_unlock_irqrestore(&ec->lock, flags);
 
 	ec_dbg_evt("Event stopped");
+
+	acpi_ec_check_event(ec);
 }
 
 static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
@@ -1426,6 +1531,48 @@ error:
 	return -ENODEV;
 }
 
+static int param_set_event_clearing(const char *val, struct kernel_param *kp)
+{
+	int result = 0;
+
+	if (!strncmp(val, "status", sizeof("status") - 1)) {
+		ec_event_clearing = ACPI_EC_EVT_TIMING_STATUS;
+		pr_info("Assuming SCI_EVT clearing on EC_SC accesses\n");
+	} else if (!strncmp(val, "query", sizeof("query") - 1)) {
+		ec_event_clearing = ACPI_EC_EVT_TIMING_QUERY;
+		pr_info("Assuming SCI_EVT clearing on QR_EC writes\n");
+	} else if (!strncmp(val, "event", sizeof("event") - 1)) {
+		ec_event_clearing = ACPI_EC_EVT_TIMING_EVENT;
+		pr_info("Assuming SCI_EVT clearing on event reads\n");
+	} else if (!strncmp(val, "method", sizeof("method") - 1)) {
+		ec_event_clearing = ACPI_EC_EVT_TIMING_METHOD;
+		pr_info("Assuming SCI_EVT clearing on _Qxx method evaluations\n");
+	} else
+		result = -EINVAL;
+	return result;
+}
+
+static int param_get_event_clearing(char *buffer, struct kernel_param *kp)
+{
+	switch (ec_event_clearing) {
+	case ACPI_EC_EVT_TIMING_STATUS:
+		return sprintf(buffer, "status");
+	case ACPI_EC_EVT_TIMING_QUERY:
+		return sprintf(buffer, "query");
+	case ACPI_EC_EVT_TIMING_EVENT:
+		return sprintf(buffer, "event");
+	case ACPI_EC_EVT_TIMING_METHOD:
+		return sprintf(buffer, "method");
+	default:
+		return sprintf(buffer, "invalid");
+	}
+	return 0;
+}
+
+module_param_call(ec_event_clearing, param_set_event_clearing, param_get_event_clearing,
+		  NULL, 0644);
+MODULE_PARM_DESC(ec_event_clearing, "Assumed SCI_EVT clearing timing");
+
 static struct acpi_driver acpi_ec_driver = {
 	.name = "ec",
 	.class = ACPI_EC_CLASS,
-- 
1.7.10

[PATCH 5/6] ACPI / EC: Fix EC_FLAGS_QUERY_HANDSHAKE platforms using new event clearing timing.

[Lv Zheng]

It is reported that on several platforms, EC firmware will not respond
non-expected QR_EC (see EC_FLAGS_QUERY_HANDSHAKE, only write QR_EC when
SCI_EVT is set).

Unfortunately, ACPI specification doesn't define when the SCI_EVT should be
cleared by the firmware, thus the original implementation queued up second
QR_EC right after writing QR_EC command and before reading the returned
event value as at that time the SCI_EVT is ensured not cleared. This
behavior is also based on the assumption that the firmware should be able
to return 0x00 to indicate "no outstanding event". This behavior did fix
issues on Samsung platforms where the spurious query value of 0x00 is
supported and didn't break platforms in my test queue.

But recently, specific Acer, Asus, Lenovo platforms keep on blaming this
change.

This patch changes the behavior to re-check the SCI_EVT a bit later and
removes EC_FLAGS_QUERY_HANDSHAKE quirks, hoping this is the Windows
compliant EC driver behavior.

This patch keeps old behavior for Samsung platforms.

In order to be robust to the possible regressions, instead of removing the
quirk directly, this patch keeps the quirk code and removes the quirk
users.

Cc: 3.16+ <stable@vger.kernel.org> # 3.16+
Link: https://bugzilla.kernel.org/show_bug.cgi?id=94411
Link: https://bugzilla.kernel.org/show_bug.cgi?id=97381
Link: https://bugzilla.kernel.org/show_bug.cgi?id=98111
Reported-and-tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
Reported-and-tested-by: Tigran Gabrielyan <tigrangab@gmail.com>
Reported-and-tested-by: Adrien D <ghbdtn@openmailbox.org>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 8477626..2f13880 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -144,7 +144,7 @@ static unsigned int ec_polling_guard __read_mostly = ACPI_EC_UDELAY_POLL;
 module_param(ec_polling_guard, uint, 0644);
 MODULE_PARM_DESC(ec_polling_guard, "Guard time(us) between EC accesses in polling modes");
 
-static unsigned int ec_event_clearing __read_mostly = ACPI_EC_EVT_TIMING_STATUS;
+static unsigned int ec_event_clearing __read_mostly = ACPI_EC_EVT_TIMING_QUERY;
 
 /*
  * If the number of false interrupts per one transaction exceeds
@@ -1385,10 +1385,13 @@ static int ec_validate_ecdt(const struct dmi_system_id *id)
 	return 0;
 }
 
+#if 0
 /*
- * Acer EC firmware refuses to respond QR_EC when SCI_EVT is not set, for
- * which case, we complete the QR_EC without issuing it to the firmware.
- * https://bugzilla.kernel.org/show_bug.cgi?id=86211
+ * Some EC firmware variations refuses to respond QR_EC when SCI_EVT is not
+ * set, for which case, we complete the QR_EC without issuing it to the
+ * firmware.
+ * https://bugzilla.kernel.org/show_bug.cgi?id=82611
+ * https://bugzilla.kernel.org/show_bug.cgi?id=97381
  */
 static int ec_flag_query_handshake(const struct dmi_system_id *id)
 {
@@ -1396,6 +1399,7 @@ static int ec_flag_query_handshake(const struct dmi_system_id *id)
 	EC_FLAGS_QUERY_HANDSHAKE = 1;
 	return 0;
 }
+#endif
 
 /*
  * On some hardware it is necessary to clear events accumulated by the EC during
@@ -1418,6 +1422,7 @@ static int ec_clear_on_resume(const struct dmi_system_id *id)
 {
 	pr_debug("Detected system needing EC poll on resume.\n");
 	EC_FLAGS_CLEAR_ON_RESUME = 1;
+	ec_event_clearing = ACPI_EC_EVT_TIMING_STATUS;
 	return 0;
 }
 
@@ -1447,9 +1452,6 @@ static struct dmi_system_id ec_dmi_table[] __initdata = {
 	{
 	ec_clear_on_resume, "Samsung hardware", {
 	DMI_MATCH(DMI_SYS_VENDOR, "SAMSUNG ELECTRONICS CO., LTD.")}, NULL},
-	{
-	ec_flag_query_handshake, "Acer hardware", {
-	DMI_MATCH(DMI_SYS_VENDOR, "Acer"), }, NULL},
 	{},
 };
 
-- 
1.7.10

[PATCH 6/6] ACPI / EC: Fix a code coverity issue when QR_EC transactions are failed.

[Lv Zheng]

When the QR_EC transaction fails, the EC_FLAGS_QUERY_PENDING flag prevents
the event handling work queue from being scheduled again.

Though there shouldn't be failed QR_EC transactions, and this gap was
efficiently used for catching and learning the SCI_EVT clearing timing
compliance issues, we need to fix this as we are not fully compatible
with all platforms/Windows to handle SCI_EVT clearing timing correctly.
Fixing this gives the EC driver the chances to recover from a state machine
failure.

So this patch fixes this issue. When nr_pending_queries drops to 0, it
clears EC_FLAGS_QUERY_PENDING at the proper position for different modes in
order to ensure SCI_EVT handling can proceed. For "event" mode, this is
already covered in advance_transaction().

Cc: 3.16+ <stable@vger.kernel.org> # 3.16+
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 2f13880..cb1c1aa 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -523,7 +523,8 @@ static void advance_transaction(struct acpi_ec *ec)
 	 */
 	if (!t || !(t->flags & ACPI_EC_COMMAND_POLL)) {
 		if (ec_event_clearing == ACPI_EC_EVT_TIMING_EVENT &&
-		    test_bit(EC_FLAGS_QUERY_GUARDING, &ec->flags)) {
+		    (!ec->nr_pending_queries ||
+		     test_bit(EC_FLAGS_QUERY_GUARDING, &ec->flags))) {
 			clear_bit(EC_FLAGS_QUERY_GUARDING, &ec->flags);
 			acpi_ec_complete_query(ec);
 		}
@@ -1069,6 +1070,17 @@ static void acpi_ec_event_handler(struct work_struct *work)
 		(void)acpi_ec_query(ec, NULL);
 		spin_lock_irqsave(&ec->lock, flags);
 		ec->nr_pending_queries--;
+		/*
+		 * Before exit, make sure that this work item can be
+		 * scheduled again. There might be QR_EC/_Qxx failures,
+		 * leaving EC_FLAGS_QUERY_PENDING uncleared and preventing
+		 * this work item from being scheduled again.
+		 */
+		if (!ec->nr_pending_queries) {
+			if (ec_event_clearing == ACPI_EC_EVT_TIMING_STATUS ||
+			    ec_event_clearing == ACPI_EC_EVT_TIMING_QUERY)
+				acpi_ec_complete_query(ec);
+		}
 	}
 	spin_unlock_irqrestore(&ec->lock, flags);
 
-- 
1.7.10

RE: [PATCH 2/6] ACPI / EC: Cleanup _Qxx evaluation work item.

[Zheng, Lv]

Hi, Rafael

This patch is conflict against a recent discovered windows EC behavior, learned from kernel Bugzilla 94411.
https://bugzilla.kernel.org/show_bug.cgi?id=94411
I need to remove it from this patchset and rebase the rest ones.
Please ignore the v1 of this patchset, I'll send v2 of this patchset after testing.

Thanks and best regards
-Lv

> From: Zheng, Lv
> Sent: Monday, June 08, 2015 1:28 PM
> 
> The _Qxx evaluation work item can be eliminated and _Qxx can be evaluated
> right in the same work item as the QR_EC transaction. This patch cleans up
> the code to achieve this.
> 
> Originally, QR_EC transaction and _Qxx evaluation were all done in the same
> work queue flushed by acpi_os_wait_events_complete(). Though the QR_EC
> transaction (previous commit) and _Qxx evaluation (this commmit) are now
> moved to the work queue that is not covered by
> acpi_os_wait_events_complete(), there is no issue can be seen in the
> suspend/resume tests.
> 
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> ---
>  drivers/acpi/ec.c |   35 ++++++++++++-----------------------
>  1 file changed, 12 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index 0ce8b6e8..b956dbc 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -904,27 +904,12 @@ void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
>  }
>  EXPORT_SYMBOL_GPL(acpi_ec_remove_query_handler);
> 
> -static void acpi_ec_run(void *cxt)
> -{
> -	struct acpi_ec_query_handler *handler = cxt;
> -
> -	if (!handler)
> -		return;
> -	ec_dbg_evt("Query(0x%02x) started", handler->query_bit);
> -	if (handler->func)
> -		handler->func(handler->data);
> -	else if (handler->handle)
> -		acpi_evaluate_object(handler->handle, NULL, NULL, NULL);
> -	ec_dbg_evt("Query(0x%02x) stopped", handler->query_bit);
> -	acpi_ec_put_query_handler(handler);
> -}
> -
>  static int acpi_ec_query(struct acpi_ec *ec, u8 *data)
>  {
>  	u8 value = 0;
>  	int result;
> -	acpi_status status;
>  	struct acpi_ec_query_handler *handler;
> +	bool handler_found = false;
>  	struct transaction t = {.command = ACPI_EC_COMMAND_QUERY,
>  				.wdata = NULL, .rdata = &value,
>  				.wlen = 0, .rlen = 1};
> @@ -947,17 +932,21 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 *data)
>  		if (value == handler->query_bit) {
>  			/* have custom handler for this bit */
>  			handler = acpi_ec_get_query_handler(handler);
> -			ec_dbg_evt("Query(0x%02x) scheduled",
> -				   handler->query_bit);
> -			status = acpi_os_execute((handler->func) ?
> -				OSL_NOTIFY_HANDLER : OSL_GPE_HANDLER,
> -				acpi_ec_run, handler);
> -			if (ACPI_FAILURE(status))
> -				result = -EBUSY;
> +			handler_found = true;
>  			break;
>  		}
>  	}
>  	mutex_unlock(&ec->mutex);
> +
> +	if (handler_found) {
> +		ec_dbg_evt("Query(0x%02x) started", handler->query_bit);
> +		if (handler->func)
> +			handler->func(handler->data);
> +		else if (handler->handle)
> +			acpi_evaluate_object(handler->handle, NULL, NULL, NULL);
> +		ec_dbg_evt("Query(0x%02x) stopped", handler->query_bit);
> +		acpi_ec_put_query_handler(handler);
> +	}
>  	return result;
>  }
> 
> --
> 1.7.10

[PATCH v2 0/5] ACPI / EC: Fix issues related to the event handling.

[Lv Zheng]

ACPI specification doesn't define the SCI_EVT clearing timing for the EC
firmware. There could be 4 possible positions the firmware may use to clear
the SCI_EVT indication:
 STATUS: After indicating SCI_EVT to the host via the status register
         (EC_SC), the target can clear SCI_EVT at any time.
 QUERY: After seeing the query request (QR_EC) written to the command
        register (EC_CMD) by the host and having prepared the responding
        event value in the data register (EC_DATA), the target can safely
        clear SCI_EVT.
 EVENT: After seeing the event response read from the data register
        (EC_DATA) by the host, the target can clear SCI_EVT.
 METHOD: After the event has been handled via the host _Qxx evaluation,
         the target can clear SCI_EVT.

The old EC driver re-checked SCI_EVT using the 3rd position, while during
the race fixes, the new EC driver was switched to re-check SCI_EVT using
the 1st position. Using the earliest position may help to reduce issues
when the other races are not fully fixed and there is an ACPI specification
statement around an expected behavior:
 The query value of zero is reserved for a spurious query result and
 indicates "no outstanding events."
This statement was the motivation of this choice.

But there are platforms do not follow this definition, since there is also
a coverity issue left in the new EC driver, the unmatched SCI_EVT clearing
timing finally was trapped by this coverity issue, and users can see that
the new EC driver stops processing further SCI_EVTs. The new EC driver then
implemented EC_FLAGS_QUERY_HANDSHAKE quirk as a workaround for such
platforms.
Now more and more reports can be seen for this SCI_EVT clearing timing
unmatched problem, we need to consider a better approach to fix all of them
instead of adding new EC_FLAGS_QUERY_HANDSHAKE quirk users.

This patchset thus finally implements 3 of 4 modes to handle the SCI_EVT
clearing timing variations (1 was proven by 94411 not compliant to the
Windows behavior). And according to the tests, chooses the next earlier
position (the 2rd position) as the default behavior.

This patchset also fixes the coverity problem so that even though the
default behavior still cannot match the Windows behavior, it is possible
for the new EC driver to proceed to handle further SCI_EVT indications.

Related bug reports are as follows:
Link: https://bugzilla.kernel.org/show_bug.cgi?id=44161
Link: https://bugzilla.kernel.org/show_bug.cgi?id=82611
Link: https://bugzilla.kernel.org/show_bug.cgi?id=94411
Link: https://bugzilla.kernel.org/show_bug.cgi?id=97381
Link: https://bugzilla.kernel.org/show_bug.cgi?id=98111

Lv Zheng (5):
  ACPI / EC: Cleanup transaction state transition.
  ACPI / EC: Convert event handling work queue into loop style.
  ACPI / EC: Add event clearing variation support.
  ACPI / EC: Fix EC_FLAGS_QUERY_HANDSHAKE platforms using new event
    clearing timing.
  ACPI / EC: Fix a code coverity issue when QR_EC transactions are
    failed.

 drivers/acpi/ec.c       |  217 +++++++++++++++++++++++++++++++++++++++++------
 drivers/acpi/internal.h |    1 +
 2 files changed, 191 insertions(+), 27 deletions(-)

-- 
1.7.10

[PATCH v2 1/5] ACPI / EC: Cleanup transaction state transition.

[Lv Zheng]

This patch collects transaction state transition code into one function. We
then could have a single function to maintain transaction transition
related behaviors. No functional changes.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
Tested-by: Tigran Gabrielyan <tigrangab@gmail.com>
Tested-by: Adrien D <ghbdtn@openmailbox.org>
---
 drivers/acpi/ec.c |   23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 149b5e7..0ce8b6e8 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -391,7 +391,7 @@ static void acpi_ec_submit_query(struct acpi_ec *ec)
 
 static void acpi_ec_complete_query(struct acpi_ec *ec)
 {
-	if (ec->curr->command == ACPI_EC_COMMAND_QUERY) {
+	if (test_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
 		clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
 		ec_dbg_req("Event stopped");
 	}
@@ -421,6 +421,15 @@ static int ec_transaction_completed(struct acpi_ec *ec)
 	return ret;
 }
 
+static inline void ec_transaction_transition(struct acpi_ec *ec, unsigned long flag)
+{
+	ec->curr->flags |= flag;
+	if (ec->curr->command == ACPI_EC_COMMAND_QUERY) {
+		if (flag == ACPI_EC_COMMAND_POLL)
+			acpi_ec_complete_query(ec);
+	}
+}
+
 static void advance_transaction(struct acpi_ec *ec)
 {
 	struct transaction *t;
@@ -449,7 +458,7 @@ static void advance_transaction(struct acpi_ec *ec)
 			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;
+					ec_transaction_transition(ec, ACPI_EC_COMMAND_COMPLETE);
 					if (t->command == ACPI_EC_COMMAND_QUERY)
 						ec_dbg_req("Command(%s) hardware completion",
 							   acpi_ec_cmd_string(t->command));
@@ -459,7 +468,7 @@ static void advance_transaction(struct acpi_ec *ec)
 				goto err;
 		} else if (t->wlen == t->wi &&
 			   (status & ACPI_EC_FLAG_IBF) == 0) {
-			t->flags |= ACPI_EC_COMMAND_COMPLETE;
+			ec_transaction_transition(ec, ACPI_EC_COMMAND_COMPLETE);
 			wakeup = true;
 		}
 		goto out;
@@ -467,17 +476,15 @@ static void advance_transaction(struct acpi_ec *ec)
 		if (EC_FLAGS_QUERY_HANDSHAKE &&
 		    !(status & ACPI_EC_FLAG_SCI) &&
 		    (t->command == ACPI_EC_COMMAND_QUERY)) {
-			t->flags |= ACPI_EC_COMMAND_POLL;
-			acpi_ec_complete_query(ec);
+			ec_transaction_transition(ec, ACPI_EC_COMMAND_POLL);
 			t->rdata[t->ri++] = 0x00;
-			t->flags |= ACPI_EC_COMMAND_COMPLETE;
+			ec_transaction_transition(ec, ACPI_EC_COMMAND_COMPLETE);
 			ec_dbg_req("Command(%s) software completion",
 				   acpi_ec_cmd_string(t->command));
 			wakeup = true;
 		} else if ((status & ACPI_EC_FLAG_IBF) == 0) {
 			acpi_ec_write_cmd(ec, t->command);
-			t->flags |= ACPI_EC_COMMAND_POLL;
-			acpi_ec_complete_query(ec);
+			ec_transaction_transition(ec, ACPI_EC_COMMAND_POLL);
 		} else
 			goto err;
 		goto out;
-- 
1.7.10

[PATCH v2 2/5] ACPI / EC: Convert event handling work queue into loop style.

[Lv Zheng]

During the period that a work queue is scheduled (queued up for run) but
hasn't been run, second schedule_work() could fail. This may not lead to
the loss of queries because QR_EC is always ensured to be submitted after
the work queue has been in the running state.

The event handling work queue can be changed into the loop style to allow
us to control the code in a more flexible way:
1. Makes it possible to add event=0x00 termination condition in the loop.
2. Increases the thoughput of the QR_EC transactions as the 2nd+ QR_EC
   transactions may be handled in the same work item used for the 1st QR_EC
   transaction, thus the delay caused by the 2nd+ work item scheduling can
   be eliminated.

Except the logging message changes and the throughput improvement, this
patch is just a funcitonal no-op.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
Tested-by: Tigran Gabrielyan <tigrangab@gmail.com>
Tested-by: Adrien D <ghbdtn@openmailbox.org>
---
 drivers/acpi/ec.c       |   33 ++++++++++++++++++++++++---------
 drivers/acpi/internal.h |    1 +
 2 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 0ce8b6e8..824f3e8 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -384,7 +384,9 @@ static bool acpi_ec_submit_flushable_request(struct acpi_ec *ec)
 static void acpi_ec_submit_query(struct acpi_ec *ec)
 {
 	if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
-		ec_dbg_req("Event started");
+		ec_dbg_evt("Command(%s) submitted/blocked",
+			   acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
+		ec->nr_pending_queries++;
 		schedule_work(&ec->work);
 	}
 }
@@ -393,7 +395,8 @@ static void acpi_ec_complete_query(struct acpi_ec *ec)
 {
 	if (test_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
 		clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
-		ec_dbg_req("Event stopped");
+		ec_dbg_evt("Command(%s) unblocked",
+			   acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
 	}
 }
 
@@ -460,8 +463,8 @@ static void advance_transaction(struct acpi_ec *ec)
 				if (t->rlen == t->ri) {
 					ec_transaction_transition(ec, ACPI_EC_COMMAND_COMPLETE);
 					if (t->command == ACPI_EC_COMMAND_QUERY)
-						ec_dbg_req("Command(%s) hardware completion",
-							   acpi_ec_cmd_string(t->command));
+						ec_dbg_evt("Command(%s) completed by hardware",
+							   acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
 					wakeup = true;
 				}
 			} else
@@ -479,8 +482,8 @@ static void advance_transaction(struct acpi_ec *ec)
 			ec_transaction_transition(ec, ACPI_EC_COMMAND_POLL);
 			t->rdata[t->ri++] = 0x00;
 			ec_transaction_transition(ec, ACPI_EC_COMMAND_COMPLETE);
-			ec_dbg_req("Command(%s) software completion",
-				   acpi_ec_cmd_string(t->command));
+			ec_dbg_evt("Command(%s) completed by software",
+				   acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
 			wakeup = true;
 		} else if ((status & ACPI_EC_FLAG_IBF) == 0) {
 			acpi_ec_write_cmd(ec, t->command);
@@ -961,11 +964,23 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 *data)
 	return result;
 }
 
-static void acpi_ec_gpe_poller(struct work_struct *work)
+static void acpi_ec_event_handler(struct work_struct *work)
 {
+	unsigned long flags;
 	struct acpi_ec *ec = container_of(work, struct acpi_ec, work);
 
-	acpi_ec_query(ec, NULL);
+	ec_dbg_evt("Event started");
+
+	spin_lock_irqsave(&ec->lock, flags);
+	while (ec->nr_pending_queries) {
+		spin_unlock_irqrestore(&ec->lock, flags);
+		(void)acpi_ec_query(ec, NULL);
+		spin_lock_irqsave(&ec->lock, flags);
+		ec->nr_pending_queries--;
+	}
+	spin_unlock_irqrestore(&ec->lock, flags);
+
+	ec_dbg_evt("Event stopped");
 }
 
 static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
@@ -1040,7 +1055,7 @@ static struct acpi_ec *make_acpi_ec(void)
 	init_waitqueue_head(&ec->wait);
 	INIT_LIST_HEAD(&ec->list);
 	spin_lock_init(&ec->lock);
-	INIT_WORK(&ec->work, acpi_ec_gpe_poller);
+	INIT_WORK(&ec->work, acpi_ec_event_handler);
 	ec->timestamp = jiffies;
 	return ec;
 }
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index cf358d4..ae919b5 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -139,6 +139,7 @@ struct acpi_ec {
 	spinlock_t lock;
 	struct work_struct work;
 	unsigned long timestamp;
+	unsigned long nr_pending_queries;
 };
 
 extern struct acpi_ec *first_ec;
-- 
1.7.10

[PATCH v2 3/5] ACPI / EC: Add event clearing variation support.

[Lv Zheng]

We've been suffering from the uncertainty of the SCI_EVT clearing timing.
This patch implements 3 of 4 possible modes to handle SCI_EVT clearing
variations. The old behavior is kept in this patch.

Status: QR_EC is re-checked as early as possible after checking previous
        SCI_EVT. This always leads to 2 QR_EC transactions per SCI_EVT
        indication and the target may implement event queue which returns
        0x00 indicating "no outstanding event".
        This is proven to be a conflict against Windows behavior, but is
        still kept in this patch to make the EC driver robust to the
        possible regressions that may occur on Samsung platforms.
Query: QR_EC is re-checked after the target has handled the QR_EC query
       request command pushed by the host.
Event: QR_EC is re-checked after the target has noticed the query event
       response data pulled by the host.
       This timing is not determined by any IRQs, so we may need to use a
       guard period in this mode, which may explain the existence of the
       ec_guard() code used by the old EC driver where the re-check timing
       is implemented in the similar way as this mode.
Method: QR_EC is re-checked as late as possible after completing the _Qxx
        evaluation. The target may implement SCI_EVT like a level triggered
        interrupt.
        It is proven on kernel bugzilla 94411 that, Windows will have all
        _Qxx evaluations parallelized. Thus unless required by further
        evidences, we needn't implement this mode as it is a conflict of
        the _Qxx parallelism requirement.

Note that, according to the reports, there are platforms that cannot be
handled using the "Status" mode without enabling the
EC_FLAGS_QUERY_HANDSHAKE quirk. But they can be handled with the other
modes according to the tests (kernel bugzilla 97381).

The following log entry can be used to confirm the differences of the 3
modes as it should appear at the different positions for the 3 modes:
  Command(QR_EC) unblocked
Status: appearing after
         EC_SC(W) = 0x84
Query: appearing after
         EC_DATA(R) = 0xXX
       where XX is the event number used to determine _QXX
Event: appearing after first
         EC_SC(R) = 0xX0 SCI_EVT=x BURST=0 CMD=0 IBF=0 OBF=0
       that is next to the following log entry:
         Command(QR_EC) completed by hardware

Link: https://bugzilla.kernel.org/show_bug.cgi?id=94411
Link: https://bugzilla.kernel.org/show_bug.cgi?id=97381
Link: https://bugzilla.kernel.org/show_bug.cgi?id=98111
Reported-and-tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
Reported-and-tested-by: Tigran Gabrielyan <tigrangab@gmail.com>
Reported-and-tested-by: Adrien D <ghbdtn@openmailbox.org>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |  137 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 132 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 824f3e8..41eb523 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -59,6 +59,38 @@
 #define ACPI_EC_FLAG_BURST	0x10	/* burst mode */
 #define ACPI_EC_FLAG_SCI	0x20	/* EC-SCI occurred */
 
+/*
+ * The SCI_EVT clearing timing is not defined by the ACPI specification.
+ * This leads to lots of practical timing issues for the host EC driver.
+ * The following variations are defined (from the target EC firmware's
+ * perspective):
+ * STATUS: After indicating SCI_EVT edge triggered IRQ to the host, the
+ *         target can clear SCI_EVT at any time so long as the host can see
+ *         the indication by reading the status register (EC_SC). So the
+ *         host should re-check SCI_EVT after the first time the SCI_EVT
+ *         indication is seen, which is the same time the query request
+ *         (QR_EC) is written to the command register (EC_CMD). SCI_EVT set
+ *         at any later time could indicate another event. Normally such
+ *         kind of EC firmware has implemented an event queue and will
+ *         return 0x00 to indicate "no outstanding event".
+ * QUERY: After seeing the query request (QR_EC) written to the command
+ *        register (EC_CMD) by the host and having prepared the responding
+ *        event value in the data register (EC_DATA), the target can safely
+ *        clear SCI_EVT because the target can confirm that the current
+ *        event is being handled by the host. The host then should check
+ *        SCI_EVT right after reading the event response from the data
+ *        register (EC_DATA).
+ * EVENT: After seeing the event response read from the data register
+ *        (EC_DATA) by the host, the target can clear SCI_EVT. As the
+ *        target requires time to notice the change in the data register
+ *        (EC_DATA), the host may be required to wait additional guarding
+ *        time before checking the SCI_EVT again. Such guarding may not be
+ *        necessary if the host is notified via another IRQ.
+ */
+#define ACPI_EC_EVT_TIMING_STATUS	0x00
+#define ACPI_EC_EVT_TIMING_QUERY	0x01
+#define ACPI_EC_EVT_TIMING_EVENT	0x02
+
 /* EC commands */
 enum ec_command {
 	ACPI_EC_COMMAND_READ = 0x80,
@@ -76,6 +108,7 @@ enum ec_command {
 
 enum {
 	EC_FLAGS_QUERY_PENDING,		/* Query is pending */
+	EC_FLAGS_QUERY_GUARDING,	/* Guard for SCI_EVT check */
 	EC_FLAGS_HANDLERS_INSTALLED,	/* Handlers for GPE and
 					 * OpReg are installed */
 	EC_FLAGS_STARTED,		/* Driver is started */
@@ -100,6 +133,8 @@ static unsigned int ec_polling_guard __read_mostly = ACPI_EC_UDELAY_POLL;
 module_param(ec_polling_guard, uint, 0644);
 MODULE_PARM_DESC(ec_polling_guard, "Guard time(us) between EC accesses in polling modes");
 
+static unsigned int ec_event_clearing __read_mostly = ACPI_EC_EVT_TIMING_STATUS;
+
 /*
  * If the number of false interrupts per one transaction exceeds
  * this threshold, will think there is a GPE storm happened and
@@ -400,6 +435,21 @@ static void acpi_ec_complete_query(struct acpi_ec *ec)
 	}
 }
 
+static bool acpi_ec_guard_event(struct acpi_ec *ec)
+{
+	if (ec_event_clearing == ACPI_EC_EVT_TIMING_STATUS ||
+	    ec_event_clearing == ACPI_EC_EVT_TIMING_QUERY ||
+	    !test_bit(EC_FLAGS_QUERY_PENDING, &ec->flags) ||
+	    (ec->curr && ec->curr->command == ACPI_EC_COMMAND_QUERY))
+		return false;
+
+	/*
+	 * Postpone the query submission to allow the firmware to proceed,
+	 * we shouldn't check SCI_EVT before the firmware reflagging it.
+	 */
+	return true;
+}
+
 static int ec_transaction_polled(struct acpi_ec *ec)
 {
 	unsigned long flags;
@@ -428,8 +478,15 @@ static inline void ec_transaction_transition(struct acpi_ec *ec, unsigned long f
 {
 	ec->curr->flags |= flag;
 	if (ec->curr->command == ACPI_EC_COMMAND_QUERY) {
-		if (flag == ACPI_EC_COMMAND_POLL)
+		if (ec_event_clearing == ACPI_EC_EVT_TIMING_STATUS &&
+		    flag == ACPI_EC_COMMAND_POLL)
+			acpi_ec_complete_query(ec);
+		if (ec_event_clearing == ACPI_EC_EVT_TIMING_QUERY &&
+		    flag == ACPI_EC_COMMAND_COMPLETE)
 			acpi_ec_complete_query(ec);
+		if (ec_event_clearing == ACPI_EC_EVT_TIMING_EVENT &&
+		    flag == ACPI_EC_COMMAND_COMPLETE)
+			set_bit(EC_FLAGS_QUERY_GUARDING, &ec->flags);
 	}
 }
 
@@ -449,6 +506,17 @@ static void advance_transaction(struct acpi_ec *ec)
 	acpi_ec_clear_gpe(ec);
 	status = acpi_ec_read_status(ec);
 	t = ec->curr;
+	/*
+	 * Another IRQ or a guarded polling mode advancement is detected,
+	 * the next QR_EC submission is then allowed.
+	 */
+	if (!t || !(t->flags & ACPI_EC_COMMAND_POLL)) {
+		if (ec_event_clearing == ACPI_EC_EVT_TIMING_EVENT &&
+		    test_bit(EC_FLAGS_QUERY_GUARDING, &ec->flags)) {
+			clear_bit(EC_FLAGS_QUERY_GUARDING, &ec->flags);
+			acpi_ec_complete_query(ec);
+		}
+	}
 	if (!t)
 		goto err;
 	if (t->flags & ACPI_EC_COMMAND_POLL) {
@@ -534,11 +602,13 @@ static int ec_guard(struct acpi_ec *ec)
 			/*
 			 * Perform wait polling
 			 *
-			 * The following check is there to keep the old
-			 * logic - no inter-transaction guarding for the
-			 * wait polling mode.
+			 * For SCI_EVT clearing timing of "event",
+			 * performing guarding before re-checking the
+			 * SCI_EVT. Otherwise, such guarding is not needed
+			 * due to the old practices.
 			 */
-			if (!ec_transaction_polled(ec))
+			if (!ec_transaction_polled(ec) &&
+			    !acpi_ec_guard_event(ec))
 				break;
 			if (wait_event_timeout(ec->wait,
 					       ec_transaction_completed(ec),
@@ -964,6 +1034,24 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 *data)
 	return result;
 }
 
+static void acpi_ec_check_event(struct acpi_ec *ec)
+{
+	unsigned long flags;
+
+	if (ec_event_clearing == ACPI_EC_EVT_TIMING_EVENT) {
+		if (ec_guard(ec)) {
+			spin_lock_irqsave(&ec->lock, flags);
+			/*
+			 * Take care of the SCI_EVT unless no one else is
+			 * taking care of it.
+			 */
+			if (!ec->curr)
+				advance_transaction(ec);
+			spin_unlock_irqrestore(&ec->lock, flags);
+		}
+	}
+}
+
 static void acpi_ec_event_handler(struct work_struct *work)
 {
 	unsigned long flags;
@@ -981,6 +1069,8 @@ static void acpi_ec_event_handler(struct work_struct *work)
 	spin_unlock_irqrestore(&ec->lock, flags);
 
 	ec_dbg_evt("Event stopped");
+
+	acpi_ec_check_event(ec);
 }
 
 static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
@@ -1437,6 +1527,43 @@ error:
 	return -ENODEV;
 }
 
+static int param_set_event_clearing(const char *val, struct kernel_param *kp)
+{
+	int result = 0;
+
+	if (!strncmp(val, "status", sizeof("status") - 1)) {
+		ec_event_clearing = ACPI_EC_EVT_TIMING_STATUS;
+		pr_info("Assuming SCI_EVT clearing on EC_SC accesses\n");
+	} else if (!strncmp(val, "query", sizeof("query") - 1)) {
+		ec_event_clearing = ACPI_EC_EVT_TIMING_QUERY;
+		pr_info("Assuming SCI_EVT clearing on QR_EC writes\n");
+	} else if (!strncmp(val, "event", sizeof("event") - 1)) {
+		ec_event_clearing = ACPI_EC_EVT_TIMING_EVENT;
+		pr_info("Assuming SCI_EVT clearing on event reads\n");
+	} else
+		result = -EINVAL;
+	return result;
+}
+
+static int param_get_event_clearing(char *buffer, struct kernel_param *kp)
+{
+	switch (ec_event_clearing) {
+	case ACPI_EC_EVT_TIMING_STATUS:
+		return sprintf(buffer, "status");
+	case ACPI_EC_EVT_TIMING_QUERY:
+		return sprintf(buffer, "query");
+	case ACPI_EC_EVT_TIMING_EVENT:
+		return sprintf(buffer, "event");
+	default:
+		return sprintf(buffer, "invalid");
+	}
+	return 0;
+}
+
+module_param_call(ec_event_clearing, param_set_event_clearing, param_get_event_clearing,
+		  NULL, 0644);
+MODULE_PARM_DESC(ec_event_clearing, "Assumed SCI_EVT clearing timing");
+
 static struct acpi_driver acpi_ec_driver = {
 	.name = "ec",
 	.class = ACPI_EC_CLASS,
-- 
1.7.10

[PATCH v2 4/5] ACPI / EC: Fix EC_FLAGS_QUERY_HANDSHAKE platforms using new event clearing timing.

[Lv Zheng]

It is reported that on several platforms, EC firmware will not respond
non-expected QR_EC (see EC_FLAGS_QUERY_HANDSHAKE, only write QR_EC when
SCI_EVT is set).

Unfortunately, ACPI specification doesn't define when the SCI_EVT should be
cleared by the firmware, thus the original implementation queued up second
QR_EC right after writing QR_EC command and before reading the returned
event value as at that time the SCI_EVT is ensured not cleared. This
behavior is also based on the assumption that the firmware should be able
to return 0x00 to indicate "no outstanding event". This behavior did fix
issues on Samsung platforms where the spurious query value of 0x00 is
supported and didn't break platforms in my test queue.

But recently, specific Acer, Asus, Lenovo platforms keep on blaming this
change.

This patch changes the behavior to re-check the SCI_EVT a bit later and
removes EC_FLAGS_QUERY_HANDSHAKE quirks, hoping this is the Windows
compliant EC driver behavior.

In order to be robust to the possible regressions, instead of removing the
quirk directly, this patch keeps the quirk code, removes the quirk users
and keeps old behavior for Samsung platforms.

Cc: 3.16+ <stable@vger.kernel.org> # 3.16+
Link: https://bugzilla.kernel.org/show_bug.cgi?id=94411
Link: https://bugzilla.kernel.org/show_bug.cgi?id=97381
Link: https://bugzilla.kernel.org/show_bug.cgi?id=98111
Reported-and-tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
Reported-and-tested-by: Tigran Gabrielyan <tigrangab@gmail.com>
Reported-and-tested-by: Adrien D <ghbdtn@openmailbox.org>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 41eb523..79817ce 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -133,7 +133,7 @@ static unsigned int ec_polling_guard __read_mostly = ACPI_EC_UDELAY_POLL;
 module_param(ec_polling_guard, uint, 0644);
 MODULE_PARM_DESC(ec_polling_guard, "Guard time(us) between EC accesses in polling modes");
 
-static unsigned int ec_event_clearing __read_mostly = ACPI_EC_EVT_TIMING_STATUS;
+static unsigned int ec_event_clearing __read_mostly = ACPI_EC_EVT_TIMING_QUERY;
 
 /*
  * If the number of false interrupts per one transaction exceeds
@@ -1381,10 +1381,13 @@ static int ec_validate_ecdt(const struct dmi_system_id *id)
 	return 0;
 }
 
+#if 0
 /*
- * Acer EC firmware refuses to respond QR_EC when SCI_EVT is not set, for
- * which case, we complete the QR_EC without issuing it to the firmware.
- * https://bugzilla.kernel.org/show_bug.cgi?id=86211
+ * Some EC firmware variations refuses to respond QR_EC when SCI_EVT is not
+ * set, for which case, we complete the QR_EC without issuing it to the
+ * firmware.
+ * https://bugzilla.kernel.org/show_bug.cgi?id=82611
+ * https://bugzilla.kernel.org/show_bug.cgi?id=97381
  */
 static int ec_flag_query_handshake(const struct dmi_system_id *id)
 {
@@ -1392,6 +1395,7 @@ static int ec_flag_query_handshake(const struct dmi_system_id *id)
 	EC_FLAGS_QUERY_HANDSHAKE = 1;
 	return 0;
 }
+#endif
 
 /*
  * On some hardware it is necessary to clear events accumulated by the EC during
@@ -1414,6 +1418,7 @@ static int ec_clear_on_resume(const struct dmi_system_id *id)
 {
 	pr_debug("Detected system needing EC poll on resume.\n");
 	EC_FLAGS_CLEAR_ON_RESUME = 1;
+	ec_event_clearing = ACPI_EC_EVT_TIMING_STATUS;
 	return 0;
 }
 
@@ -1443,9 +1448,6 @@ static struct dmi_system_id ec_dmi_table[] __initdata = {
 	{
 	ec_clear_on_resume, "Samsung hardware", {
 	DMI_MATCH(DMI_SYS_VENDOR, "SAMSUNG ELECTRONICS CO., LTD.")}, NULL},
-	{
-	ec_flag_query_handshake, "Acer hardware", {
-	DMI_MATCH(DMI_SYS_VENDOR, "Acer"), }, NULL},
 	{},
 };
 
-- 
1.7.10

[PATCH v2 5/5] ACPI / EC: Fix a code coverity issue when QR_EC transactions are failed.

[Lv Zheng]

When the QR_EC transaction fails, the EC_FLAGS_QUERY_PENDING flag prevents
the event handling work queue from being scheduled again.

Though there shouldn't be failed QR_EC transactions, and this gap was
efficiently used for catching and learning the SCI_EVT clearing timing
compliance issues, we need to fix this as we are not fully compatible
with all platforms/Windows to handle SCI_EVT clearing timing correctly.
Fixing this gives the EC driver the chances to recover from a state machine
failure.

So this patch fixes this issue. When nr_pending_queries drops to 0, it
clears EC_FLAGS_QUERY_PENDING at the proper position for different modes in
order to ensure that the SCI_EVT handling can proceed.

In order to be clearer for future ec_event_clearing modes, all checks in
this patch are written in the inclusive style, not the exclusive style.

Cc: 3.16+ <stable@vger.kernel.org> # 3.16+
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 79817ce..9d4761d 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -512,7 +512,8 @@ static void advance_transaction(struct acpi_ec *ec)
 	 */
 	if (!t || !(t->flags & ACPI_EC_COMMAND_POLL)) {
 		if (ec_event_clearing == ACPI_EC_EVT_TIMING_EVENT &&
-		    test_bit(EC_FLAGS_QUERY_GUARDING, &ec->flags)) {
+		    (!ec->nr_pending_queries ||
+		     test_bit(EC_FLAGS_QUERY_GUARDING, &ec->flags))) {
 			clear_bit(EC_FLAGS_QUERY_GUARDING, &ec->flags);
 			acpi_ec_complete_query(ec);
 		}
@@ -1065,6 +1066,17 @@ static void acpi_ec_event_handler(struct work_struct *work)
 		(void)acpi_ec_query(ec, NULL);
 		spin_lock_irqsave(&ec->lock, flags);
 		ec->nr_pending_queries--;
+		/*
+		 * Before exit, make sure that this work item can be
+		 * scheduled again. There might be QR_EC failures, leaving
+		 * EC_FLAGS_QUERY_PENDING uncleared and preventing this work
+		 * item from being scheduled again.
+		 */
+		if (!ec->nr_pending_queries) {
+			if (ec_event_clearing == ACPI_EC_EVT_TIMING_STATUS ||
+			    ec_event_clearing == ACPI_EC_EVT_TIMING_QUERY)
+				acpi_ec_complete_query(ec);
+		}
 	}
 	spin_unlock_irqrestore(&ec->lock, flags);
 
-- 
1.7.10

Re: [PATCH v2 0/5] ACPI / EC: Fix issues related to the event handling.

[Rafael J. Wysocki]

On Thursday, June 11, 2015 01:21:14 PM Lv Zheng wrote:
> ACPI specification doesn't define the SCI_EVT clearing timing for the EC
> firmware. There could be 4 possible positions the firmware may use to clear
> the SCI_EVT indication:
>  STATUS: After indicating SCI_EVT to the host via the status register
>          (EC_SC), the target can clear SCI_EVT at any time.
>  QUERY: After seeing the query request (QR_EC) written to the command
>         register (EC_CMD) by the host and having prepared the responding
>         event value in the data register (EC_DATA), the target can safely
>         clear SCI_EVT.
>  EVENT: After seeing the event response read from the data register
>         (EC_DATA) by the host, the target can clear SCI_EVT.
>  METHOD: After the event has been handled via the host _Qxx evaluation,
>          the target can clear SCI_EVT.
> 
> The old EC driver re-checked SCI_EVT using the 3rd position, while during
> the race fixes, the new EC driver was switched to re-check SCI_EVT using
> the 1st position. Using the earliest position may help to reduce issues
> when the other races are not fully fixed and there is an ACPI specification
> statement around an expected behavior:
>  The query value of zero is reserved for a spurious query result and
>  indicates "no outstanding events."
> This statement was the motivation of this choice.
> 
> But there are platforms do not follow this definition, since there is also
> a coverity issue left in the new EC driver, the unmatched SCI_EVT clearing
> timing finally was trapped by this coverity issue, and users can see that
> the new EC driver stops processing further SCI_EVTs. The new EC driver then
> implemented EC_FLAGS_QUERY_HANDSHAKE quirk as a workaround for such
> platforms.
> Now more and more reports can be seen for this SCI_EVT clearing timing
> unmatched problem, we need to consider a better approach to fix all of them
> instead of adding new EC_FLAGS_QUERY_HANDSHAKE quirk users.
> 
> This patchset thus finally implements 3 of 4 modes to handle the SCI_EVT
> clearing timing variations (1 was proven by 94411 not compliant to the
> Windows behavior). And according to the tests, chooses the next earlier
> position (the 2rd position) as the default behavior.
> 
> This patchset also fixes the coverity problem so that even though the
> default behavior still cannot match the Windows behavior, it is possible
> for the new EC driver to proceed to handle further SCI_EVT indications.
> 
> Related bug reports are as follows:
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=44161
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=82611
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=94411
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=97381
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=98111
> 
> Lv Zheng (5):
>   ACPI / EC: Cleanup transaction state transition.
>   ACPI / EC: Convert event handling work queue into loop style.
>   ACPI / EC: Add event clearing variation support.
>   ACPI / EC: Fix EC_FLAGS_QUERY_HANDSHAKE platforms using new event
>     clearing timing.
>   ACPI / EC: Fix a code coverity issue when QR_EC transactions are
>     failed.
> 
>  drivers/acpi/ec.c       |  217 +++++++++++++++++++++++++++++++++++++++++------
>  drivers/acpi/internal.h |    1 +
>  2 files changed, 191 insertions(+), 27 deletions(-)

Queued up for 4.2, thanks!


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.