Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1741903 > unrolled thread

[RFC PATCH v6 0/3] ACPI / EC: Tune the timing of EC events arrival during S3-exit

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC PATCH v6 0/3] ACPI / EC: Tune the timing of EC events arrival during S3-exit Lv Zheng <lv.zheng@intel.com> - 2017-09-29 05:00 +0200
    [RFC PATCH v6 3/3] ACPI / EC: Enable noirq stage event detection Lv Zheng <lv.zheng@intel.com> - 2017-09-29 05:00 +0200
    [RFC PATCH v6 1/3] ACPI / EC: Fix possible driver order issue by moving EC event handling earlier Lv Zheng <lv.zheng@intel.com> - 2017-09-29 05:00 +0200
    [RFC PATCH v6 2/3] ACPI / EC: Add event detection support for noirq stages Lv Zheng <lv.zheng@intel.com> - 2017-09-29 05:00 +0200

#1741903 — [RFC PATCH v6 0/3] ACPI / EC: Tune the timing of EC events arrival during S3-exit

If EC events occurred during BIOS S3-exit and early OS S3-exit steps can
be detected by OS earlier, then there can be less driver order issues
between acpi_ec_resume() and some other drivers' .resume() hook (e.x.
acpi_button_resume()).

However there are known facts that EC FW does drop EC events during S3,
and it takes time for EC FW to initialize (maximum 1.4 observed) while
Windows acts normally, so detecting EC event earlier might just be a
workaround for other drivers (they should be aware of this order issue and
deal with it themselves). As such, this patchset is marked as an RFC.

If Linux EC driver started to detect events during early OS S3-exit, it
need to timely poll EC events during noirq stages as in this stage there is
no EC event triggering source.

This patchset implements earlier EC event handling for Linux.

Lv Zheng (3):
  ACPI / EC: Fix possible driver order issue by moving EC event handling
    earlier
  ACPI / EC: Add event detection support for noirq stages
  ACPI / EC: Enable noirq stage event detection

 drivers/acpi/ec.c       | 128 +++++++++++++++++++++++++++++++++++++++++++-----
 drivers/acpi/internal.h |   1 +
 2 files changed, 118 insertions(+), 11 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1741904 — [RFC PATCH v6 3/3] ACPI / EC: Enable noirq stage event detection

This patch enables noirq stage event detection for the EC driver.

EC is a very special driver, required to detecting events throughout the
entire suspend/resume process. Thus this patch enables event detection for
EC during noirq stages to meet this requirement. This is done by making
sure that the EC sleep APIs:
  acpi_ec_block_transactions()
  acpi_ec_unblock_transactions()
rather than the EC driver suspend/resume hooks:
  acpi_ec_suspend()
  acpi_ec_resume()
are the boundary of the EC event handling during suspend/resume, so that
the ACPI sleep core can tune their invocation timing to handle special BIOS
requirements.

If this commit is bisected to be a regression culprit, please report this
to bugzilla.kernel.org for further investigation.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=196129
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Tested-by: Tomislav Ivek <tomislav.ivek@gmail.com>
---
 drivers/acpi/ec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 389c499..a48a2b3 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -157,7 +157,7 @@ static bool ec_no_wakeup __read_mostly;
 module_param(ec_no_wakeup, bool, 0644);
 MODULE_PARM_DESC(ec_no_wakeup, "Do not wake up from suspend-to-idle");
 
-static bool ec_detect_noirq_events __read_mostly;
+static bool ec_detect_noirq_events __read_mostly = true;
 module_param(ec_detect_noirq_events, bool, 0644);
 MODULE_PARM_DESC(ec_detect_noirq_events, "Enabling event detection during noirq stage");
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1741905 — [RFC PATCH v6 1/3] ACPI / EC: Fix possible driver order issue by moving EC event handling earlier

This patch tries to detect EC events earlier after resume, so that if an
event occurred before invoking acpi_ec_unblock_transactions(), it could be
detected by acpi_ec_unblock_transactions() which is the earliest EC driver
call after resume.

However after the noirq stage, if an event ocurred after
acpi_ec_unblock_transactions() and before acpi_ec_resume(), there was no
mean to detect and trigger it right then, but can only detect it and handle
it after acpi_ec_resume().

Now the final logic is:
1. If ec_freeze_events=Y, event handling is stopped in acpi_ec_suspend(),
   restarted in acpi_ec_resume();
2. If ec_freeze_events=N, event handling is stopped in
   acpi_ec_block_transactions(), restarted in
   acpi_ec_unblock_transactions();
