[PATCH 1/3] ACPI / EC: Cleanup EC GPE mask flag

[PATCH 1/3] ACPI / EC: Cleanup EC GPE mask flag

[Lv Zheng]

EC_FLAGS_COMMAND_STORM is actually used to mask GPE during IRQ processing.
This patch cleans it up using more readable flag/function names.

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

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index ddb01e9..54879c7 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -112,8 +112,7 @@ enum {
 	EC_FLAGS_EVT_HANDLER_INSTALLED, /* _Qxx handlers installed */
 	EC_FLAGS_STARTED,		/* Driver is started */
 	EC_FLAGS_STOPPED,		/* Driver is stopped */
-	EC_FLAGS_COMMAND_STORM,		/* GPE storms occurred to the
-					 * current command processing */
+	EC_FLAGS_GPE_MASKED,		/* GPE masked */
 };
 
 #define ACPI_EC_COMMAND_POLL		0x01 /* Available for command byte */
@@ -421,19 +420,19 @@ static void acpi_ec_complete_request(struct acpi_ec *ec)
 		wake_up(&ec->wait);
 }
 
-static void acpi_ec_set_storm(struct acpi_ec *ec, u8 flag)
+static void acpi_ec_mask_gpe(struct acpi_ec *ec)
 {
-	if (!test_bit(flag, &ec->flags)) {
+	if (!test_bit(EC_FLAGS_GPE_MASKED, &ec->flags)) {
 		acpi_ec_disable_gpe(ec, false);
 		ec_dbg_drv("Polling enabled");
-		set_bit(flag, &ec->flags);
+		set_bit(EC_FLAGS_GPE_MASKED, &ec->flags);
 	}
 }
 
-static void acpi_ec_clear_storm(struct acpi_ec *ec, u8 flag)
+static void acpi_ec_unmask_gpe(struct acpi_ec *ec)
 {
-	if (test_bit(flag, &ec->flags)) {
-		clear_bit(flag, &ec->flags);
+	if (test_bit(EC_FLAGS_GPE_MASKED, &ec->flags)) {
+		clear_bit(EC_FLAGS_GPE_MASKED, &ec->flags);
 		acpi_ec_enable_gpe(ec, false);
 		ec_dbg_drv("Polling disabled");
 	}
@@ -460,7 +459,7 @@ static bool acpi_ec_submit_flushable_request(struct acpi_ec *ec)
 
 static void acpi_ec_submit_query(struct acpi_ec *ec)
 {
-	acpi_ec_set_storm(ec, EC_FLAGS_COMMAND_STORM);
+	acpi_ec_mask_gpe(ec);
 	if (!acpi_ec_event_enabled(ec))
 		return;
 	if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
@@ -476,7 +475,7 @@ static void acpi_ec_complete_query(struct acpi_ec *ec)
 	if (test_and_clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags))
 		ec_dbg_evt("Command(%s) unblocked",
 			   acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
-	acpi_ec_clear_storm(ec, EC_FLAGS_COMMAND_STORM);
+	acpi_ec_unmask_gpe(ec);
 }
 
 static inline void __acpi_ec_enable_event(struct acpi_ec *ec)
@@ -688,7 +687,7 @@ static void advance_transaction(struct acpi_ec *ec)
 				++t->irq_count;
 			/* Allow triggering on 0 threshold */
 			if (t->irq_count == ec_storm_threshold)
-				acpi_ec_set_storm(ec, EC_FLAGS_COMMAND_STORM);
+				acpi_ec_mask_gpe(ec);
 		}
 	}
 out:
@@ -786,7 +785,7 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
 
 	spin_lock_irqsave(&ec->lock, tmp);
 	if (t->irq_count == ec_storm_threshold)
-		acpi_ec_clear_storm(ec, EC_FLAGS_COMMAND_STORM);
+		acpi_ec_unmask_gpe(ec);
 	ec_dbg_req("Command(%s) stopped", acpi_ec_cmd_string(t->command));
 	ec->curr = NULL;
 	/* Disable GPE for command processing (IBF=0/OBF=1) */
-- 
2.7.4

[PATCH 2/3] ACPI: EC: Add IRQ polling support for noirq stages

[Lv Zheng]

EC IRQs contain transaction IRQs (OBF/IBF) and event IRQ (SCI_EVT).

Transactions are initiated by hosts. The earliest OSPMs execution of EC
transactions is from acpi_ec_transaction(), where the common EC IRQ
handling procedure - advance_transaction() - is initiated from the task
context.
Events are initiated by targets. The earliest OSPMs execution of EC events
is from acpi_ec_gpe_handler(), where the common EC IRQ handling procedure -
advance_transaction() - is initiated from the IRQ context. However if
there is an EC transaction pending, advance_transaction() invoked from the
task context can also help to detect SCI_EVT and initiate the handling of
the EC events.

There is a noirq stage during system suspend/resume procedures. We can see
that during this stage, advance_transaction() which monitors SCI_EVT can
only be invoked from ec_poll().  Thus if there is no EC transaction
occuring in this stage, EC driver cannot detect SCI_EVT.

Note that such event IRQs may risk lost. Normally, when GPE is enabled and
GPE is flagged, IRQ can be triggered and such events thus can be detected
in the IRQ handler. But there might be GPE chips not capable of triggering
IRQs upon enabling. Thus originally we tried to use "ec_freeze_events" to
stop handling SCI_EVT earlier after suspend and re-start handling SCI_EVT
after resume to avoid such IRQ lost. The EC event handling is namely
paused during suspend/resume and "ec_freeze_events" controls the timing of
the pause.

Recently, some bug reports (see Link #1) revealed that we shouldn't pause
EC event handling too early during these system suspend/resume procedures.
Based on this fact, we should detect SCI_EVT during noirq stage, this left
us no other choice than implementing IRQ polling for SCI_EVT in this stage.

This patch adds a timer to poll EC events timely (0.5s) during system
suspend/resume noirq stages. As the bug reports may not be root caused,
and most of the platforms may not require this, this patch prepares an
option to make this behavior configurable.

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

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 54879c7..47f900c 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"
@@ -87,6 +88,31 @@
 #define ACPI_EC_EVT_TIMING_QUERY	0x01
 #define ACPI_EC_EVT_TIMING_EVENT	0x02
 
+/*
+ * There is a noirq stage during suspend/resume and EC firmware may
+ * require OS to handle EC events (SCI_EVT) during this stage.
+ * If there is no EC transactions during this stage, SCI_EVT cannot be
+ * detected. In order to still detect SCI_EVT, IRQ must be polled by the
+ * EC GPE poller. There are 3 configurable modes implemented for the EC
+ * GPE poller:
+ * NONE:    Do not enable noirq stage GPE poller.
+ * SUSPEND: GPE poller is disabled at the end of the suspend.
+ *          This mode detects SCI_EVT in suspend noirq stage, making sure
+ *          that all pre-suspend firmware events are handled before
+ *          entering a low power state. Some buggy EC firmware may require
+ *          this, otherwise some unknown firmware issues can be seen on
+ *          such platforms:
+ *          Link: https://bugzilla.kernel.org/show_bug.cgi?id=196129
+ * RESUME:  GPE poller is disabled at the begining of the resume.
+ *          This mode detects SCI_EVT in both suspend and resume noirq
+ *          stages, making sure that all post-resume firmware events are
+ *          handled as early as possible. This mode might be able to solve
+ *          some unknown driver timing issues.
+ */
+#define ACPI_EC_GPE_POLL_NONE		0x00
+#define ACPI_EC_GPE_POLL_SUSPEND	0x01
+#define ACPI_EC_GPE_POLL_RESUME		0x02
+
 /* EC commands */
 enum ec_command {
 	ACPI_EC_COMMAND_READ = 0x80,
@@ -102,6 +128,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_POLL_INTERVAL	500	/* Polling event every 500ms */
 
 enum {
 	EC_FLAGS_QUERY_ENABLED,		/* Query is enabled */
@@ -113,6 +140,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 */
@@ -136,6 +164,7 @@ 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_QUERY;
+static unsigned int ec_gpe_polling __read_mostly = ACPI_EC_GPE_POLL_NONE;
 
 /*
  * If the number of false interrupts per one transaction exceeds
@@ -150,6 +179,10 @@ static bool ec_freeze_events __read_mostly = false;
 module_param(ec_freeze_events, bool, 0644);
 MODULE_PARM_DESC(ec_freeze_events, "Disabling event handling during suspend/resume");
 
+static unsigned int ec_poll_interval __read_mostly = ACPI_EC_POLL_INTERVAL;
+module_param(ec_poll_interval, uint, 0644);
+MODULE_PARM_DESC(ec_poll_interval, "GPE polling interval(ms)");
+
 struct acpi_ec_query_handler {
 	struct list_head node;
 	acpi_ec_query_func func;
@@ -349,6 +382,53 @@ 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_poll_interval));
+}
+
+static void ec_start_gpe_poller(struct acpi_ec *ec)
+{
+	unsigned long flags;
+	bool start_tick = false;
+
+	if (ec_gpe_polling == ACPI_EC_GPE_POLL_NONE)
+		return;
+
+	spin_lock_irqsave(&ec->lock, flags);
+	if (!test_and_set_bit(EC_FLAGS_GPE_POLLING, &ec->flags))
+		start_tick = true;
+	spin_unlock_irqrestore(&ec->lock, flags);
+	if (start_tick) {
+		acpi_ec_gpe_tick(ec);
+		ec_log_drv("GPE poller started");
+	}
+}
+
+static void ec_stop_gpe_poller(struct acpi_ec *ec, bool is_resume)
+{
+	unsigned long flags;
+	bool stop_tick = false;
+
+	/*
+	 * Always disable GPE poller for "none" mode. For other modes,
+	 * disable GPE poller from proper stage.
+	 */
+	if ((ec_gpe_polling == ACPI_EC_GPE_POLL_SUSPEND && is_resume) ||
+	    (ec_gpe_polling == ACPI_EC_GPE_POLL_RESUME && !is_resume))
+		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)
@@ -972,6 +1052,7 @@ static void acpi_ec_stop(struct acpi_ec *ec, bool suspending)
 		ec_log_drv("EC stopped");
 	}
 	spin_unlock_irqrestore(&ec->lock, flags);
+	ec_stop_gpe_poller(ec, false);
 }
 
 static void acpi_ec_enter_noirq(struct acpi_ec *ec)
@@ -1257,6 +1338,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
  * -------------------------------------------------------------------------- */
@@ -1326,6 +1420,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;
@@ -1874,6 +1970,7 @@ static int acpi_ec_suspend(struct device *dev)
 	struct acpi_ec *ec =
 		acpi_driver_data(to_acpi_device(dev));
 
+	ec_start_gpe_poller(ec);
 	if (acpi_sleep_no_ec_events() && ec_freeze_events)
 		acpi_ec_disable_event(ec);
 	return 0;
@@ -1885,6 +1982,7 @@ static int acpi_ec_resume(struct device *dev)
 		acpi_driver_data(to_acpi_device(dev));
 
 	acpi_ec_enable_event(ec);
+	ec_stop_gpe_poller(ec, true);
 	return 0;
 }
 #endif
