[PATCH 1/2] ACPI / EC: Add PM operations to tune polling mode efficiency

[PATCH 1/2] ACPI / EC: Add PM operations to tune polling mode efficiency

[Lv Zheng]

When the PM code disables all IRQs, EC will be in polling mode, it is
efficient to use busy polling with zero timeout in such cases.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c       |   53 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/internal.h |    2 ++
 2 files changed, 55 insertions(+)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 999a109..c2d135d 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1613,6 +1613,58 @@ error:
 	return ret;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static void acpi_ec_enter_noirq(struct acpi_ec *ec)
+{
+	unsigned long flags;
+
+	if (ec == first_ec) {
+		spin_lock_irqsave(&ec->lock, flags);
+		ec->saved_busy_polling = ec_busy_polling;
+		ec->saved_polling_guard = ec_polling_guard;
+		ec_busy_polling = true;
+		ec_polling_guard = 0;
+		ec_log_drv("interrupt blocked");
+		spin_unlock_irqrestore(&ec->lock, flags);
+	}
+}
+
+static void acpi_ec_leave_noirq(struct acpi_ec *ec)
+{
+	unsigned long flags;
+
+	if (ec == first_ec) {
+		spin_lock_irqsave(&ec->lock, flags);
+		ec_busy_polling = ec->saved_busy_polling;
+		ec_polling_guard = ec->saved_polling_guard;
+		ec_log_drv("interrupt unblocked");
+		spin_unlock_irqrestore(&ec->lock, flags);
+	}
+}
+
+static int acpi_ec_suspend_noirq(struct device *dev)
+{
+	struct acpi_ec *ec =
+		acpi_driver_data(to_acpi_device(dev));
+
+	acpi_ec_enter_noirq(ec);
+	return 0;
+}
+
+static int acpi_ec_resume_noirq(struct device *dev)
+{
+	struct acpi_ec *ec =
+		acpi_driver_data(to_acpi_device(dev));
+
+	acpi_ec_leave_noirq(ec);
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops acpi_ec_pm = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(acpi_ec_suspend_noirq, acpi_ec_resume_noirq)
+};
+
 static int param_set_event_clearing(const char *val, struct kernel_param *kp)
 {
 	int result = 0;
@@ -1658,6 +1710,7 @@ static struct acpi_driver acpi_ec_driver = {
 		.add = acpi_ec_add,
 		.remove = acpi_ec_remove,
 		},
+	.drv.pm = &acpi_ec_pm,
 };
 
 int __init acpi_ec_init(void)
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 940218f..6996121 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -174,6 +174,8 @@ struct acpi_ec {
 	struct work_struct work;
 	unsigned long timestamp;
 	unsigned long nr_pending_queries;
+	bool saved_busy_polling;
+	unsigned int saved_polling_guard;
 };
 
 extern struct acpi_ec *first_ec;
-- 
1.7.10

[PATCH 1/2] ACPI / EC: Add PM operations to tune polling mode efficiency

[Lv Zheng]

When the PM code disables all IRQs, EC will be in polling mode, it is
efficient to use busy polling with zero timeout in such cases.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c       |   53 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/internal.h |    2 ++
 2 files changed, 55 insertions(+)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 999a109..c2d135d 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1613,6 +1613,58 @@ error:
 	return ret;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static void acpi_ec_enter_noirq(struct acpi_ec *ec)
+{
+	unsigned long flags;
+
+	if (ec == first_ec) {
+		spin_lock_irqsave(&ec->lock, flags);
+		ec->saved_busy_polling = ec_busy_polling;
+		ec->saved_polling_guard = ec_polling_guard;
+		ec_busy_polling = true;
+		ec_polling_guard = 0;
+		ec_log_drv("interrupt blocked");
+		spin_unlock_irqrestore(&ec->lock, flags);
+	}
+}
+
+static void acpi_ec_leave_noirq(struct acpi_ec *ec)
+{
+	unsigned long flags;
+
+	if (ec == first_ec) {
+		spin_lock_irqsave(&ec->lock, flags);
+		ec_busy_polling = ec->saved_busy_polling;
+		ec_polling_guard = ec->saved_polling_guard;
+		ec_log_drv("interrupt unblocked");
+		spin_unlock_irqrestore(&ec->lock, flags);
+	}
+}
+
+static int acpi_ec_suspend_noirq(struct device *dev)
+{
+	struct acpi_ec *ec =
+		acpi_driver_data(to_acpi_device(dev));
+
+	acpi_ec_enter_noirq(ec);
+	return 0;
+}
+
+static int acpi_ec_resume_noirq(struct device *dev)
+{
+	struct acpi_ec *ec =
+		acpi_driver_data(to_acpi_device(dev));
+
+	acpi_ec_leave_noirq(ec);
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops acpi_ec_pm = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(acpi_ec_suspend_noirq, acpi_ec_resume_noirq)
+};
+
 static int param_set_event_clearing(const char *val, struct kernel_param *kp)
 {
 	int result = 0;
@@ -1658,6 +1710,7 @@ static struct acpi_driver acpi_ec_driver = {
 		.add = acpi_ec_add,
 		.remove = acpi_ec_remove,
 		},
+	.drv.pm = &acpi_ec_pm,
 };
 
 int __init acpi_ec_init(void)
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 940218f..6996121 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -174,6 +174,8 @@ struct acpi_ec {
 	struct work_struct work;
 	unsigned long timestamp;
 	unsigned long nr_pending_queries;
+	bool saved_busy_polling;
+	unsigned int saved_polling_guard;
 };
 
 extern struct acpi_ec *first_ec;
-- 
1.7.10

[PATCH 2/2] ACPI / EC: Add PM operations to block event handling

[Lv Zheng]

During the early stage for boot/resume, EC shouldn't handle _Qxx as EC
shouldn't expect the AML interpreter to be fully running and other drivers
are ready to handle the notifications issued from _Qxx.

Originally, EC driver stops handling both events and transactions in
acpi_ec_block_transactions(), and restarts to handle transactions in
acpi_ec_unblock_transactions_early(), restarts to handle both events and
transactions in acpi_ec_unblock_transactions().

However "stop handling events" mean the EC driver notices that the system
is about to suspend, "start handling events" means the EC driver notices
that the system is about to resume. This can be implemented via PM ops.
With "event handling switch" implemented via suspend/resume ops, the
following 2 APIs serve for the same purpose and can be combined:
acpi_ec_unblock_transactions_early()/acpi_ec_unblock_transactions().

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c       |   63 ++++++++++++++++++++++++++++++++++-------------
 drivers/acpi/internal.h |    1 -
 drivers/acpi/sleep.c    |    4 +--
 3 files changed, 48 insertions(+), 20 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index c2d135d..6493df8 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -103,6 +103,7 @@ enum ec_command {
 					 * when trying to clear the EC */
 
 enum {
+	EC_FLAGS_QUERY_ENABLED,		/* Query is enabled */
 	EC_FLAGS_QUERY_PENDING,		/* Query is pending */
 	EC_FLAGS_QUERY_GUARDING,	/* Guard for SCI_EVT check */
 	EC_FLAGS_GPE_HANDLER_INSTALLED,	/* GPE handler installed */
@@ -423,7 +424,8 @@ static bool acpi_ec_submit_flushable_request(struct acpi_ec *ec)
 
 static void acpi_ec_submit_query(struct acpi_ec *ec)
 {
-	if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
+	if (test_bit(EC_FLAGS_QUERY_ENABLED, &ec->flags) &&
+	    !test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
 		ec_dbg_evt("Command(%s) submitted/blocked",
 			   acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
 		ec->nr_pending_queries++;
@@ -440,6 +442,26 @@ static void acpi_ec_complete_query(struct acpi_ec *ec)
 	}
 }
 
+static void acpi_ec_disable_event(struct acpi_ec *ec)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&ec->lock, flags);
+	clear_bit(EC_FLAGS_QUERY_ENABLED, &ec->flags);
+	ec_log_drv("event blocked");
+	spin_unlock_irqrestore(&ec->lock, flags);
+}
+
+static void acpi_ec_enable_event(struct acpi_ec *ec)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&ec->lock, flags);
+	set_bit(EC_FLAGS_QUERY_ENABLED, &ec->flags);
+	ec_log_drv("event unblocked");
+	spin_unlock_irqrestore(&ec->lock, flags);
+}
+
 static bool acpi_ec_guard_event(struct acpi_ec *ec)
 {
 	bool guarded = true;
@@ -913,20 +935,6 @@ void acpi_ec_block_transactions(void)
 
 void acpi_ec_unblock_transactions(void)
 {
-	struct acpi_ec *ec = first_ec;
-
-	if (!ec)
-		return;
-
-	/* Allow transactions to be carried out again */
-	acpi_ec_start(ec, true);
-
-	if (EC_FLAGS_CLEAR_ON_RESUME)
-		acpi_ec_clear(ec);
-}
-
-void acpi_ec_unblock_transactions_early(void)
-{
 	/*
 	 * Allow transactions to happen again (this function is called from
 	 * atomic context during wakeup, so we don't need to acquire the mutex).
@@ -1228,13 +1236,13 @@ static struct acpi_ec *make_acpi_ec(void)
 
 	if (!ec)
 		return NULL;
-	ec->flags = 1 << EC_FLAGS_QUERY_PENDING;
 	mutex_init(&ec->mutex);
 	init_waitqueue_head(&ec->wait);
 	INIT_LIST_HEAD(&ec->list);
 	spin_lock_init(&ec->lock);
 	INIT_WORK(&ec->work, acpi_ec_event_handler);
 	ec->timestamp = jiffies;
+	acpi_ec_disable_event(ec);
 	return ec;
 }
 
@@ -1415,7 +1423,7 @@ static int acpi_ec_add(struct acpi_device *device)
 	acpi_walk_dep_device_list(ec->handle);
 
 	/* EC is fully operational, allow queries */
-	clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
+	acpi_ec_enable_event(ec);
 
 	/* Clear stale _Q events if hardware might require that */
 	if (EC_FLAGS_CLEAR_ON_RESUME)
@@ -1659,10 +1667,31 @@ static int acpi_ec_resume_noirq(struct device *dev)
 	acpi_ec_leave_noirq(ec);
 	return 0;
 }
+
+static int acpi_ec_suspend(struct device *dev)
+{
+	struct acpi_ec *ec =
+		acpi_driver_data(to_acpi_device(dev));
+
+	acpi_ec_disable_event(ec);
+	return 0;
+}
+
+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_FLAGS_CLEAR_ON_RESUME)
+		acpi_ec_clear(ec);
+	return 0;
+}
 #endif
 
 static const struct dev_pm_ops acpi_ec_pm = {
 	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(acpi_ec_suspend_noirq, acpi_ec_resume_noirq)
+	SET_SYSTEM_SLEEP_PM_OPS(acpi_ec_suspend, acpi_ec_resume)
 };
 
 static int param_set_event_clearing(const char *val, struct kernel_param *kp)
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 6996121..29f2063 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -189,7 +189,6 @@ int acpi_ec_ecdt_probe(void);
 int acpi_ec_dsdt_probe(void);
 void acpi_ec_block_transactions(void);
 void acpi_ec_unblock_transactions(void);
-void acpi_ec_unblock_transactions_early(void);
 int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
 			      acpi_handle handle, acpi_ec_query_func func,
 			      void *data);
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 2b38c1b..bb1e0d2 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -586,7 +586,7 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
 	 */
 	acpi_disable_all_gpes();
 	/* Allow EC transactions to happen. */
-	acpi_ec_unblock_transactions_early();
+	acpi_ec_unblock_transactions();
 
 	suspend_nvs_restore();
 
@@ -784,7 +784,7 @@ static void acpi_hibernation_leave(void)
 	/* Restore the NVS memory area */
 	suspend_nvs_restore();
 	/* Allow EC transactions to happen. */
-	acpi_ec_unblock_transactions_early();
+	acpi_ec_unblock_transactions();
 }
 
 static void acpi_pm_thaw(void)
-- 
1.7.10

[PATCH 2/2] ACPI / EC: Add PM operations to block event handling

[Lv Zheng]

During the early stage for boot/resume, EC shouldn't handle _Qxx as EC
shouldn't expect the AML interpreter to be fully running and other drivers
are ready to handle the notifications issued from _Qxx.

Originally, EC driver stops handling both events and transactions in
acpi_ec_block_transactions(), and restarts to handle transactions in
acpi_ec_unblock_transactions_early(), restarts to handle both events and
transactions in acpi_ec_unblock_transactions().

