Regression between rc2 and rc6: ACPI EC problems

Regression between rc2 and rc6: ACPI EC problems

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1230 bytes --]

Hi,

when upgrading from v4.7-rc2 to rc6 on my Dell Mini 9, battery and
temperature status info got bogus. Here is the major change between the
boot logs:

ACPI : EC: EC stopped
ACPI Exception: AE_BAD_PARAMETER, Returned by Handler for [EmbeddedControl] (20160422/evregion-300)
ACPI Error: Method parse/execution failed [\_SB.PCI0.LPCB.EC0._REG] (Node f5d09870), AE_BAD_PARAMETER (20160422/psparse-542)
ACPI Exception: AE_BAD_PARAMETER, from region _REG, [EmbeddedControl] (20160422/evregion-397)
ACPI Exception: AE_BAD_PARAMETER, Returned by Handler for [EmbeddedControl] (20160422/evregion-300)
ACPI Error: Method parse/execution failed [\_SB.PCI0.LPCB.EC0._REG] (Node f5d09870), AE_BAD_PARAMETER (20160422/psparse-542)
ACPI Exception: AE_BAD_PARAMETER, from region _REG, [EmbeddedControl] (20160422/evregion-397)
ACPI : EC: GPE = 0x19, I/O: command/status = 0x66, data = 0x62
ACPI : EC: EC started

The only config changes between the kernels were enabling MMC block
devices and DM_CRYPT. Not likely to cause the above, I'd think.

Does that ring a bell for someone? This little machine has only an SSD
without TRIM, so bisecting is not really an option. I can provide more
info and test patches, though.

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Regression between rc2 and rc6: ACPI EC problems

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 1851 bytes --]

On Wednesday, July 06, 2016 11:14:11 PM Wolfram Sang wrote:
> Hi,
> 
> when upgrading from v4.7-rc2 to rc6 on my Dell Mini 9, battery and
> temperature status info got bogus. Here is the major change between the
> boot logs:
> 
> ACPI : EC: EC stopped
> ACPI Exception: AE_BAD_PARAMETER, Returned by Handler for [EmbeddedControl] (20160422/evregion-300)
> ACPI Error: Method parse/execution failed [\_SB.PCI0.LPCB.EC0._REG] (Node f5d09870), AE_BAD_PARAMETER (20160422/psparse-542)
> ACPI Exception: AE_BAD_PARAMETER, from region _REG, [EmbeddedControl] (20160422/evregion-397)
> ACPI Exception: AE_BAD_PARAMETER, Returned by Handler for [EmbeddedControl] (20160422/evregion-300)
> ACPI Error: Method parse/execution failed [\_SB.PCI0.LPCB.EC0._REG] (Node f5d09870), AE_BAD_PARAMETER (20160422/psparse-542)
> ACPI Exception: AE_BAD_PARAMETER, from region _REG, [EmbeddedControl] (20160422/evregion-397)
> ACPI : EC: GPE = 0x19, I/O: command/status = 0x66, data = 0x62
> ACPI : EC: EC started
> 
> The only config changes between the kernels were enabling MMC block
> devices and DM_CRYPT. Not likely to cause the above, I'd think.
> 
> Does that ring a bell for someone? This little machine has only an SSD
> without TRIM, so bisecting is not really an option. I can provide more
> info and test patches, though.

There is one commit related directly to EC between -rc2 and -rc6:

dcf15cbded65 ACPI / EC: Fix a boot EC regresion by restoring boot EC support for the DSDT EC

There also are two ACPICA commits in that frame that may be related to it

2f38b1b16d92 ACPICA: Namespace: Fix deadlock triggered by MLC support in dynamic table loading
da4e792550a8 Revert "ACPICA: ACPI 2.0, Hardware: Add access_width/bit_offset support for acpi_hw_write()"

but one of them is a revert, so can you please check dcf15cbded65 and 2f38b1b16d92?

Thanks,
Rafael

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: Regression between rc2 and rc6: ACPI EC problems

[Zheng, Lv]

Hi, Rafael

> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Sent: Thursday, July 7, 2016 5:29 AM
> Subject: Re: Regression between rc2 and rc6: ACPI EC problems
> 
> On Wednesday, July 06, 2016 11:14:11 PM Wolfram Sang wrote:
> > Hi,
> >
> > when upgrading from v4.7-rc2 to rc6 on my Dell Mini 9, battery and
> > temperature status info got bogus. Here is the major change between
> the
> > boot logs:
> >
> > ACPI : EC: EC stopped
> > ACPI Exception: AE_BAD_PARAMETER, Returned by Handler for
> [EmbeddedControl] (20160422/evregion-300)
> > ACPI Error: Method parse/execution failed [\_SB.PCI0.LPCB.EC0._REG]
> (Node f5d09870), AE_BAD_PARAMETER (20160422/psparse-542)
> > ACPI Exception: AE_BAD_PARAMETER, from region _REG,
> [EmbeddedControl] (20160422/evregion-397)
> > ACPI Exception: AE_BAD_PARAMETER, Returned by Handler for
> [EmbeddedControl] (20160422/evregion-300)
> > ACPI Error: Method parse/execution failed [\_SB.PCI0.LPCB.EC0._REG]
> (Node f5d09870), AE_BAD_PARAMETER (20160422/psparse-542)
> > ACPI Exception: AE_BAD_PARAMETER, from region _REG,
> [EmbeddedControl] (20160422/evregion-397)
> > ACPI : EC: GPE = 0x19, I/O: command/status = 0x66, data = 0x62
> > ACPI : EC: EC started
[Lv Zheng] 
This is a known order issue that ACPICA executes _REG(DISCONENCT) after removing the opregion handler.
But this issue is not a function failure, it only generates error garbage in the kernel log.
So we didn't fix it in a hurry.

There is another order issue detected in the same Bugzilla entry that:
_REG(CONNECT/DISCONNECT) shouldn't be invoked for SystemMemory/SystemIo and PCI_Config that under a PCI root bus containing _BBN method.
Which is also proven by the Windows behavior and spec definitions.

I was planning to fix the both issues in the ACPICA upstream.
The bug is here for your reference:
https://bugzilla.kernel.org/show_bug.cgi?id=102421

For now, since this is just a non-function-failure error report, we can ignore it.
Because sometimes, when there are several problems related, we have to make a choice to fix one of them first.

Thanks and best regards
-Lv


> >
> > The only config changes between the kernels were enabling MMC block
> > devices and DM_CRYPT. Not likely to cause the above, I'd think.
> >
> > Does that ring a bell for someone? This little machine has only an SSD
> > without TRIM, so bisecting is not really an option. I can provide more
> > info and test patches, though.
> 
> There is one commit related directly to EC between -rc2 and -rc6:
> 
> dcf15cbded65 ACPI / EC: Fix a boot EC regresion by restoring boot EC
> support for the DSDT EC
> 
> There also are two ACPICA commits in that frame that may be related to it
> 
> 2f38b1b16d92 ACPICA: Namespace: Fix deadlock triggered by MLC
> support in dynamic table loading
> da4e792550a8 Revert "ACPICA: ACPI 2.0, Hardware: Add
> access_width/bit_offset support for acpi_hw_write()"
> 
> but one of them is a revert, so can you please check dcf15cbded65 and
> 2f38b1b16d92?
> 
> Thanks,
> Rafael

RE: Regression between rc2 and rc6: ACPI EC problems

[Zheng, Lv]

Hi, Wolfram

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> owner@vger.kernel.org] On Behalf Of Wolfram Sang
> Subject: Regression between rc2 and rc6: ACPI EC problems
> 
> Hi,
> 
> when upgrading from v4.7-rc2 to rc6 on my Dell Mini 9, battery and
> temperature status info got bogus. Here is the major change between the
> boot logs:
> 
> ACPI : EC: EC stopped
> ACPI Exception: AE_BAD_PARAMETER, Returned by Handler for
> [EmbeddedControl] (20160422/evregion-300)
> ACPI Error: Method parse/execution failed [\_SB.PCI0.LPCB.EC0._REG]
> (Node f5d09870), AE_BAD_PARAMETER (20160422/psparse-542)
> ACPI Exception: AE_BAD_PARAMETER, from region _REG,
> [EmbeddedControl] (20160422/evregion-397)
> ACPI Exception: AE_BAD_PARAMETER, Returned by Handler for
> [EmbeddedControl] (20160422/evregion-300)
> ACPI Error: Method parse/execution failed [\_SB.PCI0.LPCB.EC0._REG]
> (Node f5d09870), AE_BAD_PARAMETER (20160422/psparse-542)
> ACPI Exception: AE_BAD_PARAMETER, from region _REG,
> [EmbeddedControl] (20160422/evregion-397)
> ACPI : EC: GPE = 0x19, I/O: command/status = 0x66, data = 0x62
> ACPI : EC: EC started
> 
> The only config changes between the kernels were enabling MMC block
> devices and DM_CRYPT. Not likely to cause the above, I'd think.
> 
> Does that ring a bell for someone? This little machine has only an SSD
> without TRIM, so bisecting is not really an option. I can provide more
> info and test patches, though.
[Lv Zheng] 
Please just ignore this for several release cycles.
We need strategy to fix the ACPI initialization order issues.
This is already a noted non-function-failure issue.
We just need to fix the functional ones prior than this.