@@ -1930,6 +2028,43 @@ module_param_call(ec_event_clearing, param_set_event_clearing, param_get_event_c
 		  NULL, 0644);
 MODULE_PARM_DESC(ec_event_clearing, "Assumed SCI_EVT clearing timing");
 
+static int param_set_gpe_polling(const char *val, struct kernel_param *kp)
+{
+	int result = 0;
+
+	if (!strncmp(val, "none", sizeof("none") - 1)) {
+		ec_gpe_polling = ACPI_EC_GPE_POLL_NONE;
+		pr_info("GPE noirq stage polling disabled\n");
+	} else if (!strncmp(val, "suspend", sizeof("suspend") - 1)) {
+		ec_gpe_polling = ACPI_EC_GPE_POLL_SUSPEND;
+		pr_info("GPE noirq suspend polling enabled\n");
+	} else if (!strncmp(val, "resume", sizeof("resume") - 1)) {
+		ec_gpe_polling = ACPI_EC_GPE_POLL_RESUME;
+		pr_info("GPE noirq suspend/resume polling enabled\n");
+	} else
+		result = -EINVAL;
+	return result;
+}
+
+static int param_get_gpe_polling(char *buffer, struct kernel_param *kp)
+{
+	switch (ec_gpe_polling) {
+	case ACPI_EC_GPE_POLL_NONE:
+		return sprintf(buffer, "none");
+	case ACPI_EC_GPE_POLL_SUSPEND:
+		return sprintf(buffer, "suspend");
+	case ACPI_EC_GPE_POLL_RESUME:
+		return sprintf(buffer, "resume");
+	default:
+		return sprintf(buffer, "invalid");
+	}
+	return 0;
+}
+
+module_param_call(ec_gpe_polling, param_set_gpe_polling, param_get_gpe_polling,
+		  NULL, 0644);
+MODULE_PARM_DESC(ec_gpe_polling, "Enabling GPE polling during noirq stages");
+
 static struct acpi_driver acpi_ec_driver = {
 	.name = "ec",
 	.class = ACPI_EC_CLASS,
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 9531d32..6bd36b1 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -170,6 +170,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

[PATCH 3/3] ACPI: EC: Enable noirq stage GPE polling

[Lv Zheng]

This patch enables noirq stage GPE polling for the EC driver.

EC is a very special driver, required to work throughout the entire
suspend/resume process. Thus this patch enables IRQ polling for EC during
noirq stages to avoid all kinds of possible issues.

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

Signed-off-by: Lv Zheng <lv.zheng@intel.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 47f900c..12e0c8a 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -164,7 +164,7 @@ 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_QUERY;
-static unsigned int ec_gpe_polling __read_mostly = ACPI_EC_GPE_POLL_NONE;
+static unsigned int ec_gpe_polling __read_mostly = ACPI_EC_GPE_POLL_RESUME;
 
 /*
  * If the number of false interrupts per one transaction exceeds
-- 
2.7.4

[PATCH v2 0/4] ACPI / EC: Solve EC event handling issues

[Lv Zheng]

There is a known issue in EC event handling. This patchset tries to handle
noirq stage EC event polling to fix this known issue.

In the very early version, the EC event polling mechanism is implemented by
a kernel thread to poll EC events. Now the mechanism is implemented by a
timer ticked in noirq stage to poll EC GPEs. In this newest timer version,
timer running period is shortened and thus is safer for s2idle mode.

After adding such a mechanism, we can try to handle EC events earlier after
resume, this may be able to solve some driver order issues.

Lv Zheng (4):
  ACPI / EC: Cleanup EC GPE mask flag
  ACPI / EC: Add IRQ polling support for noirq stages
  ACPI / EC: Add support to handle EC events earlier
  ACPI / EC: Enable noirq stage GPE polling

 drivers/acpi/ec.c       | 157 ++++++++++++++++++++++++++++++++++++++++++++----
 drivers/acpi/internal.h |   1 +
 2 files changed, 145 insertions(+), 13 deletions(-)

-- 
2.7.4

[PATCH v2 1/4] ACPI / EC: Cleanup EC GPE mask flag

[Lv Zheng]

EC_FLAGS_COMMAND_STORM is actually used to mask GPE during IRQ processing.
This patch cleans it up using more readable flag/function names.

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

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index ddb01e9..54879c7 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -112,8 +112,7 @@ enum {
 	EC_FLAGS_EVT_HANDLER_INSTALLED, /* _Qxx handlers installed */
 	EC_FLAGS_STARTED,		/* Driver is started */
 	EC_FLAGS_STOPPED,		/* Driver is stopped */
-	EC_FLAGS_COMMAND_STORM,		/* GPE storms occurred to the
-					 * current command processing */
+	EC_FLAGS_GPE_MASKED,		/* GPE masked */
 };
 
 #define ACPI_EC_COMMAND_POLL		0x01 /* Available for command byte */
