ACPI regression: New Acer workarounds break Samsung NP900X

ACPI regression: New Acer workarounds break Samsung NP900X

[Ortwin Glück]

Lv,

These two patches introduce a regression for Samsung notebooks and they no 
longer get ACPI interrupts for plugging the power adapter or LID switches.

Multiple people have verified that reverting these patches makes the regression 
go away.

Please see new comments in:
https://bugzilla.kernel.org/show_bug.cgi?id=44161#c184

 From 3afcf2ece453e1a8c2c6de19cdf06da3772a1b08 Mon Sep 17 00:00:00 2001
From: Lv Zheng <lv.zheng@intel.com>
Date: Thu, 21 Aug 2014 14:41:13 +0800
Subject: [PATCH] ACPI / EC: Add support to disallow QR_EC to be issued when
  SCI_EVT isn't set

 From 558e4736f2e1b0e6323adf7a5e4df77ed6cfc1a4 Mon Sep 17 00:00:00 2001
From: Lv Zheng <lv.zheng@intel.com>
Date: Thu, 21 Aug 2014 14:41:26 +0800
Subject: [PATCH] ACPI / EC: Add support to disallow QR_EC to be issued before
  completing previous QR_EC


Thanks,

Ortwin

RE: ACPI regression: New Acer workarounds break Samsung NP900X

[Zheng, Lv]

Hi,

Thanks for letting me know.

Though IMO the ACER behavior is not ACPI spec compliant, but following it still shouldn't break the others.
Because it just requires EC firmware to always flag SCI_EVT when there is an event queued up.
I couldn't see a special reason that a correct EC firmware should stop doing this.
So I didn't make it a quirk that can only apply for the ACER firmware.

I read the whole bug log, for me it looks like that the Samsung firmware need a special EC driver feature which is currently not in the Linux kernel.
According to the root cause:
Comment 100-103 at https://bugs.launchpad.net/ubuntu/+source/linux/+bug/971061/
Samsung laptops need a SCI_EVT polling mode to be implemented in the kernel.

Fortunately, I had this already implemented, you can find it here:
https://github.com/zetalog/linux/commit/0fe9406d <- it implements a polling thread in the kernel to poll SCI_EVT.
https://github.com/zetalog/linux/commit/28ef0576 <- it adds a quirk mechanism to allow Linux to always poll SCI_EVT (I need to improve it according to your report).

It just takes time to make it upstream because there is a cleanup in it to correct the storming prevention craps in the same series:
https://github.com/zetalog/linux/commit/fa4024c8
which depends on an ACPICA commit, the commit has been under discussion for more than 3 months:
https://github.com/zetalog/linux/commit/e20e0fde

Let me try to rebase the series and make the SCI_EVT polling thread independent of the storming prevention cleanups.
So that I can make it upstream first to permanently fix the Samsung laptop problems in the kernel.
I may ask you for help to test the new feature after it is ready.

Thanks and best regards
-Lv

> From: Ortwin Glück [mailto:odi@odi.ch]
> Sent: Sunday, October 26, 2014 8:17 PM
> 
> Lv,
> 
> These two patches introduce a regression for Samsung notebooks and they no
> longer get ACPI interrupts for plugging the power adapter or LID switches.
> 
> Multiple people have verified that reverting these patches makes the regression
> go away.
> 
> Please see new comments in:
> https://bugzilla.kernel.org/show_bug.cgi?id=44161#c184
> 
>  From 3afcf2ece453e1a8c2c6de19cdf06da3772a1b08 Mon Sep 17 00:00:00 2001
> From: Lv Zheng <lv.zheng@intel.com>
> Date: Thu, 21 Aug 2014 14:41:13 +0800
> Subject: [PATCH] ACPI / EC: Add support to disallow QR_EC to be issued when
>   SCI_EVT isn't set
> 
>  From 558e4736f2e1b0e6323adf7a5e4df77ed6cfc1a4 Mon Sep 17 00:00:00 2001
> From: Lv Zheng <lv.zheng@intel.com>
> Date: Thu, 21 Aug 2014 14:41:26 +0800
> Subject: [PATCH] ACPI / EC: Add support to disallow QR_EC to be issued before
>   completing previous QR_EC
> 
> 
> Thanks,
> 
> Ortwin
> 

