[PATCH 0/2] ACPI/EC: Improve EC driver's state machine to survive with firmware that refuses to respond QR_EC when SCI_EVT isn't set.

[PATCH 0/2] ACPI/EC: Improve EC driver's state machine to survive with firmware that refuses to respond QR_EC when SCI_EVT isn't set.

[Lv Zheng]

It is reported for Acer Aspire V5-573G that the EC firmware on this
platform refuses to respond QR_EC when SCI_EVT isn't set.

This is basically a firmware issue, such firmware is not completely
compliant with current ACPI specification where the 0x00 return value for
QR_EC is mentioned. Thus Linux EC driver doesn't handle this and expects
firmware can always respond something (for example, 0x00 to indicate "no
outstanding events") even when SCI_EVT isn't set.

We can see delay issue on such platform. This is because the work item that
has issued QR_EC has to wait until timeout in this case, and the _Qxx
method evaluation work item queued after QR_EC one is delayed.

A previous race fix makes the occurrence rate of this issue higher than
before:
  Commit: c0d653412fc8450370167a3268b78fc772ff9c87
  Subject: ACPI / EC: Fix race condition in ec_transaction_completed()
But this is a seperate bug for the particular buggy firmware, and we are
not able to improve the race fix to fix it, we need to add new support code
to enhance our state machine to survive this situation.

This patchset implements required enhancements to fix this issue.

PATCH 1 is the required fix for this issue and PATCH 2 is a useful
improvement.

Lv Zheng (2):
  ACPI/EC: Add support to disallow QR_EC to be issued when SCI_EVT
    isn't set.
  ACPI/EC: Add support to disallow QR_EC to be issued before completing
    the previous QR_EC.

 drivers/acpi/ec.c |   21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

-- 
1.7.10

[PATCH 0/2] ACPI/EC: Improve EC driver's state machine to survive with firmware that refuses to respond QR_EC when SCI_EVT isn't set.

[Lv Zheng]

It is reported for Acer Aspire V5-573G that the EC firmware on this
platform refuses to respond QR_EC when SCI_EVT isn't set.

This is basically a firmware issue, such firmware is not completely
compliant with current ACPI specification where the 0x00 return value for
QR_EC is mentioned. Thus Linux EC driver doesn't handle this and expects
firmware can always respond something (for example, 0x00 to indicate "no
outstanding events") even when SCI_EVT isn't set.

We can see delay issue on such platform. This is because the work item that
has issued QR_EC has to wait until timeout in this case, and the _Qxx
method evaluation work item queued after QR_EC one is delayed.

A previous race fix makes the occurrence rate of this issue higher than
before:
  Commit: c0d653412fc8450370167a3268b78fc772ff9c87
  Subject: ACPI / EC: Fix race condition in ec_transaction_completed()
But this is a seperate bug for the particular buggy firmware, and we are
not able to improve the race fix to fix it, we need to add new support code
to enhance our state machine to survive this situation.

This patchset implements required enhancements to fix this issue.

PATCH 1 is the required fix for this issue and PATCH 2 is a useful
improvement.

Lv Zheng (2):
  ACPI/EC: Add support to disallow QR_EC to be issued when SCI_EVT
    isn't set.
  ACPI/EC: Add support to disallow QR_EC to be issued before completing
    the previous QR_EC.

 drivers/acpi/ec.c |   21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

-- 
1.7.10

[PATCH 1/2] ACPI/EC: Add support to disallow QR_EC to be issued when SCI_EVT isn't set.

[Lv Zheng]

There is platform refusing to respond QR_EC when SCI_EVT isn't set. A known
such platform is Acer Aspire V5-573G.

Currently we rely on the behaviour that the EC firmware can respond
something (for example, 0x00 to indicate "no outstanding events") to QR_EC
even when SCI_EVT is not set. But reporter has complained for AC/battery
pluging/unpluging, video brightness change delay on such platform.

This is because the work item that has issued QR_EC has to wait until
timeout in this case, and the _Qxx method evaluation work item queued after
QR_EC one is delayed.

It sounds reasonable to fix this issue by:
1. Implementing SCI_EVT sanity check before issuing QR_EC in the EC
   driver's main state machine.
2. Moving QR_EC issuing out of the work queue used by _Qxx evaluation to a
   seperate IRQ handling thread.

This patch fixes this issue using solution 1. The solution 2 has been
implemented by another already published patch:
"https://lkml.org/lkml/2014/7/21/41".

By disallowing QR_EC to be issued when SCI_EVT isn't set, we are able to
handle such platform in the EC driver's main state machine. This patch
enhances the state machine in this way to survive with such malfunctioning
EC firmware.

Note that this patch can also fix CLEAR_ON_RESUME quirk which also relies
on the assumption that the platforms are able to respond even when SCI_EVT
isn't set.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=82611
Reported-and-tested-by: Alexander Mezin <mezin.alexander@gmail.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index a66ab65..5e1ed31 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -197,6 +197,8 @@ static bool 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;
+					if (t->command == ACPI_EC_COMMAND_QUERY)
+						pr_debug("hardware QR_EC completion\n");
 					wakeup = true;
 				}
 			} else