@@ -421,19 +420,19 @@ static void acpi_ec_complete_request(struct acpi_ec *ec)
 		wake_up(&ec->wait);
 }
 
-static void acpi_ec_set_storm(struct acpi_ec *ec, u8 flag)
+static void acpi_ec_mask_gpe(struct acpi_ec *ec)
 {
-	if (!test_bit(flag, &ec->flags)) {
+	if (!test_bit(EC_FLAGS_GPE_MASKED, &ec->flags)) {
 		acpi_ec_disable_gpe(ec, false);
 		ec_dbg_drv("Polling enabled");
-		set_bit(flag, &ec->flags);
+		set_bit(EC_FLAGS_GPE_MASKED, &ec->flags);
 	}
 }
 
-static void acpi_ec_clear_storm(struct acpi_ec *ec, u8 flag)
+static void acpi_ec_unmask_gpe(struct acpi_ec *ec)
 {
-	if (test_bit(flag, &ec->flags)) {
-		clear_bit(flag, &ec->flags);
+	if (test_bit(EC_FLAGS_GPE_MASKED, &ec->flags)) {
+		clear_bit(EC_FLAGS_GPE_MASKED, &ec->flags);
 		acpi_ec_enable_gpe(ec, false);
 		ec_dbg_drv("Polling disabled");
 	}
@@ -460,7 +459,7 @@ static bool acpi_ec_submit_flushable_request(struct acpi_ec *ec)
 
 static void acpi_ec_submit_query(struct acpi_ec *ec)
 {
-	acpi_ec_set_storm(ec, EC_FLAGS_COMMAND_STORM);
+	acpi_ec_mask_gpe(ec);
 	if (!acpi_ec_event_enabled(ec))
 		return;
 	if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
@@ -476,7 +475,7 @@ static void acpi_ec_complete_query(struct acpi_ec *ec)
 	if (test_and_clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags))
 		ec_dbg_evt("Command(%s) unblocked",
 			   acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
-	acpi_ec_clear_storm(ec, EC_FLAGS_COMMAND_STORM);
+	acpi_ec_unmask_gpe(ec);
 }
 
 static inline void __acpi_ec_enable_event(struct acpi_ec *ec)
@@ -688,7 +687,7 @@ static void advance_transaction(struct acpi_ec *ec)
 				++t->irq_count;
 			/* Allow triggering on 0 threshold */
 			if (t->irq_count == ec_storm_threshold)
-				acpi_ec_set_storm(ec, EC_FLAGS_COMMAND_STORM);
+				acpi_ec_mask_gpe(ec);
 		}
 	}
 out:
@@ -786,7 +785,7 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
 
 	spin_lock_irqsave(&ec->lock, tmp);
 	if (t->irq_count == ec_storm_threshold)
-		acpi_ec_clear_storm(ec, EC_FLAGS_COMMAND_STORM);
+		acpi_ec_unmask_gpe(ec);
 	ec_dbg_req("Command(%s) stopped", acpi_ec_cmd_string(t->command));
 	ec->curr = NULL;
 	/* Disable GPE for command processing (IBF=0/OBF=1) */
-- 
2.7.4

[PATCH v2 2/4] ACPI / EC: Add IRQ polling support for noirq stages

[Lv Zheng]

1. Problems:
EC IRQs contain transaction IRQs (OBF/IBF) and event IRQ (SCI_EVT).

Transactions are initiated by hosts. The earliest OSPMs execution of EC
transactions is from acpi_ec_transaction(), where the common EC IRQ
handling procedure - advance_transaction() - is initiated from the task
context.
Events are initiated by targets. The earliest OSPMs execution of EC events
is from acpi_ec_gpe_handler(), where the common EC IRQ handling procedure -
advance_transaction() - is initiated from the IRQ context.

1.1. Problem 1: Cannot detect EC event in noirq stages.
There is a noirq stage during system suspend/resume procedures. We can see
that during this stage, advance_transaction() which monitors SCI_EVT can
only be invoked from ec_poll().  Thus if there is no EC transaction
occurring in this stage, EC driver cannot detect SCI_EVT.

Note that in noirq stage if there is an EC transaction pending,
advance_transaction() invoked because of the EC transaction can also help
to detect SCI_EVT and initiate the handling of the EC events. That's why
real reports related to this problem are rare and unreproducible as whether
there is an EC transaction occurred after an undetectable EC events during
noirq stages is random.

1.2. Problem 2: Handling of EC events may stuck for some GPE chips.
Normally, when STS is set, GPE IRQs can be triggered by GPE chips when EN
bit is set. Thus it is ensured that handling of the EC events triggered in
suspend noirq stage is just deferred to resume stage and there won't be EC
event losts.

But there might be chips do not contain AND logic betwee STS and EN to
trigger GPE. In such worst case, EC events may risk lost, handling of the
lost EC events can only start when there is an EC transaction occurred.
Thus if there is no EC transaction, handling of EC events may "stuck"
after resume in such worst case.

2. Old assumption and solution:
Originally, as EC driver actually has never been fully able to handle EC
events during noirq stages, we assumed that detecting such events in noirq
stage is useless and implemented "ec_freeze_events" to stop handling
SCI_EVT earlier after suspend and re-start handling SCI_EVT after resume.
The EC event handling is namely "frozen" during suspend/resume and
"ec_freeze_events" controls the timing of the "freeze". If this could work,
we shouldn't be required to implement GPE polling in noirq stage.

Note that this solution cannot solve problem 2.

3. New fact and solution:
Recently, some bug reports (see Link #1) revealed that we shouldn't
"freeze" EC event handling too early during these system suspend/resume
procedures. Based on this fact, we should detect SCI_EVT during noirq
stage, this left us no other choice than implementing IRQ polling for
SCI_EVT in this stage.

This patch adds a timer to poll EC events timely (0.5s) during system
suspend/resume noirq stages. As the bug reports may not be root caused,
and most of the platforms may not require this, this patch prepares an
option to make this behavior configurable.

Note that this solution can also solve problem 2.

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

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 54879c7..f1cffd4 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"
@@ -87,6 +88,31 @@
 #define ACPI_EC_EVT_TIMING_QUERY	0x01
 #define ACPI_EC_EVT_TIMING_EVENT	0x02
 
+/*
+ * There is a noirq stage during suspend/resume and EC firmware may
+ * require OS to handle EC events (SCI_EVT) during this stage.
+ * If there is no EC transactions during this stage, SCI_EVT cannot be
+ * detected. In order to still detect SCI_EVT, IRQ must be polled by the
+ * EC GPE poller. There are 3 configurable modes implemented for the EC
+ * GPE poller:
+ * NONE:    Do not enable noirq stage GPE poller.
+ * SUSPEND: Enable GPE poller for suspend noirq stage.
+ *          This mode detects SCI_EVT in suspend noirq stage, making sure
+ *          that all pre-suspend firmware events are handled before
+ *          entering a low power state. Some buggy EC firmware may require
+ *          this, otherwise some unknown firmware issues can be seen on
+ *          such platforms:
+ *          Link: https://bugzilla.kernel.org/show_bug.cgi?id=196129
+ * RESUME:  Enable GPE poller for suspend/resume noirq stages.
+ *          This mode detects SCI_EVT in both suspend and resume noirq
+ *          stages, making sure that all post-resume firmware events are
+ *          handled as early as possible. This mode might be able to solve
+ *          some unknown driver timing issues.
+ */
+#define ACPI_EC_GPE_POLL_NONE		0x00
+#define ACPI_EC_GPE_POLL_SUSPEND	0x01
+#define ACPI_EC_GPE_POLL_RESUME		0x02
+
 /* EC commands */
 enum ec_command {
 	ACPI_EC_COMMAND_READ = 0x80,
@@ -102,6 +128,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_POLL_INTERVAL	500	/* Polling event every 500ms */
 
 enum {
 	EC_FLAGS_QUERY_ENABLED,		/* Query is enabled */
@@ -113,6 +140,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 */
@@ -136,6 +164,7 @@ 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_QUERY;
+static unsigned int ec_gpe_polling __read_mostly = ACPI_EC_GPE_POLL_NONE;
 
 /*
  * If the number of false interrupts per one transaction exceeds
@@ -150,6 +179,10 @@ static bool ec_freeze_events __read_mostly = false;
 module_param(ec_freeze_events, bool, 0644);
 MODULE_PARM_DESC(ec_freeze_events, "Disabling event handling during suspend/resume");
 
+static unsigned int ec_poll_interval __read_mostly = ACPI_EC_POLL_INTERVAL;
+module_param(ec_poll_interval, uint, 0644);
+MODULE_PARM_DESC(ec_poll_interval, "GPE polling interval(ms)");
+
 struct acpi_ec_query_handler {
 	struct list_head node;
 	acpi_ec_query_func func;
@@ -349,6 +382,44 @@ 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_poll_interval));
+}
+
+static void ec_start_gpe_poller(struct acpi_ec *ec)
+{
+	unsigned long flags;
+	bool start_tick = false;
+
+	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;
+
+	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)
@@ -937,6 +1008,8 @@ static void acpi_ec_start(struct acpi_ec *ec, bool resuming)
 		ec_log_drv("EC started");
 	}
 	spin_unlock_irqrestore(&ec->lock, flags);
+	if (resuming && ec_gpe_polling == ACPI_EC_GPE_POLL_RESUME)
+		ec_start_gpe_poller(ec);
 }
 
 static bool acpi_ec_stopped(struct acpi_ec *ec)
@@ -972,6 +1045,8 @@ static void acpi_ec_stop(struct acpi_ec *ec, bool suspending)
 		ec_log_drv("EC stopped");
 	}
 	spin_unlock_irqrestore(&ec->lock, flags);
+	if (suspending)
+		ec_stop_gpe_poller(ec);
 }
 
 static void acpi_ec_enter_noirq(struct acpi_ec *ec)
@@ -1257,6 +1332,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
  * -------------------------------------------------------------------------- */
@@ -1326,6 +1414,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;
@@ -1874,6 +1964,8 @@ static int acpi_ec_suspend(struct device *dev)
 	struct acpi_ec *ec =
 		acpi_driver_data(to_acpi_device(dev));
 
+	if (ec_gpe_polling != ACPI_EC_GPE_POLL_NONE)
+		ec_start_gpe_poller(ec);
 	if (acpi_sleep_no_ec_events() && ec_freeze_events)
 		acpi_ec_disable_event(ec);
 	return 0;
@@ -1885,6 +1977,7 @@ static int acpi_ec_resume(struct device *dev)
 		acpi_driver_data(to_acpi_device(dev));
 
 	acpi_ec_enable_event(ec);
+	ec_stop_gpe_poller(ec);
 	return 0;
 }
 #endif
@@ -1930,6 +2023,43 @@ module_param_call(ec_event_clearing, param_set_event_clearing, param_get_event_c
 		  NULL, 0644);
 MODULE_PARM_DESC(ec_event_clearing, "Assumed SCI_EVT clearing timing");
 
+static int param_set_gpe_polling(const char *val, struct kernel_param *kp)
+{
+	int result = 0;
+
+	if (!strncmp(val, "none", sizeof("none") - 1)) {
+		ec_gpe_polling = ACPI_EC_GPE_POLL_NONE;
+		pr_info("GPE noirq stage polling disabled\n");
+	} else if (!strncmp(val, "suspend", sizeof("suspend") - 1)) {
+		ec_gpe_polling = ACPI_EC_GPE_POLL_SUSPEND;
+		pr_info("GPE noirq suspend polling enabled\n");
+	} else if (!strncmp(val, "resume", sizeof("resume") - 1)) {
+		ec_gpe_polling = ACPI_EC_GPE_POLL_RESUME;
+		pr_info("GPE noirq suspend/resume polling enabled\n");
+	} else
+		result = -EINVAL;
+	return result;
+}
+
+static int param_get_gpe_polling(char *buffer, struct kernel_param *kp)
+{
+	switch (ec_gpe_polling) {
+	case ACPI_EC_GPE_POLL_NONE:
+		return sprintf(buffer, "none");
+	case ACPI_EC_GPE_POLL_SUSPEND:
+		return sprintf(buffer, "suspend");
+	case ACPI_EC_GPE_POLL_RESUME:
+		return sprintf(buffer, "resume");
+	default:
+		return sprintf(buffer, "invalid");
+	}
+	return 0;
+}
+
+module_param_call(ec_gpe_polling, param_set_gpe_polling, param_get_gpe_polling,
+		  NULL, 0644);
+MODULE_PARM_DESC(ec_gpe_polling, "Enabling GPE polling during noirq stages");
+
 static struct acpi_driver acpi_ec_driver = {
 	.name = "ec",
 	.class = ACPI_EC_CLASS,
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 9531d32..6bd36b1 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -170,6 +170,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

[PATCH v2 3/4] ACPI / EC: Add support to handle EC events earlier

[Lv Zheng]

Now as GPE poller is implemented, EC driver is able to detect EC events
during suspend/resume noirq stages, we can try to move EC event handling
earlier without being worried about post-resume event stuck.

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

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index f1cffd4..5f951ba 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1004,7 +1004,9 @@ 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 (!ec_freeze_events &&
+			   ec_gpe_polling == ACPI_EC_GPE_POLL_RESUME)
+			__acpi_ec_enable_event(ec);
 		ec_log_drv("EC started");
 	}
 	spin_unlock_irqrestore(&ec->lock, flags);
-- 
2.7.4

[PATCH v2 4/4] ACPI / EC: Enable noirq stage GPE polling

[Lv Zheng]

This patch enables noirq stage GPE polling for the EC driver.

EC is a very special driver, required to work throughout the entire
suspend/resume process. Thus this patch enables IRQ polling for EC during
noirq stages to avoid all kinds of possible issues.

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

Signed-off-by: Lv Zheng <lv.zheng@intel.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 5f951ba..f5d629b 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -164,7 +164,7 @@ 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_QUERY;
-static unsigned int ec_gpe_polling __read_mostly = ACPI_EC_GPE_POLL_NONE;
+static unsigned int ec_gpe_polling __read_mostly = ACPI_EC_GPE_POLL_RESUME;
 
 /*
  * If the number of false interrupts per one transaction exceeds
-- 
2.7.4

[PATCH v3 0/4] ACPI / EC: Poll more EC events during suspend/resume

[Lv Zheng]

EC events are special, required to be handled during suspend/resume. But
there is a problem preventing EC events from being detected during noirq
stages.
This patchset fixes this issue by polling EC IRQs timely during
suspend/resume noirq stages.
With this issue fixed, we should be able to handler EC events earlier
during resume, and have great opportunities to fix some driver order
issues caused by the deferred detected EC events.

v3 of this patch series only contains patch description updates.

Lv Zheng (4):
  ACPI / EC: Cleanup EC GPE mask flag
  ACPI / EC: Add IRQ polling support for noirq stages
  ACPI / EC: Add support to handle EC events earlier
  ACPI / EC: Enable noirq stage GPE polling

 drivers/acpi/ec.c       | 157 ++++++++++++++++++++++++++++++++++++++++++++----
 drivers/acpi/internal.h |   1 +
 2 files changed, 145 insertions(+), 13 deletions(-)

-- 
2.7.4

[PATCH v3 1/4] ACPI / EC: Cleanup EC GPE mask flag

[Lv Zheng]

EC_FLAGS_COMMAND_STORM is actually used to mask GPE during IRQ processing.
This patch cleans it up using more readable flag/function names.

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

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 62068a5..d338f40 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -112,8 +112,7 @@ enum {
 	EC_FLAGS_EVT_HANDLER_INSTALLED, /* _Qxx handlers installed */
 	EC_FLAGS_STARTED,		/* Driver is started */
 	EC_FLAGS_STOPPED,		/* Driver is stopped */
-	EC_FLAGS_COMMAND_STORM,		/* GPE storms occurred to the
-					 * current command processing */
+	EC_FLAGS_GPE_MASKED,		/* GPE masked */
 };
 
 #define ACPI_EC_COMMAND_POLL		0x01 /* Available for command byte */
@@ -425,19 +424,19 @@ static void acpi_ec_complete_request(struct acpi_ec *ec)
 		wake_up(&ec->wait);
 }
 
-static void acpi_ec_set_storm(struct acpi_ec *ec, u8 flag)
+static void acpi_ec_mask_gpe(struct acpi_ec *ec)
 {
-	if (!test_bit(flag, &ec->flags)) {
+	if (!test_bit(EC_FLAGS_GPE_MASKED, &ec->flags)) {
 		acpi_ec_disable_gpe(ec, false);
 		ec_dbg_drv("Polling enabled");
-		set_bit(flag, &ec->flags);
+		set_bit(EC_FLAGS_GPE_MASKED, &ec->flags);
 	}
 }
 
-static void acpi_ec_clear_storm(struct acpi_ec *ec, u8 flag)
+static void acpi_ec_unmask_gpe(struct acpi_ec *ec)
 {
-	if (test_bit(flag, &ec->flags)) {
-		clear_bit(flag, &ec->flags);
+	if (test_bit(EC_FLAGS_GPE_MASKED, &ec->flags)) {
+		clear_bit(EC_FLAGS_GPE_MASKED, &ec->flags);
 		acpi_ec_enable_gpe(ec, false);
 		ec_dbg_drv("Polling disabled");
 	}
@@ -464,7 +463,7 @@ static bool acpi_ec_submit_flushable_request(struct acpi_ec *ec)
 
 static void acpi_ec_submit_query(struct acpi_ec *ec)
 {
-	acpi_ec_set_storm(ec, EC_FLAGS_COMMAND_STORM);
+	acpi_ec_mask_gpe(ec);
 	if (!acpi_ec_event_enabled(ec))
 		return;
 	if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
@@ -480,7 +479,7 @@ static void acpi_ec_complete_query(struct acpi_ec *ec)
 	if (test_and_clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags))
 		ec_dbg_evt("Command(%s) unblocked",
 			   acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
-	acpi_ec_clear_storm(ec, EC_FLAGS_COMMAND_STORM);
+	acpi_ec_unmask_gpe(ec);
 }
 
 static inline void __acpi_ec_enable_event(struct acpi_ec *ec)
@@ -700,7 +699,7 @@ static void advance_transaction(struct acpi_ec *ec)
 				++t->irq_count;
 			/* Allow triggering on 0 threshold */
 			if (t->irq_count == ec_storm_threshold)
-				acpi_ec_set_storm(ec, EC_FLAGS_COMMAND_STORM);
+				acpi_ec_mask_gpe(ec);
 		}
 	}
 out:
@@ -798,7 +797,7 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
 
 	spin_lock_irqsave(&ec->lock, tmp);
 	if (t->irq_count == ec_storm_threshold)
-		acpi_ec_clear_storm(ec, EC_FLAGS_COMMAND_STORM);
+		acpi_ec_unmask_gpe(ec);
 	ec_dbg_req("Command(%s) stopped", acpi_ec_cmd_string(t->command));
 	ec->curr = NULL;
 	/* Disable GPE for command processing (IBF=0/OBF=1) */
-- 
2.7.4

Re: [PATCH v3 1/4] ACPI / EC: Cleanup EC GPE mask flag

[Rafael J. Wysocki]

On Friday, August 11, 2017 8:36:28 AM CEST Lv Zheng wrote:
> EC_FLAGS_COMMAND_STORM is actually used to mask GPE during IRQ processing.
> This patch cleans it up using more readable flag/function names.
> 
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> Tested-by: Tomislav Ivek <tomislav.ivek@gmail.com>

Applied, thanks!

I'm not sure about the rest of the series, though, but let me comment on the
specific patches.

Thanks,
Rafael

RE: [PATCH v3 1/4] ACPI / EC: Cleanup EC GPE mask flag

[Zheng, Lv]

Hi,

> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Subject: Re: [PATCH v3 1/4] ACPI / EC: Cleanup EC GPE mask flag
> 
> On Friday, August 11, 2017 8:36:28 AM CEST Lv Zheng wrote:
> > EC_FLAGS_COMMAND_STORM is actually used to mask GPE during IRQ processing.
> > This patch cleans it up using more readable flag/function names.
> >
> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> > Tested-by: Tomislav Ivek <tomislav.ivek@gmail.com>
> 
> Applied, thanks!

Thanks!

> 
> I'm not sure about the rest of the series, though, but let me comment on the
> specific patches.

Though it is a long bug fix story, it's actually simple and can be summarized with 1 line:
The patchset polls EC IRQs during noirq stage to fix EC event handling issues.

I've submitted this solution several years ago using an IRQ polling kernel thread.
You commented me to use a timer instead, here you are.

During these years, I tried different solutions than the "polling IRQ in noirq stage".
But they didn't work perfectly.
The noirq stage makes the EC event handling issue fixes mutual exclusive.
Fixing one issue can trigger regression for the other.
And bug reports prove that we must handle EC events during noirq stage.

So finally I picked the IRQ polling solution back, and refreshed it to follow your comment (using a timer instead of a kthread).

If you apply this patch and enable EC debugging log with "dyndbg='file ec.c +p'".
You should be able to see some event handling logs in noirq stages.
Without this solution applied, EC event handling log is silent during noirq stages.
That could be the only difference.

Thanks and best regards
Lv

[PATCH v3 2/4] ACPI / EC: Add IRQ polling support for noirq stages

[Lv Zheng]

1. Problems:
1.1. Problem 1: Cannot detect EC event in noirq stages.
EC IRQs contain transaction IRQs (OBF/IBF) and event IRQ (SCI_EVT).

Transactions are initiated by hosts. The earliest OSPMs execution of EC
transactions is from acpi_ec_transaction(), where the common EC IRQ
handling procedure - advance_transaction() - is initiated from the task
context.
Events are initiated by targets. The earliest OSPMs execution of EC events
is from acpi_ec_gpe_handler(), where the common EC IRQ handling procedure -
advance_transaction() - is initiated from the IRQ context.

During suspend/resume noirq stage, IRQ is disabled, advance_transaction()
which detects SCI_EVT can only be invoked from task context - ec_poll().
Thus if there is no EC transaction occurring in this stage, EC driver
cannot detect SCI_EVT.

Note that in noirq stage if there is an EC transaction pending,
advance_transaction() invoked because of the EC transaction can also help
to detect SCI_EVT and initiate the handling of the EC events. That's why
real reports related to this problem are rare and unreproducible as whether
there is an EC transaction occurred after an undetectable EC events during
noirq stages is random.

1.2. Problem 2: Handling of EC events may stuck for some GPE chips.
When IRQ already occurred in the underlying hardware and flagged the IRQ
status bit, enabling IRQ by flagging the IRQ enable bit may not be
sufficient to trigger edge-triggered IRQs on some IRQ silicons (let me
call these IRQ silicons as buggy to make description simpler).

Then, when EC GPE events already occurred in resume noirq stage leaving GPE
STS bit set, EC GPE may not be delivered to the CPU when EC driver
re-enables EC GPE by flagging GPE EN bit. And even worse, after this
enabling, if there is no EC transaction occurred, EC events can never be
detected again, and handling of EC events "stucks".

Note that if the upper layer IRQ silicons (APIC and etc.) and GPE IRQ
silicons are not buggy, we needn't worry about this problem. That's why the
real reports related to this problem are rare and always platform specific.

2. Old assumption and solution:
In order to solve the above issues, "ec_freeze_events" is implemented to:
A. Stop handling SCI_EVT before suspend noirq stage:
   As EC driver actually has never been fully able to handle EC events
   during noirq stages, we assumed that detecting such events in noirq
   stage is useless and thus we needn't worry about problem 1.
B. Re-start handling SCI_EVT after resume noirq stage:
   By always setting EN after resume noirq stage, it is ensured that:
   a. whatever upper layer IRQ silicons (APIC and etc.) are buggy or not,
      they won't cause problem 2, and
   b. non buggy GPE IRQ silicons won't cause problem 2, but buggy GPE IRQ
      silicons may still cause problem 2.
By narrowing down the EC event handling period during suspend/resume, we
can see that the occurring ratio of problem 2 can be reduced, only buggy
GPE IRQ silicons can still trigger problem 2.

Finally, the EC event handling is namely "frozen" for a specific period
during suspend/resume and "ec_freeze_events" controls the timing of the
"begin/end of the freeze". If freezing event handling earlier could work,
we shouldn't be required to implement GPE polling in noirq stages.

Note that this solution cannot fully solve problem 2 and solves problem 1
with assumptions.

3. New facts and solution:
Recently, some bug reports (see Link #1) revealed that we shouldn't
"freeze" EC event handling too early during these system suspend/resume
procedures.
Also enabling EC event handling too late surely changes the event
triggering timing, and may leave driver order issues during resume.

Based on these facts, we should detect SCI_EVT during noirq stage, this
left us no other choice than implementing IRQ polling for SCI_EVT in noirq
stages.

This patch adds a timer to poll EC events timely (0.5s) during system
suspend/resume noirq stages. To be regression safer, this patch also
prepares an option to make this behavior configurable.

Note that this solution can perfectly solve problem 1 and problem 2.

This patch has been validated to be able to improve situation related to
the reported bug (see Link #1) which requires to handle EC GPEs longer
during suspend.

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       | 130 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/internal.h |   1 +
 2 files changed, 131 insertions(+)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index d338f40..5be62933 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"
@@ -87,6 +88,31 @@
 #define ACPI_EC_EVT_TIMING_QUERY	0x01
 #define ACPI_EC_EVT_TIMING_EVENT	0x02
 
+/*
+ * There is a noirq stage during suspend/resume and EC firmware may
+ * require OS to handle EC events (SCI_EVT) during this stage.
+ * If there is no EC transactions during this stage, SCI_EVT cannot be
+ * detected. In order to still detect SCI_EVT, IRQ must be polled by the
+ * EC GPE poller. There are 3 configurable modes implemented for the EC
+ * GPE poller:
+ * NONE:    Do not enable noirq stage GPE poller.
+ * SUSPEND: Enable GPE poller for suspend noirq stage.
+ *          This mode detects SCI_EVT in suspend noirq stage, making sure
+ *          that all pre-suspend firmware events are handled before
+ *          entering a low power state. Some buggy EC firmware may require
+ *          this, otherwise some unknown firmware issues can be seen on
+ *          such platforms:
+ *          Link: https://bugzilla.kernel.org/show_bug.cgi?id=196129
+ * RESUME:  Enable GPE poller for suspend/resume noirq stages.
+ *          This mode detects SCI_EVT in both suspend and resume noirq
+ *          stages, making sure that all post-resume firmware events are
+ *          handled as early as possible. This mode might be able to solve
+ *          some unknown driver timing issues.
+ */
+#define ACPI_EC_GPE_POLL_NONE		0x00
+#define ACPI_EC_GPE_POLL_SUSPEND	0x01
+#define ACPI_EC_GPE_POLL_RESUME		0x02
+
 /* EC commands */
 enum ec_command {
 	ACPI_EC_COMMAND_READ = 0x80,
@@ -102,6 +128,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_POLL_INTERVAL	500	/* Polling event every 500ms */
 
 enum {
 	EC_FLAGS_QUERY_ENABLED,		/* Query is enabled */
@@ -113,6 +140,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 */
@@ -136,6 +164,7 @@ 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_QUERY;
+static unsigned int ec_gpe_polling __read_mostly = ACPI_EC_GPE_POLL_NONE;
 
 /*
  * If the number of false interrupts per one transaction exceeds
@@ -154,6 +183,10 @@ 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 unsigned int ec_poll_interval __read_mostly = ACPI_EC_POLL_INTERVAL;
+module_param(ec_poll_interval, uint, 0644);
+MODULE_PARM_DESC(ec_poll_interval, "GPE polling interval(ms)");
+
 struct acpi_ec_query_handler {
 	struct list_head node;
 	acpi_ec_query_func func;
@@ -353,6 +386,44 @@ 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_poll_interval));
+}
+
+static void ec_start_gpe_poller(struct acpi_ec *ec)
+{
+	unsigned long flags;
+	bool start_tick = false;
+
+	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;
+
+	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)
@@ -949,6 +1020,8 @@ static void acpi_ec_start(struct acpi_ec *ec, bool resuming)
 		ec_log_drv("EC started");
 	}
 	spin_unlock_irqrestore(&ec->lock, flags);
+	if (resuming && ec_gpe_polling == ACPI_EC_GPE_POLL_RESUME)
+		ec_start_gpe_poller(ec);
 }
 
 static bool acpi_ec_stopped(struct acpi_ec *ec)
@@ -984,6 +1057,8 @@ static void acpi_ec_stop(struct acpi_ec *ec, bool suspending)
 		ec_log_drv("EC stopped");
 	}
 	spin_unlock_irqrestore(&ec->lock, flags);
+	if (suspending)
+		ec_stop_gpe_poller(ec);
 }
 
 static void acpi_ec_enter_noirq(struct acpi_ec *ec)
@@ -1269,6 +1344,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
  * -------------------------------------------------------------------------- */
@@ -1338,6 +1426,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;
@@ -1886,6 +1976,8 @@ static int acpi_ec_suspend(struct device *dev)
 	struct acpi_ec *ec =
 		acpi_driver_data(to_acpi_device(dev));
 
+	if (ec_gpe_polling != ACPI_EC_GPE_POLL_NONE)
+		ec_start_gpe_poller(ec);
 	if (acpi_sleep_no_ec_events() && ec_freeze_events)
 		acpi_ec_disable_event(ec);
 	return 0;
@@ -1923,6 +2015,7 @@ static int acpi_ec_resume(struct device *dev)
 		acpi_driver_data(to_acpi_device(dev));
 
 	acpi_ec_enable_event(ec);
+	ec_stop_gpe_poller(ec);
 	return 0;
 }
 #endif
@@ -1969,6 +2062,43 @@ module_param_call(ec_event_clearing, param_set_event_clearing, param_get_event_c
 		  NULL, 0644);
 MODULE_PARM_DESC(ec_event_clearing, "Assumed SCI_EVT clearing timing");
 
+static int param_set_gpe_polling(const char *val, struct kernel_param *kp)
+{
+	int result = 0;
+
+	if (!strncmp(val, "none", sizeof("none") - 1)) {
+		ec_gpe_polling = ACPI_EC_GPE_POLL_NONE;
+		pr_info("GPE noirq stage polling disabled\n");
+	} else if (!strncmp(val, "suspend", sizeof("suspend") - 1)) {
+		ec_gpe_polling = ACPI_EC_GPE_POLL_SUSPEND;
+		pr_info("GPE noirq suspend polling enabled\n");
+	} else if (!strncmp(val, "resume", sizeof("resume") - 1)) {
+		ec_gpe_polling = ACPI_EC_GPE_POLL_RESUME;
+		pr_info("GPE noirq suspend/resume polling enabled\n");
+	} else
+		result = -EINVAL;
+	return result;
+}
+
+static int param_get_gpe_polling(char *buffer, struct kernel_param *kp)
+{
+	switch (ec_gpe_polling) {
+	case ACPI_EC_GPE_POLL_NONE:
+		return sprintf(buffer, "none");
+	case ACPI_EC_GPE_POLL_SUSPEND:
+		return sprintf(buffer, "suspend");
+	case ACPI_EC_GPE_POLL_RESUME:
+		return sprintf(buffer, "resume");
+	default:
+		return sprintf(buffer, "invalid");
+	}
+	return 0;
+}
+
+module_param_call(ec_gpe_polling, param_set_gpe_polling, param_get_gpe_polling,
+		  NULL, 0644);
+MODULE_PARM_DESC(ec_gpe_polling, "Enabling GPE polling during noirq stages");
+
 static struct acpi_driver acpi_ec_driver = {
 	.name = "ec",
 	.class = ACPI_EC_CLASS,
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 58dd7ab..705d103 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -170,6 +170,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

[PATCH v3 3/4] ACPI / EC: Add support to handle EC events earlier

[Lv Zheng]

Now as GPE poller is implemented, EC driver is able to detect EC events
during suspend/resume noirq stages, we can try to move EC event handling
earlier without being worried about post-resume event stuck. This may help
to solve driver order issues during resume.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Tested-by: Tomislav Ivek <tomislav.ivek@gmail.com>
---
 drivers/acpi/ec.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 5be62933..396e81d 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1016,7 +1016,9 @@ 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 (!ec_freeze_events &&
+			   ec_gpe_polling == ACPI_EC_GPE_POLL_RESUME)
+			__acpi_ec_enable_event(ec);
 		ec_log_drv("EC started");
 	}
 	spin_unlock_irqrestore(&ec->lock, flags);
-- 
2.7.4

[PATCH v3 4/4] ACPI / EC: Enable noirq stage GPE polling

[Lv Zheng]

This patch enables noirq stage GPE polling for the EC driver.

EC is a very special driver, required to work throughout the entire
suspend/resume process. Thus this patch enables IRQ polling for EC during
noirq stages to avoid all kinds of possible issues.

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 396e81d..8bba317 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -164,7 +164,7 @@ 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_QUERY;
-static unsigned int ec_gpe_polling __read_mostly = ACPI_EC_GPE_POLL_NONE;
+static unsigned int ec_gpe_polling __read_mostly = ACPI_EC_GPE_POLL_RESUME;
 
 /*
  * If the number of false interrupts per one transaction exceeds
-- 
2.7.4

[PATCH v4 0/3] ACPI / EC: Fix EC event handling issues

[Lv Zheng]

EC events are special, required to be handled during suspend/resume. But
there are special logics in Linux causing several issues related to the EC
event handling:
1. During noirq stage, Linux cannot detect EC events which are target
   driven.
2. When EC event handling is enabled later than the other drivers, order
   problem could be observed.
When fixing these problems, care should be taken to not to trigger
regressions to the following problem which has alredy been fixed using
different approach):
3. When EC event handling is enabled before the end of the noirq stage,
   EC event handling may stuck.
This patchset fixes these issues.

v4 of this patch series re-orders the fixes so that the fix of the problem
2 could be independent against the fix of the problem 1, this is done by
refining the fix of the problem 2, making it immune to the problem 3.

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       | 116 +++++++++++++++++++++++++++++++++++++++++++++---
 drivers/acpi/internal.h |   1 +
 2 files changed, 111 insertions(+), 6 deletions(-)

-- 
2.7.4

[PATCH v4 1/3] ACPI / EC: Fix possible driver order issue by moving EC event handling earlier

[Lv Zheng]

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, finally it could cause EC event handling
stuck. Thus this patch also adds a detection in acpi_ec_resume(), trying to
recover from the affection of the noirq stage.

The background of the affection:
  EC IRQs contain transaction IRQs (OBF/IBF) and event IRQ (SCI_EVT).
  1. Transactions are initiated by hosts. The earliest OSPMs execution of
     EC transactions is from acpi_ec_transaction(), where the common EC IRQ
     handling procedure - advance_transaction() - is initiated from the
     task context.
  2. Events are initiated by targets. The earliest OSPMs execution of EC
     events is from acpi_ec_gpe_handler(), where the common EC IRQ handling
     procedure - advance_transaction() - is initiated from the IRQ context.
  During suspend/resume noirq stage, IRQ is disabled, advance_transaction()
  which detects SCI_EVT can only be invoked from task context - ec_poll().
  Thus if there is no EC transaction occurring in this stage, EC driver
  cannot detect SCI_EVT. And the worst case is if there is no EC
  transaction after resume, EC event handling will stuck (FW flags SCI_EVT,
  but there is no triggering source for OS SW to detect it).

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>
---
 drivers/acpi/ec.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index fdfae6f..3dc4205 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -486,8 +486,11 @@ static inline void __acpi_ec_enable_event(struct acpi_ec *ec)
 {
 	if (!test_and_set_bit(EC_FLAGS_QUERY_ENABLED, &ec->flags))
 		ec_log_drv("event unblocked");
-	if (!test_bit(EC_FLAGS_QUERY_PENDING, &ec->flags))
-		advance_transaction(ec);
+	/*
+	 * Unconditionally invoke this once after enabling the event
+	 * handling mechanism to detect the pending events.
+	 */
+	advance_transaction(ec);
 }
 
 static inline void __acpi_ec_disable_event(struct acpi_ec *ec)
@@ -945,7 +948,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 (!ec_freeze_events)
+			__acpi_ec_enable_event(ec);
 		ec_log_drv("EC started");
 	}
 	spin_unlock_irqrestore(&ec->lock, flags);
@@ -1929,7 +1933,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 (ec_freeze_events)
+		acpi_ec_enable_event(ec);
+	else {
+		/*
+		 * Though whether there is an event pending has been
+		 * checked in acpi_ec_unblock_transactions() when
+		 * ec_freeze_events=N, 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

[PATCH v4 2/3] ACPI / EC: Add event detection support for noirq stages

[Lv Zheng]

1. Problems: Cannot detect EC event in noirq stages.
EC IRQs contain transaction IRQs (OBF/IBF) and event IRQ (SCI_EVT).
Transactions are initiated by hosts. The earliest OSPMs execution of EC
transactions is from acpi_ec_transaction(), where the common EC IRQ
handling procedure - advance_transaction() - is initiated from the task
context.
Events are initiated by targets. The earliest OSPMs execution of EC events
is from acpi_ec_gpe_handler(), where the common EC IRQ handling procedure -
advance_transaction() - is initiated from the IRQ context.
During suspend/resume noirq stage, IRQ is disabled, advance_transaction()
which detects SCI_EVT can only be invoked from task context - ec_poll().
Thus if there is no EC transaction occurring in this stage, EC driver
cannot detect SCI_EVT. And the worst case is if there is no EC transaction
after resume, EC event handling will stuck (FW flags SCI_EVT, but there is
no triggering source for OS to detect it).
Note that in noirq stage if there is an EC transaction pending,
advance_transaction() invoked because of the EC transaction can also help
to detect SCI_EVT and initiate the handling of the EC events. That's why
real reports related to this problem are rare and unreproducible as whether
there is an EC transaction occurred after an undetectable EC events during
noirq stages is random.

2. Old assumption and solution:
In order to solve the above issue, "ec_freeze_events" is implemented to:
stop handling SCI_EVT before suspend noirq stage and restart handling
SCI_EVT after resume noirq stage.
We assumed that detecting SCI_EVT in noirq stage is useless because there
are other conflict issues related to the EC event handling actually making
it not fully functioning during suspend/resume.
This assumption and the solution efficiently solves all issues.
Finally, the EC event handling is namely "frozen" for a specific period
during suspend/resume and "ec_freeze_events" controls the timing of the
"begin/end of the freeze". If freezing event handling earlier could work,
we shouldn't be required to implement event detection in noirq stages.

3. New facts and new problems:
Recently, some bug reports (see Link #1) revealed that we shouldn't
"freeze" EC event handling too early during these system suspend/resume
procedures.
Also enabling EC event handling too late surely changes the event
triggering timing, and may leave driver order issues during resume.
The previous commit in the same series resumes EC event handling earlier in
acpi_ec_unblock_transactions(), but that still leaves another problem to us
that we still cannot detect SCI_EVT occurred after
acpi_ec_unblock_transactions() and before acpi_ec_resume(). In order to
solve the final gap, we need to implement event detection in noirq stages.

4. New solution:
This patch adds a timer to poll EC events timely (0.5s) during system
suspend/resume noirq stages.
This patch has been validated to be able to improve situation related to
the reported bug (see Link #1) which requires to handle EC GPEs longer
during suspend.
With this patch applied, ACPI sleep core may still need to tune the order
of sleep steps by tuning the timing of invoking
acpi_ec_block/unblock_transactions() to fully solve the reported issue.
Actually ec_detect_noirq_events should always be true when ec_freeze_events
is false. But since there could be no noirq stage affection, this patch
introduces a separate runtime configurable option for it so that we can
disable it when there is no affection of the noirq stage.

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 3dc4205..9363656 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;
@@ -353,6 +365,44 @@ 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;
+
+	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;
+
+	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)
@@ -1012,6 +1062,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;
@@ -1023,16 +1079,30 @@ void acpi_ec_block_transactions(void)
 	/* Prevent transactions from being carried out */
 	acpi_ec_stop(ec, true);
 	mutex_unlock(&ec->mutex);
+	if (ec_detect_noirq_events)
+		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;
+
+	if (ec_detect_noirq_events)
+		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);
 }
 
 /* --------------------------------------------------------------------------
@@ -1273,6 +1343,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
  * -------------------------------------------------------------------------- */
@@ -1342,6 +1425,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;
@@ -1897,6 +1982,8 @@ static int acpi_ec_suspend(struct device *dev)
 	struct acpi_ec *ec =
 		acpi_driver_data(to_acpi_device(dev));
 
+	if (ec_detect_noirq_events)
+		ec_start_gpe_poller(ec);
 	if (acpi_sleep_no_ec_events() && ec_freeze_events)
 		acpi_ec_disable_event(ec);
 	return 0;
@@ -1945,6 +2032,8 @@ static int acpi_ec_resume(struct device *dev)
 		 */
 		advance_transaction(ec);
 	}
+	if (ec_detect_noirq_events)
+		ec_stop_gpe_poller(ec);
 	return 0;
 }
 #endif
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 4361c44..a76e411 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -170,6 +170,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