3. In order to handling the conflict of the edge-trigger nature of EC IRQ
   and the Linux noirq stage, advance_transaction() is invoked where the
   event handling is enabled and the noirq stage is ended.

Known issue:
1. Event ocurred between acpi_ec_unblock_transactions() and
   acpi_ec_resume() may still lead to the order issue. This can only be
   fixed by adding a periodic detection mechanism during the noirq stage.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Tested-by: Tomislav Ivek <tomislav.ivek@gmail.com>
Tested-by: Luya Tshimbalanga <luya@fedoraproject.org>
---
 drivers/acpi/ec.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index df84246..f1f320b 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -249,6 +249,11 @@ static bool acpi_ec_started(struct acpi_ec *ec)
 	       !test_bit(EC_FLAGS_STOPPED, &ec->flags);
 }
 
+static bool acpi_ec_no_sleep_events(void)
+{
+	return acpi_sleep_no_ec_events() && ec_freeze_events;
+}
+
 static bool acpi_ec_event_enabled(struct acpi_ec *ec)
 {
 	/*
@@ -260,14 +265,14 @@ static bool acpi_ec_event_enabled(struct acpi_ec *ec)
 		return false;
 	/*
 	 * However, disabling the event handling is experimental for late
-	 * stage (suspend), and is controlled by the boot parameter of
-	 * "ec_freeze_events":
+	 * stage (suspend), and is controlled by
+	 * "acpi_ec_no_sleep_events()":
 	 * 1. true:  The EC event handling is disabled before entering
 	 *           the noirq stage.
 	 * 2. false: The EC event handling is automatically disabled as
 	 *           soon as the EC driver is stopped.
 	 */
-	if (ec_freeze_events)
+	if (acpi_ec_no_sleep_events())
 		return acpi_ec_started(ec);
 	else
 		return test_bit(EC_FLAGS_STARTED, &ec->flags);
@@ -524,8 +529,8 @@ static bool acpi_ec_query_flushed(struct acpi_ec *ec)
 static void __acpi_ec_flush_event(struct acpi_ec *ec)
 {
 	/*
-	 * When ec_freeze_events is true, we need to flush events in
-	 * the proper position before entering the noirq stage.
+	 * When acpi_ec_no_sleep_events() is true, we need to flush events
+	 * in the proper position before entering the noirq stage.
 	 */
 	wait_event(ec->wait, acpi_ec_query_flushed(ec));
 	if (ec_query_wq)
@@ -948,7 +953,8 @@ static void acpi_ec_start(struct acpi_ec *ec, bool resuming)
 		if (!resuming) {
 			acpi_ec_submit_request(ec);
 			ec_dbg_ref(ec, "Increase driver");
-		}
+		} else if (!acpi_ec_no_sleep_events())
+			__acpi_ec_enable_event(ec);
 		ec_log_drv("EC started");
 	}
 	spin_unlock_irqrestore(&ec->lock, flags);
@@ -980,7 +986,7 @@ static void acpi_ec_stop(struct acpi_ec *ec, bool suspending)
 		if (!suspending) {
 			acpi_ec_complete_request(ec);
 			ec_dbg_ref(ec, "Decrease driver");
-		} else if (!ec_freeze_events)
+		} else if (!acpi_ec_no_sleep_events())
 			__acpi_ec_disable_event(ec);
 		clear_bit(EC_FLAGS_STARTED, &ec->flags);
 		clear_bit(EC_FLAGS_STOPPED, &ec->flags);
@@ -1910,7 +1916,7 @@ static int acpi_ec_suspend(struct device *dev)
 	struct acpi_ec *ec =
 		acpi_driver_data(to_acpi_device(dev));
 
-	if (acpi_sleep_no_ec_events() && ec_freeze_events)
+	if (acpi_ec_no_sleep_events())
 		acpi_ec_disable_event(ec);
 	return 0;
 }
@@ -1946,7 +1952,18 @@ static int acpi_ec_resume(struct device *dev)
 	struct acpi_ec *ec =
 		acpi_driver_data(to_acpi_device(dev));
 
-	acpi_ec_enable_event(ec);
+	if (acpi_ec_no_sleep_events())
+		acpi_ec_enable_event(ec);
+	else {
+		/*
+		 * Though whether there is an event pending has been
+		 * checked in acpi_ec_unblock_transactions() when
+		 * acpi_ec_no_sleep_events() is false, check it one more
+		 * time after noirq stage to detect events occurred after
+		 * acpi_ec_unblock_transactions().
+		 */
+		advance_transaction(ec);
+	}
 	return 0;
 }
 #endif
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1741906 — [RFC PATCH v6 2/3] ACPI / EC: Add event detection support for noirq stages

