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@xxxxxxxxx>
> ---
>  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