However "stop handling events" mean the EC driver notices that the system
is about to suspend, "start handling events" means the EC driver notices
that the system is about to resume. This can be implemented via PM ops.
With "event handling switch" implemented via suspend/resume ops, the
following 2 APIs serve for the same purpose and can be combined:
acpi_ec_unblock_transactions_early()/acpi_ec_unblock_transactions().

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c       |   63 ++++++++++++++++++++++++++++++++++-------------
 drivers/acpi/internal.h |    1 -
 drivers/acpi/sleep.c    |    4 +--
 3 files changed, 48 insertions(+), 20 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index c2d135d..6493df8 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -103,6 +103,7 @@ enum ec_command {
 					 * when trying to clear the EC */
 
 enum {
+	EC_FLAGS_QUERY_ENABLED,		/* Query is enabled */
 	EC_FLAGS_QUERY_PENDING,		/* Query is pending */
 	EC_FLAGS_QUERY_GUARDING,	/* Guard for SCI_EVT check */
 	EC_FLAGS_GPE_HANDLER_INSTALLED,	/* GPE handler installed */
@@ -423,7 +424,8 @@ static bool acpi_ec_submit_flushable_request(struct acpi_ec *ec)
 
 static void acpi_ec_submit_query(struct acpi_ec *ec)
 {
-	if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
+	if (test_bit(EC_FLAGS_QUERY_ENABLED, &ec->flags) &&
+	    !test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
 		ec_dbg_evt("Command(%s) submitted/blocked",
 			   acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
 		ec->nr_pending_queries++;
@@ -440,6 +442,26 @@ static void acpi_ec_complete_query(struct acpi_ec *ec)
 	}
 }
 
+static void acpi_ec_disable_event(struct acpi_ec *ec)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&ec->lock, flags);
+	clear_bit(EC_FLAGS_QUERY_ENABLED, &ec->flags);
+	ec_log_drv("event blocked");
+	spin_unlock_irqrestore(&ec->lock, flags);
+}
+
+static void acpi_ec_enable_event(struct acpi_ec *ec)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&ec->lock, flags);
+	set_bit(EC_FLAGS_QUERY_ENABLED, &ec->flags);
+	ec_log_drv("event unblocked");
+	spin_unlock_irqrestore(&ec->lock, flags);
+}
+
 static bool acpi_ec_guard_event(struct acpi_ec *ec)
 {
 	bool guarded = true;
@@ -913,20 +935,6 @@ void acpi_ec_block_transactions(void)
 
 void acpi_ec_unblock_transactions(void)
 {
-	struct acpi_ec *ec = first_ec;
-
-	if (!ec)
-		return;
-
-	/* Allow transactions to be carried out again */
-	acpi_ec_start(ec, true);
-
-	if (EC_FLAGS_CLEAR_ON_RESUME)
-		acpi_ec_clear(ec);
-}
-
-void acpi_ec_unblock_transactions_early(void)
-{
 	/*
 	 * Allow transactions to happen again (this function is called from
 	 * atomic context during wakeup, so we don't need to acquire the mutex).
@@ -1228,13 +1236,13 @@ static struct acpi_ec *make_acpi_ec(void)
 
 	if (!ec)
 		return NULL;
-	ec->flags = 1 << EC_FLAGS_QUERY_PENDING;
 	mutex_init(&ec->mutex);
 	init_waitqueue_head(&ec->wait);
 	INIT_LIST_HEAD(&ec->list);
 	spin_lock_init(&ec->lock);
 	INIT_WORK(&ec->work, acpi_ec_event_handler);
 	ec->timestamp = jiffies;
+	acpi_ec_disable_event(ec);
 	return ec;
 }
 
@@ -1415,7 +1423,7 @@ static int acpi_ec_add(struct acpi_device *device)
 	acpi_walk_dep_device_list(ec->handle);
 
 	/* EC is fully operational, allow queries */
-	clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
+	acpi_ec_enable_event(ec);
 
 	/* Clear stale _Q events if hardware might require that */
 	if (EC_FLAGS_CLEAR_ON_RESUME)
@@ -1659,10 +1667,31 @@ static int acpi_ec_resume_noirq(struct device *dev)
 	acpi_ec_leave_noirq(ec);
 	return 0;
 }
+
+static int acpi_ec_suspend(struct device *dev)
+{
+	struct acpi_ec *ec =
+		acpi_driver_data(to_acpi_device(dev));
+
+	acpi_ec_disable_event(ec);
+	return 0;
+}
+
+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_FLAGS_CLEAR_ON_RESUME)
+		acpi_ec_clear(ec);
+	return 0;
+}
 #endif
 
 static const struct dev_pm_ops acpi_ec_pm = {
 	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(acpi_ec_suspend_noirq, acpi_ec_resume_noirq)
+	SET_SYSTEM_SLEEP_PM_OPS(acpi_ec_suspend, acpi_ec_resume)
 };
 
 static int param_set_event_clearing(const char *val, struct kernel_param *kp)
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 6996121..29f2063 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -189,7 +189,6 @@ int acpi_ec_ecdt_probe(void);
 int acpi_ec_dsdt_probe(void);
 void acpi_ec_block_transactions(void);
 void acpi_ec_unblock_transactions(void);
-void acpi_ec_unblock_transactions_early(void);
 int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
 			      acpi_handle handle, acpi_ec_query_func func,
 			      void *data);
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 2b38c1b..bb1e0d2 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -586,7 +586,7 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
 	 */
 	acpi_disable_all_gpes();
 	/* Allow EC transactions to happen. */
-	acpi_ec_unblock_transactions_early();
+	acpi_ec_unblock_transactions();
 
 	suspend_nvs_restore();
 
@@ -784,7 +784,7 @@ static void acpi_hibernation_leave(void)
 	/* Restore the NVS memory area */
 	suspend_nvs_restore();
 	/* Allow EC transactions to happen. */
-	acpi_ec_unblock_transactions_early();
+	acpi_ec_unblock_transactions();
 }
 
 static void acpi_pm_thaw(void)
-- 
1.7.10

RE: [PATCH 2/2] ACPI / EC: Add PM operations to block event handling

[Zheng, Lv]

Hi, Rafael

We have regression report here related to EC.
https://bugzilla.kernel.org/show_bug.cgi?id=135691

After fixing that bug, I found that this patch could be improved on top of the fix.
Making suspend/resume faster.
So please drop [PATCH 2] as I will improve it.

While [PATCH 1] is still valid.
However if you want me to send [PATCH 1] again with the refreshed [PATCH 2], you can drop both.

Sorry for the noise.

Thanks and best regards
-Lv

> From: Zheng, Lv
> Subject: [PATCH 2/2] ACPI / EC: Add PM operations to block event
> handling
> 
> During the early stage for boot/resume, EC shouldn't handle _Qxx as EC
> shouldn't expect the AML interpreter to be fully running and other drivers
> are ready to handle the notifications issued from _Qxx.
> 
> Originally, EC driver stops handling both events and transactions in
> acpi_ec_block_transactions(), and restarts to handle transactions in
> acpi_ec_unblock_transactions_early(), restarts to handle both events and
> transactions in acpi_ec_unblock_transactions().
> 
> However "stop handling events" mean the EC driver notices that the
> system
> is about to suspend, "start handling events" means the EC driver notices
> that the system is about to resume. This can be implemented via PM ops.
> With "event handling switch" implemented via suspend/resume ops, the
> following 2 APIs serve for the same purpose and can be combined:
> acpi_ec_unblock_transactions_early()/acpi_ec_unblock_transactions().
> 
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> ---
>  drivers/acpi/ec.c       |   63 ++++++++++++++++++++++++++++++++++-----
> --------
>  drivers/acpi/internal.h |    1 -
>  drivers/acpi/sleep.c    |    4 +--
>  3 files changed, 48 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index c2d135d..6493df8 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -103,6 +103,7 @@ enum ec_command {
>  					 * when trying to clear the EC */
> 
>  enum {
> +	EC_FLAGS_QUERY_ENABLED,		/* Query is enabled */
>  	EC_FLAGS_QUERY_PENDING,		/* Query is pending */
>  	EC_FLAGS_QUERY_GUARDING,	/* Guard for SCI_EVT check
> */
>  	EC_FLAGS_GPE_HANDLER_INSTALLED,	/* GPE handler installed */
> @@ -423,7 +424,8 @@ static bool
> acpi_ec_submit_flushable_request(struct acpi_ec *ec)
> 
>  static void acpi_ec_submit_query(struct acpi_ec *ec)
>  {
> -	if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
> +	if (test_bit(EC_FLAGS_QUERY_ENABLED, &ec->flags) &&
> +	    !test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
>  		ec_dbg_evt("Command(%s) submitted/blocked",
> 
> acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
>  		ec->nr_pending_queries++;
> @@ -440,6 +442,26 @@ static void acpi_ec_complete_query(struct
> acpi_ec *ec)
>  	}
>  }
> 
> +static void acpi_ec_disable_event(struct acpi_ec *ec)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ec->lock, flags);
> +	clear_bit(EC_FLAGS_QUERY_ENABLED, &ec->flags);
> +	ec_log_drv("event blocked");
> +	spin_unlock_irqrestore(&ec->lock, flags);
> +}
> +
> +static void acpi_ec_enable_event(struct acpi_ec *ec)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ec->lock, flags);
> +	set_bit(EC_FLAGS_QUERY_ENABLED, &ec->flags);
> +	ec_log_drv("event unblocked");
> +	spin_unlock_irqrestore(&ec->lock, flags);
> +}
> +
>  static bool acpi_ec_guard_event(struct acpi_ec *ec)
>  {
>  	bool guarded = true;
> @@ -913,20 +935,6 @@ void acpi_ec_block_transactions(void)
> 
>  void acpi_ec_unblock_transactions(void)
>  {
> -	struct acpi_ec *ec = first_ec;
> -
> -	if (!ec)
> -		return;
> -
> -	/* Allow transactions to be carried out again */
> -	acpi_ec_start(ec, true);
> -
> -	if (EC_FLAGS_CLEAR_ON_RESUME)
> -		acpi_ec_clear(ec);
> -}
> -
> -void acpi_ec_unblock_transactions_early(void)
> -{
>  	/*
>  	 * Allow transactions to happen again (this function is called from
>  	 * atomic context during wakeup, so we don't need to acquire the
> mutex).
> @@ -1228,13 +1236,13 @@ static struct acpi_ec *make_acpi_ec(void)
> 
>  	if (!ec)
>  		return NULL;
> -	ec->flags = 1 << EC_FLAGS_QUERY_PENDING;
>  	mutex_init(&ec->mutex);
>  	init_waitqueue_head(&ec->wait);
>  	INIT_LIST_HEAD(&ec->list);
>  	spin_lock_init(&ec->lock);
>  	INIT_WORK(&ec->work, acpi_ec_event_handler);
>  	ec->timestamp = jiffies;
> +	acpi_ec_disable_event(ec);
>  	return ec;
>  }
> 
> @@ -1415,7 +1423,7 @@ static int acpi_ec_add(struct acpi_device
> *device)
>  	acpi_walk_dep_device_list(ec->handle);
> 
>  	/* EC is fully operational, allow queries */
> -	clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
> +	acpi_ec_enable_event(ec);
> 
>  	/* Clear stale _Q events if hardware might require that */
>  	if (EC_FLAGS_CLEAR_ON_RESUME)
> @@ -1659,10 +1667,31 @@ static int acpi_ec_resume_noirq(struct
> device *dev)
>  	acpi_ec_leave_noirq(ec);
>  	return 0;
>  }
> +
> +static int acpi_ec_suspend(struct device *dev)
> +{
> +	struct acpi_ec *ec =
> +		acpi_driver_data(to_acpi_device(dev));
> +
> +	acpi_ec_disable_event(ec);
> +	return 0;
> +}
> +
> +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_FLAGS_CLEAR_ON_RESUME)
> +		acpi_ec_clear(ec);
> +	return 0;
> +}
>  #endif
> 
>  static const struct dev_pm_ops acpi_ec_pm = {
>  	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(acpi_ec_suspend_noirq,
> acpi_ec_resume_noirq)
> +	SET_SYSTEM_SLEEP_PM_OPS(acpi_ec_suspend, acpi_ec_resume)
>  };
> 
>  static int param_set_event_clearing(const char *val, struct kernel_param
> *kp)
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index 6996121..29f2063 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -189,7 +189,6 @@ int acpi_ec_ecdt_probe(void);
>  int acpi_ec_dsdt_probe(void);
>  void acpi_ec_block_transactions(void);
>  void acpi_ec_unblock_transactions(void);
> -void acpi_ec_unblock_transactions_early(void);
>  int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
>  			      acpi_handle handle, acpi_ec_query_func func,
>  			      void *data);
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 2b38c1b..bb1e0d2 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -586,7 +586,7 @@ static int acpi_suspend_enter(suspend_state_t
> pm_state)
>  	 */
>  	acpi_disable_all_gpes();
>  	/* Allow EC transactions to happen. */
> -	acpi_ec_unblock_transactions_early();
> +	acpi_ec_unblock_transactions();
> 
>  	suspend_nvs_restore();
> 
> @@ -784,7 +784,7 @@ static void acpi_hibernation_leave(void)
>  	/* Restore the NVS memory area */
>  	suspend_nvs_restore();
>  	/* Allow EC transactions to happen. */
> -	acpi_ec_unblock_transactions_early();
> +	acpi_ec_unblock_transactions();
>  }
> 
>  static void acpi_pm_thaw(void)
> --
> 1.7.10

[PATCH v2 0/2] ACPI / EC: Tune suspend/resume speed using PM operations

[Lv Zheng]

There are 2 improvements can be done to the EC driver to make system
suspend/resume faster:
1. Automatically use busy polling mode when noirq is entered
2. Disallow event handling (SCI_EVT/_Qxx) during suspend/resume period

This patchset achieves such performance tuning on top of a recent
workaround that creates ec_query_wq.

Lv Zheng (2):
  ACPI / EC: Add PM operations to tune polling mode efficiency
  ACPI / EC: Add PM operations to block event handling

 drivers/acpi/ec.c       |  199 ++++++++++++++++++++++++++++++++++++-----------
 drivers/acpi/internal.h |    3 +-
 drivers/acpi/sleep.c    |    4 +-
 3 files changed, 158 insertions(+), 48 deletions(-)

-- 
1.7.10

[PATCH v2 0/2] ACPI / EC: Tune suspend/resume speed using PM operations

[Lv Zheng]

There are 2 improvements can be done to the EC driver to make system
suspend/resume faster:
1. Automatically use busy polling mode when noirq is entered
2. Disallow event handling (SCI_EVT/_Qxx) during suspend/resume period

This patchset achieves such performance tuning on top of a recent
workaround that creates ec_query_wq.

Lv Zheng (2):
  ACPI / EC: Add PM operations to tune polling mode efficiency
  ACPI / EC: Add PM operations to block event handling

 drivers/acpi/ec.c       |  199 ++++++++++++++++++++++++++++++++++++-----------
 drivers/acpi/internal.h |    3 +-
 drivers/acpi/sleep.c    |    4 +-
 3 files changed, 158 insertions(+), 48 deletions(-)

-- 
1.7.10

[PATCH v2 1/2] ACPI / EC: Add PM operations to tune polling mode efficiency

[Lv Zheng]

It is reported that on some platforms, resume speed is not fast. The cause
is: in noirq stage, EC driver is working in polling mode, and each state
machine advancement requires a context switch.

The context switch is not necessary to the EC driver's polling mode. This
patch implements PM hooks to automatically switch the driver to/from the
busy polling mode to eliminate the overhead caused by the context switch.

This finally contributes to the tuning result: acpi_pm_finish() execution
time is improved from 192ms to 6ms.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Reported-and-tested-by: Todd E Brandt <todd.e.brandt@linux.intel.com>
---
 drivers/acpi/ec.c       |   53 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/internal.h |    2 ++
 2 files changed, 55 insertions(+)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 907b450..7cdcdf6 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1619,6 +1619,58 @@ error:
 	return ret;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static void acpi_ec_enter_noirq(struct acpi_ec *ec)
+{
+	unsigned long flags;
+
+	if (ec == first_ec) {
+		spin_lock_irqsave(&ec->lock, flags);
+		ec->saved_busy_polling = ec_busy_polling;
+		ec->saved_polling_guard = ec_polling_guard;
+		ec_busy_polling = true;
+		ec_polling_guard = 0;
+		ec_log_drv("interrupt blocked");
+		spin_unlock_irqrestore(&ec->lock, flags);
+	}
+}
+
+static void acpi_ec_leave_noirq(struct acpi_ec *ec)
+{
+	unsigned long flags;
+
+	if (ec == first_ec) {
+		spin_lock_irqsave(&ec->lock, flags);
+		ec_busy_polling = ec->saved_busy_polling;
+		ec_polling_guard = ec->saved_polling_guard;
+		ec_log_drv("interrupt unblocked");
+		spin_unlock_irqrestore(&ec->lock, flags);
+	}
+}
+
+static int acpi_ec_suspend_noirq(struct device *dev)
+{
+	struct acpi_ec *ec =
+		acpi_driver_data(to_acpi_device(dev));
+
+	acpi_ec_enter_noirq(ec);
+	return 0;
+}
+
+static int acpi_ec_resume_noirq(struct device *dev)
+{
+	struct acpi_ec *ec =
+		acpi_driver_data(to_acpi_device(dev));
+
+	acpi_ec_leave_noirq(ec);
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops acpi_ec_pm = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(acpi_ec_suspend_noirq, acpi_ec_resume_noirq)
+};
+
 static int param_set_event_clearing(const char *val, struct kernel_param *kp)
 {
 	int result = 0;
@@ -1664,6 +1716,7 @@ static struct acpi_driver acpi_ec_driver = {
 		.add = acpi_ec_add,
 		.remove = acpi_ec_remove,
 		},
+	.drv.pm = &acpi_ec_pm,
 };
 
 int __init acpi_ec_init(void)
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 940218f..6996121 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -174,6 +174,8 @@ struct acpi_ec {
 	struct work_struct work;
 	unsigned long timestamp;
 	unsigned long nr_pending_queries;
+	bool saved_busy_polling;
+	unsigned int saved_polling_guard;
 };
 
 extern struct acpi_ec *first_ec;
-- 
1.7.10

[PATCH v2 1/2] ACPI / EC: Add PM operations to tune polling mode efficiency

[Lv Zheng]

It is reported that on some platforms, resume speed is not fast. The cause
is: in noirq stage, EC driver is working in polling mode, and each state
machine advancement requires a context switch.

The context switch is not necessary to the EC driver's polling mode. This
patch implements PM hooks to automatically switch the driver to/from the
busy polling mode to eliminate the overhead caused by the context switch.

This finally contributes to the tuning result: acpi_pm_finish() execution
time is improved from 192ms to 6ms.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Reported-and-tested-by: Todd E Brandt <todd.e.brandt@linux.intel.com>
---
 drivers/acpi/ec.c       |   53 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/internal.h |    2 ++
 2 files changed, 55 insertions(+)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 907b450..7cdcdf6 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1619,6 +1619,58 @@ error:
 	return ret;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static void acpi_ec_enter_noirq(struct acpi_ec *ec)
+{
+	unsigned long flags;
+
+	if (ec == first_ec) {
+		spin_lock_irqsave(&ec->lock, flags);
+		ec->saved_busy_polling = ec_busy_polling;
+		ec->saved_polling_guard = ec_polling_guard;
+		ec_busy_polling = true;
+		ec_polling_guard = 0;
+		ec_log_drv("interrupt blocked");
+		spin_unlock_irqrestore(&ec->lock, flags);
+	}
+}
+
+static void acpi_ec_leave_noirq(struct acpi_ec *ec)
+{
+	unsigned long flags;
+
+	if (ec == first_ec) {
+		spin_lock_irqsave(&ec->lock, flags);
+		ec_busy_polling = ec->saved_busy_polling;
+		ec_polling_guard = ec->saved_polling_guard;
+		ec_log_drv("interrupt unblocked");
+		spin_unlock_irqrestore(&ec->lock, flags);
+	}
+}
+
+static int acpi_ec_suspend_noirq(struct device *dev)
+{
+	struct acpi_ec *ec =
+		acpi_driver_data(to_acpi_device(dev));
+
+	acpi_ec_enter_noirq(ec);
+	return 0;
+}
+
+static int acpi_ec_resume_noirq(struct device *dev)
+{
+	struct acpi_ec *ec =
+		acpi_driver_data(to_acpi_device(dev));
+
+	acpi_ec_leave_noirq(ec);
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops acpi_ec_pm = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(acpi_ec_suspend_noirq, acpi_ec_resume_noirq)
+};
+
 static int param_set_event_clearing(const char *val, struct kernel_param *kp)
 {
 	int result = 0;
@@ -1664,6 +1716,7 @@ static struct acpi_driver acpi_ec_driver = {
 		.add = acpi_ec_add,
 		.remove = acpi_ec_remove,
 		},
+	.drv.pm = &acpi_ec_pm,
 };
 
 int __init acpi_ec_init(void)
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 940218f..6996121 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -174,6 +174,8 @@ struct acpi_ec {
 	struct work_struct work;
 	unsigned long timestamp;
 	unsigned long nr_pending_queries;
+	bool saved_busy_polling;
+	unsigned int saved_polling_guard;
 };
 
 extern struct acpi_ec *first_ec;
-- 
1.7.10

[PATCH v2 2/2] ACPI / EC: Add PM operations to block event handling

[Lv Zheng]

Originally, EC driver stops handling both events and transactions in
acpi_ec_block_transactions(), and restarts to handle transactions in
acpi_ec_unblock_transactions_early(), restarts to handle both events and
transactions in acpi_ec_unblock_transactions().

Thus, the event handling is actually stopped after the IRQ is disabled, but
the EC driver is not capable of handling SCI_EVT in noirq stage, thus it is
likely the event is not detected by the EC driver.

This patch tries to restore the old behavior for the EC driver. However,
do we actually need to handle EC events during suspend/resume stage? EC
events are mostly useless for the suspend/resume period (key strokes and
battery/thermal updates, etc.,), and the useful ones (lid close,
power/sleep button press) should have already been delivered to OS to
trigger the power saving operations. Thus EC driver should stop handling
events during this period, just like what the EC driver does during the
boot stage. And tests show this behavior is working and can make suspend
even faster when many events is triggered during this stage (for example,
during this stage, frequently trigger wifi switches).

OTOH, delivering EC events too early may confuse drivers because when the
drivers see the events, the drivers themselves may not have been resumed.

Thus this patch implements PM hooks, stops to handle event in .suspend()
hook and restarts to handle event in .resume() hook. This is different
from the original implementation, enlarging the event handling blocking
period longer:
                        Original        Current
suspend before EC       Y               Y
suspend after EC        Y               N
suspend_late            Y               N
suspend_noirq           Y (actually N)  N
resume_noirq            Y (actually N)  N
resume_late             Y               N
resume before EC        Y               N
resume after EC         Y               Y
Since this is experimental, a boot parameter is prepared to not to
disable event handling during suspend/resume period.

By implementing .resume() hook to re-enable the event handling, the
following 2 APIs serve for the same purpose (restart transactions) and can
be combined:
acpi_ec_unblock_transactions_early()/acpi_ec_unblock_transactions().

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Tested-by: Todd E Brandt <todd.e.brandt@linux.intel.com>
---
 drivers/acpi/ec.c       |  146 ++++++++++++++++++++++++++++++++---------------
 drivers/acpi/internal.h |    1 -
 drivers/acpi/sleep.c    |    4 +-
 3 files changed, 103 insertions(+), 48 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 7cdcdf6..8c3034c 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -104,6 +104,7 @@ enum ec_command {
 #define ACPI_EC_MAX_QUERIES	16	/* Maximum number of parallel queries */
 
 enum {
+	EC_FLAGS_QUERY_ENABLED,		/* Query is enabled */
 	EC_FLAGS_QUERY_PENDING,		/* Query is pending */
 	EC_FLAGS_QUERY_GUARDING,	/* Guard for SCI_EVT check */
 	EC_FLAGS_GPE_HANDLER_INSTALLED,	/* GPE handler installed */
@@ -145,6 +146,10 @@ static unsigned int ec_storm_threshold  __read_mostly = 8;
 module_param(ec_storm_threshold, uint, 0644);
 MODULE_PARM_DESC(ec_storm_threshold, "Maxim false GPE numbers not considered as GPE storm");
 
+static bool ec_freeze_events __read_mostly = true;
+module_param(ec_freeze_events, bool, 0644);
+MODULE_PARM_DESC(ec_freeze_events, "Disabling event handling during suspend/resume");
+
 struct acpi_ec_query_handler {
 	struct list_head node;
 	acpi_ec_query_func func;
@@ -427,13 +432,19 @@ static bool acpi_ec_submit_flushable_request(struct acpi_ec *ec)
 	return true;
 }
 
+static void __acpi_ec_submit_query(struct acpi_ec *ec)
+{
+	ec_dbg_evt("Command(%s) submitted/blocked",
+		   acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
+	ec->nr_pending_queries++;
+	schedule_work(&ec->work);
+}
+
 static void acpi_ec_submit_query(struct acpi_ec *ec)
 {
 	if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
-		ec_dbg_evt("Command(%s) submitted/blocked",
-			   acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
-		ec->nr_pending_queries++;
-		schedule_work(&ec->work);
+		if (test_bit(EC_FLAGS_QUERY_ENABLED, &ec->flags))
+			__acpi_ec_submit_query(ec);
 	}
 }
 
@@ -446,6 +457,70 @@ static void acpi_ec_complete_query(struct acpi_ec *ec)
 	}
 }
 
+static bool acpi_ec_query_flushed(struct acpi_ec *ec)
+{
+	bool flushed;
+	unsigned long flags;
+
+	spin_lock_irqsave(&ec->lock, flags);
+	flushed = !ec->nr_pending_queries;
+	spin_unlock_irqrestore(&ec->lock, flags);
+
+	return flushed;
+}
+
+/*
+ * Process _Q events that might have accumulated in the EC.
+ * Run with locked ec mutex.
+ */
+static void acpi_ec_clear(struct acpi_ec *ec)
+{
+	int i, status;
+	u8 value = 0;
+
+	for (i = 0; i < ACPI_EC_CLEAR_MAX; i++) {
+		status = acpi_ec_query(ec, &value);
+		if (status || !value)
+			break;
+	}
+	if (unlikely(i == ACPI_EC_CLEAR_MAX))
+		pr_warn("Warning: Maximum of %d stale EC events cleared\n", i);
+	else
+		pr_info("%d stale EC events cleared\n", i);
+}
+
+static void acpi_ec_disable_event(struct acpi_ec *ec, bool flushing)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&ec->lock, flags);
+	clear_bit(EC_FLAGS_QUERY_ENABLED, &ec->flags);
+	ec_log_drv("event blocked");
+	spin_unlock_irqrestore(&ec->lock, flags);
+	if (flushing && ec_query_wq) {
+		wait_event(ec->wait, acpi_ec_query_flushed(ec));
+		flush_workqueue(ec_query_wq);
+	}
+}
+
+static void acpi_ec_enable_event(struct acpi_ec *ec)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&ec->lock, flags);
+	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))
+		__acpi_ec_submit_query(ec);
+	else
+		advance_transaction(ec);
+	spin_unlock_irqrestore(&ec->lock, flags);
+
+	/* Drain additional events if hardware requires that */
+	if (EC_FLAGS_CLEAR_ON_RESUME)
+		acpi_ec_clear(ec);
+}
+
 static bool acpi_ec_guard_event(struct acpi_ec *ec)
 {
 	bool guarded = true;
@@ -832,27 +907,6 @@ acpi_handle ec_get_handle(void)
 }
 EXPORT_SYMBOL(ec_get_handle);
 
-/*
- * Process _Q events that might have accumulated in the EC.
- * Run with locked ec mutex.
- */
-static void acpi_ec_clear(struct acpi_ec *ec)
-{
-	int i, status;
-	u8 value = 0;
-
-	for (i = 0; i < ACPI_EC_CLEAR_MAX; i++) {
-		status = acpi_ec_query(ec, &value);
-		if (status || !value)
-			break;
-	}
-
-	if (unlikely(i == ACPI_EC_CLEAR_MAX))
-		pr_warn("Warning: Maximum of %d stale EC events cleared\n", i);
-	else
-		pr_info("%d stale EC events cleared\n", i);
-}
-
 static void acpi_ec_start(struct acpi_ec *ec, bool resuming)
 {
 	unsigned long flags;
@@ -919,20 +973,6 @@ void acpi_ec_block_transactions(void)
 
 void acpi_ec_unblock_transactions(void)
 {
-	struct acpi_ec *ec = first_ec;
-
-	if (!ec)
-		return;
-
-	/* Allow transactions to be carried out again */
-	acpi_ec_start(ec, true);
-
-	if (EC_FLAGS_CLEAR_ON_RESUME)
-		acpi_ec_clear(ec);
-}
-
-void acpi_ec_unblock_transactions_early(void)
-{
 	/*
 	 * Allow transactions to happen again (this function is called from
 	 * atomic context during wakeup, so we don't need to acquire the mutex).
@@ -1234,13 +1274,13 @@ static struct acpi_ec *make_acpi_ec(void)
 
 	if (!ec)
 		return NULL;
-	ec->flags = 1 << EC_FLAGS_QUERY_PENDING;
 	mutex_init(&ec->mutex);
 	init_waitqueue_head(&ec->wait);
 	INIT_LIST_HEAD(&ec->list);
 	spin_lock_init(&ec->lock);
 	INIT_WORK(&ec->work, acpi_ec_event_handler);
 	ec->timestamp = jiffies;
+	acpi_ec_disable_event(ec, false);
 	return ec;
 }
 
@@ -1421,11 +1461,7 @@ static int acpi_ec_add(struct acpi_device *device)
 	acpi_walk_dep_device_list(ec->handle);
 
 	/* EC is fully operational, allow queries */
-	clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
-
-	/* Clear stale _Q events if hardware might require that */
-	if (EC_FLAGS_CLEAR_ON_RESUME)
-		acpi_ec_clear(ec);
+	acpi_ec_enable_event(ec);
 	return ret;
 }
 
@@ -1665,10 +1701,30 @@ static int acpi_ec_resume_noirq(struct device *dev)
 	acpi_ec_leave_noirq(ec);
 	return 0;
 }
+
+static int acpi_ec_suspend(struct device *dev)
+{
+	struct acpi_ec *ec =
+		acpi_driver_data(to_acpi_device(dev));
+
+	if (ec_freeze_events)
+		acpi_ec_disable_event(ec, true);
+	return 0;
+}
+
+static int acpi_ec_resume(struct device *dev)
+{
+	struct acpi_ec *ec =
+		acpi_driver_data(to_acpi_device(dev));
+
+	acpi_ec_enable_event(ec);
+	return 0;
+}
 #endif
 
 static const struct dev_pm_ops acpi_ec_pm = {
 	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(acpi_ec_suspend_noirq, acpi_ec_resume_noirq)
+	SET_SYSTEM_SLEEP_PM_OPS(acpi_ec_suspend, acpi_ec_resume)
 };
 
 static int param_set_event_clearing(const char *val, struct kernel_param *kp)
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 6996121..29f2063 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -189,7 +189,6 @@ int acpi_ec_ecdt_probe(void);
 int acpi_ec_dsdt_probe(void);
 void acpi_ec_block_transactions(void);
 void acpi_ec_unblock_transactions(void);
-void acpi_ec_unblock_transactions_early(void);
 int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
 			      acpi_handle handle, acpi_ec_query_func func,
 			      void *data);
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 9788663..deb0ff7 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -586,7 +586,7 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
 	 */
 	acpi_disable_all_gpes();
 	/* Allow EC transactions to happen. */
