Message-ID: <1AE640813FDE7649BE1B193DEA596E883BC0932A@SHSMSX101.ccr.corp.intel.com>
Date:	Wed, 3 Aug 2016 01:03:25 +0000
From:	"Zheng, Lv" <lv.zheng@...el.com>
To:	"Wysocki, Rafael J" <rafael.j.wysocki@...el.com>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	"Brown, Len" <len.brown@...el.com>
CC:	Lv Zheng <zetalog@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>
Subject: RE: [PATCH v2 2/2] ACPI / EC: Add PM operations to block event
 handling

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@...el.com>
> Tested-by: Todd E Brandt <todd.e.brandt@...ux.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

Powered by blists - more mailing lists