From 2e84ea5a3269f9e1d4e7658a9893f5eac4aee5ec Mon Sep 17 00:00:00 2001 From: Sebastian Andrzej Siewior Date: Fri, 13 Nov 2020 19:13:17 +0100 Subject: [PATCH 1/6] ACPI: EC: Eliminate in_interrupt() usage advance_transaction() is using in_interrupt() to distinguish between an invocation from the interrupt handler and an invocation from another part of the stack. This looks misleading because chains like acpi_update_all_gpes() -> acpi_ev_gpe_detect() -> acpi_ev_detect_gpe() -> acpi_ec_gpe_handler() should probably also behave as if they were called from an interrupt handler. Replace in_interrupt() usage with a function parameter. Set this parameter to `true' if invoked from an interrupt handler (acpi_ec_gpe_handler() and acpi_ec_irq_handler()) and `false' otherwise. Signed-off-by: Sebastian Andrzej Siewior [ rjw: Subject edits ] Signed-off-by: Rafael J. Wysocki --- drivers/acpi/ec.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index e0cb1bcfffb2..0caf5ca1fc07 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -169,7 +169,7 @@ struct acpi_ec_query { }; static int acpi_ec_query(struct acpi_ec *ec, u8 *data); -static void advance_transaction(struct acpi_ec *ec); +static void advance_transaction(struct acpi_ec *ec, bool interrupt); static void acpi_ec_event_handler(struct work_struct *work); static void acpi_ec_event_processor(struct work_struct *work); @@ -358,7 +358,7 @@ static inline void acpi_ec_enable_gpe(struct acpi_ec *ec, bool open) * EN=1 writes. */ ec_dbg_raw("Polling quirk"); - advance_transaction(ec); + advance_transaction(ec, false); } } @@ -488,7 +488,7 @@ static inline void __acpi_ec_enable_event(struct acpi_ec *ec) * Unconditionally invoke this once after enabling the event * handling mechanism to detect the pending events. */ - advance_transaction(ec); + advance_transaction(ec, false); } static inline void __acpi_ec_disable_event(struct acpi_ec *ec) @@ -632,14 +632,13 @@ static inline void ec_transaction_transition(struct acpi_ec *ec, unsigned long f } } -static void advance_transaction(struct acpi_ec *ec) +static void advance_transaction(struct acpi_ec *ec, bool interrupt) { struct transaction *t; u8 status; bool wakeup = false; - ec_dbg_stm("%s (%d)", in_interrupt() ? "IRQ" : "TASK", - smp_processor_id()); + ec_dbg_stm("%s (%d)", 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 @@ -699,7 +698,7 @@ static void advance_transaction(struct acpi_ec *ec) * otherwise will take a not handled IRQ as a false one. */ if (!(status & ACPI_EC_FLAG_SCI)) { - if (in_interrupt() && t) { + if (interrupt && t) { if (t->irq_count < ec_storm_threshold) ++t->irq_count; /* Allow triggering on 0 threshold */ @@ -710,7 +709,7 @@ static void advance_transaction(struct acpi_ec *ec) out: if (status & ACPI_EC_FLAG_SCI) acpi_ec_submit_query(ec); - if (wakeup && in_interrupt()) + if (wakeup && interrupt) wake_up(&ec->wait); } @@ -767,7 +766,7 @@ static int ec_poll(struct acpi_ec *ec) if (!ec_guard(ec)) return 0; spin_lock_irqsave(&ec->lock, flags); - advance_transaction(ec); + advance_transaction(ec, false); spin_unlock_irqrestore(&ec->lock, flags); } while (time_before(jiffies, delay)); pr_debug("controller reset, restart transaction\n"); @@ -1216,7 +1215,7 @@ static void acpi_ec_check_event(struct acpi_ec *ec) * taking care of it. */ if (!ec->curr) - advance_transaction(ec); + advance_transaction(ec, false); spin_unlock_irqrestore(&ec->lock, flags); } } @@ -1259,7 +1258,7 @@ static void acpi_ec_handle_interrupt(struct acpi_ec *ec) unsigned long flags; spin_lock_irqsave(&ec->lock, flags); - advance_transaction(ec); + advance_transaction(ec, true); spin_unlock_irqrestore(&ec->lock, flags); } From d269fb031392d99386b3d11e899e88ae76af9466 Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Mon, 23 Nov 2020 20:00:18 +0100 Subject: [PATCH 2/6] ACPI: EC: Fold acpi_ec_clear_gpe() into its caller Fold acpi_ec_clear_gpe() which is only used in one place into its caller and clean up comments related to that function while at it. No intentional functional impact. Signed-off-by: Rafael J. Wysocki --- drivers/acpi/ec.c | 35 +++++++++++++---------------------- 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index 0caf5ca1fc07..97e595238a8c 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -372,23 +372,6 @@ static inline void acpi_ec_disable_gpe(struct acpi_ec *ec, bool close) } } -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 * -------------------------------------------------------------------------- */ @@ -639,13 +622,21 @@ static void advance_transaction(struct acpi_ec *ec, bool interrupt) bool wakeup = false; ec_dbg_stm("%s (%d)", 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. + * Clear GPE_STS upfront to allow subsequent hardware GPE_STS 0->1 + * changes to always trigger a GPE interrupt. + * + * GPE STS is a W1C register, which means: + * + * 1. Software can clear it without worrying about clearing the 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. */ - if (ec->gpe >= 0) - acpi_ec_clear_gpe(ec); + if (ec->gpe >= 0 && acpi_ec_is_gpe_raised(ec)) + acpi_clear_gpe(NULL, ec->gpe); status = acpi_ec_read_status(ec); t = ec->curr; From d2a2e6ccebb80d297f9827743f49d0f70b4a2daa Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Mon, 23 Nov 2020 20:00:25 +0100 Subject: [PATCH 3/6] ACPI: EC: Rename acpi_ec_is_gpe_raised() Rename acpi_ec_is_gpe_raised() into acpi_ec_gpe_status_set(), update its callers accordingly and drop the ternary operator (which isn't really necessary in there) from it. No intentional functional impact. Signed-off-by: Rafael J. Wysocki --- drivers/acpi/ec.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index 97e595238a8c..c4be16a495e1 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -335,12 +335,12 @@ static const char *acpi_ec_cmd_string(u8 cmd) * GPE Registers * -------------------------------------------------------------------------- */ -static inline bool acpi_ec_is_gpe_raised(struct acpi_ec *ec) +static inline bool acpi_ec_gpe_status_set(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_STATUS_SET) ? true : false; + return !!(gpe_status & ACPI_EVENT_FLAG_STATUS_SET); } static inline void acpi_ec_enable_gpe(struct acpi_ec *ec, bool open) @@ -351,7 +351,7 @@ static inline void acpi_ec_enable_gpe(struct acpi_ec *ec, bool open) BUG_ON(ec->reference_count < 1); acpi_set_gpe(NULL, ec->gpe, ACPI_GPE_ENABLE); } - if (acpi_ec_is_gpe_raised(ec)) { + if (acpi_ec_gpe_status_set(ec)) { /* * On some platforms, EN=1 writes cannot trigger GPE. So * software need to manually trigger a pseudo GPE event on @@ -635,7 +635,7 @@ static void advance_transaction(struct acpi_ec *ec, bool interrupt) * 2. As long as software can ensure only clearing it when it is set, * hardware won't set it in parallel. */ - if (ec->gpe >= 0 && acpi_ec_is_gpe_raised(ec)) + if (ec->gpe >= 0 && acpi_ec_gpe_status_set(ec)) acpi_clear_gpe(NULL, ec->gpe); status = acpi_ec_read_status(ec); From 902675fa87e3a3d433481a2df6e50c10da5e20c2 Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Mon, 23 Nov 2020 20:00:34 +0100 Subject: [PATCH 4/6] ACPI: EC: Simplify error handling in advance_transaction() Notice that the value of t in advance_transaction() does not change after its initialization and: - Initialize t upfront (and rearrange the definitions of local variables while at it). - Check it against NULL in a block executed when it is NULL. - Skip error handling for t == NULL, because a valid pointer value of t is required for the error handling. - Drop the (now redundant) check of t against NULL from the error handling block and reduce the indentation level in there. No intentional functional impact. Signed-off-by: Rafael J. Wysocki --- drivers/acpi/ec.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index c4be16a495e1..23e7b2dec98e 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -617,9 +617,9 @@ static inline void ec_transaction_transition(struct acpi_ec *ec, unsigned long f static void advance_transaction(struct acpi_ec *ec, bool interrupt) { - struct transaction *t; - u8 status; + struct transaction *t = ec->curr; bool wakeup = false; + u8 status; ec_dbg_stm("%s (%d)", interrupt ? "IRQ" : "TASK", smp_processor_id()); @@ -639,7 +639,7 @@ static void advance_transaction(struct acpi_ec *ec, bool interrupt) acpi_clear_gpe(NULL, ec->gpe); status = acpi_ec_read_status(ec); - t = ec->curr; + /* * Another IRQ or a guarded polling mode advancement is detected, * the next QR_EC submission is then allowed. @@ -651,9 +651,10 @@ static void advance_transaction(struct acpi_ec *ec, bool interrupt) clear_bit(EC_FLAGS_QUERY_GUARDING, &ec->flags); acpi_ec_complete_query(ec); } + if (!t) + goto out; } - if (!t) - goto err; + if (t->flags & ACPI_EC_COMMAND_POLL) { if (t->wlen > t->wi) { if ((status & ACPI_EC_FLAG_IBF) == 0) @@ -688,14 +689,13 @@ static void advance_transaction(struct acpi_ec *ec, bool interrupt) * If SCI bit is set, then don't think it's a false IRQ * otherwise will take a not handled IRQ as a false one. */ - if (!(status & ACPI_EC_FLAG_SCI)) { - if (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_mask_events(ec); - } + if (!(status & ACPI_EC_FLAG_SCI) && interrupt) { + if (t->irq_count < ec_storm_threshold) + ++t->irq_count; + + /* Allow triggering on 0 threshold */ + if (t->irq_count == ec_storm_threshold) + acpi_ec_mask_events(ec); } out: if (status & ACPI_EC_FLAG_SCI) From 631734fce3fa27ec5d6f456fc1dd3699617c9efb Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Mon, 23 Nov 2020 20:00:42 +0100 Subject: [PATCH 5/6] ACPI: EC: Untangle error handling in advance_transaction() Introduce acpi_ec_spurious_interrupt() for recording spurious interrupts and use it for error handling in advance_transaction(), drop the (now redundant) original error handling block from there along with a frew goto statements that are not necessary any more. No intentional functional impact. Signed-off-by: Rafael J. Wysocki --- drivers/acpi/ec.c | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index 23e7b2dec98e..091f0e9f37a0 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -615,6 +615,16 @@ static inline void ec_transaction_transition(struct acpi_ec *ec, unsigned long f } } +static void acpi_ec_spurious_interrupt(struct acpi_ec *ec, struct transaction *t) +{ + if (t->irq_count < ec_storm_threshold) + ++t->irq_count; + + /* Trigger if the threshold is 0 too. */ + if (t->irq_count == ec_storm_threshold) + acpi_ec_mask_events(ec); +} + static void advance_transaction(struct acpi_ec *ec, bool interrupt) { struct transaction *t = ec->curr; @@ -659,8 +669,8 @@ static void advance_transaction(struct acpi_ec *ec, bool interrupt) if (t->wlen > t->wi) { if ((status & ACPI_EC_FLAG_IBF) == 0) acpi_ec_write_data(ec, t->wdata[t->wi++]); - else - goto err; + else if (interrupt && !(status & ACPI_EC_FLAG_SCI)) + acpi_ec_spurious_interrupt(ec, t); } else if (t->rlen > t->ri) { if ((status & ACPI_EC_FLAG_OBF) == 1) { t->rdata[t->ri++] = acpi_ec_read_data(ec); @@ -671,32 +681,19 @@ static void advance_transaction(struct acpi_ec *ec, bool interrupt) acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY)); wakeup = true; } - } else - goto err; + } else if (interrupt && !(status & ACPI_EC_FLAG_SCI)) { + acpi_ec_spurious_interrupt(ec, t); + } } else if (t->wlen == t->wi && (status & ACPI_EC_FLAG_IBF) == 0) { ec_transaction_transition(ec, ACPI_EC_COMMAND_COMPLETE); wakeup = true; } - goto out; } else if (!(status & ACPI_EC_FLAG_IBF)) { acpi_ec_write_cmd(ec, t->command); ec_transaction_transition(ec, ACPI_EC_COMMAND_POLL); - goto out; } -err: - /* - * If SCI bit is set, then don't think it's a false IRQ - * otherwise will take a not handled IRQ as a false one. - */ - if (!(status & ACPI_EC_FLAG_SCI) && interrupt) { - if (t->irq_count < ec_storm_threshold) - ++t->irq_count; - /* Allow triggering on 0 threshold */ - if (t->irq_count == ec_storm_threshold) - acpi_ec_mask_events(ec); - } out: if (status & ACPI_EC_FLAG_SCI) acpi_ec_submit_query(ec); From 2a39a30f0d9b5243962a19b2d5a48a8ac3a9a292 Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Mon, 23 Nov 2020 20:01:01 +0100 Subject: [PATCH 6/6] ACPI: EC: Clean up status flags checks in advance_transaction() Eliminate comparisons from the status flags checks in advance_transaction() (especially from the one that is only correct, because the value of the flag checked in there is 1) and rearrange the code for more clarity while at it. No intentional functional impact. Signed-off-by: Rafael J. Wysocki --- drivers/acpi/ec.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index 091f0e9f37a0..13565629ce0a 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -667,25 +667,24 @@ static void advance_transaction(struct acpi_ec *ec, bool interrupt) if (t->flags & ACPI_EC_COMMAND_POLL) { if (t->wlen > t->wi) { - if ((status & ACPI_EC_FLAG_IBF) == 0) + if (!(status & ACPI_EC_FLAG_IBF)) acpi_ec_write_data(ec, t->wdata[t->wi++]); else if (interrupt && !(status & ACPI_EC_FLAG_SCI)) acpi_ec_spurious_interrupt(ec, t); } else if (t->rlen > t->ri) { - if ((status & ACPI_EC_FLAG_OBF) == 1) { + if (status & ACPI_EC_FLAG_OBF) { t->rdata[t->ri++] = acpi_ec_read_data(ec); if (t->rlen == t->ri) { ec_transaction_transition(ec, ACPI_EC_COMMAND_COMPLETE); + wakeup = true; if (t->command == ACPI_EC_COMMAND_QUERY) ec_dbg_evt("Command(%s) completed by hardware", acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY)); - wakeup = true; } } else if (interrupt && !(status & ACPI_EC_FLAG_SCI)) { acpi_ec_spurious_interrupt(ec, t); } - } else if (t->wlen == t->wi && - (status & ACPI_EC_FLAG_IBF) == 0) { + } else if (t->wlen == t->wi && !(status & ACPI_EC_FLAG_IBF)) { ec_transaction_transition(ec, ACPI_EC_COMMAND_COMPLETE); wakeup = true; } @@ -697,6 +696,7 @@ static void advance_transaction(struct acpi_ec *ec, bool interrupt) out: if (status & ACPI_EC_FLAG_SCI) acpi_ec_submit_query(ec); + if (wakeup && interrupt) wake_up(&ec->wait); }