[PATCH v4 3/3] ACPI / EC: Enable noirq stage event detection

[Lv Zheng]

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 9363656..36ce5e7 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

RE: [PATCH v4 0/3] ACPI / EC: Fix EC event handling issues

[Zheng, Lv]

Hi, Rafael

I'm now working on v5 of this series.
Which
1. splits root causes in a more detailed way,
2. clarifies root causes in patch description with real bugs, and
3. is safer according to the known EC FW behaviors.
So you can discard v3/v4 from the patchwork site.
And I'll post v5 when everything is cleared to me.

Thanks and best regards
Lv

> From: Zheng, Lv
> Subject: [PATCH v4 0/3] ACPI / EC: Fix EC event handling issues
> 
> EC events are special, required to be handled during suspend/resume. But
> there are special logics in Linux causing several issues related to the EC
> event handling:
> 1. During noirq stage, Linux cannot detect EC events which are target
>    driven.
> 2. When EC event handling is enabled later than the other drivers, order
>    problem could be observed.
> When fixing these problems, care should be taken to not to trigger
> regressions to the following problem which has alredy been fixed using
> different approach):
> 3. When EC event handling is enabled before the end of the noirq stage,
>    EC event handling may stuck.
> This patchset fixes these issues.
> 
> v4 of this patch series re-orders the fixes so that the fix of the problem
> 2 could be independent against the fix of the problem 1, this is done by
> refining the fix of the problem 2, making it immune to the problem 3.
> 
> 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       | 116 +++++++++++++++++++++++++++++++++++++++++++++---
>  drivers/acpi/internal.h |   1 +
>  2 files changed, 111 insertions(+), 6 deletions(-)
> 
> --
> 2.7.4

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