-	acpi_ec_unblock_transactions_early();
+	acpi_ec_unblock_transactions();
 
 	suspend_nvs_restore();
 
@@ -784,7 +784,7 @@ static void acpi_hibernation_leave(void)
 	/* Restore the NVS memory area */
 	suspend_nvs_restore();
 	/* Allow EC transactions to happen. */
-	acpi_ec_unblock_transactions_early();
+	acpi_ec_unblock_transactions();
 }
 
 static void acpi_pm_thaw(void)
-- 
1.7.10

[PATCH v2 2/2] ACPI / EC: Add PM operations to block event handling

[Lv Zheng]

Originally, EC driver stops handling both events and transactions in
acpi_ec_block_transactions(), and restarts to handle transactions in
acpi_ec_unblock_transactions_early(), restarts to handle both events and
transactions in acpi_ec_unblock_transactions().

Thus, the event handling is actually stopped after the IRQ is disabled, but
the EC driver is not capable of handling SCI_EVT in noirq stage, thus it is
likely the event is not detected by the EC driver.

This patch tries to restore the old behavior for the EC driver. However,
do we actually need to handle EC events during suspend/resume stage? EC
events are mostly useless for the suspend/resume period (key strokes and
battery/thermal updates, etc.,), and the useful ones (lid close,
power/sleep button press) should have already been delivered to OS to
trigger the power saving operations. Thus EC driver should stop handling
events during this period, just like what the EC driver does during the
boot stage. And tests show this behavior is working and can make suspend
even faster when many events is triggered during this stage (for example,
during this stage, frequently trigger wifi switches).

OTOH, delivering EC events too early may confuse drivers because when the
drivers see the events, the drivers themselves may not have been resumed.

Thus this patch implements PM hooks, stops to handle event in .suspend()
hook and restarts to handle event in .resume() hook. This is different
from the original implementation, enlarging the event handling blocking
period longer:
                        Original        Current
suspend before EC       Y               Y
suspend after EC        Y               N
suspend_late            Y               N
suspend_noirq           Y (actually N)  N
resume_noirq            Y (actually N)  N
resume_late             Y               N
resume before EC        Y               N
resume after EC         Y               Y
Since this is experimental, a boot parameter is prepared to not to
disable event handling during suspend/resume period.

By implementing .resume() hook to re-enable the event handling, the
following 2 APIs serve for the same purpose (restart transactions) and can
be combined:
acpi_ec_unblock_transactions_early()/acpi_ec_unblock_transactions().

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Tested-by: Todd E Brandt <todd.e.brandt@linux.intel.com>
---
 drivers/acpi/ec.c       |  146 ++++++++++++++++++++++++++++++++---------------
 drivers/acpi/internal.h |    1 -
 drivers/acpi/sleep.c    |    4 +-
 3 files changed, 103 insertions(+), 48 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 7cdcdf6..8c3034c 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -104,6 +104,7 @@ enum ec_command {
 #define ACPI_EC_MAX_QUERIES	16	/* Maximum number of parallel queries */
 
 enum {
+	EC_FLAGS_QUERY_ENABLED,		/* Query is enabled */
 	EC_FLAGS_QUERY_PENDING,		/* Query is pending */
 	EC_FLAGS_QUERY_GUARDING,	/* Guard for SCI_EVT check */
 	EC_FLAGS_GPE_HANDLER_INSTALLED,	/* GPE handler installed */
@@ -145,6 +146,10 @@ static unsigned int ec_storm_threshold  __read_mostly = 8;
 module_param(ec_storm_threshold, uint, 0644);
 MODULE_PARM_DESC(ec_storm_threshold, "Maxim false GPE numbers not considered as GPE storm");
 
+static bool ec_freeze_events __read_mostly = true;
+module_param(ec_freeze_events, bool, 0644);
+MODULE_PARM_DESC(ec_freeze_events, "Disabling event handling during suspend/resume");
+
 struct acpi_ec_query_handler {
 	struct list_head node;
 	acpi_ec_query_func func;
@@ -427,13 +432,19 @@ static bool acpi_ec_submit_flushable_request(struct acpi_ec *ec)
 	return true;
 }
 
+static void __acpi_ec_submit_query(struct acpi_ec *ec)
+{
+	ec_dbg_evt("Command(%s) submitted/blocked",
+		   acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
+	ec->nr_pending_queries++;
+	schedule_work(&ec->work);
+}
+
 static void acpi_ec_submit_query(struct acpi_ec *ec)
 {
 	if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
-		ec_dbg_evt("Command(%s) submitted/blocked",
-			   acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
-		ec->nr_pending_queries++;
-		schedule_work(&ec->work);
+		if (test_bit(EC_FLAGS_QUERY_ENABLED, &ec->flags))
+			__acpi_ec_submit_query(ec);
 	}
 }
 
@@ -446,6 +457,70 @@ static void acpi_ec_complete_query(struct acpi_ec *ec)
 	}
 }
 
+static bool acpi_ec_query_flushed(struct acpi_ec *ec)
+{
+	bool flushed;
+	unsigned long flags;
+
+	spin_lock_irqsave(&ec->lock, flags);
+	flushed = !ec->nr_pending_queries;
+	spin_unlock_irqrestore(&ec->lock, flags);
+
+	return flushed;
+}
+
+/*
+ * Process _Q events that might have accumulated in the EC.
+ * Run with locked ec mutex.
+ */
+static void acpi_ec_clear(struct acpi_ec *ec)
+{
+	int i, status;
+	u8 value = 0;
+
+	for (i = 0; i < ACPI_EC_CLEAR_MAX; i++) {
+		status = acpi_ec_query(ec, &value);
+		if (status || !value)
+			break;
+	}
+	if (unlikely(i == ACPI_EC_CLEAR_MAX))
+		pr_warn("Warning: Maximum of %d stale EC events cleared\n", i);
+	else
+		pr_info("%d stale EC events cleared\n", i);
+}
+
+static void acpi_ec_disable_event(struct acpi_ec *ec, bool flushing)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&ec->lock, flags);
+	clear_bit(EC_FLAGS_QUERY_ENABLED, &ec->flags);
+	ec_log_drv("event blocked");
+	spin_unlock_irqrestore(&ec->lock, flags);
+	if (flushing && ec_query_wq) {
+		wait_event(ec->wait, acpi_ec_query_flushed(ec));
+		flush_workqueue(ec_query_wq);
+	}
+}
+
+static void acpi_ec_enable_event(struct acpi_ec *ec)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&ec->lock, flags);
+	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))
+		__acpi_ec_submit_query(ec);
+	else
+		advance_transaction(ec);
+	spin_unlock_irqrestore(&ec->lock, flags);
+
+	/* Drain additional events if hardware requires that */
+	if (EC_FLAGS_CLEAR_ON_RESUME)
+		acpi_ec_clear(ec);
+}
+
 static bool acpi_ec_guard_event(struct acpi_ec *ec)
 {
 	bool guarded = true;
@@ -832,27 +907,6 @@ acpi_handle ec_get_handle(void)
 }
 EXPORT_SYMBOL(ec_get_handle);
 
-/*
- * Process _Q events that might have accumulated in the EC.
- * Run with locked ec mutex.
- */
-static void acpi_ec_clear(struct acpi_ec *ec)
-{
-	int i, status;
-	u8 value = 0;
-
-	for (i = 0; i < ACPI_EC_CLEAR_MAX; i++) {
-		status = acpi_ec_query(ec, &value);
-		if (status || !value)
-			break;
-	}
-
-	if (unlikely(i == ACPI_EC_CLEAR_MAX))
-		pr_warn("Warning: Maximum of %d stale EC events cleared\n", i);
-	else
-		pr_info("%d stale EC events cleared\n", i);
-}
-
 static void acpi_ec_start(struct acpi_ec *ec, bool resuming)
 {
 	unsigned long flags;
@@ -919,20 +973,6 @@ void acpi_ec_block_transactions(void)
 
 void acpi_ec_unblock_transactions(void)
 {
-	struct acpi_ec *ec = first_ec;
-
-	if (!ec)
-		return;
-
-	/* Allow transactions to be carried out again */
-	acpi_ec_start(ec, true);
-
-	if (EC_FLAGS_CLEAR_ON_RESUME)
-		acpi_ec_clear(ec);
-}
-
-void acpi_ec_unblock_transactions_early(void)
-{
 	/*
 	 * Allow transactions to happen again (this function is called from
 	 * atomic context during wakeup, so we don't need to acquire the mutex).
@@ -1234,13 +1274,13 @@ static struct acpi_ec *make_acpi_ec(void)
 
 	if (!ec)
 		return NULL;
-	ec->flags = 1 << EC_FLAGS_QUERY_PENDING;
 	mutex_init(&ec->mutex);
 	init_waitqueue_head(&ec->wait);
 	INIT_LIST_HEAD(&ec->list);
 	spin_lock_init(&ec->lock);
 	INIT_WORK(&ec->work, acpi_ec_event_handler);
 	ec->timestamp = jiffies;
+	acpi_ec_disable_event(ec, false);
 	return ec;
 }
 
@@ -1421,11 +1461,7 @@ static int acpi_ec_add(struct acpi_device *device)
 	acpi_walk_dep_device_list(ec->handle);
 
 	/* EC is fully operational, allow queries */
-	clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
-
-	/* Clear stale _Q events if hardware might require that */
-	if (EC_FLAGS_CLEAR_ON_RESUME)
-		acpi_ec_clear(ec);
+	acpi_ec_enable_event(ec);
 	return ret;
 }
 
@@ -1665,10 +1701,30 @@ static int acpi_ec_resume_noirq(struct device *dev)
 	acpi_ec_leave_noirq(ec);
 	return 0;
 }
+
+static int acpi_ec_suspend(struct device *dev)
+{
+	struct acpi_ec *ec =
+		acpi_driver_data(to_acpi_device(dev));
+
+	if (ec_freeze_events)
+		acpi_ec_disable_event(ec, true);
+	return 0;
+}
+
+static int acpi_ec_resume(struct device *dev)
+{
+	struct acpi_ec *ec =
+		acpi_driver_data(to_acpi_device(dev));
+
+	acpi_ec_enable_event(ec);
+	return 0;
+}
 #endif
 
 static const struct dev_pm_ops acpi_ec_pm = {
 	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(acpi_ec_suspend_noirq, acpi_ec_resume_noirq)
+	SET_SYSTEM_SLEEP_PM_OPS(acpi_ec_suspend, acpi_ec_resume)
 };
 
 static int param_set_event_clearing(const char *val, struct kernel_param *kp)
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 6996121..29f2063 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -189,7 +189,6 @@ int acpi_ec_ecdt_probe(void);
 int acpi_ec_dsdt_probe(void);
 void acpi_ec_block_transactions(void);
 void acpi_ec_unblock_transactions(void);
-void acpi_ec_unblock_transactions_early(void);
 int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
 			      acpi_handle handle, acpi_ec_query_func func,
 			      void *data);
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 9788663..deb0ff7 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -586,7 +586,7 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
 	 */
 	acpi_disable_all_gpes();
 	/* Allow EC transactions to happen. */
-	acpi_ec_unblock_transactions_early();
+	acpi_ec_unblock_transactions();
 
 	suspend_nvs_restore();
 
@@ -784,7 +784,7 @@ static void acpi_hibernation_leave(void)
 	/* Restore the NVS memory area */
 	suspend_nvs_restore();
 	/* Allow EC transactions to happen. */
-	acpi_ec_unblock_transactions_early();
+	acpi_ec_unblock_transactions();
 }
 
 static void acpi_pm_thaw(void)
-- 
1.7.10

RE: [PATCH v2 2/2] ACPI / EC: Add PM operations to block event handling

[Zheng, Lv]

Hi, Rafael

This patch in fact contains many fixes.
I'll split this patch into several small patches to make it easier for the reviewers.
Thus I'll re-send this patch using a separate series and re-send only patch 1 as v3 to this thread.
Sorry for the noise.

Thanks and best regards
-Lv

