ACPI / EC: Fix issues related to the event handling.

---
 drivers/acpi/ec.c |   23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

drivers/acpi/ec.c +15 -8

index 149b5e7..0ce8b6e8 100644
@@ -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;

Signature · 1 line

1.7.10

Lv Zheng

author

2015-06-08 05:28 UTC #2 lore raw

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

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 <[email protected]>

---
 drivers/acpi/ec.c |   35 ++++++++++++-----------------------
 1 file changed, 12 insertions(+), 23 deletions(-)

drivers/acpi/ec.c +12 ~~-23~~

index 0ce8b6e8..b956dbc 100644
@@ -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;
 }

Signature · 1 line

1.7.10

Zheng, Lv

author

2015-06-09 14:42 UTC #3 lore raw

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

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

Quoted 83 lines

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.

Signed-off-by Lv Zheng <[email protected]>

---
 drivers/acpi/ec.c |   35 ++++++++++++-----------------------
 1 file changed, 12 insertions(+), 23 deletions(-)

drivers/acpi/ec.c +12 ~~-24~~

index 0ce8b6e8..b956dbc 100644
@@ -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

Lv Zheng

author

2015-06-08 05:28 UTC #4 lore raw

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

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 <[email protected]>

---
 drivers/acpi/ec.c       |   33 ++++++++++++++++++++++++---------
 drivers/acpi/internal.h |    1 +
 2 files changed, 25 insertions(+), 9 deletions(-)

drivers/acpi/ec.c +24 -9

index b956dbc..7a30f59 100644
@@ -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;
 }

drivers/acpi/internal.h +1 -0

index cf358d4..ae919b5 100644
@@ -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;

Signature · 1 line

1.7.10

Lv Zheng

author

2015-06-08 05:28 UTC #5 lore raw

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

Link https://bugzilla.kernel.org/show_bug.cgi?id=94411

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=97381

Link https://bugzilla.kernel.org/show_bug.cgi?id=98111

Reported-and-tested-by Gabriele Mazzotta <[email protected]>

Reported-and-tested-by Tigran Gabrielyan <[email protected]>

Reported-and-tested-by Adrien D <[email protected]>

Signed-off-by Lv Zheng <[email protected]>

---
 drivers/acpi/ec.c |  157 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 152 insertions(+), 5 deletions(-)

drivers/acpi/ec.c +152 -5

index 7a30f59..8477626 100644
@@ -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,

Signature · 1 line

1.7.10

Lv Zheng

author

2015-06-08 05:28 UTC #6 lore raw

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

Link https://bugzilla.kernel.org/show_bug.cgi?id=94411

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+ <[email protected]> # 3.16+

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 <[email protected]>

Reported-and-tested-by Tigran Gabrielyan <[email protected]>

Reported-and-tested-by Adrien D <[email protected]>

Signed-off-by Lv Zheng <[email protected]>

---
 drivers/acpi/ec.c |   16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

drivers/acpi/ec.c +9 -7

index 8477626..2f13880 100644
@@ -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},
 	{},
 };

Signature · 1 line

1.7.10

Lv Zheng

author

2015-06-08 05:28 UTC #7 lore raw

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

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+ <[email protected]> # 3.16+

Signed-off-by Lv Zheng <[email protected]>

---
 drivers/acpi/ec.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

drivers/acpi/ec.c +13 -1

index 2f13880..cb1c1aa 100644
@@ -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);

Signature · 1 line

1.7.10

Lv Zheng

author

2015-06-11 05:21 UTC #8 lore raw

v2 cover ACPI / EC: Fix issues related to the event handling.

Link https://bugzilla.kernel.org/show_bug.cgi?id=44161

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=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(-)

Signature · 1 line

1.7.10

Lv Zheng

author

2015-06-11 05:21 UTC #9 lore raw

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

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 <[email protected]>

---
 drivers/acpi/ec.c |   23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

drivers/acpi/ec.c +15 -8

index 149b5e7..0ce8b6e8 100644
@@ -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;

Signature · 1 line

1.7.10

Lv Zheng

author

2015-06-11 05:21 UTC #10 lore raw

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

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 <[email protected]>

---
 drivers/acpi/ec.c       |   33 ++++++++++++++++++++++++---------
 drivers/acpi/internal.h |    1 +
 2 files changed, 25 insertions(+), 9 deletions(-)

drivers/acpi/ec.c +24 -9

index 0ce8b6e8..824f3e8 100644
@@ -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;
 }

drivers/acpi/internal.h +1 -0

index cf358d4..ae919b5 100644
@@ -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;

Signature · 1 line

1.7.10

Lv Zheng

author

2015-06-11 05:21 UTC #11 lore raw

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

Link https://bugzilla.kernel.org/show_bug.cgi?id=94411

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=97381

Link https://bugzilla.kernel.org/show_bug.cgi?id=98111

Reported-and-tested-by Gabriele Mazzotta <[email protected]>

Reported-and-tested-by Tigran Gabrielyan <[email protected]>

Reported-and-tested-by Adrien D <[email protected]>

Signed-off-by Lv Zheng <[email protected]>

---
 drivers/acpi/ec.c |  137 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 132 insertions(+), 5 deletions(-)

drivers/acpi/ec.c +132 -5

index 824f3e8..41eb523 100644
@@ -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,

Signature · 1 line

1.7.10

Lv Zheng

author

2015-06-11 05:21 UTC #12 lore raw

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

Link https://bugzilla.kernel.org/show_bug.cgi?id=94411

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).

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+ <[email protected]> # 3.16+

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 <[email protected]>

Reported-and-tested-by Tigran Gabrielyan <[email protected]>

Reported-and-tested-by Adrien D <[email protected]>

Signed-off-by Lv Zheng <[email protected]>

---
 drivers/acpi/ec.c |   16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

drivers/acpi/ec.c +9 -7

index 41eb523..79817ce 100644
@@ -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},
 	{},
 };

Signature · 1 line

1.7.10

Lv Zheng

author

2015-06-11 05:21 UTC #13 lore raw

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

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

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+ <[email protected]> # 3.16+

Signed-off-by Lv Zheng <[email protected]>

---
 drivers/acpi/ec.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

drivers/acpi/ec.c +13 -1

index 79817ce..9d4761d 100644
@@ -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);

Signature · 1 line

1.7.10

Rafael J. Wysocki

Applied

2015-06-15 22:53 UTC #14 lore raw