Thanks and best regards
-Lv

RE: Regression between rc2 and rc6: ACPI EC problems

[Zheng, Lv]

Hi, Walfram

You can register onto the kernel Bugzilla and monitor this bug for further updates:
https://bugzilla.kernel.org/show_bug.cgi?id=102421

Thanks and best regards
-Lv

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> owner@vger.kernel.org] On Behalf Of Wolfram Sang
> Subject: Regression between rc2 and rc6: ACPI EC problems
> 
> Hi,
> 
> when upgrading from v4.7-rc2 to rc6 on my Dell Mini 9, battery and
> temperature status info got bogus. Here is the major change between the
> boot logs:
> 
> ACPI : EC: EC stopped
> ACPI Exception: AE_BAD_PARAMETER, Returned by Handler for
> [EmbeddedControl] (20160422/evregion-300)
> ACPI Error: Method parse/execution failed [\_SB.PCI0.LPCB.EC0._REG]
> (Node f5d09870), AE_BAD_PARAMETER (20160422/psparse-542)
> ACPI Exception: AE_BAD_PARAMETER, from region _REG,
> [EmbeddedControl] (20160422/evregion-397)
> ACPI Exception: AE_BAD_PARAMETER, Returned by Handler for
> [EmbeddedControl] (20160422/evregion-300)
> ACPI Error: Method parse/execution failed [\_SB.PCI0.LPCB.EC0._REG]
> (Node f5d09870), AE_BAD_PARAMETER (20160422/psparse-542)
> ACPI Exception: AE_BAD_PARAMETER, from region _REG,
> [EmbeddedControl] (20160422/evregion-397)
> ACPI : EC: GPE = 0x19, I/O: command/status = 0x66, data = 0x62
> ACPI : EC: EC started
> 
> The only config changes between the kernels were enabling MMC block
> devices and DM_CRYPT. Not likely to cause the above, I'd think.
> 
> Does that ring a bell for someone? This little machine has only an SSD
> without TRIM, so bisecting is not really an option. I can provide more
> info and test patches, though.
> 
> Thanks,
> 
>    Wolfram

RE: Regression between rc2 and rc6: ACPI EC problems

[Zheng, Lv]

