Message-ID: <2277351.09XTT17U9b@vostro.rjw.lan>
Date:	Wed, 12 Nov 2014 02:16:36 +0100
From:	"Rafael J. Wysocki" <rjw@...ysocki.net>
To:	Lv Zheng <lv.zheng@...el.com>
Cc:	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	Len Brown <len.brown@...el.com>, Lv Zheng <zetalog@...il.com>,
	linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org
Subject: Re: [PATCH 5/6] ACPI/EC: Cleanup QR_SC command processing by adding a kernel thread to poll EC events.

On Monday, November 03, 2014 01:16:37 PM Lv Zheng wrote:
> There is wait code in the QR_SC command processing, which makes it not
> suitable to be put into a work queue item (see bug 82611). And there is
> case that the SCI_EVT cannot trigger GPE, though all commands have polling
> mode implemented, the event cannot be polled (see bug 77431).
> 
> So if the QR_SC command can be put into a seperate IRQ thread, then the
> work queue will not be blocked by the QR_SC command processing and we can
> also trigger polling using the thread. Using IRQ thread also allows us to
> change the EC GPE handler into the threaded IRQ model when possible.
> 
> This patch thus adds the IRQ polling thread for SCI_EVT polling and removes
> QR_SC processing work item.
> 
> The reasons why we do not put a loop in the event poller to poll event
> until the returned query value is 0:
>   Some platforms return non 0 query value even when SCI_EVT=0, if we put a
>   loop in the poller, our command flush mechanism could never execute to
>   an end thus the system suspending process could be blocked. One SCI_EVT
>   triggering one QR_EC is current logic and has been proven to be working
>   for so long time.
> 
> The reasons why it is not implemented directly using threaded IRQ are:
> 1. ACPICA doesn't support threaded IRQ as not all of the OSPMs support
>    threaded IRQ while GPE handler registerations are done through ACPICA.
> 2. The IRQ processing code need to be identical for both the IRQ handler
>    and the thread callback, while currently, though the command GPE
>    handling is ready for both IRQ and polling mode, only the event GPE is
>    is polled in the event polling thread and the command is polled in the
>    user threads.
> So we use a standalone kernel thread, if the above situations are changed
> in the future, we can easily convert the code into the threaded IRQ style.
> 
> The old EC_FLAGS_QUERY_PENDING is converted to EC_FLAGS_EVENT_ENABLED in
> this patch, so that its naming is consistent with EC_FLAGS_EVENT_PENDING.
> The original flag doesn't co-work with SCI_EVT well, this patch refines
> its usage by enforcing a event polling wakeup indication as:
>   EC_FLAGS_EVENT_ENABLED && EC_FLAGS_EVENT_PENDING
> So unless the both of the flags are set, the threaded event poller will
> not be woken up.
> 
> This patch invokes acpi_ec_submit_request() after having detected SCI_EVT
> and invokes acpi_ec_complete_request() before having the QR_EC command
> processed. This is useful for implementing GPE storm prevention for
> malicous "level triggered" SCI_EVT. But the storm prevention is not
> implemented in this patch.
> 
> Since the acpi_ec_submit_request() invoked after detecting the SCI_EVT is
> paired with acpi_ec_complete_request() invoked after completing QR_EC
> command, acpi_ec_submit_flushable_request() then need to be modified to
> allow QR_EC command to be submitted during this period to revert the
> increased reference count. This period can be determined by the event
> polling indication:
>   EC_FLAGS_EVENT_ENABLED && EC_FLAGS_EVENT_PENDING
> Without enhancing this check in acpi_ec_submit_flushable_request(), QR_EC
> command will not be executed to decrease the reference count added after
> detecting the SCI_EVT, thus the system suspending will be blocked because
> the reference count equals to 2. Such check is common for flushing code.
> 
> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=82611
> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=77431
> Signed-off-by: Lv Zheng <lv.zheng@...el.com>
> Tested-by: Ortwin Glück <odi@....ch>
> ---
>  drivers/acpi/ec.c       |  194 ++++++++++++++++++++++++++++++++++-------------
>  drivers/acpi/internal.h |    1 +
>  2 files changed, 144 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index a76794a..7089081 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -44,6 +44,7 @@
>  #include <linux/slab.h>
>  #include <linux/acpi.h>
>  #include <linux/dmi.h>
> +#include <linux/kthread.h>
>  #include <asm/io.h>
>  
>  #include "internal.h"
> @@ -75,7 +76,8 @@ enum ec_command {
>  					 * when trying to clear the EC */
>  
>  enum {
> -	EC_FLAGS_QUERY_PENDING,		/* Query is pending */
> +	EC_FLAGS_EVENT_ENABLED,		/* Event is enabled */
> +	EC_FLAGS_EVENT_PENDING,		/* Event is pending */
>  	EC_FLAGS_GPE_STORM,		/* GPE storm detected */
>  	EC_FLAGS_HANDLERS_INSTALLED,	/* Handlers for GPE and
>  					 * OpReg are installed */
> @@ -124,6 +126,7 @@ struct transaction {
>  static struct acpi_ec_query_handler *
>  acpi_ec_get_query_handler(struct acpi_ec_query_handler *handler);
>  static void acpi_ec_put_query_handler(struct acpi_ec_query_handler *handler);
> +static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data);
>  
>  struct acpi_ec *boot_ec, *first_ec;
>  EXPORT_SYMBOL(first_ec);
> @@ -149,6 +152,12 @@ static bool acpi_ec_flushed(struct acpi_ec *ec)
>  	return ec->reference_count == 1;
>  }
>  
> +static bool acpi_ec_has_pending_event(struct acpi_ec *ec)
> +{
> +	return test_bit(EC_FLAGS_EVENT_ENABLED, &ec->flags) &&
> +	       test_bit(EC_FLAGS_EVENT_PENDING, &ec->flags);
> +}
> +
>  /* --------------------------------------------------------------------------
>   *                           GPE Enhancement
>   * -------------------------------------------------------------------------- */
> @@ -177,20 +186,93 @@ static void acpi_ec_complete_request(struct acpi_ec *ec)
>   *                                      the flush operation is not in
>   *                                      progress
>   * @ec: the EC device
> + * @check_event: check whether event is pending
>   *
>   * This function must be used before taking a new action that should hold
>   * the reference count.  If this function returns false, then the action
>   * must be discarded or it will prevent the flush operation from being
>   * completed.
> + *
> + * During flushing, QR_EC command need to pass this check when there is a
> + * pending event, so that the reference count held for the pending event
> + * can be decreased by the completion of the QR_EC command.
>   */
> -static bool acpi_ec_submit_flushable_request(struct acpi_ec *ec)
> +static bool acpi_ec_submit_flushable_request(struct acpi_ec *ec,
> +					     bool check_event)
>  {
> -	if (!acpi_ec_started(ec))
> -		return false;
> +	if (!acpi_ec_started(ec)) {
> +		if (!check_event || !acpi_ec_has_pending_event(ec))
> +			return false;
> +	}
>  	acpi_ec_submit_request(ec);
>  	return true;
>  }
>  
> +static void acpi_ec_enable_event(struct acpi_ec *ec)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ec->lock, flags);
> +	/* Hold reference for pending event */
> +	if (!acpi_ec_submit_flushable_request(ec, false)) {
> +		spin_unlock_irqrestore(&ec->lock, flags);
> +		return;
> +	}
> +	set_bit(EC_FLAGS_EVENT_ENABLED, &ec->flags);
> +	if (test_bit(EC_FLAGS_EVENT_PENDING, &ec->flags)) {
> +		pr_debug("***** Event pending *****\n");
> +		wake_up_process(ec->thread);
> +		spin_unlock_irqrestore(&ec->lock, flags);
> +		return;
> +	}
> +	acpi_ec_complete_request(ec);
> +	spin_unlock_irqrestore(&ec->lock, flags);
> +}
> +
> +static void __acpi_ec_set_event(struct acpi_ec *ec)
> +{
> +	/* Hold reference for pending event */
> +	if (!acpi_ec_submit_flushable_request(ec, false))
> +		return;
> +	if (!test_and_set_bit(EC_FLAGS_EVENT_PENDING, &ec->flags)) {
> +		pr_debug("***** Event pending *****\n");
> +		if (test_bit(EC_FLAGS_EVENT_ENABLED, &ec->flags)) {
> +			wake_up_process(ec->thread);
> +			return;
> +		}
> +	}
> +	acpi_ec_complete_request(ec);
> +}
> +
> +static void __acpi_ec_complete_event(struct acpi_ec *ec)
> +{
> +	if (test_and_clear_bit(EC_FLAGS_EVENT_PENDING, &ec->flags)) {
> +		/* Unhold reference for pending event */
> +		acpi_ec_complete_request(ec);
> +		pr_debug("***** Event running *****\n");
> +	}
> +}
> +
> +int acpi_ec_wait_for_event(struct acpi_ec *ec)
> +{
> +	unsigned long flags;
> +
> +	set_current_state(TASK_INTERRUPTIBLE);
> +	while (!kthread_should_stop()) {
> +		spin_lock_irqsave(&ec->lock, flags);
> +		if (acpi_ec_has_pending_event(ec)) {
> +			spin_unlock_irqrestore(&ec->lock, flags);
> +			__set_current_state(TASK_RUNNING);
> +			return 0;
> +		}
> +		spin_unlock_irqrestore(&ec->lock, flags);
> +		schedule();
> +		set_current_state(TASK_INTERRUPTIBLE);
> +	}
> +	__set_current_state(TASK_RUNNING);
> +	return -1;
> +}
> +
>  /* --------------------------------------------------------------------------
>   *                           Transaction Management
>   * -------------------------------------------------------------------------- */
> @@ -298,7 +380,7 @@ static bool advance_transaction(struct acpi_ec *ec)
>  			t->flags |= ACPI_EC_COMMAND_COMPLETE;
>  			wakeup = true;
>  		}
> -		return wakeup;
> +		goto out;
>  	} else {
>  		if (EC_FLAGS_QUERY_HANDSHAKE &&
>  		    !(status & ACPI_EC_FLAG_SCI) &&
> @@ -312,9 +394,11 @@ static bool advance_transaction(struct acpi_ec *ec)
>  		} else if ((status & ACPI_EC_FLAG_IBF) == 0) {
>  			acpi_ec_write_cmd(ec, t->command);
>  			t->flags |= ACPI_EC_COMMAND_POLL;
> +			if (t->command == ACPI_EC_COMMAND_QUERY)
> +				__acpi_ec_complete_event(ec);
>  		} else
>  			goto err;
> -		return wakeup;
> +		goto out;
>  	}
>  err:
>  	/*
> @@ -325,6 +409,10 @@ err:
>  		if (in_interrupt() && t)
>  			++t->irq_count;
>  	}
> +out:
> +	if (status & ACPI_EC_FLAG_SCI &&
> +	    (!t || t->flags & ACPI_EC_COMMAND_COMPLETE))
> +		__acpi_ec_set_event(ec);
>  	return wakeup;
>  }
>  
> @@ -335,17 +423,6 @@ static void start_transaction(struct acpi_ec *ec)
>  	(void)advance_transaction(ec);
>  }
>  
> -static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data);
> -
> -static int ec_check_sci_sync(struct acpi_ec *ec, u8 state)
> -{
> -	if (state & ACPI_EC_FLAG_SCI) {
> -		if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags))
> -			return acpi_ec_sync_query(ec, NULL);
> -	}
> -	return 0;
> -}
> -
>  static int ec_poll(struct acpi_ec *ec)
>  {
>  	unsigned long flags;
> @@ -384,12 +461,13 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
>  	unsigned long tmp;
>  	int ret = 0;
>  
> +	bool is_query = !!(t->command == ACPI_EC_COMMAND_QUERY);
>  	if (EC_FLAGS_MSI)
>  		udelay(ACPI_EC_MSI_UDELAY);
>  	/* start transaction */
>  	spin_lock_irqsave(&ec->lock, tmp);
>  	/* Enable GPE for command processing (IBF=0/OBF=1) */
> -	if (!acpi_ec_submit_flushable_request(ec)) {
> +	if (!acpi_ec_submit_flushable_request(ec, is_query)) {
>  		ret = -EINVAL;
>  		goto unlock;
>  	}
> @@ -398,10 +476,6 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
>  	pr_debug("***** Command(%s) started *****\n",
>  		 acpi_ec_cmd_string(t->command));
>  	start_transaction(ec);
> -	if (ec->curr->command == ACPI_EC_COMMAND_QUERY) {
> -		clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
> -		pr_debug("***** Event stopped *****\n");
> -	}
>  	spin_unlock_irqrestore(&ec->lock, tmp);
>  	ret = ec_poll(ec);
>  	spin_lock_irqsave(&ec->lock, tmp);
> @@ -440,8 +514,6 @@ static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t)
>  
>  	status = acpi_ec_transaction_unlocked(ec, t);
>  
> -	/* check if we received SCI during transaction */
> -	ec_check_sci_sync(ec, acpi_ec_read_status(ec));
>  	if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) {
>  		msleep(1);
>  		/* It is safe to enable the GPE outside of the transaction. */
> @@ -792,29 +864,6 @@ static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data)
>  	return 0;
>  }
>  
> -static void acpi_ec_gpe_query(void *ec_cxt)
> -{
> -	struct acpi_ec *ec = ec_cxt;
> -
> -	if (!ec)
> -		return;
> -	mutex_lock(&ec->mutex);
> -	acpi_ec_sync_query(ec, NULL);
> -	mutex_unlock(&ec->mutex);
> -}
> -
> -static int ec_check_sci(struct acpi_ec *ec, u8 state)
> -{
> -	if (state & ACPI_EC_FLAG_SCI) {
> -		if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
> -			pr_debug("***** Event started *****\n");
> -			return acpi_os_execute(OSL_NOTIFY_HANDLER,
> -				acpi_ec_gpe_query, ec);
> -		}
> -	}
> -	return 0;
> -}
> -
>  static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
>  	u32 gpe_number, void *data)
>  {
> @@ -825,10 +874,46 @@ static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
>  	if (advance_transaction(ec))
>  		wake_up(&ec->wait);
>  	spin_unlock_irqrestore(&ec->lock, flags);
> -	ec_check_sci(ec, acpi_ec_read_status(ec));
>  	return ACPI_INTERRUPT_HANDLED | ACPI_REENABLE_GPE;
>  }
>  
> +static int acpi_ec_event_poller(void *context)
> +{
> +	struct acpi_ec *ec = context;
> +
> +	while (!acpi_ec_wait_for_event(ec)) {
> +		pr_debug("***** Event poller started *****\n");
> +		mutex_lock(&ec->mutex);
> +		(void)acpi_ec_sync_query(ec, NULL);
> +		mutex_unlock(&ec->mutex);
> +		pr_debug("***** Event poller stopped *****\n");
> +	}
> +	return 0;
> +}
> +
> +static int ec_create_event_poller(struct acpi_ec *ec)
> +{
> +	struct task_struct *t;
> +
> +	t = kthread_run(acpi_ec_event_poller, ec, "ec/gpe-%lu", ec->gpe);

Does it have to be a kernel thread?

What about using a workqueue instead?

> +	if (IS_ERR(t)) {
> +		pr_err("failed to create event poller %lu\n", ec->gpe);
> +		return PTR_ERR(t);
> +	}
> +	get_task_struct(t);
> +	ec->thread = t;
> +	return 0;
> +}
> +
> +static void ec_delete_event_poller(struct acpi_ec *ec)
> +{
> +	struct task_struct *t = ec->thread;
> +
> +	ec->thread = NULL;
> +	kthread_stop(t);
> +	put_task_struct(t);
> +}
> +
>  /* --------------------------------------------------------------------------
>   *                           Address Space Management
>   * -------------------------------------------------------------------------- */
> @@ -884,7 +969,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);
> @@ -940,15 +1024,21 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
>  
>  static int ec_install_handlers(struct acpi_ec *ec)
>  {
> +	int ret;
>  	acpi_status status;
>  
>  	if (test_bit(EC_FLAGS_HANDLERS_INSTALLED, &ec->flags))
>  		return 0;
> +	ret = ec_create_event_poller(ec);
> +	if (ret)
> +		return ret;
>  	status = acpi_install_gpe_handler(NULL, ec->gpe,
>  				  ACPI_GPE_EDGE_TRIGGERED,
>  				  &acpi_ec_gpe_handler, ec);
> -	if (ACPI_FAILURE(status))
> +	if (ACPI_FAILURE(status)) {
> +		ec_delete_event_poller(ec);
>  		return -ENODEV;
> +	}
>  
>  	acpi_ec_start(ec);
>  	status = acpi_install_address_space_handler(ec->handle,
> @@ -968,6 +1058,7 @@ static int ec_install_handlers(struct acpi_ec *ec)
>  			acpi_ec_stop(ec);
>  			acpi_remove_gpe_handler(NULL, ec->gpe,
>  				&acpi_ec_gpe_handler);
> +			ec_delete_event_poller(ec);
>  			return -ENODEV;
>  		}
>  	}
> @@ -985,6 +1076,7 @@ static void ec_remove_handlers(struct acpi_ec *ec)
>  	if (ACPI_FAILURE(acpi_remove_gpe_handler(NULL, ec->gpe,
>  				&acpi_ec_gpe_handler)))
>  		pr_err("failed to remove gpe handler\n");
> +	ec_delete_event_poller(ec);
>  	clear_bit(EC_FLAGS_HANDLERS_INSTALLED, &ec->flags);
>  }
>  
> @@ -1032,7 +1124,7 @@ static int acpi_ec_add(struct acpi_device *device)
>  	ret = ec_install_handlers(ec);
>  
>  	/* 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) {
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index bbcfe0b..20a569c 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -128,6 +128,7 @@ struct acpi_ec {
>  	struct list_head list;
>  	struct transaction *curr;
>  	spinlock_t lock;
> +	struct task_struct *thread;
>  };
>  
>  extern struct acpi_ec *first_ec;
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists