[PATCH 0/5] ACPI: EC: Cleanups in advance_transaction()

[PATCH 0/5] ACPI: EC: Cleanups in advance_transaction()

[Rafael J. Wysocki]

Hi All,

Just a few cleanups related to the advance_transaction() routine in ec.c.

Please see patch changelogs for details.

Thanks!

[PATCH 1/5] ACPI: EC: Fold acpi_ec_clear_gpe() into its caller

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

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 <rafael.j.wysocki@intel.com>
---
 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;
-- 
2.26.2

[PATCH 2/5] ACPI: EC: Rename acpi_ec_is_gpe_raised()

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

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 <rafael.j.wysocki@intel.com>
---
 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);
-- 
2.26.2

[PATCH 3/5] ACPI: EC: Simplify error handling in advance_transaction()

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

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 <rafael.j.wysocki@intel.com>
---
 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)
-- 
2.26.2

[PATCH 4/5] ACPI: EC: Untangle error handling in advance_transaction()

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

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 <rafael.j.wysocki@intel.com>
---
 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);
-- 
2.26.2

[PATCH 5/5] ACPI: EC: Clean up status flags checks in advance_transaction()

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

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 <rafael.j.wysocki@intel.com>
---
 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);
 }
-- 
2.26.2