> From: Zheng, Lv
> Sent: Friday, July 29, 2016 6:06 PM
> Subject: [PATCH v2 2/2] ACPI / EC: Add PM operations to block event
> handling
> 
> Originally, EC driver stops handling both events and transactions in
> acpi_ec_block_transactions(), and restarts to handle transactions in
> acpi_ec_unblock_transactions_early(), restarts to handle both events and
> transactions in acpi_ec_unblock_transactions().
> 
> Thus, the event handling is actually stopped after the IRQ is disabled, but
> the EC driver is not capable of handling SCI_EVT in noirq stage, thus it is
> likely the event is not detected by the EC driver.
> 
> This patch tries to restore the old behavior for the EC driver. However,
> do we actually need to handle EC events during suspend/resume stage? EC
> events are mostly useless for the suspend/resume period (key strokes and
> battery/thermal updates, etc.,), and the useful ones (lid close,
> power/sleep button press) should have already been delivered to OS to
> trigger the power saving operations. Thus EC driver should stop handling
> events during this period, just like what the EC driver does during the
> boot stage. And tests show this behavior is working and can make suspend
> even faster when many events is triggered during this stage (for example,
> during this stage, frequently trigger wifi switches).
> 
> OTOH, delivering EC events too early may confuse drivers because when
> the
> drivers see the events, the drivers themselves may not have been resumed.
> 
> Thus this patch implements PM hooks, stops to handle event in .suspend()
> hook and restarts to handle event in .resume() hook. This is different
> from the original implementation, enlarging the event handling blocking
> period longer:
>                         Original        Current
> suspend before EC       Y               Y
> suspend after EC        Y               N
> suspend_late            Y               N
> suspend_noirq           Y (actually N)  N
> resume_noirq            Y (actually N)  N
> resume_late             Y               N
> resume before EC        Y               N
> resume after EC         Y               Y
> Since this is experimental, a boot parameter is prepared to not to
> disable event handling during suspend/resume period.
> 
> By implementing .resume() hook to re-enable the event handling, the
> following 2 APIs serve for the same purpose (restart transactions) and can
> be combined:
> acpi_ec_unblock_transactions_early()/acpi_ec_unblock_transactions().
> 
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> Tested-by: Todd E Brandt <todd.e.brandt@linux.intel.com>
> ---
>  drivers/acpi/ec.c       |  146 ++++++++++++++++++++++++++++++++-------
> --------
>  drivers/acpi/internal.h |    1 -
>  drivers/acpi/sleep.c    |    4 +-
>  3 files changed, 103 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index 7cdcdf6..8c3034c 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -104,6 +104,7 @@ enum ec_command {
>  #define ACPI_EC_MAX_QUERIES	16	/* Maximum number of
> parallel queries */
> 
>  enum {
> +	EC_FLAGS_QUERY_ENABLED,		/* Query is enabled */
>  	EC_FLAGS_QUERY_PENDING,		/* Query is pending */
>  	EC_FLAGS_QUERY_GUARDING,	/* Guard for SCI_EVT check
> */
>  	EC_FLAGS_GPE_HANDLER_INSTALLED,	/* GPE handler installed */
> @@ -145,6 +146,10 @@ static unsigned int ec_storm_threshold
> __read_mostly = 8;
>  module_param(ec_storm_threshold, uint, 0644);
>  MODULE_PARM_DESC(ec_storm_threshold, "Maxim false GPE numbers
> not considered as GPE storm");
> 
> +static bool ec_freeze_events __read_mostly = true;
> +module_param(ec_freeze_events, bool, 0644);
> +MODULE_PARM_DESC(ec_freeze_events, "Disabling event handling
> during suspend/resume");
> +
>  struct acpi_ec_query_handler {
>  	struct list_head node;
>  	acpi_ec_query_func func;
> @@ -427,13 +432,19 @@ static bool
> acpi_ec_submit_flushable_request(struct acpi_ec *ec)
>  	return true;
>  }
> 
> +static void __acpi_ec_submit_query(struct acpi_ec *ec)
> +{
> +	ec_dbg_evt("Command(%s) submitted/blocked",
> +		   acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
> +	ec->nr_pending_queries++;
> +	schedule_work(&ec->work);
> +}
> +
>  static void acpi_ec_submit_query(struct acpi_ec *ec)
>  {
>  	if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
> -		ec_dbg_evt("Command(%s) submitted/blocked",
> -
> acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
> -		ec->nr_pending_queries++;
> -		schedule_work(&ec->work);
> +		if (test_bit(EC_FLAGS_QUERY_ENABLED, &ec->flags))
> +			__acpi_ec_submit_query(ec);
>  	}
>  }
> 
> @@ -446,6 +457,70 @@ static void acpi_ec_complete_query(struct
> acpi_ec *ec)
>  	}
>  }
> 
> +static bool acpi_ec_query_flushed(struct acpi_ec *ec)
> +{
> +	bool flushed;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ec->lock, flags);
> +	flushed = !ec->nr_pending_queries;
> +	spin_unlock_irqrestore(&ec->lock, flags);
> +
> +	return flushed;
> +}
> +
> +/*
> + * Process _Q events that might have accumulated in the EC.
> + * Run with locked ec mutex.
> + */
> +static void acpi_ec_clear(struct acpi_ec *ec)
> +{
> +	int i, status;
> +	u8 value = 0;
> +
> +	for (i = 0; i < ACPI_EC_CLEAR_MAX; i++) {
> +		status = acpi_ec_query(ec, &value);
> +		if (status || !value)
> +			break;
> +	}
> +	if (unlikely(i == ACPI_EC_CLEAR_MAX))
> +		pr_warn("Warning: Maximum of %d stale EC events
> cleared\n", i);
> +	else
> +		pr_info("%d stale EC events cleared\n", i);
> +}
> +
> +static void acpi_ec_disable_event(struct acpi_ec *ec, bool flushing)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ec->lock, flags);
> +	clear_bit(EC_FLAGS_QUERY_ENABLED, &ec->flags);
> +	ec_log_drv("event blocked");
> +	spin_unlock_irqrestore(&ec->lock, flags);
> +	if (flushing && ec_query_wq) {
> +		wait_event(ec->wait, acpi_ec_query_flushed(ec));
> +		flush_workqueue(ec_query_wq);
> +	}
> +}
> +
> +static void acpi_ec_enable_event(struct acpi_ec *ec)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ec->lock, flags);
> +	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))
> +		__acpi_ec_submit_query(ec);
> +	else
> +		advance_transaction(ec);
> +	spin_unlock_irqrestore(&ec->lock, flags);
> +
> +	/* Drain additional events if hardware requires that */
> +	if (EC_FLAGS_CLEAR_ON_RESUME)
> +		acpi_ec_clear(ec);
> +}
> +
>  static bool acpi_ec_guard_event(struct acpi_ec *ec)
>  {
>  	bool guarded = true;
> @@ -832,27 +907,6 @@ acpi_handle ec_get_handle(void)
>  }
>  EXPORT_SYMBOL(ec_get_handle);
> 
> -/*
> - * Process _Q events that might have accumulated in the EC.
> - * Run with locked ec mutex.
> - */
> -static void acpi_ec_clear(struct acpi_ec *ec)
> -{
> -	int i, status;
> -	u8 value = 0;
> -
> -	for (i = 0; i < ACPI_EC_CLEAR_MAX; i++) {
> -		status = acpi_ec_query(ec, &value);
> -		if (status || !value)
> -			break;
> -	}
> -
> -	if (unlikely(i == ACPI_EC_CLEAR_MAX))
> -		pr_warn("Warning: Maximum of %d stale EC events
> cleared\n", i);
> -	else
> -		pr_info("%d stale EC events cleared\n", i);
> -}
> -
>  static void acpi_ec_start(struct acpi_ec *ec, bool resuming)
>  {
>  	unsigned long flags;
> @@ -919,20 +973,6 @@ void acpi_ec_block_transactions(void)
> 
>  void acpi_ec_unblock_transactions(void)
>  {
> -	struct acpi_ec *ec = first_ec;
> -
> -	if (!ec)
> -		return;
> -
> -	/* Allow transactions to be carried out again */
> -	acpi_ec_start(ec, true);
> -
> -	if (EC_FLAGS_CLEAR_ON_RESUME)
> -		acpi_ec_clear(ec);
> -}
> -
> -void acpi_ec_unblock_transactions_early(void)
> -{
>  	/*
>  	 * Allow transactions to happen again (this function is called from
>  	 * atomic context during wakeup, so we don't need to acquire the
> mutex).
> @@ -1234,13 +1274,13 @@ static struct acpi_ec *make_acpi_ec(void)
> 
>  	if (!ec)
>  		return NULL;
> -	ec->flags = 1 << EC_FLAGS_QUERY_PENDING;
>  	mutex_init(&ec->mutex);
>  	init_waitqueue_head(&ec->wait);
>  	INIT_LIST_HEAD(&ec->list);
>  	spin_lock_init(&ec->lock);
>  	INIT_WORK(&ec->work, acpi_ec_event_handler);
>  	ec->timestamp = jiffies;
> +	acpi_ec_disable_event(ec, false);
>  	return ec;
>  }
> 
> @@ -1421,11 +1461,7 @@ static int acpi_ec_add(struct acpi_device
> *device)
>  	acpi_walk_dep_device_list(ec->handle);
> 
>  	/* EC is fully operational, allow queries */
> -	clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
> -
> -	/* Clear stale _Q events if hardware might require that */
> -	if (EC_FLAGS_CLEAR_ON_RESUME)
> -		acpi_ec_clear(ec);
> +	acpi_ec_enable_event(ec);
>  	return ret;
>  }
> 
> @@ -1665,10 +1701,30 @@ static int acpi_ec_resume_noirq(struct
> device *dev)
>  	acpi_ec_leave_noirq(ec);
>  	return 0;
>  }
> +
> +static int acpi_ec_suspend(struct device *dev)
> +{
> +	struct acpi_ec *ec =
> +		acpi_driver_data(to_acpi_device(dev));
> +
> +	if (ec_freeze_events)
> +		acpi_ec_disable_event(ec, true);
> +	return 0;
> +}
> +
> +static int acpi_ec_resume(struct device *dev)
> +{
> +	struct acpi_ec *ec =
> +		acpi_driver_data(to_acpi_device(dev));
> +
> +	acpi_ec_enable_event(ec);
> +	return 0;
> +}
>  #endif
> 
>  static const struct dev_pm_ops acpi_ec_pm = {
>  	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(acpi_ec_suspend_noirq,
> acpi_ec_resume_noirq)
> +	SET_SYSTEM_SLEEP_PM_OPS(acpi_ec_suspend, acpi_ec_resume)
>  };
> 
>  static int param_set_event_clearing(const char *val, struct kernel_param
> *kp)
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index 6996121..29f2063 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -189,7 +189,6 @@ int acpi_ec_ecdt_probe(void);
>  int acpi_ec_dsdt_probe(void);
>  void acpi_ec_block_transactions(void);
>  void acpi_ec_unblock_transactions(void);
> -void acpi_ec_unblock_transactions_early(void);
>  int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
>  			      acpi_handle handle, acpi_ec_query_func func,
>  			      void *data);
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 9788663..deb0ff7 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -586,7 +586,7 @@ static int acpi_suspend_enter(suspend_state_t
> pm_state)
>  	 */
>  	acpi_disable_all_gpes();
>  	/* Allow EC transactions to happen. */
> -	acpi_ec_unblock_transactions_early();
> +	acpi_ec_unblock_transactions();
> 
>  	suspend_nvs_restore();
> 
> @@ -784,7 +784,7 @@ static void acpi_hibernation_leave(void)
>  	/* Restore the NVS memory area */
>  	suspend_nvs_restore();
>  	/* Allow EC transactions to happen. */
> -	acpi_ec_unblock_transactions_early();
> +	acpi_ec_unblock_transactions();
>  }
> 
>  static void acpi_pm_thaw(void)
> --
> 1.7.10

[PATCH v3] ACPI / EC: Add PM operations to tune polling mode efficiency for suspend/resume noirq stage

[Lv Zheng]

It is reported that on some platforms, resume speed is not fast. The cause
is: in noirq stage, EC driver is working in polling mode, and each state
machine advancement requires a context switch.

The context switch is not necessary to the EC driver's polling mode. This
patch implements PM hooks to automatically switch the driver to/from the
busy polling mode to eliminate the overhead caused by the context switch.

This finally contributes to the tuning result: acpi_pm_finish() execution
time is improved from 192ms to 6ms.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Reported-and-tested-by: Todd E Brandt <todd.e.brandt@linux.intel.com>
---
 drivers/acpi/ec.c       |   53 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/internal.h |    2 ++
 2 files changed, 55 insertions(+)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index e7bd57c..6f6c7d1 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1619,6 +1619,58 @@ error:
 	return ret;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static void acpi_ec_enter_noirq(struct acpi_ec *ec)
+{
+	unsigned long flags;
+
+	if (ec == first_ec) {
+		spin_lock_irqsave(&ec->lock, flags);
+		ec->saved_busy_polling = ec_busy_polling;
+		ec->saved_polling_guard = ec_polling_guard;
+		ec_busy_polling = true;
+		ec_polling_guard = 0;
+		ec_log_drv("interrupt blocked");
+		spin_unlock_irqrestore(&ec->lock, flags);
+	}
+}
+
+static void acpi_ec_leave_noirq(struct acpi_ec *ec)
+{
+	unsigned long flags;
+
+	if (ec == first_ec) {
+		spin_lock_irqsave(&ec->lock, flags);
+		ec_busy_polling = ec->saved_busy_polling;
+		ec_polling_guard = ec->saved_polling_guard;
+		ec_log_drv("interrupt unblocked");
+		spin_unlock_irqrestore(&ec->lock, flags);
+	}
+}
+
+static int acpi_ec_suspend_noirq(struct device *dev)
+{
+	struct acpi_ec *ec =
+		acpi_driver_data(to_acpi_device(dev));
+
+	acpi_ec_enter_noirq(ec);
+	return 0;
+}
+
+static int acpi_ec_resume_noirq(struct device *dev)
+{
+	struct acpi_ec *ec =
+		acpi_driver_data(to_acpi_device(dev));
+
+	acpi_ec_leave_noirq(ec);
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops acpi_ec_pm = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(acpi_ec_suspend_noirq, acpi_ec_resume_noirq)
+};
+
 static int param_set_event_clearing(const char *val, struct kernel_param *kp)
 {
 	int result = 0;
@@ -1664,6 +1716,7 @@ static struct acpi_driver acpi_ec_driver = {
 		.add = acpi_ec_add,
 		.remove = acpi_ec_remove,
 		},
+	.drv.pm = &acpi_ec_pm,
 };
 
 static inline int acpi_ec_query_init(void)
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 940218f..6996121 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -174,6 +174,8 @@ struct acpi_ec {
 	struct work_struct work;
 	unsigned long timestamp;
 	unsigned long nr_pending_queries;
+	bool saved_busy_polling;
+	unsigned int saved_polling_guard;
 };
 
 extern struct acpi_ec *first_ec;
-- 
1.7.10

[PATCH v3 0/5] ACPI / EC: Move the event handling out of the noirq stage

[Lv Zheng]

There is a known issue in the EC driver.

1. During the suspend/resume process, the EC driver is required to work
   even in the noirq stage. And currently the event handling is
   disabled/enabled in the noirq stage.
2. Unfortunately, the EC driver only implements the polling mode for
   handling the transactions, and is not prepared to handle events in the
   polling mode.
The result of the above conflict is: if an SCI_EVT is indicated during the
noirq stage (especially after resuming), the EC driver is not able to
detect the SCI_EVT unless:
1. GPE STS not cleared: When the GPE is enabled, advance_transaction() will
                        be invoked to poll SCI_EVT.
2. EC transaction occurred: When an EC transaction ocurred,
                            advance_transaction() will be invoked in the
                            polling mode.
If an SCI_EVT ocurred during noirq stage and the above 2 triggering sources
cannot handle it, then a problem could be seen by the users:
 If SCI_EVT is left set after resuming and there is still no EC
 transactions, this event cannot be handled.

This patchset tries to disable/enable the event handling out of the noirq
stage to fix this issue.

Though it is always correct to enable the event handling in a later stage
during resuming as long as the event won't be lost, it may not be correct
to disable the event handling in an earlier stage during suspending. This
patchset thus put a boot parameter for the suspending part tuning in order
to be able to respond to the possible regressions.

Since the event handling is disabled for a longer period during
suspend/resume, this patchset can also tune the suspend/resume speed
faster.

Lv Zheng (5):
  ACPI / EC: Add EC_FLAGS_QUERY_ENABLED to reveal a hidden logic
  ACPI / EC: Fix an issue that SCI_EVT cannot be detected after event
    is enabled
  ACPI / EC: Add PM operations to improve event handling for resume
    process
  ACPI / EC: Add PM operations to improve event handling for suspend
    process
  ACPI / EC: Enable event freeze mode to improve event handling
    efficiency for suspend process

 drivers/acpi/ec.c       |  177 +++++++++++++++++++++++++++++++++++------------
 drivers/acpi/internal.h |    1 -
 drivers/acpi/sleep.c    |    4 +-
 3 files changed, 136 insertions(+), 46 deletions(-)

-- 
1.7.10

[PATCH v3 0/5] ACPI / EC: Move the event handling out of the noirq stage

[Lv Zheng]

There is a known issue in the EC driver.

1. During the suspend/resume process, the EC driver is required to work
   even in the noirq stage. And currently the event handling is
   disabled/enabled in the noirq stage.