[Lv Zheng]

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

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

[Lv Zheng]

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

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

[Zhang, Rui]

> -----Original Message-----
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> owner@vger.kernel.org] On Behalf Of Lv Zheng
> Sent: Friday, September 29, 2017 10:50 AM
> To: Wysocki, Rafael J <rafael.j.wysocki@intel.com>; Rafael J . Wysocki
> <rjw@rjwysocki.net>; Brown, Len <len.brown@intel.com>
> Cc: Zheng, Lv <lv.zheng@intel.com>; Lv Zheng <zetalog@gmail.com>; linux-
> kernel@vger.kernel.org; linux-acpi@vger.kernel.org
> Subject: [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>

I don't know what issue this patch has been tested for. Lv, can you please clarify?

I agree with lv that it can probably fix some issues brought by the device order issue.
And I'll be glad to push this after we have verified it is really helpful.
Lv,
Do you still remember the bug report for the lid issue?

Thanks,
rui
> ---
>  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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the
> body of a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

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

[Zheng, Lv]

Hi, Rui

> From: Zhang, Rui
> Subject: RE: [RFC PATCH v6 1/3] ACPI / EC: Fix possible driver order issue by moving EC event handling
> earlier
> 
> 
> > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> > Subject: [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>
> 
> I don't know what issue this patch has been tested for. Lv, can you please clarify?

The testers' names are listed here because they have tried the commit and no
regressions can be found on their test platforms.

> 
> I agree with lv that it can probably fix some issues brought by the device order issue.
> And I'll be glad to push this after we have verified it is really helpful.
> Lv,
> Do you still remember the bug report for the lid issue?

Maybe you should ask Benjamin.
Let me Cc him for further investigation.

Thanks,
Lv

> 
> Thanks,
> rui
> > ---
> >  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
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the
> > body of a message to majordomo@vger.kernel.org More majordomo info at
> > http://vger.kernel.org/majordomo-info.html

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

[Benjamin Tissoires]

On Nov 24 2017 or thereabouts, Zheng, Lv wrote:
> Hi, Rui
> 
> > From: Zhang, Rui
> > Subject: RE: [RFC PATCH v6 1/3] ACPI / EC: Fix possible driver order issue by moving EC event handling
> > earlier
> > 
> > 
> > > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> > > Subject: [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>
> > 
> > I don't know what issue this patch has been tested for. Lv, can you please clarify?
> 
> The testers' names are listed here because they have tried the commit and no
> regressions can be found on their test platforms.
> 
> > 
> > I agree with lv that it can probably fix some issues brought by the device order issue.
> > And I'll be glad to push this after we have verified it is really helpful.
> > Lv,
> > Do you still remember the bug report for the lid issue?
> 
> Maybe you should ask Benjamin.
> Let me Cc him for further investigation.

AFAICT, the lid issue was a complete mess. This patch seems to give back
some hope. One situation that was happening was that the LID
notification was coming before the system was ready to handle (this is
all from memory) and that meant that the state of the system was wrong
(it thought the lid was closed while it was not). Any patch that would
guarantee the ordering between the module related to this issue would be
more than welcome.

Cheers,
Benjamin

> 
> Thanks,
> Lv
> 
> > 
> > Thanks,
> > rui
> > > ---
> > >  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
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the
> > > body of a message to majordomo@vger.kernel.org More majordomo info at
> > > http://vger.kernel.org/majordomo-info.html

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

[Lv Zheng]

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

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

[Zhang, Rui]

> -----Original Message-----
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> owner@vger.kernel.org] On Behalf Of Lv Zheng
> Sent: Friday, September 29, 2017 10:50 AM
> To: Wysocki, Rafael J <rafael.j.wysocki@intel.com>; Rafael J . Wysocki
> <rjw@rjwysocki.net>; Brown, Len <len.brown@intel.com>
> Cc: Zheng, Lv <lv.zheng@intel.com>; Lv Zheng <zetalog@gmail.com>; linux-
> kernel@vger.kernel.org; linux-acpi@vger.kernel.org
> Subject: [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>

No, the patches to fix the Fan spin issue (https://bugzilla.kernel.org/show_bug.cgi?id=196129) have already been applied by Rafael.
This patch, together with patch 3/3 don't fix anything.
I'm not saying the patch is wrong, but they should be verified to fix a real issue before submitting for upstream.

Thanks,
rui
> ---
>  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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the
> body of a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

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

[Lv Zheng]

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