Hi, Rafael

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
> Subject: Re: Regression between rc2 and rc6: ACPI EC problems
> 
> On Thursday, July 07, 2016 12:26:49 AM Zheng, Lv wrote:
> > Hi, Rafael
> >
> > > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > > Sent: Thursday, July 7, 2016 5:29 AM
> > > Subject: Re: Regression between rc2 and rc6: ACPI EC problems
> > >
> > > On Wednesday, July 06, 2016 11:14:11 PM Wolfram Sang wrote:
> > > > Hi,
> > > >
> > > > when upgrading from v4.7-rc2 to rc6 on my Dell Mini 9, battery and
> > > > temperature status info got bogus. Here is the major change
> between
> > > the
> > > > boot logs:
> > > >
> > > > ACPI : EC: EC stopped
> > > > ACPI Exception: AE_BAD_PARAMETER, Returned by Handler for
> > > [EmbeddedControl] (20160422/evregion-300)
> > > > ACPI Error: Method parse/execution failed [\_SB.PCI0.LPCB.EC0._REG]
> > > (Node f5d09870), AE_BAD_PARAMETER (20160422/psparse-542)
> > > > ACPI Exception: AE_BAD_PARAMETER, from region _REG,
> > > [EmbeddedControl] (20160422/evregion-397)
> > > > ACPI Exception: AE_BAD_PARAMETER, Returned by Handler for
> > > [EmbeddedControl] (20160422/evregion-300)
> > > > ACPI Error: Method parse/execution failed [\_SB.PCI0.LPCB.EC0._REG]
> > > (Node f5d09870), AE_BAD_PARAMETER (20160422/psparse-542)
> > > > ACPI Exception: AE_BAD_PARAMETER, from region _REG,
> > > [EmbeddedControl] (20160422/evregion-397)
> > > > ACPI : EC: GPE = 0x19, I/O: command/status = 0x66, data = 0x62
> > > > ACPI : EC: EC started
> > [Lv Zheng]
> > This is a known order issue that ACPICA executes _REG(DISCONENCT)
> after removing the opregion handler.
> > But this issue is not a function failure, it only generates error garbage in
> the kernel log.
> > So we didn't fix it in a hurry.
> >
> > There is another order issue detected in the same Bugzilla entry that:
> > _REG(CONNECT/DISCONNECT) shouldn't be invoked for
> SystemMemory/SystemIo and PCI_Config that under a PCI root bus
> containing _BBN method.
> > Which is also proven by the Windows behavior and spec definitions.
> >
> > I was planning to fix the both issues in the ACPICA upstream.
> > The bug is here for your reference:
> > https://bugzilla.kernel.org/show_bug.cgi?id=102421
> >
> > For now, since this is just a non-function-failure error report, we can
> ignore it.
> > Because sometimes, when there are several problems related, we have
> to make a choice to fix one of them first.
> 
> Well, please look at the report again.  It says:
> 
> "when upgrading from v4.7-rc2 to rc6 on my Dell Mini 9, battery and
> temperature status info got bogus".
> 
> So, it very much *is* a functional issue and I'm not going to ignore it.
[Lv Zheng] 
What does the bogus mean here?
Anyway, I'll post a fix for this first.
I'll do this right now.

Thanks and best regards
-Lv

Re: Regression between rc2 and rc6: ACPI EC problems

[Rafael J. Wysocki]

On Thursday, July 07, 2016 12:26:49 AM Zheng, Lv wrote:
> Hi, Rafael
> 
> > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > Sent: Thursday, July 7, 2016 5:29 AM
> > Subject: Re: Regression between rc2 and rc6: ACPI EC problems
> > 
> > On Wednesday, July 06, 2016 11:14:11 PM Wolfram Sang wrote:
> > > Hi,
> > >
> > > when upgrading from v4.7-rc2 to rc6 on my Dell Mini 9, battery and
> > > temperature status info got bogus. Here is the major change between
> > the
> > > boot logs:
> > >
> > > ACPI : EC: EC stopped
> > > ACPI Exception: AE_BAD_PARAMETER, Returned by Handler for
> > [EmbeddedControl] (20160422/evregion-300)
> > > ACPI Error: Method parse/execution failed [\_SB.PCI0.LPCB.EC0._REG]
> > (Node f5d09870), AE_BAD_PARAMETER (20160422/psparse-542)
> > > ACPI Exception: AE_BAD_PARAMETER, from region _REG,
> > [EmbeddedControl] (20160422/evregion-397)
> > > ACPI Exception: AE_BAD_PARAMETER, Returned by Handler for
> > [EmbeddedControl] (20160422/evregion-300)
> > > ACPI Error: Method parse/execution failed [\_SB.PCI0.LPCB.EC0._REG]
> > (Node f5d09870), AE_BAD_PARAMETER (20160422/psparse-542)
> > > ACPI Exception: AE_BAD_PARAMETER, from region _REG,
> > [EmbeddedControl] (20160422/evregion-397)
> > > ACPI : EC: GPE = 0x19, I/O: command/status = 0x66, data = 0x62
> > > ACPI : EC: EC started
> [Lv Zheng] 
> This is a known order issue that ACPICA executes _REG(DISCONENCT) after removing the opregion handler.
> But this issue is not a function failure, it only generates error garbage in the kernel log.
> So we didn't fix it in a hurry.
> 
> There is another order issue detected in the same Bugzilla entry that:
> _REG(CONNECT/DISCONNECT) shouldn't be invoked for SystemMemory/SystemIo and PCI_Config that under a PCI root bus containing _BBN method.
> Which is also proven by the Windows behavior and spec definitions.
> 
> I was planning to fix the both issues in the ACPICA upstream.
> The bug is here for your reference:
> https://bugzilla.kernel.org/show_bug.cgi?id=102421
> 
> For now, since this is just a non-function-failure error report, we can ignore it.
> Because sometimes, when there are several problems related, we have to make a choice to fix one of them first.