@@ -208,7 +210,20 @@ static bool advance_transaction(struct acpi_ec *ec)
 		}
 		return wakeup;
 	} else {
-		if ((status & ACPI_EC_FLAG_IBF) == 0) {
+		/*
+		 * There is firmware refusing to respond QR_EC when SCI_EVT
+		 * is not set, for which case, we complete the QR_EC
+		 * without issuing it to the firmware.
+		 * https://bugzilla.kernel.org/show_bug.cgi?id=86211
+		 */
+		if (!(status & ACPI_EC_FLAG_SCI) &&
+		    (t->command == ACPI_EC_COMMAND_QUERY)) {
+			t->flags |= ACPI_EC_COMMAND_POLL;
+			t->rdata[t->ri++] = 0x00;
+			t->flags |= ACPI_EC_COMMAND_COMPLETE;
+			pr_debug("software QR_EC completion\n");
+			wakeup = true;
+		} else if ((status & ACPI_EC_FLAG_IBF) == 0) {
 			acpi_ec_write_cmd(ec, t->command);
 			t->flags |= ACPI_EC_COMMAND_POLL;
 		} else
-- 
1.7.10

[PATCH 1/2] ACPI/EC: Add support to disallow QR_EC to be issued when SCI_EVT isn't set.

[Lv Zheng]

There is platform refusing to respond QR_EC when SCI_EVT isn't set. A known
such platform is Acer Aspire V5-573G.

Currently we rely on the behaviour that the EC firmware can respond
something (for example, 0x00 to indicate "no outstanding events") to QR_EC
even when SCI_EVT is not set. But reporter has complained for AC/battery
pluging/unpluging, video brightness change delay on such platform.

This is because the work item that has issued QR_EC has to wait until
timeout in this case, and the _Qxx method evaluation work item queued after
QR_EC one is delayed.

It sounds reasonable to fix this issue by:
1. Implementing SCI_EVT sanity check before issuing QR_EC in the EC
   driver's main state machine.
2. Moving QR_EC issuing out of the work queue used by _Qxx evaluation to a
   seperate IRQ handling thread.

This patch fixes this issue using solution 1. The solution 2 has been
implemented by another already published patch:
"https://lkml.org/lkml/2014/7/21/41".

By disallowing QR_EC to be issued when SCI_EVT isn't set, we are able to
handle such platform in the EC driver's main state machine. This patch
enhances the state machine in this way to survive with such malfunctioning
EC firmware.

Note that this patch can also fix CLEAR_ON_RESUME quirk which also relies
on the assumption that the platforms are able to respond even when SCI_EVT
isn't set.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=82611
Reported-and-tested-by: Alexander Mezin <mezin.alexander@gmail.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index a66ab65..5e1ed31 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -197,6 +197,8 @@ static bool 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;
+					if (t->command == ACPI_EC_COMMAND_QUERY)
+						pr_debug("hardware QR_EC completion\n");
 					wakeup = true;
 				}
 			} else
@@ -208,7 +210,20 @@ static bool advance_transaction(struct acpi_ec *ec)
 		}
 		return wakeup;
 	} else {
-		if ((status & ACPI_EC_FLAG_IBF) == 0) {
+		/*
+		 * There is firmware refusing to respond QR_EC when SCI_EVT
+		 * is not set, for which case, we complete the QR_EC
+		 * without issuing it to the firmware.
+		 * https://bugzilla.kernel.org/show_bug.cgi?id=86211
+		 */
+		if (!(status & ACPI_EC_FLAG_SCI) &&
+		    (t->command == ACPI_EC_COMMAND_QUERY)) {
+			t->flags |= ACPI_EC_COMMAND_POLL;
+			t->rdata[t->ri++] = 0x00;
+			t->flags |= ACPI_EC_COMMAND_COMPLETE;
+			pr_debug("software QR_EC completion\n");
+			wakeup = true;
+		} else if ((status & ACPI_EC_FLAG_IBF) == 0) {
 			acpi_ec_write_cmd(ec, t->command);
 			t->flags |= ACPI_EC_COMMAND_POLL;
 		} else
-- 
1.7.10

[PATCH 2/2] ACPI/EC: Add support to disallow QR_EC to be issued before completing the previous QR_EC.

[Lv Zheng]

There is platform refusing to respond QR_EC when SCI_EVT isn't set.
A known such platform is Acer Aspire V5-573G.

By disallowing QR_EC issuing without completing the previous one, we are
able to reduce the possibilities to trigger issues on such platforms.

Note that this fix can only reduce the occurrence rate of this issue, but
this issue may still occur when such a platform doesn't clear SCI_EVT
before or immediately after completing the previous QR_EC transaction. This
patch cannot fix CLEAR_ON_RESUME quirk which also relies on the assumption
that the platforms are able to respond even when SCI_EVT isn't set.

But this patch is still useful as it can help to reduce the number of
scheduled QR_EC work items.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=82611
Reported-and-tested-by: Alexander Mezin <mezin.alexander@gmail.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 5e1ed31..9922cc4 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -303,11 +303,11 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
 	/* following two actions should be kept atomic */
 	ec->curr = t;
 	start_transaction(ec);
-	if (ec->curr->command == ACPI_EC_COMMAND_QUERY)
-		clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
 	spin_unlock_irqrestore(&ec->lock, tmp);
 	ret = ec_poll(ec);
 	spin_lock_irqsave(&ec->lock, tmp);
+	if (ec->curr->command == ACPI_EC_COMMAND_QUERY)
+		clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
 	ec->curr = NULL;
 	spin_unlock_irqrestore(&ec->lock, tmp);
 	return ret;
-- 
1.7.10

[PATCH 2/2] ACPI/EC: Add support to disallow QR_EC to be issued before completing the previous QR_EC.

[Lv Zheng]

There is platform refusing to respond QR_EC when SCI_EVT isn't set.
A known such platform is Acer Aspire V5-573G.

By disallowing QR_EC issuing without completing the previous one, we are
able to reduce the possibilities to trigger issues on such platforms.

Note that this fix can only reduce the occurrence rate of this issue, but
this issue may still occur when such a platform doesn't clear SCI_EVT
before or immediately after completing the previous QR_EC transaction. This
patch cannot fix CLEAR_ON_RESUME quirk which also relies on the assumption
that the platforms are able to respond even when SCI_EVT isn't set.

But this patch is still useful as it can help to reduce the number of
scheduled QR_EC work items.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=82611
Reported-and-tested-by: Alexander Mezin <mezin.alexander@gmail.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 5e1ed31..9922cc4 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -303,11 +303,11 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
 	/* following two actions should be kept atomic */
 	ec->curr = t;
 	start_transaction(ec);
-	if (ec->curr->command == ACPI_EC_COMMAND_QUERY)
-		clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
 	spin_unlock_irqrestore(&ec->lock, tmp);
 	ret = ec_poll(ec);
 	spin_lock_irqsave(&ec->lock, tmp);
+	if (ec->curr->command == ACPI_EC_COMMAND_QUERY)
+		clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
 	ec->curr = NULL;
 	spin_unlock_irqrestore(&ec->lock, tmp);
 	return ret;
-- 
1.7.10

RE: [PATCH 1/2] ACPI/EC: Add support to disallow QR_EC to be issued when SCI_EVT isn't set.

[Wysocki, Rafael J]

Instead of putting a link to the BZ entry in the comment, please what machine is affected in there. Looks good otherwise.

Rafael



-------- Original message --------
>From "Zheng, Lv" <lv.zheng@intel.com>
Date: 21/08/2014 08:41 (GMT+01:00)
To "Wysocki, Rafael J" <rafael.j.wysocki@intel.com>,"Brown, Len" <len.brown@intel.com>
Cc "Zheng, Lv" <lv.zheng@intel.com>,Lv Zheng <zetalog@gmail.com>,linux-kernel@vger.kernel.org,linux-acpi@vger.kernel.org
Subject [PATCH 1/2] ACPI/EC: Add support to disallow QR_EC to be issued when SCI_EVT isn't set.


There is platform refusing to respond QR_EC when SCI_EVT isn't set. A known
such platform is Acer Aspire V5-573G.

Currently we rely on the behaviour that the EC firmware can respond
something (for example, 0x00 to indicate "no outstanding events") to QR_EC
even when SCI_EVT is not set. But reporter has complained for AC/battery
pluging/unpluging, video brightness change delay on such platform.

This is because the work item that has issued QR_EC has to wait until
timeout in this case, and the _Qxx method evaluation work item queued after
QR_EC one is delayed.

It sounds reasonable to fix this issue by:
1. Implementing SCI_EVT sanity check before issuing QR_EC in the EC
   driver's main state machine.
2. Moving QR_EC issuing out of the work queue used by _Qxx evaluation to a
   seperate IRQ handling thread.

This patch fixes this issue using solution 1. The solution 2 has been
implemented by another already published patch:
"https://lkml.org/lkml/2014/7/21/41".

By disallowing QR_EC to be issued when SCI_EVT isn't set, we are able to
handle such platform in the EC driver's main state machine. This patch
enhances the state machine in this way to survive with such malfunctioning
EC firmware.

Note that this patch can also fix CLEAR_ON_RESUME quirk which also relies
on the assumption that the platforms are able to respond even when SCI_EVT
isn't set.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=82611
Reported-and-tested-by: Alexander Mezin <mezin.alexander@gmail.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index a66ab65..5e1ed31 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -197,6 +197,8 @@ static bool 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;
+                                       if (t->command == ACPI_EC_COMMAND_QUERY)
+                                               pr_debug("hardware QR_EC completion\n");
                                         wakeup = true;
                                 }
                         } else