This patch adds a timer to poll EC events:
1. between acpi_ec_suspend() and acpi_ec_block_transactions(),
2. between acpi_ec_unblock_transactions() and acpi_ec_resume().
During these periods, if an EC event occurred, we have not mean to detect
it. Thus the events occurred in late S3-entry could be dropped, and the
events occurred in early S3-exit could be deferred to acpi_ec_resume().

This patch solves event losses in S3-entry and resume order in S3-exit by
timely polling EC events during these periods.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=196129 [#1]
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Tested-by: Tomislav Ivek <tomislav.ivek@gmail.com>
---
 drivers/acpi/ec.c       | 93 +++++++++++++++++++++++++++++++++++++++++++++++--
 drivers/acpi/internal.h |  1 +
 2 files changed, 92 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index f1f320b..389c499 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -40,6 +40,7 @@
 #include <linux/slab.h>
 #include <linux/acpi.h>
 #include <linux/dmi.h>
+#include <linux/timer.h>
 #include <asm/io.h>
 
 #include "internal.h"
@@ -102,6 +103,7 @@ enum ec_command {
 #define ACPI_EC_CLEAR_MAX	100	/* Maximum number of events to query
 					 * when trying to clear the EC */
 #define ACPI_EC_MAX_QUERIES	16	/* Maximum number of parallel queries */
+#define ACPI_EC_EVENT_INTERVAL	500	/* Detecting event every 500ms */
 
 enum {
 	EC_FLAGS_QUERY_ENABLED,		/* Query is enabled */
@@ -113,6 +115,7 @@ enum {
 	EC_FLAGS_STARTED,		/* Driver is started */
 	EC_FLAGS_STOPPED,		/* Driver is stopped */
 	EC_FLAGS_GPE_MASKED,		/* GPE masked */
+	EC_FLAGS_GPE_POLLING,		/* GPE polling is enabled */
 };
 
 #define ACPI_EC_COMMAND_POLL		0x01 /* Available for command byte */
@@ -154,6 +157,15 @@ static bool ec_no_wakeup __read_mostly;
 module_param(ec_no_wakeup, bool, 0644);
 MODULE_PARM_DESC(ec_no_wakeup, "Do not wake up from suspend-to-idle");
 
+static bool ec_detect_noirq_events __read_mostly;
+module_param(ec_detect_noirq_events, bool, 0644);
+MODULE_PARM_DESC(ec_detect_noirq_events, "Enabling event detection during noirq stage");
+
+static unsigned int
+ec_detect_noirq_interval __read_mostly = ACPI_EC_EVENT_INTERVAL;
+module_param(ec_detect_noirq_interval, uint, 0644);
+MODULE_PARM_DESC(ec_detect_noirq_interval, "Event detection interval(ms) during noirq stage");
+
 struct acpi_ec_query_handler {
 	struct list_head node;
 	acpi_ec_query_func func;
@@ -358,6 +370,48 @@ static inline bool acpi_ec_is_gpe_raised(struct acpi_ec *ec)
 	return (gpe_status & ACPI_EVENT_FLAG_STATUS_SET) ? true : false;
 }
 
+static void acpi_ec_gpe_tick(struct acpi_ec *ec)
+{
+	mod_timer(&ec->timer,
+		  jiffies + msecs_to_jiffies(ec_detect_noirq_interval));
+}
+
+static void ec_start_gpe_poller(struct acpi_ec *ec)
+{
+	unsigned long flags;
+	bool start_tick = false;
+
+	if (!acpi_ec_no_sleep_events() || !ec_detect_noirq_events)
+		return;
+	spin_lock_irqsave(&ec->lock, flags);
+	if (!test_and_set_bit(EC_FLAGS_GPE_POLLING, &ec->flags)) {
+		ec_log_drv("GPE poller started");
+		start_tick = true;
+		/* kick off GPE polling without delay */
+		advance_transaction(ec);
+	}
+	spin_unlock_irqrestore(&ec->lock, flags);
+	if (start_tick)
+		acpi_ec_gpe_tick(ec);
+}
+
+static void ec_stop_gpe_poller(struct acpi_ec *ec)
+{
+	unsigned long flags;
+	bool stop_tick = false;
+
+	if (!acpi_ec_no_sleep_events() || !ec_detect_noirq_events)
+		return;
+	spin_lock_irqsave(&ec->lock, flags);
+	if (test_and_clear_bit(EC_FLAGS_GPE_POLLING, &ec->flags))
+		stop_tick = true;
+	spin_unlock_irqrestore(&ec->lock, flags);
+	if (stop_tick) {
+		del_timer_sync(&ec->timer);
+		ec_log_drv("GPE poller stopped");
+	}
+}
+
 static inline void acpi_ec_enable_gpe(struct acpi_ec *ec, bool open)
 {
 	if (open)
@@ -1017,6 +1071,12 @@ static void acpi_ec_leave_noirq(struct acpi_ec *ec)
 	spin_unlock_irqrestore(&ec->lock, flags);
 }
 
+/*
+ * Note: this API is prepared for tuning the order of the ACPI
+ * suspend/resume steps as the last entry of EC during suspend, thus it
+ * must be invoked after acpi_ec_suspend() or everything should be done in
+ * acpi_ec_suspend().
+ */
 void acpi_ec_block_transactions(void)
 {
 	struct acpi_ec *ec = first_ec;
@@ -1028,16 +1088,28 @@ void acpi_ec_block_transactions(void)
 	/* Prevent transactions from being carried out */
 	acpi_ec_stop(ec, true);
 	mutex_unlock(&ec->mutex);
+	ec_stop_gpe_poller(ec);
 }
 
+/*
+ * Note: this API is prepared for tuning the order of the ACPI
+ * suspend/resume steps as the first entry of EC during resume, thus it
+ * must be invoked before acpi_ec_resume() or everything should be done in
+ * acpi_ec_resume().
+ */
 void acpi_ec_unblock_transactions(void)
 {
+	struct acpi_ec *ec = first_ec;
+
+	if (!ec)
+		return;
+
+	ec_start_gpe_poller(ec);
 	/*
 	 * Allow transactions to happen again (this function is called from
 	 * atomic context during wakeup, so we don't need to acquire the mutex).
 	 */
-	if (first_ec)
-		acpi_ec_start(first_ec, true);
+	acpi_ec_start(ec, true);
 }
 
 /* --------------------------------------------------------------------------
@@ -1278,6 +1350,19 @@ static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
 	return ACPI_INTERRUPT_HANDLED;
 }
 
+static void acpi_ec_gpe_poller(ulong arg)
+{
+	struct acpi_ec *ec = (struct acpi_ec *)arg;
+	unsigned long flags;
+
+	spin_lock_irqsave(&ec->lock, flags);
+	ec_dbg_drv("GPE polling begin");
+	advance_transaction(ec);
+	ec_dbg_drv("GPE polling end");
+	spin_unlock_irqrestore(&ec->lock, flags);
+	acpi_ec_gpe_tick(ec);
+}
+
 /* --------------------------------------------------------------------------
  *                           Address Space Management
  * -------------------------------------------------------------------------- */
@@ -1347,6 +1432,8 @@ static struct acpi_ec *acpi_ec_alloc(void)
 	INIT_LIST_HEAD(&ec->list);
 	spin_lock_init(&ec->lock);
 	INIT_WORK(&ec->work, acpi_ec_event_handler);
+	init_timer(&ec->timer);
+	setup_timer(&ec->timer, acpi_ec_gpe_poller, (ulong)ec);
 	ec->timestamp = jiffies;
 	ec->busy_polling = true;
 	ec->polling_guard = 0;
@@ -1918,6 +2005,7 @@ static int acpi_ec_suspend(struct device *dev)
 
 	if (acpi_ec_no_sleep_events())
 		acpi_ec_disable_event(ec);
+	ec_start_gpe_poller(ec);
 	return 0;
 }
 
@@ -1952,6 +2040,7 @@ static int acpi_ec_resume(struct device *dev)
 	struct acpi_ec *ec =
 		acpi_driver_data(to_acpi_device(dev));
 
+	ec_stop_gpe_poller(ec);
 	if (acpi_ec_no_sleep_events())
 		acpi_ec_enable_event(ec);
 	else {
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index ede83d3..708b379 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -171,6 +171,7 @@ struct acpi_ec {
 	struct transaction *curr;
 	spinlock_t lock;
 	struct work_struct work;
+	struct timer_list timer;
 	unsigned long timestamp;
 	unsigned long nr_pending_queries;
 	bool busy_polling;
-- 
2.7.4

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web