From 0c78808f51d11564a29728091e87b80d6e54d499 Mon Sep 17 00:00:00 2001 From: Lv Zheng Date: Wed, 14 Jan 2015 19:28:22 +0800 Subject: ACPI / EC: Cleanup transaction wakeup code This patch moves transaction wakeup code into advance_transaction(). No functional changes. Signed-off-by: Lv Zheng Signed-off-by: Rafael J. Wysocki --- drivers/acpi/ec.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index 1b5853f384e..3e19123bb18 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -200,7 +200,7 @@ static int ec_transaction_completed(struct acpi_ec *ec) return ret; } -static bool advance_transaction(struct acpi_ec *ec) +static void advance_transaction(struct acpi_ec *ec) { struct transaction *t; u8 status; @@ -235,7 +235,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) && @@ -251,7 +251,7 @@ static bool advance_transaction(struct acpi_ec *ec) t->flags |= ACPI_EC_COMMAND_POLL; } else goto err; - return wakeup; + goto out; } err: /* @@ -262,14 +262,16 @@ err: if (in_interrupt() && t) ++t->irq_count; } - return wakeup; +out: + if (wakeup && in_interrupt()) + wake_up(&ec->wait); } static void start_transaction(struct acpi_ec *ec) { ec->curr->irq_count = ec->curr->wi = ec->curr->ri = 0; ec->curr->flags = 0; - (void)advance_transaction(ec); + advance_transaction(ec); } static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data); @@ -304,7 +306,7 @@ static int ec_poll(struct acpi_ec *ec) return 0; } spin_lock_irqsave(&ec->lock, flags); - (void)advance_transaction(ec); + advance_transaction(ec); spin_unlock_irqrestore(&ec->lock, flags); } while (time_before(jiffies, delay)); pr_debug("controller reset, restart transaction\n"); @@ -688,8 +690,7 @@ static u32 acpi_ec_gpe_handler(acpi_handle gpe_device, struct acpi_ec *ec = data; spin_lock_irqsave(&ec->lock, flags); - if (advance_transaction(ec)) - wake_up(&ec->wait); + advance_transaction(ec); spin_unlock_irqrestore(&ec->lock, flags); ec_check_sci(ec, acpi_ec_read_status(ec)); return ACPI_INTERRUPT_HANDLED | ACPI_REENABLE_GPE; -- cgit v1.2.3-70-g09d2 From 01305d4139cd345aa9f64cecfd7fc7b237e1444e Mon Sep 17 00:00:00 2001 From: Lv Zheng Date: Wed, 14 Jan 2015 19:28:28 +0800 Subject: ACPI / EC: Add reference counting for query handlers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch adds reference counting for query handlers in order to eliminate kmalloc()/kfree() usage. Signed-off-by: Lv Zheng Tested-by: Steffen Weber Tested-by: Ortwin Glück Signed-off-by: Rafael J. Wysocki --- drivers/acpi/ec.c | 43 +++++++++++++++++++++++++++++++++---------- 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index 3e19123bb18..87b9911c903 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -105,6 +105,7 @@ struct acpi_ec_query_handler { acpi_handle handle; void *data; u8 query_bit; + struct kref kref; }; struct transaction { @@ -580,6 +581,27 @@ static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 *data) /* -------------------------------------------------------------------------- Event Management -------------------------------------------------------------------------- */ +static struct acpi_ec_query_handler * +acpi_ec_get_query_handler(struct acpi_ec_query_handler *handler) +{ + if (handler) + kref_get(&handler->kref); + return handler; +} + +static void acpi_ec_query_handler_release(struct kref *kref) +{ + struct acpi_ec_query_handler *handler = + container_of(kref, struct acpi_ec_query_handler, kref); + + kfree(handler); +} + +static void acpi_ec_put_query_handler(struct acpi_ec_query_handler *handler) +{ + kref_put(&handler->kref, acpi_ec_query_handler_release); +} + int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit, acpi_handle handle, acpi_ec_query_func func, void *data) @@ -595,6 +617,7 @@ int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit, handler->func = func; handler->data = data; mutex_lock(&ec->mutex); + kref_init(&handler->kref); list_add(&handler->node, &ec->list); mutex_unlock(&ec->mutex); return 0; @@ -604,15 +627,18 @@ EXPORT_SYMBOL_GPL(acpi_ec_add_query_handler); void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit) { struct acpi_ec_query_handler *handler, *tmp; + LIST_HEAD(free_list); mutex_lock(&ec->mutex); list_for_each_entry_safe(handler, tmp, &ec->list, node) { if (query_bit == handler->query_bit) { - list_del(&handler->node); - kfree(handler); + list_del_init(&handler->node); + list_add(&handler->node, &free_list); } } mutex_unlock(&ec->mutex); + list_for_each_entry(handler, &free_list, node) + acpi_ec_put_query_handler(handler); } EXPORT_SYMBOL_GPL(acpi_ec_remove_query_handler); @@ -628,14 +654,14 @@ static void acpi_ec_run(void *cxt) else if (handler->handle) acpi_evaluate_object(handler->handle, NULL, NULL, NULL); pr_debug("##### Query(0x%02x) stopped #####\n", handler->query_bit); - kfree(handler); + acpi_ec_put_query_handler(handler); } static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data) { u8 value = 0; int status; - struct acpi_ec_query_handler *handler, *copy; + struct acpi_ec_query_handler *handler; status = acpi_ec_query_unlocked(ec, &value); if (data) @@ -646,15 +672,12 @@ static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data) list_for_each_entry(handler, &ec->list, node) { if (value == handler->query_bit) { /* have custom handler for this bit */ - copy = kmalloc(sizeof(*handler), GFP_KERNEL); - if (!copy) - return -ENOMEM; - memcpy(copy, handler, sizeof(*copy)); + handler = acpi_ec_get_query_handler(handler); pr_debug("##### Query(0x%02x) scheduled #####\n", handler->query_bit); - return acpi_os_execute((copy->func) ? + return acpi_os_execute((handler->func) ? OSL_NOTIFY_HANDLER : OSL_GPE_HANDLER, - acpi_ec_run, copy); + acpi_ec_run, handler); } } return 0; -- cgit v1.2.3-70-g09d2 From c2cf5769fae1ce208ea00fa85298d1d19969300a Mon Sep 17 00:00:00 2001 From: Lv Zheng Date: Wed, 14 Jan 2015 19:28:33 +0800 Subject: ACPI / EC: Fix returning values in acpi_ec_sync_query() The returning value of acpi_os_execute() is erroneously handled as errno. This patch corrects it by returning EBUSY to indicate the work queue item creation failure. Signed-off-by: Lv Zheng Signed-off-by: Rafael J. Wysocki --- drivers/acpi/ec.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index 87b9911c903..a94ee9f7def 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -660,14 +660,15 @@ static void acpi_ec_run(void *cxt) static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data) { u8 value = 0; - int status; + int result; + acpi_status status; struct acpi_ec_query_handler *handler; - status = acpi_ec_query_unlocked(ec, &value); + result = acpi_ec_query_unlocked(ec, &value); if (data) *data = value; - if (status) - return status; + if (result) + return result; list_for_each_entry(handler, &ec->list, node) { if (value == handler->query_bit) { @@ -675,12 +676,15 @@ static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data) handler = acpi_ec_get_query_handler(handler); pr_debug("##### Query(0x%02x) scheduled #####\n", handler->query_bit); - return acpi_os_execute((handler->func) ? + status = acpi_os_execute((handler->func) ? OSL_NOTIFY_HANDLER : OSL_GPE_HANDLER, acpi_ec_run, handler); + if (ACPI_FAILURE(status)) + result = -EBUSY; + break; } } - return 0; + return result; } static void acpi_ec_gpe_query(void *ec_cxt) -- cgit v1.2.3-70-g09d2 From f3e14329517ee190d34455d729430ef9d0473675 Mon Sep 17 00:00:00 2001 From: Lv Zheng Date: Wed, 14 Jan 2015 19:28:41 +0800 Subject: ACPI / EC: Fix a code path that global lock is not held Currently QR_EC is queued up on CPU 0 to be safe with SMM because there is no global lock held for acpi_ec_gpe_query(). As we are about to move QR_EC to a non CPU 0 bound work queue to avoid invoking kmalloc() in advance_transaction(), we have to acquire global lock for the new QR_EC work item to avoid regressions. Known issue: 1. Global lock for acpi_ec_clear(). This is an existing issue that acpi_ec_clear() which invokes acpi_ec_sync_query() also suffers from the same issue. But this patch's target is only the code to invoke acpi_ec_sync_query() in a CPU 0 bound work queue item, and the acpi_ec_clear() can be automatically fixed by further patch that merges the redundant code, so it is left unchanged. Signed-off-by: Lv Zheng Signed-off-by: Rafael J. Wysocki --- drivers/acpi/ec.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index a94ee9f7def..3c97122eacd 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -690,11 +690,21 @@ static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data) static void acpi_ec_gpe_query(void *ec_cxt) { struct acpi_ec *ec = ec_cxt; + acpi_status status; + u32 glk; if (!ec) return; mutex_lock(&ec->mutex); + if (ec->global_lock) { + status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk); + if (ACPI_FAILURE(status)) + goto unlock; + } acpi_ec_sync_query(ec, NULL); + if (ec->global_lock) + acpi_release_global_lock(glk); +unlock: mutex_unlock(&ec->mutex); } -- cgit v1.2.3-70-g09d2 From 74443bbed72ab22ee005ecb6ecdc657a8018e1db Mon Sep 17 00:00:00 2001 From: Lv Zheng Date: Wed, 14 Jan 2015 19:28:47 +0800 Subject: ACPI / EC: Fix issues related to the SCI_EVT handling This patch fixes 2 issues related to the draining behavior. But it doesn't implement the draining support, it only cleans up code so that further draining support is possible. The draining behavior is expected by some platforms (for example, Samsung) where SCI_EVT is set only once for a set of events and might be cleared for the very first QR_EC command issued after SCI_EVT is set. EC firmware on such platforms will return 0x00 to indicate "no outstanding event". Thus after seeing an SCI_EVT indication, EC driver need to fetch events until 0x00 returned (see acpi_ec_clear()). Issue 1 - acpi_ec_submit_query(): It's reported on Samsung laptops that SCI_EVT isn't checked when the transactions are advanced in ec_poll(), which leads to SCI_EVT triggering source lost: If no EC GPE IRQs are arrived after that, EC driver cannot detect this event and handle it. See comment 244/247 for kernel bugzilla 44161. This patch fixes this issue by moving SCI_EVT checks into advance_transaction(). So that SCI_EVT is checked each time we are going to handle the EC firmware indications. And this check will happen for both IRQ context and task context. Since after doing that, SCI_EVT is also checked after completing a transaction, ec_check_sci() and ec_check_sci_sync() can be removed. Issue 2 - acpi_ec_complete_query(): We expect to clear EC_FLAGS_QUERY_PENDING to allow queuing another draining QR_EC after writing a QR_EC command and before reading the event. After reading the event, SCI_EVT might be cleared by the firmware, thus it may not be possible to queue such a draining QR_EC at that time. But putting the EC_FLAGS_QUERY_PENDING clearing code after start_transaction() is wrong as there are chances that after start_transaction(), QR_EC can fail to be sent. If this happens, EC_FLAG_QUERY_PENDING will be cleared earlier. As a consequence, the draining QR_EC will also be queued earlier than expected. This patch also moves this code into advance_transaction() where QR_EC is just sent (ACPI_EC_COMMAND_POLL flagged) to fix this issue. Notes: 1. After introducing the 2 SCI_EVT related handlings into advance_transaction(), a next QR_EC can be queued right after writing the current QR_EC command and before reading the event. But this still hasn't implemented the draining behavior as the draining support requires: If a previous returned event value isn't 0x00, a draining QR_EC need to be issued even when SCI_EVT isn't set. 2. In this patch, acpi_os_execute() is also converted into a seperate work item to avoid invoking kmalloc() in the atomic context. We can do this because of the previous global lock fix. 3. Originally, EC_FLAGS_EVENT_PENDING is also used to avoid queuing up multiple work items (created by acpi_os_execute()), this can be covered by only using a single work item. But this patch still keeps this flag as there are different usages in the driver initialization steps relying on this flag. Link: https://bugzilla.kernel.org/show_bug.cgi?id=44161 Reported-by: Kieran Clancy Signed-off-by: Lv Zheng Signed-off-by: Rafael J. Wysocki --- drivers/acpi/ec.c | 59 +++++++++++++++++++++---------------------------- drivers/acpi/internal.h | 1 + 2 files changed, 26 insertions(+), 34 deletions(-) diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index 3c97122eacd..89e89b21dd5 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -120,6 +120,8 @@ struct transaction { u8 flags; }; +static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data); + struct acpi_ec *boot_ec, *first_ec; EXPORT_SYMBOL(first_ec); @@ -189,6 +191,22 @@ static const char *acpi_ec_cmd_string(u8 cmd) #define acpi_ec_cmd_string(cmd) "UNDEF" #endif +static void acpi_ec_submit_query(struct acpi_ec *ec) +{ + if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) { + pr_debug("***** Event started *****\n"); + schedule_work(&ec->work); + } +} + +static void acpi_ec_complete_query(struct acpi_ec *ec) +{ + if (ec->curr->command == ACPI_EC_COMMAND_QUERY) { + clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags); + pr_debug("***** Event stopped *****\n"); + } +} + static int ec_transaction_completed(struct acpi_ec *ec) { unsigned long flags; @@ -242,6 +260,7 @@ static void advance_transaction(struct acpi_ec *ec) !(status & ACPI_EC_FLAG_SCI) && (t->command == ACPI_EC_COMMAND_QUERY)) { t->flags |= ACPI_EC_COMMAND_POLL; + acpi_ec_complete_query(ec); t->rdata[t->ri++] = 0x00; t->flags |= ACPI_EC_COMMAND_COMPLETE; pr_debug("***** Command(%s) software completion *****\n", @@ -250,6 +269,7 @@ static void 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; + acpi_ec_complete_query(ec); } else goto err; goto out; @@ -264,6 +284,8 @@ err: ++t->irq_count; } out: + if (status & ACPI_EC_FLAG_SCI) + acpi_ec_submit_query(ec); if (wakeup && in_interrupt()) wake_up(&ec->wait); } @@ -275,17 +297,6 @@ static void start_transaction(struct acpi_ec *ec) 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; @@ -333,10 +344,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); @@ -376,8 +383,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. */ @@ -687,14 +692,12 @@ static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data) return result; } -static void acpi_ec_gpe_query(void *ec_cxt) +static void acpi_ec_gpe_poller(struct work_struct *work) { - struct acpi_ec *ec = ec_cxt; acpi_status status; u32 glk; + struct acpi_ec *ec = container_of(work, struct acpi_ec, work); - if (!ec) - return; mutex_lock(&ec->mutex); if (ec->global_lock) { status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk); @@ -708,18 +711,6 @@ unlock: 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) { @@ -729,7 +720,6 @@ static u32 acpi_ec_gpe_handler(acpi_handle gpe_device, spin_lock_irqsave(&ec->lock, flags); advance_transaction(ec); spin_unlock_irqrestore(&ec->lock, flags); - ec_check_sci(ec, acpi_ec_read_status(ec)); return ACPI_INTERRUPT_HANDLED | ACPI_REENABLE_GPE; } @@ -793,6 +783,7 @@ static struct acpi_ec *make_acpi_ec(void) init_waitqueue_head(&ec->wait); INIT_LIST_HEAD(&ec->list); spin_lock_init(&ec->lock); + INIT_WORK(&ec->work, acpi_ec_gpe_poller); return ec; } diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index 163e82f536f..dc420787ffc 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -127,6 +127,7 @@ struct acpi_ec { struct list_head list; struct transaction *curr; spinlock_t lock; + struct work_struct work; }; extern struct acpi_ec *first_ec; -- cgit v1.2.3-70-g09d2 From 550b3aac5a72c4209f1ad3bc0ade663d5cb36f7f Mon Sep 17 00:00:00 2001 From: Lv Zheng Date: Wed, 14 Jan 2015 19:28:53 +0800 Subject: ACPI / EC: Cleanup QR_EC related code The QR_EC related code pieces have redundants, this patch merges them into acpi_ec_query() which invokes acpi_ec_transaction() where EC mutex and the global lock are already held. After doing so, query handler traversal still need to be locked by EC mutex after invoking acpi_ec_transaction(). Note that EC event handling is sequential. We fetch one event from firmware event queue and process it until 0x00 or error returned. So we don't need to hold mutex for whole acpi_ec_clear() process to determine whether we should continue to drain. And for the same reason, we don't need to hold mutex for the whole procedure from the QR_EC transaction to the query handler traversal. Signed-off-by: Lv Zheng Signed-off-by: Rafael J. Wysocki --- drivers/acpi/ec.c | 70 ++++++++++++++++--------------------------------------- 1 file changed, 20 insertions(+), 50 deletions(-) diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index 89e89b21dd5..c385606bbce 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -120,7 +120,7 @@ struct transaction { u8 flags; }; -static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data); +static int acpi_ec_query(struct acpi_ec *ec, u8 *data); struct acpi_ec *boot_ec, *first_ec; EXPORT_SYMBOL(first_ec); @@ -508,7 +508,7 @@ static void acpi_ec_clear(struct acpi_ec *ec) u8 value = 0; for (i = 0; i < ACPI_EC_CLEAR_MAX; i++) { - status = acpi_ec_sync_query(ec, &value); + status = acpi_ec_query(ec, &value); if (status || !value) break; } @@ -539,14 +539,11 @@ void acpi_ec_unblock_transactions(void) if (!ec) return; - mutex_lock(&ec->mutex); /* Allow transactions to be carried out again */ clear_bit(EC_FLAGS_BLOCKED, &ec->flags); if (EC_FLAGS_CLEAR_ON_RESUME) acpi_ec_clear(ec); - - mutex_unlock(&ec->mutex); } void acpi_ec_unblock_transactions_early(void) @@ -559,30 +556,6 @@ void acpi_ec_unblock_transactions_early(void) clear_bit(EC_FLAGS_BLOCKED, &first_ec->flags); } -static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 *data) -{ - int result; - u8 d; - struct transaction t = {.command = ACPI_EC_COMMAND_QUERY, - .wdata = NULL, .rdata = &d, - .wlen = 0, .rlen = 1}; - - if (!ec || !data) - return -EINVAL; - /* - * Query the EC to find out which _Qxx method we need to evaluate. - * Note that successful completion of the query causes the ACPI_EC_SCI - * bit to be cleared (and thus clearing the interrupt source). - */ - result = acpi_ec_transaction_unlocked(ec, &t); - if (result) - return result; - if (!d) - return -ENODATA; - *data = d; - return 0; -} - /* -------------------------------------------------------------------------- Event Management -------------------------------------------------------------------------- */ @@ -662,19 +635,30 @@ static void acpi_ec_run(void *cxt) acpi_ec_put_query_handler(handler); } -static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data) +static int acpi_ec_query(struct acpi_ec *ec, u8 *data) { u8 value = 0; int result; acpi_status status; struct acpi_ec_query_handler *handler; + struct transaction t = {.command = ACPI_EC_COMMAND_QUERY, + .wdata = NULL, .rdata = &value, + .wlen = 0, .rlen = 1}; - result = acpi_ec_query_unlocked(ec, &value); - if (data) - *data = value; + /* + * Query the EC to find out which _Qxx method we need to evaluate. + * Note that successful completion of the query causes the ACPI_EC_SCI + * bit to be cleared (and thus clearing the interrupt source). + */ + result = acpi_ec_transaction(ec, &t); if (result) return result; + if (data) + *data = value; + if (!value) + return -ENODATA; + mutex_lock(&ec->mutex); list_for_each_entry(handler, &ec->list, node) { if (value == handler->query_bit) { /* have custom handler for this bit */ @@ -689,26 +673,15 @@ static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data) break; } } + mutex_unlock(&ec->mutex); return result; } static void acpi_ec_gpe_poller(struct work_struct *work) { - acpi_status status; - u32 glk; struct acpi_ec *ec = container_of(work, struct acpi_ec, work); - mutex_lock(&ec->mutex); - if (ec->global_lock) { - status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk); - if (ACPI_FAILURE(status)) - goto unlock; - } - acpi_ec_sync_query(ec, NULL); - if (ec->global_lock) - acpi_release_global_lock(glk); -unlock: - mutex_unlock(&ec->mutex); + acpi_ec_query(ec, NULL); } static u32 acpi_ec_gpe_handler(acpi_handle gpe_device, @@ -932,11 +905,8 @@ static int acpi_ec_add(struct acpi_device *device) clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags); /* Clear stale _Q events if hardware might require that */ - if (EC_FLAGS_CLEAR_ON_RESUME) { - mutex_lock(&ec->mutex); + if (EC_FLAGS_CLEAR_ON_RESUME) acpi_ec_clear(ec); - mutex_unlock(&ec->mutex); - } return ret; } -- cgit v1.2.3-70-g09d2 From ca37bfdfbc8d0a3ec73e4b97bb26dcfa51d515aa Mon Sep 17 00:00:00 2001 From: Lv Zheng Date: Thu, 5 Feb 2015 16:27:22 +0800 Subject: ACPI / EC: Fix several GPE handling issues by deploying ACPI_GPE_DISPATCH_RAW_HANDLER mode. This patch switches EC driver into ACPI_GPE_DISPATCH_RAW_HANDLER mode where the GPE lock is not held for acpi_ec_gpe_handler() and the ACPICA internal GPE enabling/disabling/clearing operations are bypassed so that further improvements are possible with the GPE APIs. There are 2 strong reasons for deploying raw GPE handler mode in the EC driver: 1. Some hardware logics can control their interrupts via their own registers, so their interrupts can be disabled/enabled/acknowledged without using the super IRQ controller provided functions. While there is no mean (EC commands) for the EC driver to achieve this. 2. During suspending, the EC driver is still working for a while to complete the platform firmware provided functionailities using ec_poll() after all GPEs are disabled (see acpi_ec_block_transactions()), which means the EC driver will drive the EC GPE out of the GPE core's control. Without deploying the raw GPE handler mode, we can see many races between the EC driver and the GPE core due to the above restrictions: 1. There is a race condition due to ACPICA internal GPE disabling/clearing/enabling logics in acpi_ev_gpe_dispatch(): Orignally EC GPE is disabled (EN=0), cleared (STS=0) before invoking a GPE handler and re-enabled (EN=1) after invoking a GPE handler in acpi_ev_gpe_dispatch(). When re-enabling appears, GPE may be flagged (STS=1). ================================================================= (event pending A) ================================================================= acpi_ev_gpe_dispatch() ec_poll() EN=0 STS=0 acpi_ec_gpe_handler() ***************************************************************** (event handling A) Lock(EC) advance_transaction() EC_SC read ================================================================= (event pending B) ================================================================= EC_SC handled Unlock(EC) ***************************************************************** ***************************************************************** (event handling B) Lock(EC) advance_transaction() EC_SC read ================================================================= (event pending C) ================================================================= EC_SC handled Unlock(EC) ***************************************************************** EN=1 This race condition is the root cause of different issues on different silicon variations. A. Silicon variation A: On some platforms, GPE will be triggered due to "writing 1 to EN when STS=1". This is because both EN and STS lines are wired to the GPE trigger line. 1. Issue 1: We can see no-op acpi_ec_gpe_handler() invoked on such platforms. This is because: a. event pending B: An event can arrive after ACPICA's GPE clearing performed in acpi_ev_gpe_dispatch(), this event may fail to be detected by EC_SC read that is performed before its arrival; b. event handling B: The event can be handled in ec_poll() because EC lock is released after acpi_ec_gpe_handler() invocation; c. There is no code in ec_poll() to clear STS but the GPE can still be triggered by the EN=1 write performed in acpi_ev_finish_gpe(), this leads to a no-op EC GPE handler invocation; d. As no-op GPE handler invocations are counted by the EC driver to trigger the command storming conditions, the wrong no-op GPE handler invocations thus can easily trigger wrong command storming conditions. Note 1: If we removed GPE disabling/enabling code from acpi_ev_gpe_dispatch(), we could still see no-op GPE handlers triggered by the event arriving after the GPE clearing and before the GPE handling on both silicon variation A and B. This can only occur if the CPU is very slow (timing slice between STS=0 write and EC_SC read should be short enough before hardware sets another GPE indication). Thus this is very rare and is not what we need to fix. B. Silicon variation B: On other platforms, GPE may not be triggered due to "writing 1 to EN when STS=1". This is because only STS line is wired to the GPE trigger line. 2. Issue 2: We can see GPE loss on such platforms. This is because: a. event pending B vs. event handling A: An event can arrive after ACPICA's GPE handling performed in acpi_ev_gpe_dispatch(), or event pending C vs. event handling B: An event can arrive after Linux's GPE handling performed in ec_poll(), these events may fail to be detected by EC_SC read that is performed before their arrival; b. The GPE cannot be triggered by EN=1 write performed in acpi_ev_finish_gpe(); c. If no polling mechanism is implemented in the driver for the pending event (for example, SCI_EVT), this event is lost due to no GPE being triggered. Note 2: On most platforms, there might be another rule that GPE may not be triggered due to "writing 1 to STS when STS=1 and EN=1". Then on silicon variation B, an even worse case is if the issue 2 event loss happens, further events may never trigger GPE again on such platforms due to being blocked by the current STS=1. Unless someone clears STS, all events have to be polled. 2. There is a race condition due to lacking in GPE status checking in EC driver: Originally, GPE status is checked in ACPICA core but not checked in the GPE handler. Thus since the status checking and handling is not locked, it can be interrupted by another handling path. ================================================================= (event pending A) ================================================================= acpi_ev_gpe_detect() ec_poll() if (EN==1 && STS==1) ***************************************************************** (event handling A) Lock(EC) advance_transaction() EC_SC read EC_SC handled Unlock(EC) ***************************************************************** acpi_ev_gpe_dispatch() EN=0 STS=0 acpi_ec_gpe_handler() ***************************************************************** (event handling B) Lock(EC) advance_transaction() EC_SC read Unlock(EC) ***************************************************************** 3. Issue 3: We can see no-op acpi_ec_gpe_handler() invoked on both silicon variation A and B. This is because: a. event pending A: An event can arrive to trigger an EC GPE and ACPICA checks it and is about to invoke the EC GPE handler; b. event handling A: The event can be handled in ec_poll() because EC lock is not held after the GPE status checking; c. event handling B: Then when the EC GPE handler is invoked, it becomes a no-op GPE handler invocation. d. As no-op GPE handler invocations are counted by the EC driver to trigger the command storming conditions, the wrong no-op GPE handler invocations thus can easily trigger wrong command storming conditions. Note 3: This no-op GPE handler invocation is rare because the time between the IRQ arrival and the acpi_ec_gpe_handler() invocation is less than the timeout value waited in ec_poll(). So most of the no-op GPE handler invocations are caused by the reason described in issue 1. 3. There is a race condition due to ACPICA internal GPE clearing logic in acpi_enable_gpe(): During runtime, acpi_enable_gpe() can be invoked by the EC storming prevention code. When it is invoked, GPE may be flagged (STS=1). ================================================================= (event pending A) ================================================================= acpi_ev_gpe_dispatch() acpi_ec_transaction() EN=0 STS=0 acpi_ec_gpe_handler() ***************************************************************** (event handling A) Lock(EC) advance_transaction() EC_SC read EC_SC handled Unlock(EC) ***************************************************************** EN=1 ? Lock(EC) Unlock(EC) ================================================================= (event pending B) ================================================================= acpi_enable_gpe() STS=0 EN=1 4. Issue 4: We can see GPE loss on both silicon variation A and B platforms. This is because: a. event pending B: An event can arrive right before ACPICA's GPE clearing performed in acpi_enable_gpe(); b. If the GPE is cleared when GPE is disabled, then EN=1 write in acpi_enable_gpe() cannot trigger this GPE; c. If no polling mechanism is implemented in the driver for this event (for example, SCI_EVT), this event is lost due to no GPE being triggered. Note 4: Currently we don't have this issue, but after we switch the EC driver into ACPI_GPE_DISPATCH_RAW_HANDLER mode, we need to take care of handling this because the EN=1 write in acpi_ev_gpe_dispatch() will be abandoned. There might be more race issues for the current GPE handler usages. This is because the EC IRQ's enabling/disabling/checking/clearing/handling operations should be locked by a single lock that is under the EC driver's control to achieve the serialization. Which means we need to invoke GPE APIs with EC driver's lock held and all ACPICA internal GPE operations related to the GPE handler should be abandoned. Invoking GPE APIs inside of the EC driver lock and bypassing ACPICA internal GPE operations requires the ACPI_GPE_DISPATCH_RAW_HANDLER mode where the same lock used by the APIs are released prior than invoking the handlers. Otherwise, we can see dead locks due to circular locking dependencies (see Reference below). This patch then switches the EC driver into the ACPI_GPE_DISPATCH_RAW_HANDLER mode so that it can perform correct GPE operations using the GPE APIs: 1. Bypasses EN modifications performed in acpi_ev_gpe_dispatch() by using acpi_install_gpe_raw_handler() and invoking all GPE APIs with EC spin lock held. This can fix issue 1 as it makes a non frequent GPE enabling/disabling environment. 2. Bypasses STS clearing performed in acpi_enable_gpe() by replacing acpi_enable_gpe()/acpi_disable_gpe() with acpi_set_gpe(). This can fix issue 4. And this can also help to fix issue 1 as it makes a no sudden GPE clearing environment when GPE is frequently enabled/disabled. 3. Ensures STS acknowledged before handling by invoking acpi_clear_gpe() in advance_transaction(). This can finally fix issue 1 even in a frequent GPE enabling/disabling environment. And this can also finally fix issue 3 when issue 2 is fixed. Note 3: GPE clearing is edge triggered W1C, which means we can clear it right before handling it. Since all EC GPE indications are handled in advance_transaction() by previous commits, we can now move GPE clearing into it to implement the correct GPE clearing. Note 4: We can use acpi_set_gpe() which is not shared GPE safer instead of acpi_enable_gpe()/acpi_disable_gpe() because EC GPE is not shared by other hardware, which is mentioned in the ACPI specification 5.0, 12.6 Interrupt Model: "OSPM driver treats this as an edge event (the EC SCI cannot be shared)". So we can stop using shared GPE safer APIs acpi_enable_gpe()/acpi_disable_gpe() in the EC driver. Otherwise cleanups need to be made in acpi_ev_enable_gpe() to bypass the GPE clearing logic before keeping acpi_enable_gpe(). This patch also invokes advance_transaction() when GPE is re-enabled in the task context which: 1. Ensures EN=1 can trigger GPE by checking and handling EC status register right after EN=1 writes. This can fix issue 2. After applying this patch, without frequent GPE enablings considered: ================================================================= (event pending A) ================================================================= acpi_ec_gpe_handler() ec_poll() ***************************************************************** (event handling A) Lock(EC) advance_transaction() if STS==1 STS=0 EC_SC read ================================================================= (event pending B) ================================================================= EC_SC handled Unlock(EC) ***************************************************************** ***************************************************************** (event handling B) Lock(EC) advance_transaction() if STS==1 STS=0 EC_SC read ================================================================= (event pending C) ================================================================= EC_SC handled Unlock(EC) ***************************************************************** The event pending for issue 1 (event pending B) can arrive as a next GPE due to the previous IRQ context STS=0 write. And if it is handled by ec_poll() (event handling B), as it is also acknowledged by ec_poll(), the event pending for issue 2 (event pending C) can properly arrive as a next GPE after the task context STS=0 write. So no GPE will be lost and never triggered due to GPE clearing performed in the wrong position. And since all GPE handling is performed after a locked GPE status checking, we can hardly see no-op GPE handler invocations due to issue 1 and 3. We may still see no-op GPE handler invocations due to "Note 1", but as it is inevitable, it needn't be fixed. After applying this patch, with frequent GPE enablings considered: ================================================================= (event pending A) ================================================================= acpi_ec_gpe_handler() acpi_ec_transaction() ***************************************************************** (event handling A) Lock(EC) advance_transaction() if STS==1 STS=0 EC_SC read ================================================================= (event pending B) ================================================================= EC_SC handled Unlock(EC) ***************************************************************** ***************************************************************** (event handling B) Lock(EC) EN=1 if STS==1 advance_transaction() if STS==1 STS=0 EC_SC read ================================================================= (event pending C) ================================================================= EC_SC handled Unlock(EC) ***************************************************************** The event pending for issue 2 can be manually handled by advance_transaction(). And after the STS=0 write performed in the manual triggered advance_transaction(), GPE can always arrive. So no GPE will be lost due to frequent GPE disabling/enabling performed in the driver like issue 4. Note 5: It's ideally when EN=1 write occurred, an IRQ thread should be woken up to handle the GPE when the GPE was raised. But this requires the IRQ thread to contain the poller code for all EC GPE indications, while currently some of the indications are handled in the user tasks. It then is very hard for the code to determine whether a user task should be invoked or the poller work item should be scheduled. So we have to invoke advance_transaction() directly now and it leaves us such a restriction for the GPE re-enabling: it must be performed in the task context to avoid starving the GPEs. As a conclusion: we can see the EC GPE is always handled in serial after deploying the raw GPE handler mode: Lock(EC) if (STS==1) STS=0 EC_SC read EC_SC handled Unlock(EC) The EC driver specific lock is responsible to make the EC GPE handling processes serialized so that EC can handle its GPE from both IRQ and task contexts and the next IRQ can be ensured to arrive after this process. Note 6: We have many EC_FLAGS_MSI qurik users in the current driver. They all seem to be suffering from unexpected GPE triggering source lost. And they are false root caused to a timing issue. Since EC communication protocol has already flow control defined, timing shouldn't be the root cause, while this fix might be fixing the root cause of the old bugs. Link: https://lkml.org/lkml/2014/11/4/974 Link: https://lkml.org/lkml/2014/11/18/316 Link: https://www.spinics.net/lists/linux-acpi/msg54340.html Signed-off-by: Lv Zheng Signed-off-by: Rafael J. Wysocki --- drivers/acpi/ec.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 73 insertions(+), 8 deletions(-) diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index c385606bbce..2540870e89f 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -121,6 +121,7 @@ struct transaction { }; static int acpi_ec_query(struct acpi_ec *ec, u8 *data); +static void advance_transaction(struct acpi_ec *ec); struct acpi_ec *boot_ec, *first_ec; EXPORT_SYMBOL(first_ec); @@ -132,7 +133,7 @@ static int EC_FLAGS_CLEAR_ON_RESUME; /* Needs acpi_ec_clear() on boot/resume */ static int EC_FLAGS_QUERY_HANDSHAKE; /* Needs QR_EC issued when SCI_EVT set */ /* -------------------------------------------------------------------------- - * Transaction Management + * EC Registers * -------------------------------------------------------------------------- */ static inline u8 acpi_ec_read_status(struct acpi_ec *ec) @@ -191,6 +192,64 @@ static const char *acpi_ec_cmd_string(u8 cmd) #define acpi_ec_cmd_string(cmd) "UNDEF" #endif +/* -------------------------------------------------------------------------- + * GPE Registers + * -------------------------------------------------------------------------- */ + +static inline bool acpi_ec_is_gpe_raised(struct acpi_ec *ec) +{ + acpi_event_status gpe_status = 0; + + (void)acpi_get_gpe_status(NULL, ec->gpe, &gpe_status); + return (gpe_status & ACPI_EVENT_FLAG_SET) ? true : false; +} + +static inline void acpi_ec_enable_gpe(struct acpi_ec *ec, bool open) +{ + if (open) + acpi_enable_gpe(NULL, ec->gpe); + else + acpi_set_gpe(NULL, ec->gpe, ACPI_GPE_ENABLE); + if (acpi_ec_is_gpe_raised(ec)) { + /* + * On some platforms, EN=1 writes cannot trigger GPE. So + * software need to manually trigger a pseudo GPE event on + * EN=1 writes. + */ + pr_debug("***** Polling quirk *****\n"); + advance_transaction(ec); + } +} + +static inline void acpi_ec_disable_gpe(struct acpi_ec *ec, bool close) +{ + if (close) + acpi_disable_gpe(NULL, ec->gpe); + else + acpi_set_gpe(NULL, ec->gpe, ACPI_GPE_DISABLE); +} + +static inline void acpi_ec_clear_gpe(struct acpi_ec *ec) +{ + /* + * GPE STS is a W1C register, which means: + * 1. Software can clear it without worrying about clearing other + * GPEs' STS bits when the hardware sets them in parallel. + * 2. As long as software can ensure only clearing it when it is + * set, hardware won't set it in parallel. + * So software can clear GPE in any contexts. + * Warning: do not move the check into advance_transaction() as the + * EC commands will be sent without GPE raised. + */ + if (!acpi_ec_is_gpe_raised(ec)) + return; + acpi_clear_gpe(NULL, ec->gpe); +} + +/* -------------------------------------------------------------------------- + * Transaction Management + * -------------------------------------------------------------------------- */ + static void acpi_ec_submit_query(struct acpi_ec *ec) { if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) { @@ -227,6 +286,12 @@ static void advance_transaction(struct acpi_ec *ec) pr_debug("===== %s (%d) =====\n", in_interrupt() ? "IRQ" : "TASK", smp_processor_id()); + /* + * By always clearing STS before handling all indications, we can + * ensure a hardware STS 0->1 change after this clearing can always + * trigger a GPE interrupt. + */ + acpi_ec_clear_gpe(ec); status = acpi_ec_read_status(ec); t = ec->curr; if (!t) @@ -378,7 +443,7 @@ static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t) /* disable GPE during transaction if storm is detected */ if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) { /* It has to be disabled, so that it doesn't trigger. */ - acpi_disable_gpe(NULL, ec->gpe); + acpi_ec_disable_gpe(ec, false); } status = acpi_ec_transaction_unlocked(ec, t); @@ -386,7 +451,7 @@ static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t) if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) { msleep(1); /* It is safe to enable the GPE outside of the transaction. */ - acpi_enable_gpe(NULL, ec->gpe); + acpi_ec_enable_gpe(ec, false); } else if (t->irq_count > ec_storm_threshold) { pr_info("GPE storm detected(%d GPEs), " "transactions will use polling mode\n", @@ -693,7 +758,7 @@ static u32 acpi_ec_gpe_handler(acpi_handle gpe_device, spin_lock_irqsave(&ec->lock, flags); advance_transaction(ec); spin_unlock_irqrestore(&ec->lock, flags); - return ACPI_INTERRUPT_HANDLED | ACPI_REENABLE_GPE; + return ACPI_INTERRUPT_HANDLED; } /* -------------------------------------------------------------------------- @@ -812,13 +877,13 @@ static int ec_install_handlers(struct acpi_ec *ec) if (test_bit(EC_FLAGS_HANDLERS_INSTALLED, &ec->flags)) return 0; - status = acpi_install_gpe_handler(NULL, ec->gpe, + status = acpi_install_gpe_raw_handler(NULL, ec->gpe, ACPI_GPE_EDGE_TRIGGERED, &acpi_ec_gpe_handler, ec); if (ACPI_FAILURE(status)) return -ENODEV; - acpi_enable_gpe(NULL, ec->gpe); + acpi_ec_enable_gpe(ec, true); status = acpi_install_address_space_handler(ec->handle, ACPI_ADR_SPACE_EC, &acpi_ec_space_handler, @@ -833,7 +898,7 @@ static int ec_install_handlers(struct acpi_ec *ec) pr_err("Fail in evaluating the _REG object" " of EC device. Broken bios is suspected.\n"); } else { - acpi_disable_gpe(NULL, ec->gpe); + acpi_ec_disable_gpe(ec, true); acpi_remove_gpe_handler(NULL, ec->gpe, &acpi_ec_gpe_handler); return -ENODEV; @@ -848,7 +913,7 @@ static void ec_remove_handlers(struct acpi_ec *ec) { if (!test_bit(EC_FLAGS_HANDLERS_INSTALLED, &ec->flags)) return; - acpi_disable_gpe(NULL, ec->gpe); + acpi_ec_disable_gpe(ec, true); if (ACPI_FAILURE(acpi_remove_address_space_handler(ec->handle, ACPI_ADR_SPACE_EC, &acpi_ec_space_handler))) pr_err("failed to remove space handler\n"); -- cgit v1.2.3-70-g09d2 From 9e295ac14d6a59180beed0735e6a504c2ee87761 Mon Sep 17 00:00:00 2001 From: Lv Zheng Date: Thu, 5 Feb 2015 16:27:29 +0800 Subject: ACPI / EC: Reduce ec_poll() by referencing the last register access timestamp. Timeout in the ec_poll() doesn't refer to the last register access time. It thus can win the competition against the acpi_ec_gpe_handler() if a transaction takes longer than 1ms but individual register accesses are less than 1ms. In some cases, it can make the following silicon bug easier to be triggered: GPE EN is not wired to the GPE trigger line, so when GPE STS is already set when 1 is written to GPE EN, no GPE can be triggered. This patch adds register access timestamp reference support for ec_poll() to reduce the number of ec_poll() invocations. Reported-by: Venkat Raghavulu Signed-off-by: Lv Zheng Signed-off-by: Rafael J. Wysocki --- drivers/acpi/ec.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index 2540870e89f..e000cf70378 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -71,6 +71,7 @@ enum ec_command { #define ACPI_EC_DELAY 500 /* Wait 500ms max. during EC ops */ #define ACPI_EC_UDELAY_GLK 1000 /* Wait 1ms max. to get global lock */ #define ACPI_EC_MSI_UDELAY 550 /* Wait 550us for MSI EC */ +#define ACPI_EC_UDELAY_POLL 1000 /* Wait 1ms for EC transaction polling */ #define ACPI_EC_CLEAR_MAX 100 /* Maximum number of events to query * when trying to clear the EC */ @@ -118,6 +119,7 @@ struct transaction { u8 wlen; u8 rlen; u8 flags; + unsigned long timestamp; }; static int acpi_ec_query(struct acpi_ec *ec, u8 *data); @@ -155,6 +157,7 @@ static inline u8 acpi_ec_read_data(struct acpi_ec *ec) { u8 x = inb(ec->data_addr); + ec->curr->timestamp = jiffies; pr_debug("EC_DATA(R) = 0x%2.2x\n", x); return x; } @@ -163,12 +166,14 @@ static inline void acpi_ec_write_cmd(struct acpi_ec *ec, u8 command) { pr_debug("EC_SC(W) = 0x%2.2x\n", command); outb(command, ec->command_addr); + ec->curr->timestamp = jiffies; } static inline void acpi_ec_write_data(struct acpi_ec *ec, u8 data) { pr_debug("EC_DATA(W) = 0x%2.2x\n", data); outb(data, ec->data_addr); + ec->curr->timestamp = jiffies; } #ifdef DEBUG @@ -359,6 +364,7 @@ static void start_transaction(struct acpi_ec *ec) { ec->curr->irq_count = ec->curr->wi = ec->curr->ri = 0; ec->curr->flags = 0; + ec->curr->timestamp = jiffies; advance_transaction(ec); } @@ -370,20 +376,25 @@ static int ec_poll(struct acpi_ec *ec) while (repeat--) { unsigned long delay = jiffies + msecs_to_jiffies(ec_delay); + unsigned long usecs = ACPI_EC_UDELAY_POLL; do { /* don't sleep with disabled interrupts */ if (EC_FLAGS_MSI || irqs_disabled()) { - udelay(ACPI_EC_MSI_UDELAY); + usecs = ACPI_EC_MSI_UDELAY; + udelay(usecs); if (ec_transaction_completed(ec)) return 0; } else { if (wait_event_timeout(ec->wait, ec_transaction_completed(ec), - msecs_to_jiffies(1))) + usecs_to_jiffies(usecs))) return 0; } spin_lock_irqsave(&ec->lock, flags); - advance_transaction(ec); + if (time_after(jiffies, + ec->curr->timestamp + + usecs_to_jiffies(usecs))) + advance_transaction(ec); spin_unlock_irqrestore(&ec->lock, flags); } while (time_before(jiffies, delay)); pr_debug("controller reset, restart transaction\n"); -- cgit v1.2.3-70-g09d2 From a8d4fc227f312edea06bb4ebbeeb6db89c798e91 Mon Sep 17 00:00:00 2001 From: Lv Zheng Date: Thu, 5 Feb 2015 16:27:35 +0800 Subject: ACPI / EC: Update revision due to raw handler mode. The bug fixes around GPE races have been done to the EC driver by the previous commits. This patch increases the revision to 3 to indicate the behavior differences between the old and the new drivers. The copyright/authorship notices are also updated. Signed-off-by: Lv Zheng Signed-off-by: Rafael J. Wysocki --- drivers/acpi/ec.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index e000cf70378..55632539392 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -1,8 +1,8 @@ /* - * ec.c - ACPI Embedded Controller Driver (v2.2) + * ec.c - ACPI Embedded Controller Driver (v3) * - * Copyright (C) 2001-2014 Intel Corporation - * Author: 2014 Lv Zheng + * Copyright (C) 2001-2015 Intel Corporation + * Author: 2014, 2015 Lv Zheng * 2006, 2007 Alexey Starikovskiy * 2006 Denis Sadykov * 2004 Luming Yu -- cgit v1.2.3-70-g09d2 From ad479e7f47ca09c3f3190603ead4d01cf8fe6fa8 Mon Sep 17 00:00:00 2001 From: Lv Zheng Date: Fri, 6 Feb 2015 08:57:52 +0800 Subject: ACPI / EC: Introduce STARTED/STOPPED flags to replace BLOCKED flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit By using the 2 flags, we can indicate an inter-mediate state where the current transactions should be completed while the new transactions should be dropped. The comparison of the old flag and the new flags: Old New about to set BLOCKED STOPPED set / STARTED set BLOCKED set STOPPED clear / STARTED clear BLOCKED clear STOPPED clear / STARTED set A new period can be indicated by the 2 flags. The new period is between the point where we are about to set BLOCKED and the point when the BLOCKED is set. The new flags facilitate us with acpi_ec_started() check to allow the EC transaction to be submitted during the new period. This period thus can be used as a grace period for the EC transaction flushing. The only functional change after applying this patch is: 1. The GPE enabling/disabling is protected by the EC specific lock. We can do this because of recent ACPICA GPE API enhancement. This is reasonable as the GPE disabling/enabling state should only be determined by the EC driver's state machine which is protected by the EC spinlock. Signed-off-by: Lv Zheng Tested-by: Ortwin Glück Signed-off-by: Rafael J. Wysocki --- drivers/acpi/ec.c | 65 +++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 54 insertions(+), 11 deletions(-) diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index 55632539392..a6179b71c57 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -80,7 +80,8 @@ enum { EC_FLAGS_GPE_STORM, /* GPE storm detected */ EC_FLAGS_HANDLERS_INSTALLED, /* Handlers for GPE and * OpReg are installed */ - EC_FLAGS_BLOCKED, /* Transactions are blocked */ + EC_FLAGS_STARTED, /* Driver is started */ + EC_FLAGS_STOPPED, /* Driver is stopped */ }; #define ACPI_EC_COMMAND_POLL 0x01 /* Available for command byte */ @@ -134,6 +135,16 @@ static int EC_FLAGS_SKIP_DSDT_SCAN; /* Not all BIOS survive early DSDT scan */ static int EC_FLAGS_CLEAR_ON_RESUME; /* Needs acpi_ec_clear() on boot/resume */ static int EC_FLAGS_QUERY_HANDSHAKE; /* Needs QR_EC issued when SCI_EVT set */ +/* -------------------------------------------------------------------------- + * Device Flags + * -------------------------------------------------------------------------- */ + +static bool acpi_ec_started(struct acpi_ec *ec) +{ + return test_bit(EC_FLAGS_STARTED, &ec->flags) && + !test_bit(EC_FLAGS_STOPPED, &ec->flags); +} + /* -------------------------------------------------------------------------- * EC Registers * -------------------------------------------------------------------------- */ @@ -415,6 +426,10 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, udelay(ACPI_EC_MSI_UDELAY); /* start transaction */ spin_lock_irqsave(&ec->lock, tmp); + if (!acpi_ec_started(ec)) { + ret = -EINVAL; + goto unlock; + } /* following two actions should be kept atomic */ ec->curr = t; pr_debug("***** Command(%s) started *****\n", @@ -426,6 +441,7 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, pr_debug("***** Command(%s) stopped *****\n", acpi_ec_cmd_string(t->command)); ec->curr = NULL; +unlock: spin_unlock_irqrestore(&ec->lock, tmp); return ret; } @@ -440,10 +456,6 @@ static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t) if (t->rdata) memset(t->rdata, 0, t->rlen); mutex_lock(&ec->mutex); - if (test_bit(EC_FLAGS_BLOCKED, &ec->flags)) { - status = -EINVAL; - goto unlock; - } if (ec->global_lock) { status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk); if (ACPI_FAILURE(status)) { @@ -595,6 +607,37 @@ static void acpi_ec_clear(struct acpi_ec *ec) pr_info("%d stale EC events cleared\n", i); } +static void acpi_ec_start(struct acpi_ec *ec, bool resuming) +{ + unsigned long flags; + + spin_lock_irqsave(&ec->lock, flags); + if (!test_and_set_bit(EC_FLAGS_STARTED, &ec->flags)) { + pr_debug("+++++ Starting EC +++++\n"); + if (!resuming) + acpi_ec_enable_gpe(ec, true); + pr_info("+++++ EC started +++++\n"); + } + spin_unlock_irqrestore(&ec->lock, flags); +} + +static void acpi_ec_stop(struct acpi_ec *ec, bool suspending) +{ + unsigned long flags; + + spin_lock_irqsave(&ec->lock, flags); + if (acpi_ec_started(ec)) { + pr_debug("+++++ Stopping EC +++++\n"); + set_bit(EC_FLAGS_STOPPED, &ec->flags); + if (!suspending) + acpi_ec_disable_gpe(ec, true); + clear_bit(EC_FLAGS_STARTED, &ec->flags); + clear_bit(EC_FLAGS_STOPPED, &ec->flags); + pr_info("+++++ EC stopped +++++\n"); + } + spin_unlock_irqrestore(&ec->lock, flags); +} + void acpi_ec_block_transactions(void) { struct acpi_ec *ec = first_ec; @@ -604,7 +647,7 @@ void acpi_ec_block_transactions(void) mutex_lock(&ec->mutex); /* Prevent transactions from being carried out */ - set_bit(EC_FLAGS_BLOCKED, &ec->flags); + acpi_ec_stop(ec, true); mutex_unlock(&ec->mutex); } @@ -616,7 +659,7 @@ void acpi_ec_unblock_transactions(void) return; /* Allow transactions to be carried out again */ - clear_bit(EC_FLAGS_BLOCKED, &ec->flags); + acpi_ec_start(ec, true); if (EC_FLAGS_CLEAR_ON_RESUME) acpi_ec_clear(ec); @@ -629,7 +672,7 @@ void acpi_ec_unblock_transactions_early(void) * atomic context during wakeup, so we don't need to acquire the mutex). */ if (first_ec) - clear_bit(EC_FLAGS_BLOCKED, &first_ec->flags); + acpi_ec_start(first_ec, true); } /* -------------------------------------------------------------------------- @@ -894,7 +937,7 @@ static int ec_install_handlers(struct acpi_ec *ec) if (ACPI_FAILURE(status)) return -ENODEV; - acpi_ec_enable_gpe(ec, true); + acpi_ec_start(ec, false); status = acpi_install_address_space_handler(ec->handle, ACPI_ADR_SPACE_EC, &acpi_ec_space_handler, @@ -909,7 +952,7 @@ static int ec_install_handlers(struct acpi_ec *ec) pr_err("Fail in evaluating the _REG object" " of EC device. Broken bios is suspected.\n"); } else { - acpi_ec_disable_gpe(ec, true); + acpi_ec_stop(ec, false); acpi_remove_gpe_handler(NULL, ec->gpe, &acpi_ec_gpe_handler); return -ENODEV; @@ -924,7 +967,7 @@ static void ec_remove_handlers(struct acpi_ec *ec) { if (!test_bit(EC_FLAGS_HANDLERS_INSTALLED, &ec->flags)) return; - acpi_ec_disable_gpe(ec, true); + acpi_ec_stop(ec, false); if (ACPI_FAILURE(acpi_remove_address_space_handler(ec->handle, ACPI_ADR_SPACE_EC, &acpi_ec_space_handler))) pr_err("failed to remove space handler\n"); -- cgit v1.2.3-70-g09d2 From 9887d22add48f24ca3a7605c89b0a21ed337f185 Mon Sep 17 00:00:00 2001 From: Lv Zheng Date: Fri, 6 Feb 2015 08:57:59 +0800 Subject: ACPI / EC: Add command flushing support. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch implements the EC command flushing support. During the grace period indicated by EC_FLAGS_STARTED and EC_FLAGS_STOPPED, all submitted EC command transactions can be completed and new submissions are prevented before suspending so that the EC hardware can be ensured to be in the idle state when the system is resumed. There is a good indicator for flush support: All acpi_ec_submit_request() is invoked after checking driver state with acpi_ec_started() except the first one. This means all code paths can be flushed as fast as possible by discarding the requests occurred after the flush operation. The reference increased for such kind of code path is wrapped by acpi_ec_submit_flushable_request(). Signed-off-by: Lv Zheng Tested-by: Ortwin Glück Signed-off-by: Rafael J. Wysocki --- drivers/acpi/ec.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++--- drivers/acpi/internal.h | 1 + 2 files changed, 66 insertions(+), 3 deletions(-) diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index a6179b71c57..1fa14634c3f 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -145,6 +145,11 @@ static bool acpi_ec_started(struct acpi_ec *ec) !test_bit(EC_FLAGS_STOPPED, &ec->flags); } +static bool acpi_ec_flushed(struct acpi_ec *ec) +{ + return ec->reference_count == 1; +} + /* -------------------------------------------------------------------------- * EC Registers * -------------------------------------------------------------------------- */ @@ -266,6 +271,44 @@ static inline void acpi_ec_clear_gpe(struct acpi_ec *ec) * Transaction Management * -------------------------------------------------------------------------- */ +static void acpi_ec_submit_request(struct acpi_ec *ec) +{ + ec->reference_count++; + if (ec->reference_count == 1) + acpi_ec_enable_gpe(ec, true); +} + +static void acpi_ec_complete_request(struct acpi_ec *ec) +{ + bool flushed = false; + + ec->reference_count--; + if (ec->reference_count == 0) + acpi_ec_disable_gpe(ec, true); + flushed = acpi_ec_flushed(ec); + if (flushed) + wake_up(&ec->wait); +} + +/* + * acpi_ec_submit_flushable_request() - Increase the reference count unless + * the flush operation is not in + * progress + * @ec: the EC device + * + * 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. + */ +static bool acpi_ec_submit_flushable_request(struct acpi_ec *ec) +{ + if (!acpi_ec_started(ec)) + return false; + acpi_ec_submit_request(ec); + return true; +} + static void acpi_ec_submit_query(struct acpi_ec *ec) { if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) { @@ -426,7 +469,8 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, udelay(ACPI_EC_MSI_UDELAY); /* start transaction */ spin_lock_irqsave(&ec->lock, tmp); - if (!acpi_ec_started(ec)) { + /* Enable GPE for command processing (IBF=0/OBF=1) */ + if (!acpi_ec_submit_flushable_request(ec)) { ret = -EINVAL; goto unlock; } @@ -441,6 +485,8 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, pr_debug("***** Command(%s) stopped *****\n", acpi_ec_cmd_string(t->command)); ec->curr = NULL; + /* Disable GPE for command processing (IBF=0/OBF=1) */ + acpi_ec_complete_request(ec); unlock: spin_unlock_irqrestore(&ec->lock, tmp); return ret; @@ -614,13 +660,25 @@ static void acpi_ec_start(struct acpi_ec *ec, bool resuming) spin_lock_irqsave(&ec->lock, flags); if (!test_and_set_bit(EC_FLAGS_STARTED, &ec->flags)) { pr_debug("+++++ Starting EC +++++\n"); + /* Enable GPE for event processing (SCI_EVT=1) */ if (!resuming) - acpi_ec_enable_gpe(ec, true); + acpi_ec_submit_request(ec); pr_info("+++++ EC started +++++\n"); } spin_unlock_irqrestore(&ec->lock, flags); } +static bool acpi_ec_stopped(struct acpi_ec *ec) +{ + unsigned long flags; + bool flushed; + + spin_lock_irqsave(&ec->lock, flags); + flushed = acpi_ec_flushed(ec); + spin_unlock_irqrestore(&ec->lock, flags); + return flushed; +} + static void acpi_ec_stop(struct acpi_ec *ec, bool suspending) { unsigned long flags; @@ -629,8 +687,12 @@ static void acpi_ec_stop(struct acpi_ec *ec, bool suspending) if (acpi_ec_started(ec)) { pr_debug("+++++ Stopping EC +++++\n"); set_bit(EC_FLAGS_STOPPED, &ec->flags); + spin_unlock_irqrestore(&ec->lock, flags); + wait_event(ec->wait, acpi_ec_stopped(ec)); + spin_lock_irqsave(&ec->lock, flags); + /* Disable GPE for event processing (SCI_EVT=1) */ if (!suspending) - acpi_ec_disable_gpe(ec, true); + acpi_ec_complete_request(ec); clear_bit(EC_FLAGS_STARTED, &ec->flags); clear_bit(EC_FLAGS_STOPPED, &ec->flags); pr_info("+++++ EC stopped +++++\n"); diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index dc420787ffc..7dc69d82f65 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -122,6 +122,7 @@ struct acpi_ec { unsigned long data_addr; unsigned long global_lock; unsigned long flags; + unsigned long reference_count; struct mutex mutex; wait_queue_head_t wait; struct list_head list; -- cgit v1.2.3-70-g09d2 From e1d4d90fc0313d3d58cbd7912c90f8ef24df45ff Mon Sep 17 00:00:00 2001 From: Lv Zheng Date: Fri, 6 Feb 2015 08:58:05 +0800 Subject: ACPI / EC: Refine command storm prevention support This patch refines EC command storm prevention support. Current command storming code is wrong, when the storming condition is detected, it only flags the condition without doing anything for the current command but performing storming prevention for the follow-up commands. So: 1. The first command which suffers from the storming still suffers from storming. 2. The follow-up commands which may not suffer from the storming are unconditionally forced into the storming prevention mode. Ideally, we should only enable storm prevention immediately after detection for the current command so that the next command can try the power/performance efficient interrupt mode again. This patch improves the command storm prevention by disabling GPE right after the detection and re-enabling it right before completing the command transaction using the GPE storming prevention APIs. This thus deploys the following GPE handling model: 1. acpi_enable_gpe()/acpi_disable_gpe() for reference count changes: This set of APIs are used for EC usage reference counting. 2. acpi_set_gpe(ACPI_GPE_ENABLE)/acpi_set_gpe(ACPI_GPE_DISABLE): This set of APIs are used for preventing GPE storm. They must be invoked when the reference count > 0. Note that as the storming prevention should always happen when there is an outstanding request, or GPE enabling value will be messed up by the races. This patch also adds BUG_ON() to enforces this rule to prevent future bugs. The msleep(1) used after completing a transaction is useless now as this sounds like a guard time only useful for platforms that need the EC_FLAGS_MSI quirks while we have fixed GPE race issues using the previous raw handler mode enabling. It is kept to avoid regressions. A seperate patch which deletes EC_FLAGS_MSI quirks should take care of deleting it. Signed-off-by: Lv Zheng Signed-off-by: Rafael J. Wysocki --- drivers/acpi/ec.c | 55 ++++++++++++++++++++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 19 deletions(-) diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index 1fa14634c3f..982b67faaaf 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -77,11 +77,12 @@ enum ec_command { enum { EC_FLAGS_QUERY_PENDING, /* Query is pending */ - EC_FLAGS_GPE_STORM, /* GPE storm detected */ EC_FLAGS_HANDLERS_INSTALLED, /* Handlers for GPE and * OpReg are installed */ EC_FLAGS_STARTED, /* Driver is started */ EC_FLAGS_STOPPED, /* Driver is stopped */ + EC_FLAGS_COMMAND_STORM, /* GPE storms occurred to the + * current command processing */ }; #define ACPI_EC_COMMAND_POLL 0x01 /* Available for command byte */ @@ -229,8 +230,10 @@ static inline void acpi_ec_enable_gpe(struct acpi_ec *ec, bool open) { if (open) acpi_enable_gpe(NULL, ec->gpe); - else + else { + BUG_ON(ec->reference_count < 1); acpi_set_gpe(NULL, ec->gpe, ACPI_GPE_ENABLE); + } if (acpi_ec_is_gpe_raised(ec)) { /* * On some platforms, EN=1 writes cannot trigger GPE. So @@ -246,8 +249,10 @@ static inline void acpi_ec_disable_gpe(struct acpi_ec *ec, bool close) { if (close) acpi_disable_gpe(NULL, ec->gpe); - else + else { + BUG_ON(ec->reference_count < 1); acpi_set_gpe(NULL, ec->gpe, ACPI_GPE_DISABLE); + } } static inline void acpi_ec_clear_gpe(struct acpi_ec *ec) @@ -290,6 +295,24 @@ static void acpi_ec_complete_request(struct acpi_ec *ec) wake_up(&ec->wait); } +static void acpi_ec_set_storm(struct acpi_ec *ec, u8 flag) +{ + if (!test_bit(flag, &ec->flags)) { + acpi_ec_disable_gpe(ec, false); + pr_debug("+++++ Polling enabled +++++\n"); + set_bit(flag, &ec->flags); + } +} + +static void acpi_ec_clear_storm(struct acpi_ec *ec, u8 flag) +{ + if (test_bit(flag, &ec->flags)) { + clear_bit(flag, &ec->flags); + acpi_ec_enable_gpe(ec, false); + pr_debug("+++++ Polling disabled +++++\n"); + } +} + /* * acpi_ec_submit_flushable_request() - Increase the reference count unless * the flush operation is not in @@ -404,8 +427,13 @@ err: * otherwise will take a not handled IRQ as a false one. */ if (!(status & ACPI_EC_FLAG_SCI)) { - if (in_interrupt() && t) - ++t->irq_count; + if (in_interrupt() && t) { + if (t->irq_count < ec_storm_threshold) + ++t->irq_count; + /* Allow triggering on 0 threshold */ + if (t->irq_count == ec_storm_threshold) + acpi_ec_set_storm(ec, EC_FLAGS_COMMAND_STORM); + } } out: if (status & ACPI_EC_FLAG_SCI) @@ -482,6 +510,8 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, spin_unlock_irqrestore(&ec->lock, tmp); ret = ec_poll(ec); spin_lock_irqsave(&ec->lock, tmp); + if (t->irq_count == ec_storm_threshold) + acpi_ec_clear_storm(ec, EC_FLAGS_COMMAND_STORM); pr_debug("***** Command(%s) stopped *****\n", acpi_ec_cmd_string(t->command)); ec->curr = NULL; @@ -509,24 +539,11 @@ static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t) goto unlock; } } - /* disable GPE during transaction if storm is detected */ - if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) { - /* It has to be disabled, so that it doesn't trigger. */ - acpi_ec_disable_gpe(ec, false); - } status = acpi_ec_transaction_unlocked(ec, t); - if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) { + if (test_bit(EC_FLAGS_COMMAND_STORM, &ec->flags)) msleep(1); - /* It is safe to enable the GPE outside of the transaction. */ - acpi_ec_enable_gpe(ec, false); - } else if (t->irq_count > ec_storm_threshold) { - pr_info("GPE storm detected(%d GPEs), " - "transactions will use polling mode\n", - t->irq_count); - set_bit(EC_FLAGS_GPE_STORM, &ec->flags); - } if (ec->global_lock) acpi_release_global_lock(glk); unlock: -- cgit v1.2.3-70-g09d2 From f252cb09e1cb46834014aaa3814fbfb2352e9071 Mon Sep 17 00:00:00 2001 From: Lv Zheng Date: Fri, 6 Feb 2015 08:58:10 +0800 Subject: ACPI / EC: Add query flushing support MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch implementes the QR_EC flushing support. Grace periods are implemented from the detection of an SCI_EVT to the submission/completion of the QR_EC transaction. During this period, all EC command transactions are allowed to be submitted. Note that query periods and event periods are intentionally distiguished to allow further improvements. 1. Query period: from the detection of an SCI_EVT to the sumission of the QR_EC command. This period is used for storming prevention, as currently QR_EC is deferred to a work queue rather than directly issued from the IRQ context even there is no other transactions pending, so malicous SCI_EVT GPE can act like "level triggered" to trigger a GPE storm. We need to be prepared for this. And in the future, we may change it to be a part of the advance_transaction() where we will try QR_EC submission in appropriate positions to avoid such GPE storming. 2. Event period: from the detection of an SCI_EVT to the completion of the QR_EC command. We may extend it to the completion of _Qxx evaluation. This is actually a grace period for event flushing, but we only flush queries due to the reason stated in known issue 1. That's also why we use EC_FLAGS_EVENT_xxx. During this period, QR_EC transactions need to pass the flushable submission check. In this patch, the following flags are implemented: 1. EC_FLAGS_EVENT_ENABLED: this is derived from the old EC_FLAGS_QUERY_PENDING flag which can block SCI_EVT handlings. With this flag, the logics implemented by the original flag are extended: 1. Old logic: unless both of the flags are set, the event poller will not be scheduled, and 2. New logic: as soon as both of the flags are set, the evet poller will be scheduled. 2. EC_FLAGS_EVENT_DETECTED: this is also derived from the old EC_FLAGS_QUERY_PENDING flag which can block SCI_EVT detection. It thus can be used to indicate the storming prevention period for query submission. acpi_ec_submit_request()/acpi_ec_complete_request() are invoked to implement this period so that acpi_set_gpe() can be invoked under the "reference count > 0" condition. 3. EC_FLAGS_EVENT_PENDING: this is newly added to indicate the grace period for event flushing (query flushing for now). acpi_ec_submit_request()/acpi_ec_complete_request() are invoked to implement this period so that the flushing process can wait until the event handling (query transaction for now) to be completed. Link: https://bugzilla.kernel.org/show_bug.cgi?id=82611 Link: https://bugzilla.kernel.org/show_bug.cgi?id=77431 Signed-off-by: Lv Zheng Tested-by: Ortwin Glück Signed-off-by: Rafael J. Wysocki --- drivers/acpi/ec.c | 101 +++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 85 insertions(+), 16 deletions(-) diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index 982b67faaaf..40002ae7db2 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -76,7 +76,9 @@ 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_EVENT_DETECTED, /* Event is detected */ EC_FLAGS_HANDLERS_INSTALLED, /* Handlers for GPE and * OpReg are installed */ EC_FLAGS_STARTED, /* Driver is started */ @@ -151,6 +153,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_DETECTED, &ec->flags) || + test_bit(EC_FLAGS_EVENT_PENDING, &ec->flags); +} + /* -------------------------------------------------------------------------- * EC Registers * -------------------------------------------------------------------------- */ @@ -318,36 +326,93 @@ static void acpi_ec_clear_storm(struct acpi_ec *ec, u8 flag) * the flush operation is not in * progress * @ec: the EC device + * @allow_event: whether event should be handled * * 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 allow_event) { - if (!acpi_ec_started(ec)) - return false; + if (!acpi_ec_started(ec)) { + if (!allow_event || !acpi_ec_has_pending_event(ec)) + return false; + } acpi_ec_submit_request(ec); return true; } -static void acpi_ec_submit_query(struct acpi_ec *ec) +static void acpi_ec_submit_event(struct acpi_ec *ec) { - if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) { - pr_debug("***** Event started *****\n"); + if (!test_bit(EC_FLAGS_EVENT_DETECTED, &ec->flags) || + !test_bit(EC_FLAGS_EVENT_ENABLED, &ec->flags)) + return; + /* Hold reference for pending event */ + if (!acpi_ec_submit_flushable_request(ec, true)) + return; + if (!test_and_set_bit(EC_FLAGS_EVENT_PENDING, &ec->flags)) { + pr_debug("***** Event query started *****\n"); schedule_work(&ec->work); + return; + } + acpi_ec_complete_request(ec); +} + +static void acpi_ec_complete_event(struct acpi_ec *ec) +{ + if (ec->curr->command == ACPI_EC_COMMAND_QUERY) { + clear_bit(EC_FLAGS_EVENT_PENDING, &ec->flags); + pr_debug("***** Event query stopped *****\n"); + /* Unhold reference for pending event */ + acpi_ec_complete_request(ec); + /* Check if there is another SCI_EVT detected */ + acpi_ec_submit_event(ec); + } +} + +static void acpi_ec_submit_detection(struct acpi_ec *ec) +{ + /* Hold reference for query submission */ + if (!acpi_ec_submit_flushable_request(ec, false)) + return; + if (!test_and_set_bit(EC_FLAGS_EVENT_DETECTED, &ec->flags)) { + pr_debug("***** Event detection blocked *****\n"); + acpi_ec_submit_event(ec); + return; } + acpi_ec_complete_request(ec); } -static void acpi_ec_complete_query(struct acpi_ec *ec) +static void acpi_ec_complete_detection(struct acpi_ec *ec) { if (ec->curr->command == ACPI_EC_COMMAND_QUERY) { - clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags); - pr_debug("***** Event stopped *****\n"); + clear_bit(EC_FLAGS_EVENT_DETECTED, &ec->flags); + pr_debug("***** Event detetion unblocked *****\n"); + /* Unhold reference for query submission */ + acpi_ec_complete_request(ec); } } +static void acpi_ec_enable_event(struct acpi_ec *ec) +{ + unsigned long flags; + + spin_lock_irqsave(&ec->lock, flags); + set_bit(EC_FLAGS_EVENT_ENABLED, &ec->flags); + /* + * An event may be pending even with SCI_EVT=0, so QR_EC should + * always be issued right after started. + */ + acpi_ec_submit_detection(ec); + spin_unlock_irqrestore(&ec->lock, flags); +} + static int ec_transaction_completed(struct acpi_ec *ec) { unsigned long flags; @@ -389,6 +454,7 @@ static void advance_transaction(struct acpi_ec *ec) t->rdata[t->ri++] = acpi_ec_read_data(ec); if (t->rlen == t->ri) { t->flags |= ACPI_EC_COMMAND_COMPLETE; + acpi_ec_complete_event(ec); if (t->command == ACPI_EC_COMMAND_QUERY) pr_debug("***** Command(%s) hardware completion *****\n", acpi_ec_cmd_string(t->command)); @@ -399,6 +465,7 @@ static void advance_transaction(struct acpi_ec *ec) } else if (t->wlen == t->wi && (status & ACPI_EC_FLAG_IBF) == 0) { t->flags |= ACPI_EC_COMMAND_COMPLETE; + acpi_ec_complete_event(ec); wakeup = true; } goto out; @@ -407,16 +474,17 @@ static void advance_transaction(struct acpi_ec *ec) !(status & ACPI_EC_FLAG_SCI) && (t->command == ACPI_EC_COMMAND_QUERY)) { t->flags |= ACPI_EC_COMMAND_POLL; - acpi_ec_complete_query(ec); + acpi_ec_complete_detection(ec); t->rdata[t->ri++] = 0x00; t->flags |= ACPI_EC_COMMAND_COMPLETE; + acpi_ec_complete_event(ec); pr_debug("***** Command(%s) software completion *****\n", acpi_ec_cmd_string(t->command)); wakeup = true; } else if ((status & ACPI_EC_FLAG_IBF) == 0) { acpi_ec_write_cmd(ec, t->command); t->flags |= ACPI_EC_COMMAND_POLL; - acpi_ec_complete_query(ec); + acpi_ec_complete_detection(ec); } else goto err; goto out; @@ -437,7 +505,7 @@ err: } out: if (status & ACPI_EC_FLAG_SCI) - acpi_ec_submit_query(ec); + acpi_ec_submit_detection(ec); if (wakeup && in_interrupt()) wake_up(&ec->wait); } @@ -498,7 +566,7 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, /* 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, true)) { ret = -EINVAL; goto unlock; } @@ -879,7 +947,9 @@ static void acpi_ec_gpe_poller(struct work_struct *work) { struct acpi_ec *ec = container_of(work, struct acpi_ec, work); + pr_debug("***** Event poller started *****\n"); acpi_ec_query(ec, NULL); + pr_debug("***** Event poller stopped *****\n"); } static u32 acpi_ec_gpe_handler(acpi_handle gpe_device, @@ -949,7 +1019,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); @@ -1100,7 +1169,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) -- cgit v1.2.3-70-g09d2 From b5bca896ef3c4dd8aa1aac55256cb094c7376a77 Mon Sep 17 00:00:00 2001 From: Lv Zheng Date: Fri, 6 Feb 2015 08:58:16 +0800 Subject: ACPI / EC: Add GPE reference counting debugging messages This patch enhances debugging with the GPE reference count messages added. No functional changes. Signed-off-by: Lv Zheng Signed-off-by: Rafael J. Wysocki --- drivers/acpi/ec.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index 40002ae7db2..14d0c89ada2 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -31,6 +31,7 @@ /* Uncomment next line to get verbose printout */ /* #define DEBUG */ +#define DEBUG_REF 0 #define pr_fmt(fmt) "ACPI : EC: " fmt #include @@ -90,6 +91,13 @@ enum { #define ACPI_EC_COMMAND_POLL 0x01 /* Available for command byte */ #define ACPI_EC_COMMAND_COMPLETE 0x02 /* Completed last byte */ +#define ec_debug_ref(ec, fmt, ...) \ + do { \ + if (DEBUG_REF) \ + pr_debug("%lu: " fmt, ec->reference_count, \ + ## __VA_ARGS__); \ + } while (0) + /* ec.c is compiled in acpi namespace so this shows up as acpi.ec_delay param */ static unsigned int ec_delay __read_mostly = ACPI_EC_DELAY; module_param(ec_delay, uint, 0644); @@ -356,12 +364,14 @@ static void acpi_ec_submit_event(struct acpi_ec *ec) /* Hold reference for pending event */ if (!acpi_ec_submit_flushable_request(ec, true)) return; + ec_debug_ref(ec, "Increase event\n"); if (!test_and_set_bit(EC_FLAGS_EVENT_PENDING, &ec->flags)) { pr_debug("***** Event query started *****\n"); schedule_work(&ec->work); return; } acpi_ec_complete_request(ec); + ec_debug_ref(ec, "Decrease event\n"); } static void acpi_ec_complete_event(struct acpi_ec *ec) @@ -371,6 +381,7 @@ static void acpi_ec_complete_event(struct acpi_ec *ec) pr_debug("***** Event query stopped *****\n"); /* Unhold reference for pending event */ acpi_ec_complete_request(ec); + ec_debug_ref(ec, "Decrease event\n"); /* Check if there is another SCI_EVT detected */ acpi_ec_submit_event(ec); } @@ -381,12 +392,14 @@ static void acpi_ec_submit_detection(struct acpi_ec *ec) /* Hold reference for query submission */ if (!acpi_ec_submit_flushable_request(ec, false)) return; + ec_debug_ref(ec, "Increase query\n"); if (!test_and_set_bit(EC_FLAGS_EVENT_DETECTED, &ec->flags)) { pr_debug("***** Event detection blocked *****\n"); acpi_ec_submit_event(ec); return; } acpi_ec_complete_request(ec); + ec_debug_ref(ec, "Decrease query\n"); } static void acpi_ec_complete_detection(struct acpi_ec *ec) @@ -396,6 +409,7 @@ static void acpi_ec_complete_detection(struct acpi_ec *ec) pr_debug("***** Event detetion unblocked *****\n"); /* Unhold reference for query submission */ acpi_ec_complete_request(ec); + ec_debug_ref(ec, "Decrease query\n"); } } @@ -570,6 +584,7 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, ret = -EINVAL; goto unlock; } + ec_debug_ref(ec, "Increase command\n"); /* following two actions should be kept atomic */ ec->curr = t; pr_debug("***** Command(%s) started *****\n", @@ -585,6 +600,7 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, ec->curr = NULL; /* Disable GPE for command processing (IBF=0/OBF=1) */ acpi_ec_complete_request(ec); + ec_debug_ref(ec, "Decrease command\n"); unlock: spin_unlock_irqrestore(&ec->lock, tmp); return ret; @@ -746,8 +762,10 @@ static void acpi_ec_start(struct acpi_ec *ec, bool resuming) if (!test_and_set_bit(EC_FLAGS_STARTED, &ec->flags)) { pr_debug("+++++ Starting EC +++++\n"); /* Enable GPE for event processing (SCI_EVT=1) */ - if (!resuming) + if (!resuming) { acpi_ec_submit_request(ec); + ec_debug_ref(ec, "Increase driver\n"); + } pr_info("+++++ EC started +++++\n"); } spin_unlock_irqrestore(&ec->lock, flags); @@ -776,8 +794,10 @@ static void acpi_ec_stop(struct acpi_ec *ec, bool suspending) wait_event(ec->wait, acpi_ec_stopped(ec)); spin_lock_irqsave(&ec->lock, flags); /* Disable GPE for event processing (SCI_EVT=1) */ - if (!suspending) + if (!suspending) { acpi_ec_complete_request(ec); + ec_debug_ref(ec, "Decrease driver\n"); + } clear_bit(EC_FLAGS_STARTED, &ec->flags); clear_bit(EC_FLAGS_STOPPED, &ec->flags); pr_info("+++++ EC stopped +++++\n"); -- cgit v1.2.3-70-g09d2