@@ -208,7 +210,20 @@ static bool advance_transaction(struct acpi_ec *ec)
                 }
                 return wakeup;
         } else {
-               if ((status & ACPI_EC_FLAG_IBF) == 0) {
+               /*
+                * There is firmware refusing to respond QR_EC when SCI_EVT
+                * is not set, for which case, we complete the QR_EC
+                * without issuing it to the firmware.
+                * https://bugzilla.kernel.org/show_bug.cgi?id=86211
+                */
+               if (!(status & ACPI_EC_FLAG_SCI) &&
+                   (t->command == ACPI_EC_COMMAND_QUERY)) {
+                       t->flags |= ACPI_EC_COMMAND_POLL;
+                       t->rdata[t->ri++] = 0x00;
+                       t->flags |= ACPI_EC_COMMAND_COMPLETE;
+                       pr_debug("software QR_EC completion\n");
+                       wakeup = true;
+               } else if ((status & ACPI_EC_FLAG_IBF) == 0) {
                         acpi_ec_write_cmd(ec, t->command);
                         t->flags |= ACPI_EC_COMMAND_POLL;
                 } else
--
1.7.10

RE: [PATCH 2/2] ACPI/EC: Add support to disallow QR_EC to be issued before completing the previous QR_EC.

[Wysocki, Rafael J]

Looks OK to me.

Rafael



-------- Original message --------
>From "Zheng, Lv" <lv.zheng@intel.com>
Date: 21/08/2014 08:41 (GMT+01:00)
To "Wysocki, Rafael J" <rafael.j.wysocki@intel.com>,"Brown, Len" <len.brown@intel.com>
Cc "Zheng, Lv" <lv.zheng@intel.com>,Lv Zheng <zetalog@gmail.com>,linux-kernel@vger.kernel.org,linux-acpi@vger.kernel.org
Subject [PATCH 2/2] ACPI/EC: Add support to disallow QR_EC to be issued before completing the previous QR_EC.


There is platform refusing to respond QR_EC when SCI_EVT isn't set.
A known such platform is Acer Aspire V5-573G.

By disallowing QR_EC issuing without completing the previous one, we are
able to reduce the possibilities to trigger issues on such platforms.

Note that this fix can only reduce the occurrence rate of this issue, but
this issue may still occur when such a platform doesn't clear SCI_EVT
before or immediately after completing the previous QR_EC transaction. This
patch cannot fix CLEAR_ON_RESUME quirk which also relies on the assumption
that the platforms are able to respond even when SCI_EVT isn't set.

But this patch is still useful as it can help to reduce the number of
scheduled QR_EC work items.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=82611
Reported-and-tested-by: Alexander Mezin <mezin.alexander@gmail.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 5e1ed31..9922cc4 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -303,11 +303,11 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
         /* following two actions should be kept atomic */
         ec->curr = t;
         start_transaction(ec);
-       if (ec->curr->command == ACPI_EC_COMMAND_QUERY)
-               clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
         spin_unlock_irqrestore(&ec->lock, tmp);
         ret = ec_poll(ec);
         spin_lock_irqsave(&ec->lock, tmp);
+       if (ec->curr->command == ACPI_EC_COMMAND_QUERY)
+               clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
         ec->curr = NULL;
         spin_unlock_irqrestore(&ec->lock, tmp);
         return ret;
--
1.7.10

RE: [PATCH 2/2] ACPI/EC: Add support to disallow QR_EC to be issued before completing the previous QR_EC.

[Zheng, Lv]

Hi, Rafael

I think PATCH 1 is required for now and this PATCH is not that useful if you are planning to merge my storming fix, the logic has already been cleaned there.

This flag originally is used to mark the period after SCI_EVT is indicated and before the QR_EC is polled.
This flag is useful because during this period some malfunctioning firmware may trigger GPE storm as we haven't handled SCI_EVT right in the IRQ context.
But so far we haven’t ACPICA GPE APIs ready to achieve such storm prevention, the original use case is not that obvious now….
Actually this flag is currently only used to masking query issuing...

This patch change the flag to be used for this case – marking the period after SCI_EVT is indicated and before the QR_EC is completed.
Since the original usage is actually not implemented, we can do this change.

But you also can ignore this patch as the cleanup in the storming series is cleaner than this.
In that series, the both cases are marked and thus can be protected by further code.

So it’s up to you whether you need PATCH 2 or not. ☺
If you took it, I would re-base the storming series to reflect this change.

Thanks and best regards
-Lv


From: Wysocki, Rafael J 
Sent: Friday, August 22, 2014 3:55 AM
To: Zheng, Lv; Brown, Len
Cc: Lv Zheng; linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org
Subject: RE: [PATCH 2/2] ACPI/EC: Add support to disallow QR_EC to be issued before completing the previous QR_EC.

Looks OK to me.

Rafael



-------- Original message --------
From "Zheng, Lv" <lv.zheng@intel.com> 
Date: 21/08/2014 08:41 (GMT+01:00) 
To "Wysocki, Rafael J" <rafael.j.wysocki@intel.com>,"Brown, Len" <len.brown@intel.com> 
Cc "Zheng, Lv" <lv.zheng@intel.com>,Lv Zheng <zetalog@gmail.com>,linux-kernel@vger.kernel.org,linux-acpi@vger.kernel.org 
Subject [PATCH 2/2] ACPI/EC: Add support to disallow QR_EC to be issued before completing the previous QR_EC. 

There is platform refusing to respond QR_EC when SCI_EVT isn't set.
A known such platform is Acer Aspire V5-573G.

By disallowing QR_EC issuing without completing the previous one, we are
able to reduce the possibilities to trigger issues on such platforms.

Note that this fix can only reduce the occurrence rate of this issue, but
this issue may still occur when such a platform doesn't clear SCI_EVT
before or immediately after completing the previous QR_EC transaction. This
patch cannot fix CLEAR_ON_RESUME quirk which also relies on the assumption
that the platforms are able to respond even when SCI_EVT isn't set.

But this patch is still useful as it can help to reduce the number of
scheduled QR_EC work items.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=82611
Reported-and-tested-by: Alexander Mezin <mezin.alexander@gmail.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 5e1ed31..9922cc4 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -303,11 +303,11 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
         /* following two actions should be kept atomic */
         ec->curr = t;
         start_transaction(ec);
-       if (ec->curr->command == ACPI_EC_COMMAND_QUERY)
-               clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
         spin_unlock_irqrestore(&ec->lock, tmp);
         ret = ec_poll(ec);
         spin_lock_irqsave(&ec->lock, tmp);
+       if (ec->curr->command == ACPI_EC_COMMAND_QUERY)
+               clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
         ec->curr = NULL;
         spin_unlock_irqrestore(&ec->lock, tmp);
         return ret;
-- 
1.7.10

RE: [PATCH 2/2] ACPI/EC: Add support to disallow QR_EC to be issued before completing the previous QR_EC.

[Zheng, Lv]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3973 bytes --]

Hi, Rafael

I think PATCH 1 is required for now and this PATCH is not that useful if you are planning to merge my storming fix, the logic has already been cleaned there.

This flag originally is used to mark the period after SCI_EVT is indicated and before the QR_EC is polled.
This flag is useful because during this period some malfunctioning firmware may trigger GPE storm as we haven't handled SCI_EVT right in the IRQ context.
But so far we haven’t ACPICA GPE APIs ready to achieve such storm prevention, the original use case is not that obvious now….
Actually this flag is currently only used to masking query issuing...

This patch change the flag to be used for this case – marking the period after SCI_EVT is indicated and before the QR_EC is completed.
Since the original usage is actually not implemented, we can do this change.

But you also can ignore this patch as the cleanup in the storming series is cleaner than this.
In that series, the both cases are marked and thus can be protected by further code.

So it’s up to you whether you need PATCH 2 or not. ☺
If you took it, I would re-base the storming series to reflect this change.

Thanks and best regards
-Lv


From: Wysocki, Rafael J 
Sent: Friday, August 22, 2014 3:55 AM
To: Zheng, Lv; Brown, Len
Cc: Lv Zheng; linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org
Subject: RE: [PATCH 2/2] ACPI/EC: Add support to disallow QR_EC to be issued before completing the previous QR_EC.

Looks OK to me.

Rafael



-------- Original message --------
>From "Zheng, Lv" <lv.zheng@intel.com> 
Date: 21/08/2014 08:41 (GMT+01:00) 
To "Wysocki, Rafael J" <rafael.j.wysocki@intel.com>,"Brown, Len" <len.brown@intel.com> 
Cc "Zheng, Lv" <lv.zheng@intel.com>,Lv Zheng <zetalog@gmail.com>,linux-kernel@vger.kernel.org,linux-acpi@vger.kernel.org 
Subject [PATCH 2/2] ACPI/EC: Add support to disallow QR_EC to be issued before completing the previous QR_EC. 

There is platform refusing to respond QR_EC when SCI_EVT isn't set.
A known such platform is Acer Aspire V5-573G.

By disallowing QR_EC issuing without completing the previous one, we are
able to reduce the possibilities to trigger issues on such platforms.

Note that this fix can only reduce the occurrence rate of this issue, but
this issue may still occur when such a platform doesn't clear SCI_EVT
before or immediately after completing the previous QR_EC transaction. This
patch cannot fix CLEAR_ON_RESUME quirk which also relies on the assumption
that the platforms are able to respond even when SCI_EVT isn't set.

But this patch is still useful as it can help to reduce the number of
scheduled QR_EC work items.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=82611
Reported-and-tested-by: Alexander Mezin <mezin.alexander@gmail.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 5e1ed31..9922cc4 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -303,11 +303,11 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
         /* following two actions should be kept atomic */
         ec->curr = t;
         start_transaction(ec);
-       if (ec->curr->command == ACPI_EC_COMMAND_QUERY)
-               clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
         spin_unlock_irqrestore(&ec->lock, tmp);
         ret = ec_poll(ec);
         spin_lock_irqsave(&ec->lock, tmp);
+       if (ec->curr->command == ACPI_EC_COMMAND_QUERY)
+               clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
         ec->curr = NULL;
         spin_unlock_irqrestore(&ec->lock, tmp);
         return ret;
-- 
1.7.10
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 2/2] ACPI/EC: Add support to disallow QR_EC to be issued before completing the previous QR_EC.

[Rafael J. Wysocki]

On Friday, August 22, 2014 01:06:29 AM Zheng, Lv wrote:
> Hi, Rafael
> 
> I think PATCH 1 is required for now and this PATCH is not that useful if you are planning to merge my storming fix, the logic has already been cleaned there.

We need both [1-2/2] for -stable and they need to go to the mainline first.

Rafael