2. Unfortunately, the EC driver only implements the polling mode for
   handling the transactions, and is not prepared to handle events in the
   polling mode.
The result of the above conflict is: if an SCI_EVT is indicated during the
noirq stage (especially after resuming), the EC driver is not able to
detect the SCI_EVT unless:
1. GPE STS not cleared: When the GPE is enabled, advance_transaction() will
                        be invoked to poll SCI_EVT.
2. EC transaction occurred: When an EC transaction ocurred,
                            advance_transaction() will be invoked in the
                            polling mode.
If an SCI_EVT ocurred during noirq stage and the above 2 triggering sources
cannot handle it, then a problem could be seen by the users:
 If SCI_EVT is left set after resuming and there is still no EC
 transactions, this event cannot be handled.

This patchset tries to disable/enable the event handling out of the noirq
stage to fix this issue.

Though it is always correct to enable the event handling in a later stage
during resuming as long as the event won't be lost, it may not be correct
to disable the event handling in an earlier stage during suspending. This
patchset thus put a boot parameter for the suspending part tuning in order
to be able to respond to the possible regressions.

Since the event handling is disabled for a longer period during
suspend/resume, this patchset can also tune the suspend/resume speed
faster.

Lv Zheng (5):
  ACPI / EC: Add EC_FLAGS_QUERY_ENABLED to reveal a hidden logic
  ACPI / EC: Fix an issue that SCI_EVT cannot be detected after event
    is enabled
  ACPI / EC: Add PM operations to improve event handling for resume
    process
  ACPI / EC: Add PM operations to improve event handling for suspend
    process
  ACPI / EC: Enable event freeze mode to improve event handling
    efficiency for suspend process

 drivers/acpi/ec.c       |  177 +++++++++++++++++++++++++++++++++++------------
 drivers/acpi/internal.h |    1 -
 drivers/acpi/sleep.c    |    4 +-
 3 files changed, 136 insertions(+), 46 deletions(-)

-- 
1.7.10

[PATCH v3 1/5] ACPI / EC: Add EC_FLAGS_QUERY_ENABLED to reveal a hidden logic

[Lv Zheng]

There is a hidden logic in the EC driver:
1. During boot, EC_FLAGS_QUERY_PENDING is responsible for blocking event
   handling;
2. During suspend, EC_FLAGS_STARTED is responsible for blocking event
   handling.
This patch uses a new EC_FLAGS_QUERY_ENABLED flag to make this hidden
logic explicit and have code cleaned up. No functional change.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Tested-by: Todd E Brandt <todd.e.brandt@linux.intel.com>
---
 drivers/acpi/ec.c |  103 ++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 71 insertions(+), 32 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 6f6c7d1..4ab34d7 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -104,6 +104,7 @@ enum ec_command {
 #define ACPI_EC_MAX_QUERIES	16	/* Maximum number of parallel queries */
 
 enum {
+	EC_FLAGS_QUERY_ENABLED,		/* Query is enabled */
 	EC_FLAGS_QUERY_PENDING,		/* Query is pending */
 	EC_FLAGS_QUERY_GUARDING,	/* Guard for SCI_EVT check */
 	EC_FLAGS_GPE_HANDLER_INSTALLED,	/* GPE handler installed */
@@ -239,6 +240,22 @@ static bool acpi_ec_started(struct acpi_ec *ec)
 	       !test_bit(EC_FLAGS_STOPPED, &ec->flags);
 }
 
+static bool acpi_ec_event_enabled(struct acpi_ec *ec)
+{
+	/*
+	 * There is an OSPM early stage logic. During the early stages
+	 * (boot/resume), OSPMs shouldn't enable the event handling, only
+	 * the EC transactions are allowed to be performed.
+	 */
+	if (!test_bit(EC_FLAGS_QUERY_ENABLED, &ec->flags))
+		return false;
+	/*
+	 * The EC event handling is automatically disabled as soon as the
+	 * EC driver is stopped.
+	 */
+	return test_bit(EC_FLAGS_STARTED, &ec->flags);
+}
+
 static bool acpi_ec_flushed(struct acpi_ec *ec)
 {
 	return ec->reference_count == 1;
@@ -429,7 +446,8 @@ static bool acpi_ec_submit_flushable_request(struct acpi_ec *ec)
 
 static void acpi_ec_submit_query(struct acpi_ec *ec)
 {
-	if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
+	if (acpi_ec_event_enabled(ec) &&
+	    !test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
 		ec_dbg_evt("Command(%s) submitted/blocked",
 			   acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
 		ec->nr_pending_queries++;
@@ -446,6 +464,52 @@ static void acpi_ec_complete_query(struct acpi_ec *ec)
 	}
 }
 
+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");
+}
+
+static inline void __acpi_ec_disable_event(struct acpi_ec *ec)
+{
+	if (test_and_clear_bit(EC_FLAGS_QUERY_ENABLED, &ec->flags))
+		ec_log_drv("event blocked");
+}
+
+/*
+ * Process _Q events that might have accumulated in the EC.
+ * Run with locked ec mutex.
+ */
+static void acpi_ec_clear(struct acpi_ec *ec)
+{
+	int i, status;
+	u8 value = 0;
+
+	for (i = 0; i < ACPI_EC_CLEAR_MAX; i++) {
+		status = acpi_ec_query(ec, &value);
+		if (status || !value)
+			break;
+	}
+	if (unlikely(i == ACPI_EC_CLEAR_MAX))
+		pr_warn("Warning: Maximum of %d stale EC events cleared\n", i);
+	else
+		pr_info("%d stale EC events cleared\n", i);
+}
+
+static void acpi_ec_enable_event(struct acpi_ec *ec)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&ec->lock, flags);
+	if (acpi_ec_started(ec))
+		__acpi_ec_enable_event(ec);
+	spin_unlock_irqrestore(&ec->lock, flags);
+
+	/* Drain additional events if hardware requires that */
+	if (EC_FLAGS_CLEAR_ON_RESUME)
+		acpi_ec_clear(ec);
+}
+
 static bool acpi_ec_guard_event(struct acpi_ec *ec)
 {
 	bool guarded = true;
@@ -832,27 +896,6 @@ acpi_handle ec_get_handle(void)
 }
 EXPORT_SYMBOL(ec_get_handle);
 
-/*
- * Process _Q events that might have accumulated in the EC.
- * Run with locked ec mutex.
- */
-static void acpi_ec_clear(struct acpi_ec *ec)
-{
-	int i, status;
-	u8 value = 0;
-
-	for (i = 0; i < ACPI_EC_CLEAR_MAX; i++) {
-		status = acpi_ec_query(ec, &value);
-		if (status || !value)
-			break;
-	}
-
-	if (unlikely(i == ACPI_EC_CLEAR_MAX))
-		pr_warn("Warning: Maximum of %d stale EC events cleared\n", i);
-	else
-		pr_info("%d stale EC events cleared\n", i);
-}
-
 static void acpi_ec_start(struct acpi_ec *ec, bool resuming)
 {
 	unsigned long flags;
@@ -864,7 +907,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
+			__acpi_ec_enable_event(ec);
 		ec_log_drv("EC started");
 	}
 	spin_unlock_irqrestore(&ec->lock, flags);
@@ -896,7 +940,8 @@ 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
+			__acpi_ec_disable_event(ec);
 		clear_bit(EC_FLAGS_STARTED, &ec->flags);
 		clear_bit(EC_FLAGS_STOPPED, &ec->flags);
 		ec_log_drv("EC stopped");
@@ -927,8 +972,7 @@ void acpi_ec_unblock_transactions(void)
 	/* Allow transactions to be carried out again */
 	acpi_ec_start(ec, true);
 
-	if (EC_FLAGS_CLEAR_ON_RESUME)
-		acpi_ec_clear(ec);
+	acpi_ec_enable_event(ec);
 }
 
 void acpi_ec_unblock_transactions_early(void)
@@ -1234,7 +1278,6 @@ static struct acpi_ec *make_acpi_ec(void)
 
 	if (!ec)
 		return NULL;
-	ec->flags = 1 << EC_FLAGS_QUERY_PENDING;
 	mutex_init(&ec->mutex);
 	init_waitqueue_head(&ec->wait);
 	INIT_LIST_HEAD(&ec->list);
@@ -1421,11 +1464,7 @@ static int acpi_ec_add(struct acpi_device *device)
 	acpi_walk_dep_device_list(ec->handle);
 
 	/* EC is fully operational, allow queries */
-	clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
-
-	/* Clear stale _Q events if hardware might require that */
-	if (EC_FLAGS_CLEAR_ON_RESUME)
-		acpi_ec_clear(ec);
+	acpi_ec_enable_event(ec);
 	return ret;
 }
 
-- 
1.7.10

[PATCH v3 1/5] ACPI / EC: Add EC_FLAGS_QUERY_ENABLED to reveal a hidden logic

[Lv Zheng]

There is a hidden logic in the EC driver:
1. During boot, EC_FLAGS_QUERY_PENDING is responsible for blocking event
   handling;
2. During suspend, EC_FLAGS_STARTED is responsible for blocking event
   handling.
This patch uses a new EC_FLAGS_QUERY_ENABLED flag to make this hidden
logic explicit and have code cleaned up. No functional change.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Tested-by: Todd E Brandt <todd.e.brandt@linux.intel.com>
---
 drivers/acpi/ec.c |  103 ++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 71 insertions(+), 32 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 6f6c7d1..4ab34d7 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -104,6 +104,7 @@ enum ec_command {
 #define ACPI_EC_MAX_QUERIES	16	/* Maximum number of parallel queries */
 
 enum {
+	EC_FLAGS_QUERY_ENABLED,		/* Query is enabled */
 	EC_FLAGS_QUERY_PENDING,		/* Query is pending */
 	EC_FLAGS_QUERY_GUARDING,	/* Guard for SCI_EVT check */
 	EC_FLAGS_GPE_HANDLER_INSTALLED,	/* GPE handler installed */
@@ -239,6 +240,22 @@ static bool acpi_ec_started(struct acpi_ec *ec)
 	       !test_bit(EC_FLAGS_STOPPED, &ec->flags);
 }
 
+static bool acpi_ec_event_enabled(struct acpi_ec *ec)
+{
+	/*
+	 * There is an OSPM early stage logic. During the early stages
+	 * (boot/resume), OSPMs shouldn't enable the event handling, only
+	 * the EC transactions are allowed to be performed.
+	 */
+	if (!test_bit(EC_FLAGS_QUERY_ENABLED, &ec->flags))
+		return false;
+	/*
+	 * The EC event handling is automatically disabled as soon as the
+	 * EC driver is stopped.
+	 */
+	return test_bit(EC_FLAGS_STARTED, &ec->flags);
+}
+
 static bool acpi_ec_flushed(struct acpi_ec *ec)
 {
 	return ec->reference_count == 1;
@@ -429,7 +446,8 @@ static bool acpi_ec_submit_flushable_request(struct acpi_ec *ec)
 
 static void acpi_ec_submit_query(struct acpi_ec *ec)
 {
-	if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
+	if (acpi_ec_event_enabled(ec) &&
+	    !test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
 		ec_dbg_evt("Command(%s) submitted/blocked",
 			   acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
 		ec->nr_pending_queries++;
@@ -446,6 +464,52 @@ static void acpi_ec_complete_query(struct acpi_ec *ec)
 	}
 }
 
+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");
+}
+
+static inline void __acpi_ec_disable_event(struct acpi_ec *ec)
+{
+	if (test_and_clear_bit(EC_FLAGS_QUERY_ENABLED, &ec->flags))
+		ec_log_drv("event blocked");
+}
+
+/*
+ * Process _Q events that might have accumulated in the EC.
+ * Run with locked ec mutex.
+ */
+static void acpi_ec_clear(struct acpi_ec *ec)
+{
+	int i, status;
+	u8 value = 0;
+
+	for (i = 0; i < ACPI_EC_CLEAR_MAX; i++) {
+		status = acpi_ec_query(ec, &value);
+		if (status || !value)
+			break;
+	}
+	if (unlikely(i == ACPI_EC_CLEAR_MAX))
+		pr_warn("Warning: Maximum of %d stale EC events cleared\n", i);
+	else
+		pr_info("%d stale EC events cleared\n", i);
+}
+
+static void acpi_ec_enable_event(struct acpi_ec *ec)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&ec->lock, flags);
+	if (acpi_ec_started(ec))
+		__acpi_ec_enable_event(ec);
+	spin_unlock_irqrestore(&ec->lock, flags);
+
+	/* Drain additional events if hardware requires that */
+	if (EC_FLAGS_CLEAR_ON_RESUME)
+		acpi_ec_clear(ec);
+}
+
 static bool acpi_ec_guard_event(struct acpi_ec *ec)
 {
 	bool guarded = true;
@@ -832,27 +896,6 @@ acpi_handle ec_get_handle(void)
 }
 EXPORT_SYMBOL(ec_get_handle);
 
-/*
- * Process _Q events that might have accumulated in the EC.
- * Run with locked ec mutex.
- */
-static void acpi_ec_clear(struct acpi_ec *ec)
-{
-	int i, status;
-	u8 value = 0;
-
-	for (i = 0; i < ACPI_EC_CLEAR_MAX; i++) {
-		status = acpi_ec_query(ec, &value);
-		if (status || !value)
-			break;
-	}
-
-	if (unlikely(i == ACPI_EC_CLEAR_MAX))
-		pr_warn("Warning: Maximum of %d stale EC events cleared\n", i);
-	else
-		pr_info("%d stale EC events cleared\n", i);
-}
-
 static void acpi_ec_start(struct acpi_ec *ec, bool resuming)
 {
 	unsigned long flags;
@@ -864,7 +907,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
+			__acpi_ec_enable_event(ec);
 		ec_log_drv("EC started");
 	}
 	spin_unlock_irqrestore(&ec->lock, flags);
@@ -896,7 +940,8 @@ 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
+			__acpi_ec_disable_event(ec);
 		clear_bit(EC_FLAGS_STARTED, &ec->flags);
 		clear_bit(EC_FLAGS_STOPPED, &ec->flags);
 		ec_log_drv("EC stopped");
@@ -927,8 +972,7 @@ void acpi_ec_unblock_transactions(void)
 	/* Allow transactions to be carried out again */
 	acpi_ec_start(ec, true);
 
-	if (EC_FLAGS_CLEAR_ON_RESUME)
-		acpi_ec_clear(ec);
+	acpi_ec_enable_event(ec);
 }
 
 void acpi_ec_unblock_transactions_early(void)
@@ -1234,7 +1278,6 @@ static struct acpi_ec *make_acpi_ec(void)
 
 	if (!ec)
 		return NULL;
-	ec->flags = 1 << EC_FLAGS_QUERY_PENDING;
 	mutex_init(&ec->mutex);
 	init_waitqueue_head(&ec->wait);
 	INIT_LIST_HEAD(&ec->list);
