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@xxxxxxxxx>
> Tested-by: Todd E Brandt <todd.e.brandt@xxxxxxxxxxxxxxx>
> ---
>  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