[PATCH v2 0/2] ACPI/EC: Fix regressions on Samsung hardware.

[Lv Zheng]

It is reported that the Samsung EC firmware behaves differently than Acer
EC firmware. And then previous 2 commits that fix Acer issues have broken
Samsung hardware.

This patchset fixes the regressions.

Lv Zheng (2):
  Revert "ACPI / EC: Add support to disallow QR_EC to be issued before
    completing previous QR_EC"
  ACPI/EC: Fix a regression caused by conflict firmware behavior
    between Samsung and Acer.

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

-- 
1.7.10

[PATCH v2 1/2] Revert "ACPI / EC: Add support to disallow QR_EC to be issued before completing previous QR_EC"

[Lv Zheng]

It is reported that the following commit breaks Samsung hardware:
 Commit: 558e4736f2e1b0e6323adf7a5e4df77ed6cfc1a4.
 Subject: ACPI / EC: Add support to disallow QR_EC to be issued before
          completing previous QR_EC

Which means the Samsung behavior conflicts with the Acer behavior.

1. Samsung may behave like:
   [ +event 1 ] SCI_EVT set
   [ +event 2 ] SCI_EVT set
                              write QR_EC
                              read event
   [ -event 1 ] SCI_EVT clear
   Without the above commit, Samsung can work:
   [ +event 1 ] SCI_EVT set
   [ +event 2 ] SCI_EVT set
                              write QR_EC
                              CAN prepare next QR_EC as SCI_EVT=1
                              read event
   [ -event 1 ] SCI_EVT clear
                              write QR_EC
                              read event
   [ -event 2 ] SCI_EVT clear
   With the above commit, Samsung cannot work:
   [ +event 1 ] SCI_EVT set
   [ +event 2 ] SCI_EVT set
                              write QR_EC
                              read event
   [ -event 1 ] SCI_EVT clear
                              CANNOT prepare next QR_EC as SCI_EVT=0
2. Acer may behave like:
   [ +event 1 ] SCI_EVT set
   [ +event 2 ]
                              write QR_EC
                              read event
   [ -event 1 ] SCI_EVT clear
   [ +event 2 ] SCI_EVT set
   Without the above commit, Acer cannot work when there is only 1 event:
   [ +event 1 ] SCI_EVT set
                              write QR_EC
                              can prepared next QR_EC as SCI_EVT=1
                              read event
   [ -event 1 ] SCI_EVT clear
                              CANNOT write QR_EC as SCI_EVT=0
   With the above commit, Acer can work:
   [ +event 1 ] SCI_EVT set
   [ +event 2 ]
                              write QR_EC
                              read event
   [ -event 1 ] SCI_EVT set
                              can prepare next QR_EC because SCI_EVT=0
                              CAN write QR_EC as SCI_EVT=1

Since Acer can also work with only the following commit applied:
 Commit: 3afcf2ece453e1a8c2c6de19cdf06da3772a1b08
 Subject: ACPI / EC: Add support to disallow QR_EC to be issued when
          SCI_EVT isn't set
We can revert the above commit.

This reverts commit 558e4736f2e1b0e6323adf7a5e4df77ed6cfc1a4.

Conflicts:

	drivers/acpi/ec.c

Reference: https://bugzilla.kernel.org/show_bug.cgi?id=44161
Reported-and-tested-by: Ortwin Gl¨¹ck <odi@odi.ch>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 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 3d304ff..31b699f 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -334,13 +334,13 @@ 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);
-	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);
 		pr_debug("***** Event stopped *****\n");
 	}
+	spin_unlock_irqrestore(&ec->lock, tmp);
+	ret = ec_poll(ec);
+	spin_lock_irqsave(&ec->lock, tmp);
 	pr_debug("***** Command(%s) stopped *****\n",
 		 acpi_ec_cmd_string(t->command));
 	ec->curr = NULL;