Well, please look at the report again.  It says:

"when upgrading from v4.7-rc2 to rc6 on my Dell Mini 9, battery and
temperature status info got bogus".

So, it very much *is* a functional issue and I'm not going to ignore it.

Thanks,
Rafael

[RFC PATCH] ACPI / EC: Fix an order issue in ec_remove_handlers()

[Lv Zheng]

There is an order issue in ec_remove_handlers() that the functions invoked
in it are not invoked in the reversed order of their appearance in
ec_install_handlers(). This existing issue has been triggered by the
following commit:
  Commit: dcf15cbded656a12335bc4151f3f75f10080a375
  Subject: ACPI / EC: Fix a boot EC regresion by restoring boot EC
The commit invokes ec_remove_handlers() during runtime, thus uncovers this
issue. This patch fixes this regression.

Fixes: dcf15cbded65 ("ACPI / EC: Fix a boot EC regresion by restoring boot EC")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=102421
Reported-by: Wolfram Sang <wsa@the-dreams.de>
Reported-by: Nicholas <nkudriavtsev@gmail.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: Nicholas <nkudriavtsev@gmail.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index b1050a0..9ff3d4b 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1331,7 +1331,12 @@ static int ec_install_handlers(struct acpi_ec *ec)
 
 static void ec_remove_handlers(struct acpi_ec *ec)
 {
-	acpi_ec_stop(ec, false);
+	if (test_bit(EC_FLAGS_GPE_HANDLER_INSTALLED, &ec->flags)) {
+		if (ACPI_FAILURE(acpi_remove_gpe_handler(NULL, ec->gpe,
+					&acpi_ec_gpe_handler)))
+			pr_err("failed to remove gpe handler\n");
+		clear_bit(EC_FLAGS_GPE_HANDLER_INSTALLED, &ec->flags);
+	}
 
 	if (test_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags)) {
 		if (ACPI_FAILURE(acpi_remove_address_space_handler(ec->handle,
@@ -1340,12 +1345,7 @@ static void ec_remove_handlers(struct acpi_ec *ec)
 		clear_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
 	}
 
-	if (test_bit(EC_FLAGS_GPE_HANDLER_INSTALLED, &ec->flags)) {
-		if (ACPI_FAILURE(acpi_remove_gpe_handler(NULL, ec->gpe,
-					&acpi_ec_gpe_handler)))
-			pr_err("failed to remove gpe handler\n");
-		clear_bit(EC_FLAGS_GPE_HANDLER_INSTALLED, &ec->flags);
-	}
+	acpi_ec_stop(ec, false);
 }
 
 static struct acpi_ec *acpi_ec_alloc(void)
-- 
1.7.10

[RFC PATCH] ACPI / EC: Fix an order issue in ec_remove_handlers()

[Lv Zheng]

There is an order issue in ec_remove_handlers() that the functions invoked
in it are not invoked in the reversed order of their appearance in
ec_install_handlers(). This existing issue has been triggered by the
following commit:
  Commit: dcf15cbded656a12335bc4151f3f75f10080a375
  Subject: ACPI / EC: Fix a boot EC regresion by restoring boot EC
The commit invokes ec_remove_handlers() during runtime, thus uncovers this
issue. This patch fixes this regression.

Fixes: dcf15cbded65 ("ACPI / EC: Fix a boot EC regresion by restoring boot EC")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=102421
Reported-by: Wolfram Sang <wsa@the-dreams.de>
Reported-by: Nicholas <nkudriavtsev@gmail.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: Nicholas <nkudriavtsev@gmail.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index b1050a0..9ff3d4b 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1331,7 +1331,12 @@ static int ec_install_handlers(struct acpi_ec *ec)
 
 static void ec_remove_handlers(struct acpi_ec *ec)
 {
-	acpi_ec_stop(ec, false);
+	if (test_bit(EC_FLAGS_GPE_HANDLER_INSTALLED, &ec->flags)) {
+		if (ACPI_FAILURE(acpi_remove_gpe_handler(NULL, ec->gpe,
+					&acpi_ec_gpe_handler)))
+			pr_err("failed to remove gpe handler\n");
+		clear_bit(EC_FLAGS_GPE_HANDLER_INSTALLED, &ec->flags);
+	}
 
 	if (test_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags)) {
 		if (ACPI_FAILURE(acpi_remove_address_space_handler(ec->handle,
@@ -1340,12 +1345,7 @@ static void ec_remove_handlers(struct acpi_ec *ec)
 		clear_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
 	}
 
-	if (test_bit(EC_FLAGS_GPE_HANDLER_INSTALLED, &ec->flags)) {
-		if (ACPI_FAILURE(acpi_remove_gpe_handler(NULL, ec->gpe,
-					&acpi_ec_gpe_handler)))
-			pr_err("failed to remove gpe handler\n");
-		clear_bit(EC_FLAGS_GPE_HANDLER_INSTALLED, &ec->flags);
-	}
+	acpi_ec_stop(ec, false);
 }
 
 static struct acpi_ec *acpi_ec_alloc(void)
-- 
1.7.10

[UPDATE RFC PATCH v2] ACPI / EC: Fix an order issue in ec_remove_handlers()

[Lv Zheng]

(Updated to add v2 indicator)

There is an order issue in ec_remove_handlers() that the functions invoked
in it are not invoked in the reversed order of their appearance in
ec_install_handlers(). This existing issue has been triggered by the
following commit:
  Commit: dcf15cbded656a12335bc4151f3f75f10080a375
  Subject: ACPI / EC: Fix a boot EC regresion by restoring boot EC
The commit invokes ec_remove_handlers() during runtime, thus uncovers this
issue. This patch fixes this regression.

Fixes: dcf15cbded65 ("ACPI / EC: Fix a boot EC regresion by restoring boot EC")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=102421
Reported-by: Wolfram Sang <wsa@the-dreams.de>
Reported-by: Nicholas <nkudriavtsev@gmail.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: Nicholas <nkudriavtsev@gmail.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index b1050a0..d513208 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1331,8 +1331,6 @@ static int ec_install_handlers(struct acpi_ec *ec)
 
 static void ec_remove_handlers(struct acpi_ec *ec)
 {
-	acpi_ec_stop(ec, false);
-
 	if (test_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags)) {
 		if (ACPI_FAILURE(acpi_remove_address_space_handler(ec->handle,
 					ACPI_ADR_SPACE_EC, &acpi_ec_space_handler)))
@@ -1340,6 +1338,17 @@ static void ec_remove_handlers(struct acpi_ec *ec)
 		clear_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
 	}
 
+	/*
+	 * Disabling EC (transactions) after removing the operation region
+	 * handler. This order is required because _REG(DISCONNECT) may
+	 * access the EmbeddedControl operation regions.
+	 *
+	 * Flushing transactions before removing the GPE handler. This is
+	 * required by the current ACPICA GPE design. ACPICA GPE will block
+	 * a GPE if there is no way to handle it.
+	 */
+	acpi_ec_stop(ec, false);
+
 	if (test_bit(EC_FLAGS_GPE_HANDLER_INSTALLED, &ec->flags)) {
 		if (ACPI_FAILURE(acpi_remove_gpe_handler(NULL, ec->gpe,
 					&acpi_ec_gpe_handler)))
-- 
1.7.10

RE: [RFC PATCH] ACPI / EC: Fix an order issue in ec_remove_handlers()

[Zheng, Lv]

This one and the previous one contain problems.
A patch marked as [UPDATE RFC v2] is the correct fix.
Sorry for the noise.

Thanks and best regards
-Lv

> From: Zheng, Lv
> Subject: [RFC PATCH] ACPI / EC: Fix an order issue in ec_remove_handlers()
> 
> There is an order issue in ec_remove_handlers() that the functions invoked
> in it are not invoked in the reversed order of their appearance in
> ec_install_handlers(). This existing issue has been triggered by the
> following commit:
>   Commit: dcf15cbded656a12335bc4151f3f75f10080a375
>   Subject: ACPI / EC: Fix a boot EC regresion by restoring boot EC
> The commit invokes ec_remove_handlers() during runtime, thus uncovers
> this
> issue. This patch fixes this regression.
> 
> Fixes: dcf15cbded65 ("ACPI / EC: Fix a boot EC regresion by restoring boot
> EC")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=102421
> Reported-by: Wolfram Sang <wsa@the-dreams.de>
> Reported-by: Nicholas <nkudriavtsev@gmail.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: Nicholas <nkudriavtsev@gmail.com>
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> ---
>  drivers/acpi/ec.c |   14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> Index: linux-acpica/drivers/acpi/ec.c
> ==========================================================
> =========
> --- linux-acpica.orig/drivers/acpi/ec.c
> +++ linux-acpica/drivers/acpi/ec.c
> @@ -1331,8 +1331,6 @@ static int ec_install_handlers(struct ac
> 
>  static void ec_remove_handlers(struct acpi_ec *ec)
>  {
> -	acpi_ec_stop(ec, false);
> -
>  	if (test_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags)) {
>  		if (ACPI_FAILURE(acpi_remove_address_space_handler(ec-
> >handle,
>  					ACPI_ADR_SPACE_EC,
> &acpi_ec_space_handler)))
> @@ -1340,6 +1338,17 @@ static void ec_remove_handlers(struct ac
>  		clear_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
>  	}
> 
> +	/*
> +	 * Disabling EC (transactions) after removing the operation region
> +	 * handler. This order is required because _REG(DISCONNECT) may
> +	 * access the EmbeddedControl operation regions.
> +	 *
> +	 * Flushing transactions before removing the GPE handler. This is
> +	 * required by the current ACPICA GPE design. ACPICA GPE will
> block
> +	 * a GPE if there is no way to handle it.
> +	 */
> +	acpi_ec_stop(ec, false);
> +
>  	if (test_bit(EC_FLAGS_GPE_HANDLER_INSTALLED, &ec->flags)) {
>  		if (ACPI_FAILURE(acpi_remove_gpe_handler(NULL, ec->gpe,
>  					&acpi_ec_gpe_handler)))

Re: [UPDATE RFC PATCH v2] ACPI / EC: Fix an order issue in ec_remove_handlers()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1147 bytes --]

On Thu, Jul 07, 2016 at 12:37:10PM +0800, Lv Zheng wrote:
> (Updated to add v2 indicator)
> 
> There is an order issue in ec_remove_handlers() that the functions invoked
> in it are not invoked in the reversed order of their appearance in
> ec_install_handlers(). This existing issue has been triggered by the
> following commit:
>   Commit: dcf15cbded656a12335bc4151f3f75f10080a375
>   Subject: ACPI / EC: Fix a boot EC regresion by restoring boot EC
> The commit invokes ec_remove_handlers() during runtime, thus uncovers this
> issue. This patch fixes this regression.
> 
> Fixes: dcf15cbded65 ("ACPI / EC: Fix a boot EC regresion by restoring boot EC")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=102421
> Reported-by: Wolfram Sang <wsa@the-dreams.de>
> Reported-by: Nicholas <nkudriavtsev@gmail.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: Nicholas <nkudriavtsev@gmail.com>
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>

Tested-by: Wolfram Sang <wsa@the-dreams.de>

The status infos are back to normal and the EC error reports are gone
from the log. Looks good! Thanks for the quick fix.

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [UPDATE RFC PATCH v2] ACPI / EC: Fix an order issue in ec_remove_handlers()

[Zheng, Lv]

Hi,

> From: Wolfram Sang [mailto:wsa@the-dreams.de]
> Subject: Re: [UPDATE RFC PATCH v2] ACPI / EC: Fix an order issue in
> ec_remove_handlers()
> 
> On Thu, Jul 07, 2016 at 12:37:10PM +0800, Lv Zheng wrote:
> > (Updated to add v2 indicator)
> >
> > There is an order issue in ec_remove_handlers() that the functions
> invoked
> > in it are not invoked in the reversed order of their appearance in
> > ec_install_handlers(). This existing issue has been triggered by the
> > following commit:
> >   Commit: dcf15cbded656a12335bc4151f3f75f10080a375
> >   Subject: ACPI / EC: Fix a boot EC regresion by restoring boot EC
> > The commit invokes ec_remove_handlers() during runtime, thus
> uncovers this
> > issue. This patch fixes this regression.
> >
> > Fixes: dcf15cbded65 ("ACPI / EC: Fix a boot EC regresion by restoring
> boot EC")
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=102421
> > Reported-by: Wolfram Sang <wsa@the-dreams.de>
> > Reported-by: Nicholas <nkudriavtsev@gmail.com>
> > Cc: Wolfram Sang <wsa@the-dreams.de>
> > Cc: Nicholas <nkudriavtsev@gmail.com>
> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> 
> Tested-by: Wolfram Sang <wsa@the-dreams.de>
> 
> The status infos are back to normal and the EC error reports are gone
> from the log. Looks good! Thanks for the quick fix.

[Lv Zheng] 
Thanks for the test, I'll refresh the patch with your tested-by added and RFC removed.

Best regards
-Lv

[PATCH v3] ACPI / EC: Fix an order issue in ec_remove_handlers()

[Lv Zheng]

There is an order issue in ec_remove_handlers() that the functions invoked
in it are not able to handle the current restrictions (see comments below).
If ec_remove_handlers() is invoked during runtime after _REG(CONNECT) is
invoked, this issue will be uncovered.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=102421
Reported-and-tested-by: Wolfram Sang <wsa@the-dreams.de>
Reported-by: Nicholas <nkudriavtsev@gmail.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index b1050a0..84073a6 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1331,8 +1331,6 @@ static int ec_install_handlers(struct acpi_ec *ec)
 
 static void ec_remove_handlers(struct acpi_ec *ec)
 {
-	acpi_ec_stop(ec, false);
-
 	if (test_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags)) {
 		if (ACPI_FAILURE(acpi_remove_address_space_handler(ec->handle,
 					ACPI_ADR_SPACE_EC, &acpi_ec_space_handler)))
@@ -1340,6 +1338,19 @@ static void ec_remove_handlers(struct acpi_ec *ec)
 		clear_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
 	}
 
+	/*
+	 * Stops handling the EC transactions after removing the operation
+	 * region handler. This is required because _REG(DISCONNECT)
+	 * invoked during the removal can result in new EC transactions.
+	 *
+	 * Flushes the EC requests and thus disables the GPE before
+	 * removing the GPE handler. This is required by the current ACPICA
+	 * GPE core. ACPICA GPE core will automatically disable a GPE when
+	 * it is indicated but there is no way to handle it. So the drivers
+	 * must disable the GPEs prior than removing the GPE handlers.
+	 */
+	acpi_ec_stop(ec, false);
+
 	if (test_bit(EC_FLAGS_GPE_HANDLER_INSTALLED, &ec->flags)) {
 		if (ACPI_FAILURE(acpi_remove_gpe_handler(NULL, ec->gpe,
 					&acpi_ec_gpe_handler)))
-- 
1.7.10

[PATCH v3] ACPI / EC: Fix an order issue in ec_remove_handlers()

[Lv Zheng]

There is an order issue in ec_remove_handlers() that the functions invoked
in it are not able to handle the current restrictions (see comments below).
If ec_remove_handlers() is invoked during runtime after _REG(CONNECT) is
invoked, this issue will be uncovered.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=102421
Reported-and-tested-by: Wolfram Sang <wsa@the-dreams.de>
Reported-by: Nicholas <nkudriavtsev@gmail.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index b1050a0..84073a6 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1331,8 +1331,6 @@ static int ec_install_handlers(struct acpi_ec *ec)
 
 static void ec_remove_handlers(struct acpi_ec *ec)
 {
-	acpi_ec_stop(ec, false);
-
 	if (test_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags)) {
 		if (ACPI_FAILURE(acpi_remove_address_space_handler(ec->handle,
 					ACPI_ADR_SPACE_EC, &acpi_ec_space_handler)))
@@ -1340,6 +1338,19 @@ static void ec_remove_handlers(struct acpi_ec *ec)
 		clear_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
 	}
 
+	/*
+	 * Stops handling the EC transactions after removing the operation
+	 * region handler. This is required because _REG(DISCONNECT)
+	 * invoked during the removal can result in new EC transactions.
+	 *
+	 * Flushes the EC requests and thus disables the GPE before
+	 * removing the GPE handler. This is required by the current ACPICA
+	 * GPE core. ACPICA GPE core will automatically disable a GPE when
+	 * it is indicated but there is no way to handle it. So the drivers
+	 * must disable the GPEs prior than removing the GPE handlers.
+	 */
+	acpi_ec_stop(ec, false);
+
 	if (test_bit(EC_FLAGS_GPE_HANDLER_INSTALLED, &ec->flags)) {
 		if (ACPI_FAILURE(acpi_remove_gpe_handler(NULL, ec->gpe,
 					&acpi_ec_gpe_handler)))
-- 
1.7.10