@@ -1421,11 +1464,7 @@ static int acpi_ec_add(struct acpi_device *device)
 	acpi_walk_dep_device_list(ec->handle);
 
 	/* EC is fully operational, allow queries */
-	clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
-
-	/* Clear stale _Q events if hardware might require that */
-	if (EC_FLAGS_CLEAR_ON_RESUME)
-		acpi_ec_clear(ec);
+	acpi_ec_enable_event(ec);
 	return ret;
 }
 
-- 
1.7.10

[PATCH v3 2/5] ACPI / EC: Fix an issue that SCI_EVT cannot be detected after event is enabled

[Lv Zheng]

After enabling the EC event handling, Linux is still in the noirq stage, if
there is no triggering source (EC transaction, GPE STS status),
advance_transaction() will not be invoked and SCI_EVT cannot be detected.
This patch adds one more triggering source after enabling the EC event
handling to poll the pending SCI_EVT.

Known issues:
1. Still no SCI_EVT triggering source
   There could still be no SCI_EVT triggering source after handling the
   first SCI_EVT (polled by this patch if any). Because after handling the
   first SCI_EVT, Linux could still be in noirq stage and there could still
   be no further triggering source in this stage. Then the second SCI_EVT
   indicated during this stage still cannot be detected by the EC driver.
   With this improvement applied, it is then possible to move
   acpi_ec_enable_event() out of the noirq stage to fix this issue (if the
   first SCI_EVT is handled out of the noirq stage, the follow-up SCI_EVTs
   should be able to trigger IRQs).

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Tested-by: Todd E Brandt <todd.e.brandt@linux.intel.com>
---
 drivers/acpi/ec.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 4ab34d7..79305be 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -468,6 +468,8 @@ 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);
 }
 
 static inline void __acpi_ec_disable_event(struct acpi_ec *ec)
-- 
1.7.10

[PATCH v3 2/5] ACPI / EC: Fix an issue that SCI_EVT cannot be detected after event is enabled

[Lv Zheng]

After enabling the EC event handling, Linux is still in the noirq stage, if
there is no triggering source (EC transaction, GPE STS status),
advance_transaction() will not be invoked and SCI_EVT cannot be detected.
This patch adds one more triggering source after enabling the EC event
handling to poll the pending SCI_EVT.

Known issues:
1. Still no SCI_EVT triggering source
   There could still be no SCI_EVT triggering source after handling the
   first SCI_EVT (polled by this patch if any). Because after handling the
   first SCI_EVT, Linux could still be in noirq stage and there could still
   be no further triggering source in this stage. Then the second SCI_EVT
   indicated during this stage still cannot be detected by the EC driver.
   With this improvement applied, it is then possible to move
   acpi_ec_enable_event() out of the noirq stage to fix this issue (if the
   first SCI_EVT is handled out of the noirq stage, the follow-up SCI_EVTs
   should be able to trigger IRQs).

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Tested-by: Todd E Brandt <todd.e.brandt@linux.intel.com>
---
 drivers/acpi/ec.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 4ab34d7..79305be 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -468,6 +468,8 @@ 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);
 }
 
 static inline void __acpi_ec_disable_event(struct acpi_ec *ec)
-- 
1.7.10

[PATCH v3 3/5] ACPI / EC: Add PM operations to improve event handling for resume process

[Lv Zheng]

This patch makes 2 changes:

1. Restore old behavior
Originally, EC driver stops handling both events and transactions in
acpi_ec_block_transactions(), and restarts to handle transactions in
acpi_ec_unblock_transactions_early(), restarts to handle both events and
transactions in acpi_ec_unblock_transactions().
While currently, EC driver still stops handling both events and
transactions in acpi_ec_block_transactions(), but restarts to handle both
events and transactions in acpi_ec_unblock_transactions_early().
This patch tries to restore the old behavior by dropping
__acpi_ec_enable_event() from acpi_unblock_transactions_early().

2. Improve old behavior
However this still cannot fix the real issue as both of the
acpi_ec_unblock_xxx() functions are invoked in the noirq stage. Since the
EC driver actually doesn't implement the event handling in the polling
mode, re-enabling the event handling too early in the noirq stage could
result in the problem that if there is no triggering source causing
advance_transaction() to be invoked, pending SCI_EVT cannot be detected by
the EC driver and _Qxx cannot be triggered.
It actually makes sense to restart the event handling in any point during
resuming after the noirq stage. Just like the boot stage where the event
handling is enabled in .add(), this patch further moves
acpi_ec_enable_event() to .resume(). After doing that, the following 2
functions can be combined:
acpi_ec_unblock_transactions_early()/acpi_ec_unblock_transactions().

The differences of the event handling availability between the old behavior
(this patch isn't applied) and the new behavior (this patch is applied) are
as follows:
                        !Applied        Applied
before suspend          Y               Y
suspend before EC       Y               Y
suspend after EC        Y               Y
suspend_late            Y               Y
suspend_noirq           Y (actually N)  Y (actually N)
resume_noirq            Y (actually N)  Y (actually N)
resume_late             Y (actually N)  Y (actually N)
resume before EC        Y (actually N)  Y (actually N)
resume after EC         Y (actually N)  Y
after resume            Y (actually N)  Y
Where "actually N" means if there is no triggering source, the EC driver
is actually not able to notice the pending SCI_EVT occurred in the noirq
stage. So we can clearly see that this patch has improved the situation.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Tested-by: Todd E Brandt <todd.e.brandt@linux.intel.com>
---
 drivers/acpi/ec.c       |   26 +++++++++++---------------
 drivers/acpi/internal.h |    1 -
 drivers/acpi/sleep.c    |    4 ++--
 3 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 79305be..8d5444d 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -909,8 +909,7 @@ 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
-			__acpi_ec_enable_event(ec);
+		}
 		ec_log_drv("EC started");
 	}
 	spin_unlock_irqrestore(&ec->lock, flags);
@@ -966,19 +965,6 @@ void acpi_ec_block_transactions(void)
 
 void acpi_ec_unblock_transactions(void)
 {
-	struct acpi_ec *ec = first_ec;
-
-	if (!ec)
-		return;
-
-	/* Allow transactions to be carried out again */
-	acpi_ec_start(ec, true);
-
-	acpi_ec_enable_event(ec);
-}
-
-void acpi_ec_unblock_transactions_early(void)
-{
 	/*
 	 * Allow transactions to happen again (this function is called from
 	 * atomic context during wakeup, so we don't need to acquire the mutex).
@@ -1706,10 +1692,20 @@ static int acpi_ec_resume_noirq(struct device *dev)
 	acpi_ec_leave_noirq(ec);
 	return 0;
 }
+
+static int acpi_ec_resume(struct device *dev)
+{
+	struct acpi_ec *ec =
+		acpi_driver_data(to_acpi_device(dev));
+
+	acpi_ec_enable_event(ec);
+	return 0;
+}
 #endif
 
 static const struct dev_pm_ops acpi_ec_pm = {
 	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(acpi_ec_suspend_noirq, acpi_ec_resume_noirq)
+	SET_SYSTEM_SLEEP_PM_OPS(NULL, acpi_ec_resume)
 };
 
 static int param_set_event_clearing(const char *val, struct kernel_param *kp)
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 6996121..29f2063 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -189,7 +189,6 @@ int acpi_ec_ecdt_probe(void);
 int acpi_ec_dsdt_probe(void);
 void acpi_ec_block_transactions(void);
 void acpi_ec_unblock_transactions(void);
-void acpi_ec_unblock_transactions_early(void);
 int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
 			      acpi_handle handle, acpi_ec_query_func func,
 			      void *data);
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 9788663..deb0ff7 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -586,7 +586,7 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
 	 */
 	acpi_disable_all_gpes();
 	/* Allow EC transactions to happen. */
-	acpi_ec_unblock_transactions_early();
+	acpi_ec_unblock_transactions();
 
 	suspend_nvs_restore();
 
@@ -784,7 +784,7 @@ static void acpi_hibernation_leave(void)
 	/* Restore the NVS memory area */
 	suspend_nvs_restore();
 	/* Allow EC transactions to happen. */
-	acpi_ec_unblock_transactions_early();
+	acpi_ec_unblock_transactions();
 }
 
 static void acpi_pm_thaw(void)
-- 
1.7.10

[PATCH v3 3/5] ACPI / EC: Add PM operations to improve event handling for resume process

[Lv Zheng]

This patch makes 2 changes:

1. Restore old behavior
Originally, EC driver stops handling both events and transactions in
acpi_ec_block_transactions(), and restarts to handle transactions in
acpi_ec_unblock_transactions_early(), restarts to handle both events and
transactions in acpi_ec_unblock_transactions().
While currently, EC driver still stops handling both events and
transactions in acpi_ec_block_transactions(), but restarts to handle both
events and transactions in acpi_ec_unblock_transactions_early().
This patch tries to restore the old behavior by dropping
__acpi_ec_enable_event() from acpi_unblock_transactions_early().

2. Improve old behavior
However this still cannot fix the real issue as both of the
acpi_ec_unblock_xxx() functions are invoked in the noirq stage. Since the
EC driver actually doesn't implement the event handling in the polling
mode, re-enabling the event handling too early in the noirq stage could
result in the problem that if there is no triggering source causing
advance_transaction() to be invoked, pending SCI_EVT cannot be detected by
the EC driver and _Qxx cannot be triggered.
It actually makes sense to restart the event handling in any point during
resuming after the noirq stage. Just like the boot stage where the event
handling is enabled in .add(), this patch further moves
acpi_ec_enable_event() to .resume(). After doing that, the following 2
functions can be combined:
acpi_ec_unblock_transactions_early()/acpi_ec_unblock_transactions().

The differences of the event handling availability between the old behavior
(this patch isn't applied) and the new behavior (this patch is applied) are
as follows:
                        !Applied        Applied
before suspend          Y               Y
suspend before EC       Y               Y
suspend after EC        Y               Y
suspend_late            Y               Y
suspend_noirq           Y (actually N)  Y (actually N)
resume_noirq            Y (actually N)  Y (actually N)
resume_late             Y (actually N)  Y (actually N)
resume before EC        Y (actually N)  Y (actually N)
resume after EC         Y (actually N)  Y
after resume            Y (actually N)  Y
Where "actually N" means if there is no triggering source, the EC driver
is actually not able to notice the pending SCI_EVT occurred in the noirq
stage. So we can clearly see that this patch has improved the situation.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Tested-by: Todd E Brandt <todd.e.brandt@linux.intel.com>
---
 drivers/acpi/ec.c       |   26 +++++++++++---------------
 drivers/acpi/internal.h |    1 -
 drivers/acpi/sleep.c    |    4 ++--
 3 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 79305be..8d5444d 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -909,8 +909,7 @@ 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
-			__acpi_ec_enable_event(ec);
+		}
 		ec_log_drv("EC started");
 	}
 	spin_unlock_irqrestore(&ec->lock, flags);
@@ -966,19 +965,6 @@ void acpi_ec_block_transactions(void)
 
 void acpi_ec_unblock_transactions(void)
 {
-	struct acpi_ec *ec = first_ec;
-
-	if (!ec)
-		return;
-
-	/* Allow transactions to be carried out again */
-	acpi_ec_start(ec, true);
-
-	acpi_ec_enable_event(ec);
-}
-
-void acpi_ec_unblock_transactions_early(void)
-{
 	/*
 	 * Allow transactions to happen again (this function is called from
 	 * atomic context during wakeup, so we don't need to acquire the mutex).
@@ -1706,10 +1692,20 @@ static int acpi_ec_resume_noirq(struct device *dev)
 	acpi_ec_leave_noirq(ec);
 	return 0;
 }
+
+static int acpi_ec_resume(struct device *dev)
+{
+	struct acpi_ec *ec =
+		acpi_driver_data(to_acpi_device(dev));
+
+	acpi_ec_enable_event(ec);
+	return 0;
+}
 #endif
 
 static const struct dev_pm_ops acpi_ec_pm = {
 	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(acpi_ec_suspend_noirq, acpi_ec_resume_noirq)
+	SET_SYSTEM_SLEEP_PM_OPS(NULL, acpi_ec_resume)
 };
 
 static int param_set_event_clearing(const char *val, struct kernel_param *kp)
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 6996121..29f2063 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -189,7 +189,6 @@ int acpi_ec_ecdt_probe(void);
 int acpi_ec_dsdt_probe(void);
 void acpi_ec_block_transactions(void);
 void acpi_ec_unblock_transactions(void);
-void acpi_ec_unblock_transactions_early(void);
 int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
 			      acpi_handle handle, acpi_ec_query_func func,
 			      void *data);
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 9788663..deb0ff7 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -586,7 +586,7 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
 	 */
 	acpi_disable_all_gpes();
 	/* Allow EC transactions to happen. */
-	acpi_ec_unblock_transactions_early();
+	acpi_ec_unblock_transactions();
 
 	suspend_nvs_restore();
 
@@ -784,7 +784,7 @@ static void acpi_hibernation_leave(void)
 	/* Restore the NVS memory area */
 	suspend_nvs_restore();
 	/* Allow EC transactions to happen. */
-	acpi_ec_unblock_transactions_early();
+	acpi_ec_unblock_transactions();
 }
 
 static void acpi_pm_thaw(void)
-- 
1.7.10

[PATCH v3 4/5] ACPI / EC: Add PM operations to improve event handling for suspend process

[Lv Zheng]

In the original EC driver, though the event handling is not explicitly
stopped, the EC driver is actually not able to handle events during the
noirq stage as the EC driver is not prepared to handle the EC events in the
polling mode. So if there is no advance_transaction() triggered, the EC
driver couldn't notice the EC events.
However, do we actually need to handle EC events during suspend/resume
stage? EC events are mostly useless for the suspend/resume period (key
strokes and battery/thermal updates, etc.,), and the useful ones (lid
close, power/sleep button press) should have already been delivered to the
OSPM to trigger the power saving operations.
Thus this patch implements acpi_ec_disable_event() to be a reverse call of
acpi_ec_enable_event(), with which, the EC driver is able to stop handling
the EC events in a position before entering the noirq stage.

Since there are actually 2 choices for us:
1. implement event handling in polling mode;
2. stop event handling before entering noirq stage.
And this patch only implements the second choice using .suspend() callback.
Thus this is experimental (first choice is better? or different hook
position is better?). This patch finally keeps the old behavior by default
and prepares a boot parameter to enable this feature.

The differences of the event handling availability between the old behavior
(this patch is not applied) and the new behavior (this patch is applied)
are as follows:
                        !FreezeEvents   FreezeEvents
before suspend          Y               Y
suspend before EC       Y               Y
suspend after EC        Y               N
suspend_late            Y               N
suspend_noirq           Y (actually N)  N
resume_noirq            Y (actually N)  N
resume_late             Y (actually N)  N
resume before EC        Y (actually N)  N
resume after EC         Y               Y
after resume            Y               Y
Where "actually N" means if there is no EC transactions, the EC driver
is actually not able to notice the pending events.

We can see that FreezeEvents is the only approach now can actually flush
the EC event handling with both query commands and _Qxx evaluations
flushed, other modes can only flush the EC event handling with only query
commands flushed, _Qxx evaluations occurred after stopping the EC driver
may end up failure due to the failure of the EC transaction carried out in
the _Qxx control methods.

We also can see that this feature should be able to trigger some platform
notifications later than resuming other drivers.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Tested-by: Todd E Brandt <todd.e.brandt@linux.intel.com>
---
 drivers/acpi/ec.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 59 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 8d5444d..2bec709 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -146,6 +146,10 @@ static unsigned int ec_storm_threshold  __read_mostly = 8;
 module_param(ec_storm_threshold, uint, 0644);
 MODULE_PARM_DESC(ec_storm_threshold, "Maxim false GPE numbers not considered as GPE storm");
 
+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");
+
 struct acpi_ec_query_handler {
 	struct list_head node;
 	acpi_ec_query_func func;
@@ -250,10 +254,18 @@ static bool acpi_ec_event_enabled(struct acpi_ec *ec)
 	if (!test_bit(EC_FLAGS_QUERY_ENABLED, &ec->flags))
 		return false;
 	/*
-	 * The EC event handling is automatically disabled as soon as the
-	 * EC driver is stopped.
+	 * However, disabling the event handling is experimental for late
+	 * stage (suspend), and is controlled by the boot parameter of
+	 * "ec_freeze_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.
 	 */
-	return test_bit(EC_FLAGS_STARTED, &ec->flags);
+	if (ec_freeze_events)
+		return acpi_ec_started(ec);
+	else
+		return test_bit(EC_FLAGS_STARTED, &ec->flags);
 }
 
 static bool acpi_ec_flushed(struct acpi_ec *ec)
@@ -512,6 +524,38 @@ static void acpi_ec_enable_event(struct acpi_ec *ec)
 		acpi_ec_clear(ec);
 }
 
+static bool acpi_ec_query_flushed(struct acpi_ec *ec)
+{
+	bool flushed;
+	unsigned long flags;
+
+	spin_lock_irqsave(&ec->lock, flags);
+	flushed = !ec->nr_pending_queries;
+	spin_unlock_irqrestore(&ec->lock, flags);
+	return flushed;
+}
+
+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.
+	 */
+	wait_event(ec->wait, acpi_ec_query_flushed(ec));
+	if (ec_query_wq)
+		flush_workqueue(ec_query_wq);
+}
+
+static void acpi_ec_disable_event(struct acpi_ec *ec)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&ec->lock, flags);
+	__acpi_ec_disable_event(ec);
+	spin_unlock_irqrestore(&ec->lock, flags);
+	__acpi_ec_flush_event(ec);
+}
+
 static bool acpi_ec_guard_event(struct acpi_ec *ec)
 {
 	bool guarded = true;
@@ -941,7 +985,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
+		} else if (!ec_freeze_events)
 			__acpi_ec_disable_event(ec);
 		clear_bit(EC_FLAGS_STARTED, &ec->flags);
 		clear_bit(EC_FLAGS_STOPPED, &ec->flags);
@@ -1693,6 +1737,16 @@ static int acpi_ec_resume_noirq(struct device *dev)
 	return 0;
 }
 
+static int acpi_ec_suspend(struct device *dev)
+{
+	struct acpi_ec *ec =
+		acpi_driver_data(to_acpi_device(dev));
+
+	if (ec_freeze_events)
+		acpi_ec_disable_event(ec);
+	return 0;
+}
+
 static int acpi_ec_resume(struct device *dev)
 {
 	struct acpi_ec *ec =
@@ -1705,7 +1759,7 @@ static int acpi_ec_resume(struct device *dev)
 
 static const struct dev_pm_ops acpi_ec_pm = {
 	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(acpi_ec_suspend_noirq, acpi_ec_resume_noirq)
-	SET_SYSTEM_SLEEP_PM_OPS(NULL, acpi_ec_resume)
+	SET_SYSTEM_SLEEP_PM_OPS(acpi_ec_suspend, acpi_ec_resume)
 };
 
 static int param_set_event_clearing(const char *val, struct kernel_param *kp)
-- 
1.7.10

[PATCH v3 4/5] ACPI / EC: Add PM operations to improve event handling for suspend process

[Lv Zheng]

In the original EC driver, though the event handling is not explicitly
stopped, the EC driver is actually not able to handle events during the
noirq stage as the EC driver is not prepared to handle the EC events in the
polling mode. So if there is no advance_transaction() triggered, the EC
driver couldn't notice the EC events.
However, do we actually need to handle EC events during suspend/resume
stage? EC events are mostly useless for the suspend/resume period (key
strokes and battery/thermal updates, etc.,), and the useful ones (lid
close, power/sleep button press) should have already been delivered to the
OSPM to trigger the power saving operations.
Thus this patch implements acpi_ec_disable_event() to be a reverse call of
acpi_ec_enable_event(), with which, the EC driver is able to stop handling
the EC events in a position before entering the noirq stage.

Since there are actually 2 choices for us:
1. implement event handling in polling mode;
2. stop event handling before entering noirq stage.
And this patch only implements the second choice using .suspend() callback.
Thus this is experimental (first choice is better? or different hook
position is better?). This patch finally keeps the old behavior by default
and prepares a boot parameter to enable this feature.

The differences of the event handling availability between the old behavior
(this patch is not applied) and the new behavior (this patch is applied)
are as follows:
                        !FreezeEvents   FreezeEvents
before suspend          Y               Y
suspend before EC       Y               Y
suspend after EC        Y               N
suspend_late            Y               N
suspend_noirq           Y (actually N)  N
resume_noirq            Y (actually N)  N
resume_late             Y (actually N)  N
resume before EC        Y (actually N)  N
resume after EC         Y               Y
after resume            Y               Y
Where "actually N" means if there is no EC transactions, the EC driver
is actually not able to notice the pending events.

We can see that FreezeEvents is the only approach now can actually flush
the EC event handling with both query commands and _Qxx evaluations
flushed, other modes can only flush the EC event handling with only query
commands flushed, _Qxx evaluations occurred after stopping the EC driver
may end up failure due to the failure of the EC transaction carried out in
the _Qxx control methods.

We also can see that this feature should be able to trigger some platform
notifications later than resuming other drivers.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Tested-by: Todd E Brandt <todd.e.brandt@linux.intel.com>
---
 drivers/acpi/ec.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 59 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 8d5444d..2bec709 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -146,6 +146,10 @@ static unsigned int ec_storm_threshold  __read_mostly = 8;
 module_param(ec_storm_threshold, uint, 0644);
 MODULE_PARM_DESC(ec_storm_threshold, "Maxim false GPE numbers not considered as GPE storm");
 
+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");
+
 struct acpi_ec_query_handler {
 	struct list_head node;
 	acpi_ec_query_func func;
@@ -250,10 +254,18 @@ static bool acpi_ec_event_enabled(struct acpi_ec *ec)
 	if (!test_bit(EC_FLAGS_QUERY_ENABLED, &ec->flags))
 		return false;
 	/*
-	 * The EC event handling is automatically disabled as soon as the
-	 * EC driver is stopped.
+	 * However, disabling the event handling is experimental for late
+	 * stage (suspend), and is controlled by the boot parameter of
+	 * "ec_freeze_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.
 	 */
-	return test_bit(EC_FLAGS_STARTED, &ec->flags);
+	if (ec_freeze_events)
+		return acpi_ec_started(ec);
+	else
+		return test_bit(EC_FLAGS_STARTED, &ec->flags);
 }
 
 static bool acpi_ec_flushed(struct acpi_ec *ec)
@@ -512,6 +524,38 @@ static void acpi_ec_enable_event(struct acpi_ec *ec)
 		acpi_ec_clear(ec);
 }
 
+static bool acpi_ec_query_flushed(struct acpi_ec *ec)
+{
+	bool flushed;
+	unsigned long flags;
+
+	spin_lock_irqsave(&ec->lock, flags);
+	flushed = !ec->nr_pending_queries;
+	spin_unlock_irqrestore(&ec->lock, flags);
+	return flushed;
+}
+
+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.
+	 */
+	wait_event(ec->wait, acpi_ec_query_flushed(ec));
+	if (ec_query_wq)
+		flush_workqueue(ec_query_wq);
+}
+
+static void acpi_ec_disable_event(struct acpi_ec *ec)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&ec->lock, flags);
+	__acpi_ec_disable_event(ec);
+	spin_unlock_irqrestore(&ec->lock, flags);
+	__acpi_ec_flush_event(ec);
+}
+
 static bool acpi_ec_guard_event(struct acpi_ec *ec)
 {
 	bool guarded = true;
@@ -941,7 +985,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
+		} else if (!ec_freeze_events)
 			__acpi_ec_disable_event(ec);
 		clear_bit(EC_FLAGS_STARTED, &ec->flags);
 		clear_bit(EC_FLAGS_STOPPED, &ec->flags);
@@ -1693,6 +1737,16 @@ static int acpi_ec_resume_noirq(struct device *dev)
 	return 0;
 }
 
+static int acpi_ec_suspend(struct device *dev)
+{
+	struct acpi_ec *ec =
+		acpi_driver_data(to_acpi_device(dev));
+
+	if (ec_freeze_events)
+		acpi_ec_disable_event(ec);
+	return 0;
+}
+
 static int acpi_ec_resume(struct device *dev)
 {
 	struct acpi_ec *ec =
@@ -1705,7 +1759,7 @@ static int acpi_ec_resume(struct device *dev)
 
 static const struct dev_pm_ops acpi_ec_pm = {
 	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(acpi_ec_suspend_noirq, acpi_ec_resume_noirq)
-	SET_SYSTEM_SLEEP_PM_OPS(NULL, acpi_ec_resume)
+	SET_SYSTEM_SLEEP_PM_OPS(acpi_ec_suspend, acpi_ec_resume)
 };
 
 static int param_set_event_clearing(const char *val, struct kernel_param *kp)
-- 
1.7.10

[PATCH v3 5/5] ACPI / EC: Enable event freeze mode to improve event handling for suspend process

[Lv Zheng]

This patch enables the event freeze mode, flushing the EC event handling in
.suspend() callback. This feature is experimental, if it is bisected out to
be the cause of the real issues, please report the issues to the kernel
bugzilla for further root causing and improvement.

This mode eliminates useless _Qxx handling during the power saving
operations, thus can help to tune the power saving operations faster. Tests
show that this mode can efficiently block flooding _Qxx during the suspend
process and tune the speed of the suspend faster.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Tested-by: Todd E Brandt <todd.e.brandt@linux.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 2bec709..1925589 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -146,7 +146,7 @@ static unsigned int ec_storm_threshold  __read_mostly = 8;
 module_param(ec_storm_threshold, uint, 0644);
 MODULE_PARM_DESC(ec_storm_threshold, "Maxim false GPE numbers not considered as GPE storm");
 
-static bool ec_freeze_events __read_mostly = false;
+static bool ec_freeze_events __read_mostly = true;
 module_param(ec_freeze_events, bool, 0644);
 MODULE_PARM_DESC(ec_freeze_events, "Disabling event handling during suspend/resume");
 
-- 
1.7.10

[PATCH v3 5/5] ACPI / EC: Enable event freeze mode to improve event handling for suspend process

[Lv Zheng]

This patch enables the event freeze mode, flushing the EC event handling in
.suspend() callback. This feature is experimental, if it is bisected out to
be the cause of the real issues, please report the issues to the kernel
bugzilla for further root causing and improvement.

This mode eliminates useless _Qxx handling during the power saving
operations, thus can help to tune the power saving operations faster. Tests
show that this mode can efficiently block flooding _Qxx during the suspend
process and tune the speed of the suspend faster.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Tested-by: Todd E Brandt <todd.e.brandt@linux.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 2bec709..1925589 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -146,7 +146,7 @@ static unsigned int ec_storm_threshold  __read_mostly = 8;
 module_param(ec_storm_threshold, uint, 0644);
 MODULE_PARM_DESC(ec_storm_threshold, "Maxim false GPE numbers not considered as GPE storm");
 
-static bool ec_freeze_events __read_mostly = false;
+static bool ec_freeze_events __read_mostly = true;
 module_param(ec_freeze_events, bool, 0644);
 MODULE_PARM_DESC(ec_freeze_events, "Disabling event handling during suspend/resume");
 
-- 
1.7.10

Re: [PATCH v3 0/5] ACPI / EC: Move the event handling out of the noirq stage

[Rafael J. Wysocki]

On Wednesday, August 03, 2016 04:01:09 PM Lv Zheng wrote:
> There is a known issue in the EC driver.
> 
> 1. During the suspend/resume process, the EC driver is required to work
>    even in the noirq stage. And currently the event handling is
>    disabled/enabled in the noirq stage.
> 2. Unfortunately, the EC driver only implements the polling mode for
>    handling the transactions, and is not prepared to handle events in the
>    polling mode.
> The result of the above conflict is: if an SCI_EVT is indicated during the
> noirq stage (especially after resuming), the EC driver is not able to
> detect the SCI_EVT unless:
> 1. GPE STS not cleared: When the GPE is enabled, advance_transaction() will
>                         be invoked to poll SCI_EVT.
> 2. EC transaction occurred: When an EC transaction ocurred,
>                             advance_transaction() will be invoked in the
>                             polling mode.
> If an SCI_EVT ocurred during noirq stage and the above 2 triggering sources
> cannot handle it, then a problem could be seen by the users:
>  If SCI_EVT is left set after resuming and there is still no EC
>  transactions, this event cannot be handled.
> 
> This patchset tries to disable/enable the event handling out of the noirq
> stage to fix this issue.
> 
> Though it is always correct to enable the event handling in a later stage
> during resuming as long as the event won't be lost, it may not be correct
> to disable the event handling in an earlier stage during suspending. This
> patchset thus put a boot parameter for the suspending part tuning in order
> to be able to respond to the possible regressions.
> 
> Since the event handling is disabled for a longer period during
> suspend/resume, this patchset can also tune the suspend/resume speed
> faster.
> 
> Lv Zheng (5):
>   ACPI / EC: Add EC_FLAGS_QUERY_ENABLED to reveal a hidden logic
>   ACPI / EC: Fix an issue that SCI_EVT cannot be detected after event
>     is enabled
>   ACPI / EC: Add PM operations to improve event handling for resume
>     process
>   ACPI / EC: Add PM operations to improve event handling for suspend
>     process
>   ACPI / EC: Enable event freeze mode to improve event handling
>     efficiency for suspend process

Wholes series applied.

Thanks,
Rafael