-- 
1.7.10

[PATCH v2 2/2] ACPI/EC: Fix a regression caused by conflict firmware behavior between Samsung and Acer.

[Lv Zheng]

It is reported that Samsung laptops that need to poll events are broken by
the following commit:
 Commit 3afcf2ece453e1a8c2c6de19cdf06da3772a1b08
 Subject: ACPI / EC: Add support to disallow QR_EC to be issued when SCI_EVT isn't set

The behaviors of the 2 vendor firmwares are conflict:
1. Acer: OSPM shouldn't issue QR_EC unless SCI_EVT is set, firmware
         automatically sets SCI_EVT as long as there is event queued up.
2. Samsung: OSPM should issue QR_EC whatever SCI_EVT is set, firmware
            returns 0 when there is no event queued up.

This patch is a quick fix to distinguish the behaviors to make Acer
behavior only effective for Acer EC firmware so that the breakages on
Samsung EC firmware can be avoided.

Reference: https://bugzilla.kernel.org/show_bug.cgi?id=44161
Reported-and-tested-by: Ortwin Gl¨¹ck <odi@odi.ch>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 31b699f..5f9b74b 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -126,6 +126,7 @@ static int EC_FLAGS_MSI; /* Out-of-spec MSI controller */
 static int EC_FLAGS_VALIDATE_ECDT; /* ASUStec ECDTs need to be validated */
 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 */
 
 /* --------------------------------------------------------------------------
  *                           Transaction Management
@@ -236,13 +237,8 @@ static bool advance_transaction(struct acpi_ec *ec)
 		}
 		return wakeup;
 	} else {
-		/*
-		 * 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) &&
+		if (EC_FLAGS_QUERY_HANDSHAKE &&
+		    !(status & ACPI_EC_FLAG_SCI) &&
 		    (t->command == ACPI_EC_COMMAND_QUERY)) {
 			t->flags |= ACPI_EC_COMMAND_POLL;
 			t->rdata[t->ri++] = 0x00;
@@ -1012,6 +1008,18 @@ static int ec_enlarge_storm_threshold(const struct dmi_system_id *id)
 }
 
 /*
+ * Acer EC firmware refuses 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
+ */
+static int ec_flag_query_handshake(const struct dmi_system_id *id)
+{
+	pr_debug("Detected the EC firmware requiring QR_EC issued when SCI_EVT set\n");
+	EC_FLAGS_QUERY_HANDSHAKE = 1;
+	return 0;
+}
+
+/*
  * On some hardware it is necessary to clear events accumulated by the EC during
  * sleep. These ECs stop reporting GPEs until they are manually polled, if too
  * many events are accumulated. (e.g. Samsung Series 5/9 notebooks)
@@ -1085,6 +1093,9 @@ static struct dmi_system_id ec_dmi_table[] __initdata = {
 	{
 	ec_clear_on_resume, "Samsung hardware", {
 	DMI_MATCH(DMI_SYS_VENDOR, "SAMSUNG ELECTRONICS CO., LTD.")}, NULL},
+	{
+	ec_flag_query_handshake, "Acer hardware", {
+	DMI_MATCH(DMI_SYS_VENDOR, "Acer"), }, NULL},
 	{},
 };
 
-- 
1.7.10

Re: [PATCH v2 0/2] ACPI/EC: Fix regressions on Samsung hardware.

[Rafael J. Wysocki]

On Wednesday, October 29, 2014 11:33:33 AM Lv Zheng wrote:
> It is reported that the Samsung EC firmware behaves differently than Acer
> EC firmware. And then previous 2 commits that fix Acer issues have broken
> Samsung hardware.
> 
> This patchset fixes the regressions.
> 
> Lv Zheng (2):
>   Revert "ACPI / EC: Add support to disallow QR_EC to be issued before
>     completing previous QR_EC"
>   ACPI/EC: Fix a regression caused by conflict firmware behavior
>     between Samsung and Acer.

Both patches applied, thanks Lv!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.