[PATCH 0/4] ACPI: EC: Various cleanups

[PATCH 0/4] ACPI: EC: Various cleanups

[Hans de Goede]

Hi All,

Here is a set of cleanups / removal of no longer necessary
quirks (or so I believe please review carefully). These are all
things which I noticed while working on my:
"[RFC 0/4] ACPI[CA]: fix ECDT EC probe ordering issues" series.

Regards,

Hans

p.s.
Talking about my "[RFC 0/4] ACPI[CA]: fix ECDT EC probe ordering issues"
series, it would be great if someone can take a look at this and let me
know if that series seems sane. Then I can convert the ACPICA changes
from kernel patches to upstream github acpica patches and submit a
pull-req for those at github.


Hans de Goede (4):
  ACPI: EC: Remove duplicate ThinkPad X1 Carbon 6th entry from DMI
    quirks
  ACPI: EC: Drop the EC_FLAGS_IGNORE_DSDT_GPE quirk
  ACPI: EC: Re-use boot_ec when possible even when
    EC_FLAGS_TRUST_DSDT_GPE is set
  ACPI: EC: Drop unused ident initializers from dmi_system_id tables

 drivers/acpi/ec.c | 140 ++++++++++++++++------------------------------
 1 file changed, 48 insertions(+), 92 deletions(-)

-- 
2.36.0

[PATCH 1/4] ACPI: EC: Remove duplicate ThinkPad X1 Carbon 6th entry from DMI quirks

[Hans de Goede]

Somehow the "ThinkPad X1 Carbon 6th" entry ended up twice in the
struct dmi_system_id acpi_ec_no_wakeup[] array. Remove one of
the entries.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/ec.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index cd86e68d6b98..2efbecb342b2 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -2225,13 +2225,6 @@ static const struct dmi_system_id acpi_ec_no_wakeup[] = {
 			DMI_MATCH(DMI_PRODUCT_FAMILY, "Thinkpad X1 Carbon 6th"),
 		},
 	},
-	{
-		.ident = "ThinkPad X1 Carbon 6th",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
-			DMI_MATCH(DMI_PRODUCT_FAMILY, "ThinkPad X1 Carbon 6th"),
-		},
-	},
 	{
 		.ident = "ThinkPad X1 Yoga 3rd",
 		.matches = {
-- 
2.36.0

[PATCH 2/4] ACPI: EC: Drop the EC_FLAGS_IGNORE_DSDT_GPE quirk

[Hans de Goede]

It seems that these quirks are no longer necessary since
commit 69b957c26b32 ("ACPI: EC: Fix possible issues related to EC
initialization order"), which has fixed this in a generic manner.

There are 3 commits adding DMI entries with this quirk (adding multiple
DMI entries per commit). 2/3 commits are from before the generic fix.

Which leaves commit 6306f0431914 ("ACPI: EC: Make more Asus laptops
use ECDT _GPE"), which was committed way after the generic fix.
But this was just due to slow upstreaming of it. This commit stems
from Endless from 15 Aug 2017 (committed upstream 20 May 2021):
https://github.com/endlessm/linux/pull/288

The current code should work fine without this:
1. The EC_FLAGS_IGNORE_DSDT_GPE flag is only checked in ec_parse_device(),
   like this:

	if (boot_ec && boot_ec_is_ecdt && EC_FLAGS_IGNORE_DSDT_GPE) {
		ec->gpe = boot_ec->gpe;
	} else {
		/* parse GPE */
	}

2. ec_parse_device() is only called from acpi_ec_add() and
   acpi_ec_dsdt_probe()

3. acpi_ec_dsdt_probe() starts with:

	if (boot_ec)
		return;

   so it only calls ec_parse_device() when boot_ec == NULL, meaning that
   the quirk never triggers for this call. So only the call in
   acpi_ec_add() matters.

4. acpi_ec_add() does the following after the ec_parse_device() call:

	if (boot_ec && ec->command_addr == boot_ec->command_addr &&
	    ec->data_addr == boot_ec->data_addr &&
	    !EC_FLAGS_TRUST_DSDT_GPE) {
		/*
		 * Trust PNP0C09 namespace location rather than
		 * ECDT ID. But trust ECDT GPE rather than _GPE
		 * because of ASUS quirks, so do not change
		 * boot_ec->gpe to ec->gpe.
		 */
		boot_ec->handle = ec->handle;
		acpi_handle_debug(ec->handle, "duplicated.\n");
		acpi_ec_free(ec);
		ec = boot_ec;
	}

The quirk only matters if boot_ec != NULL and EC_FLAGS_TRUST_DSDT_GPE
is never set at the same time as EC_FLAGS_IGNORE_DSDT_GPE.

That means that if the addresses match we always enter this if block and
then only the ec->handle part of the data stored in ec by ec_parse_device()
is used and the rest is thrown away, after which ec is made to point
to boot_ec, at which point ec->gpe == boot_ec->gpe, so the same result
as with the quirk set, independent of the value of the quirk.

Also note the comment in this block which indicates that the gpe result
from ec_parse_device() is deliberately not taken to deal with buggy
Asus laptops and all DMI quirks setting EC_FLAGS_IGNORE_DSDT_GPE are for
Asus laptops.

Based on the above I believe that unless on some quirked laptops
the ECDT and DSDT EC addresses do not match we can drop the quirk.

I've checked dmesg output to ensure the ECDT and DSDT EC addresses match
for quirked models using https://linux-hardware.org hw-probe reports.

I've been able to confirm that the addresses match for the following
models this way: GL702VMK, X505BA, X505BP, X550VXK, X580VD.
Whereas for the following models I could find any dmesg output:
FX502VD, FX502VE, X542BA, X542BP.

Note the models without dmesg all were submitted in patches with a batch
of models and other models from the same batch checkout ok.

This, combined with that all the code adding the quirks was written before
the generic fix makes me believe that it is safe to remove this quirk now.

Cc: Lv Zheng <lv.zheng@intel.com>
Cc: Chris Chiu <chris.chiu@canonical.com>
Cc: Jian-Hong Pan <jhp@endlessos.org>
Cc: Carlo Caione <carlo@caione.org>
Cc: Daniel Drake <drake@endlessm.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Note this has not been tested by me on any of the laptops for which
the quirk is removed, this is purely based on my reading of the code.
Please review carefully.
---
 drivers/acpi/ec.c | 75 ++++++-----------------------------------------
 1 file changed, 9 insertions(+), 66 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 2efbecb342b2..b1316aba844d 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -180,7 +180,6 @@ static struct workqueue_struct *ec_wq;
 static struct workqueue_struct *ec_query_wq;
 
 static int EC_FLAGS_CORRECT_ECDT; /* Needs ECDT port address correction */
-static int EC_FLAGS_IGNORE_DSDT_GPE; /* Needs ECDT GPE as correction setting */
 static int EC_FLAGS_TRUST_DSDT_GPE; /* Needs DSDT GPE as correction setting */
 static int EC_FLAGS_CLEAR_ON_RESUME; /* Needs acpi_ec_clear() on boot/resume */
 
@@ -1407,24 +1406,16 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
 	if (ec->data_addr == 0 || ec->command_addr == 0)
 		return AE_OK;
 
-	if (boot_ec && boot_ec_is_ecdt && EC_FLAGS_IGNORE_DSDT_GPE) {
-		/*
-		 * Always inherit the GPE number setting from the ECDT
-		 * EC.
-		 */
-		ec->gpe = boot_ec->gpe;
-	} else {
-		/* Get GPE bit assignment (EC events). */
-		/* TODO: Add support for _GPE returning a package */
-		status = acpi_evaluate_integer(handle, "_GPE", NULL, &tmp);
-		if (ACPI_SUCCESS(status))
-			ec->gpe = tmp;
+	/* Get GPE bit assignment (EC events). */
+	/* TODO: Add support for _GPE returning a package */
+	status = acpi_evaluate_integer(handle, "_GPE", NULL, &tmp);
+	if (ACPI_SUCCESS(status))
+		ec->gpe = tmp;
+	/*
+	 * Errors are non-fatal, allowing for ACPI Reduced Hardware
+	 * platforms which use GpioInt instead of GPE.
+	 */
 
-		/*
-		 * Errors are non-fatal, allowing for ACPI Reduced Hardware
-		 * platforms which use GpioInt instead of GPE.
-		 */
-	}
 	/* Use the global lock for all EC transactions? */
 	tmp = 0;
 	acpi_evaluate_integer(handle, "_GLK", NULL, &tmp);
@@ -1863,60 +1854,12 @@ static int ec_honor_dsdt_gpe(const struct dmi_system_id *id)
 	return 0;
 }
 
-/*
- * Some DSDTs contain wrong GPE setting.
- * Asus FX502VD/VE, GL702VMK, X550VXK, X580VD
- * https://bugzilla.kernel.org/show_bug.cgi?id=195651
- */
-static int ec_honor_ecdt_gpe(const struct dmi_system_id *id)
-{
-	pr_debug("Detected system needing ignore DSDT GPE setting.\n");
-	EC_FLAGS_IGNORE_DSDT_GPE = 1;
-	return 0;
-}
-
 static const struct dmi_system_id ec_dmi_table[] __initconst = {
 	{
 	ec_correct_ecdt, "MSI MS-171F", {
 	DMI_MATCH(DMI_SYS_VENDOR, "Micro-Star"),
 	DMI_MATCH(DMI_PRODUCT_NAME, "MS-171F"),}, NULL},
 	{
-	ec_honor_ecdt_gpe, "ASUS FX502VD", {
-	DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
-	DMI_MATCH(DMI_PRODUCT_NAME, "FX502VD"),}, NULL},
-	{
-	ec_honor_ecdt_gpe, "ASUS FX502VE", {
-	DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
-	DMI_MATCH(DMI_PRODUCT_NAME, "FX502VE"),}, NULL},
-	{
-	ec_honor_ecdt_gpe, "ASUS GL702VMK", {
-	DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
-	DMI_MATCH(DMI_PRODUCT_NAME, "GL702VMK"),}, NULL},
-	{
-	ec_honor_ecdt_gpe, "ASUSTeK COMPUTER INC. X505BA", {
-	DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
-	DMI_MATCH(DMI_PRODUCT_NAME, "X505BA"),}, NULL},
-	{
-	ec_honor_ecdt_gpe, "ASUSTeK COMPUTER INC. X505BP", {
-	DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
-	DMI_MATCH(DMI_PRODUCT_NAME, "X505BP"),}, NULL},
-	{
-	ec_honor_ecdt_gpe, "ASUSTeK COMPUTER INC. X542BA", {
-	DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
-	DMI_MATCH(DMI_PRODUCT_NAME, "X542BA"),}, NULL},
-	{
-	ec_honor_ecdt_gpe, "ASUSTeK COMPUTER INC. X542BP", {
-	DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
-	DMI_MATCH(DMI_PRODUCT_NAME, "X542BP"),}, NULL},
-	{
-	ec_honor_ecdt_gpe, "ASUS X550VXK", {
-	DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
-	DMI_MATCH(DMI_PRODUCT_NAME, "X550VXK"),}, NULL},
-	{
-	ec_honor_ecdt_gpe, "ASUS X580VD", {
-	DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
-	DMI_MATCH(DMI_PRODUCT_NAME, "X580VD"),}, NULL},
-	{
 	/* https://bugzilla.kernel.org/show_bug.cgi?id=209989 */
 	ec_honor_dsdt_gpe, "HP Pavilion Gaming Laptop 15-cx0xxx", {
 	DMI_MATCH(DMI_SYS_VENDOR, "HP"),
-- 
2.36.0

[PATCH 3/4] ACPI: EC: Re-use boot_ec when possible even when EC_FLAGS_TRUST_DSDT_GPE is set

[Hans de Goede]

EC_FLAGS_TRUST_DSDT_GPE only does anything when the:

	if (boot_ec && ec->command_addr == boot_ec->command_addr &&
	    ec->data_addr == boot_ec->data_addr)

conditions are all true. Normally acpi_ec_add() would re-use the boot_ec
struct acpi_ec in this case. But when the EC_FLAGS_TRUST_DSDT_GPE flag was
set the code would continue with a newly allocated (second) struct acpi_ec.

There is no reason to use a second struct acpi_ec if all the above checks
match. Instead just change boot_ec->gpe to ec->gpe, when the flag is set,
similar to how this is already one done for boot_ec->handle.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Note this has not been tested by me on the one laptop model which uses
this quirk. This is purely based on my reading of the code.
Please review carefully.
---
 drivers/acpi/ec.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index b1316aba844d..a4c16b8540e5 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1618,15 +1618,18 @@ static int acpi_ec_add(struct acpi_device *device)
 		}
 
 		if (boot_ec && ec->command_addr == boot_ec->command_addr &&
-		    ec->data_addr == boot_ec->data_addr &&
-		    !EC_FLAGS_TRUST_DSDT_GPE) {
+		    ec->data_addr == boot_ec->data_addr) {
 			/*
-			 * Trust PNP0C09 namespace location rather than
-			 * ECDT ID. But trust ECDT GPE rather than _GPE
-			 * because of ASUS quirks, so do not change
-			 * boot_ec->gpe to ec->gpe.
+			 * Trust PNP0C09 namespace location rather than ECDT ID.
+			 * But trust ECDT GPE rather than _GPE because of ASUS
+			 * quirks. So do not change boot_ec->gpe to ec->gpe,
+			 * except when the TRUST_DSDT_GPE quirk is set.
 			 */
 			boot_ec->handle = ec->handle;
+
+			if (EC_FLAGS_TRUST_DSDT_GPE)
+				boot_ec->gpe = ec->gpe;
+
 			acpi_handle_debug(ec->handle, "duplicated.\n");
 			acpi_ec_free(ec);
 			ec = boot_ec;
-- 
2.36.0

[PATCH 4/4] ACPI: EC: Drop unused ident initializers from dmi_system_id tables

[Hans de Goede]

Drop the unused const string ident initializers from
the dmi_system_id tables to make the object size a bit smaller.

While at it also use proper named struct-member initializers for
the ec_dmi_table[].

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/ec.c | 43 ++++++++++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index a4c16b8540e5..79ba7b04417b 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1859,18 +1859,38 @@ static int ec_honor_dsdt_gpe(const struct dmi_system_id *id)
 
 static const struct dmi_system_id ec_dmi_table[] __initconst = {
 	{
-	ec_correct_ecdt, "MSI MS-171F", {
-	DMI_MATCH(DMI_SYS_VENDOR, "Micro-Star"),
-	DMI_MATCH(DMI_PRODUCT_NAME, "MS-171F"),}, NULL},
+		/*
+		 * MSI MS-171F
+		 * https://bugzilla.kernel.org/show_bug.cgi?id=12461
+		 */
+		.callback = ec_correct_ecdt,
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Micro-Star"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "MS-171F"),
+		},
+	},
 	{
-	/* https://bugzilla.kernel.org/show_bug.cgi?id=209989 */
-	ec_honor_dsdt_gpe, "HP Pavilion Gaming Laptop 15-cx0xxx", {
-	DMI_MATCH(DMI_SYS_VENDOR, "HP"),
-	DMI_MATCH(DMI_PRODUCT_NAME, "HP Pavilion Gaming Laptop 15-cx0xxx"),}, NULL},
+		/*
+		 * HP Pavilion Gaming Laptop 15-cx0xxx
+		 * https://bugzilla.kernel.org/show_bug.cgi?id=209989
+		 */
+		.callback = ec_honor_dsdt_gpe,
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "HP"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "HP Pavilion Gaming Laptop 15-cx0xxx"),
+		},
+	},
 	{
-	ec_clear_on_resume, "Samsung hardware", {
-	DMI_MATCH(DMI_SYS_VENDOR, "SAMSUNG ELECTRONICS CO., LTD.")}, NULL},
-	{},
+		/*
+		 * Samsung hardware
+		 * https://bugzilla.kernel.org/show_bug.cgi?id=44161
+		 */
+		.callback = ec_clear_on_resume,
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "SAMSUNG ELECTRONICS CO., LTD."),
+		},
+	},
+	{}
 };
 
 void __init acpi_ec_ecdt_probe(void)
@@ -2165,21 +2185,18 @@ static int acpi_ec_init_workqueues(void)
 
 static const struct dmi_system_id acpi_ec_no_wakeup[] = {
 	{
-		.ident = "Thinkpad X1 Carbon 6th",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
 			DMI_MATCH(DMI_PRODUCT_FAMILY, "Thinkpad X1 Carbon 6th"),
 		},
 	},
 	{
-		.ident = "ThinkPad X1 Yoga 3rd",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
 			DMI_MATCH(DMI_PRODUCT_FAMILY, "ThinkPad X1 Yoga 3rd"),
 		},
 	},
 	{
-		.ident = "HP ZHAN 66 Pro",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "HP"),
 			DMI_MATCH(DMI_PRODUCT_FAMILY, "103C_5336AN HP ZHAN 66 Pro"),
-- 
2.36.0

Re: [PATCH 2/4] ACPI: EC: Drop the EC_FLAGS_IGNORE_DSDT_GPE quirk

[Daniel Drake]

Hi Hans,

Thanks for looking at this.

On Mon, Jun 20, 2022 at 5:26 PM Hans de Goede <hdegoede@redhat.com> wrote:
> Which leaves commit 6306f0431914 ("ACPI: EC: Make more Asus laptops
> use ECDT _GPE"), which was committed way after the generic fix.
> But this was just due to slow upstreaming of it. This commit stems
> from Endless from 15 Aug 2017 (committed upstream 20 May 2021):
> https://github.com/endlessm/linux/pull/288
>
> The current code should work fine without this:

Your explanation of the code flow seems clear and logical, but I have
not checked the details. This is a bit of a tricky issue as you have
probably seen from history, we went in a couple of wrong directions
before we spotted the real cause.

The one thing I don't see clearly in your explanation (which I may
have read too quickly) is how the generic fix 69b957c26b32 is
responsible for making this a "no-op" code flow now.

We don't have access to any of the affected hardware any more, unfortunately.

For certainty I wonder if you could recreate this on another system.
Something like:
1. Create DSDT override which has the wrong _GPE value
2. Run the original 2017 kernel with that override and observe that
Linux uses that wrong value
3. Apply the generic fix and check that it uses the right one from the ECDT now

Daniel

Re: [PATCH 2/4] ACPI: EC: Drop the EC_FLAGS_IGNORE_DSDT_GPE quirk

[Hans de Goede]

Hi,

On 6/20/22 12:47, Daniel Drake wrote:
> Hi Hans,
> 
> Thanks for looking at this.
> 
> On Mon, Jun 20, 2022 at 5:26 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> Which leaves commit 6306f0431914 ("ACPI: EC: Make more Asus laptops
>> use ECDT _GPE"), which was committed way after the generic fix.
>> But this was just due to slow upstreaming of it. This commit stems
>> from Endless from 15 Aug 2017 (committed upstream 20 May 2021):
>> https://github.com/endlessm/linux/pull/288
>>
>> The current code should work fine without this:
> 
> Your explanation of the code flow seems clear and logical, but I have
> not checked the details. This is a bit of a tricky issue as you have
> probably seen from history, we went in a couple of wrong directions
> before we spotted the real cause.
> 
> The one thing I don't see clearly in your explanation (which I may
> have read too quickly) is how the generic fix 69b957c26b32 is
> responsible for making this a "no-op" code flow now.

It is a no-op now because after that commit the acpi_ec struct
which gets allocated when parsing the ECDT now gets re-used
when parsing the DSDT if the EC's cmd + data addresses match.

When we enter the if for re-using that boot_ec acpi_ec struct then
only boot_ec->handle is re-used; and we keep boot_ec->gpe to the
value set when parsing the ECDT.

The quirk does:

ec->gpe is boot_ec->gpe, but since we throw ec away now
(after taking ec->handle) and reuse boot_ec->gpe we will end
up using boot_ec->gpe just as the quirk caused us to do before
we started re-using the boot_ec acpi_ec struct.

Regards,

Hans

Re: [PATCH 2/4] ACPI: EC: Drop the EC_FLAGS_IGNORE_DSDT_GPE quirk

[Daniel Drake]

On Mon, Jun 20, 2022 at 10:33 PM Hans de Goede <hdegoede@redhat.com> wrote:
> It is a no-op now because after that commit the acpi_ec struct
> which gets allocated when parsing the ECDT now gets re-used
> when parsing the DSDT if the EC's cmd + data addresses match.

Got it. Seems rather clear indeed.

Can you point out how to check these addresses from dmesg output. We
have several of them saved here from these models, including for some
of the ones you weren't able to find logs for.

Daniel

Re: [PATCH 2/4] ACPI: EC: Drop the EC_FLAGS_IGNORE_DSDT_GPE quirk

[Hans de Goede]

Hi,

On 6/21/22 04:37, Daniel Drake wrote:
> On Mon, Jun 20, 2022 at 10:33 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> It is a no-op now because after that commit the acpi_ec struct
>> which gets allocated when parsing the ECDT now gets re-used
>> when parsing the DSDT if the EC's cmd + data addresses match.
> 
> Got it. Seems rather clear indeed.
> 
> Can you point out how to check these addresses from dmesg output. We
> have several of them saved here from these models, including for some
> of the ones you weren't able to find logs for.

If you look for "ACPI: EC: " in the dmesg you shoud find 2 blocks
of log lines like this (note the output has changed somewhat
overtime):

[    0.642373] ACPI: EC: EC started
[    0.642375] ACPI: EC: interrupt blocked
[    0.651140] ACPI: EC: EC_CMD/EC_SC=0x66, EC_DATA=0x62
[    0.651142] ACPI: EC: Boot ECDT EC used to handle transactions

[    1.191469] ACPI: EC: interrupt unblocked
[    1.191471] ACPI: EC: event unblocked
[    1.191491] ACPI: EC: EC_CMD/EC_SC=0x66, EC_DATA=0x62
[    1.191493] ACPI: EC: GPE=0x16
[    1.191496] ACPI: \_SB_.PCI0.LPCB.EC__: Boot ECDT EC initialization complete
[    1.191501] ACPI: \_SB_.PCI0.LPCB.EC__: EC: Used to handle transactions and events

The first block is the ECDT probing, the second one is the DSDT probing
and the addresses are in the:

[    0.651140] ACPI: EC: EC_CMD/EC_SC=0x66, EC_DATA=0x62

lines.

Regards,

Hans

Re: [PATCH 2/4] ACPI: EC: Drop the EC_FLAGS_IGNORE_DSDT_GPE quirk

[Daniel Drake]

On Tue, Jun 21, 2022 at 4:37 PM Hans de Goede <hdegoede@redhat.com> wrote:
> If you look for "ACPI: EC: " in the dmesg you shoud find 2 blocks
> of log lines like this (note the output has changed somewhat
> overtime):

Got it.

I was able to verify matching addresses for GL702VMK, X505BA, X505BP,
X580VD, X542BP.

I also have logs for FX502VD, FX502VE and X550VX but they only log one
set of addresses, some kernel version along the way did not log both.

Don't have logs for X542BA.

I think this plus the code tracing is overly convincing,

Reviewed-by: Daniel Drake <drake@endlessos.org>

Thanks.

Re: [PATCH 2/4] ACPI: EC: Drop the EC_FLAGS_IGNORE_DSDT_GPE quirk

[Hans de Goede]

Hi,

On 6/21/22 14:38, Daniel Drake wrote:
> On Tue, Jun 21, 2022 at 4:37 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> If you look for "ACPI: EC: " in the dmesg you shoud find 2 blocks
>> of log lines like this (note the output has changed somewhat
>> overtime):
> 
> Got it.
> 
> I was able to verify matching addresses for GL702VMK, X505BA, X505BP,
> X580VD, X542BP.
> 
> I also have logs for FX502VD, FX502VE and X550VX but they only log one
> set of addresses, some kernel version along the way did not log both.
> 
> Don't have logs for X542BA.
> 
> I think this plus the code tracing is overly convincing,
> 
> Reviewed-by: Daniel Drake <drake@endlessos.org>

Thank you.

Regards,

Hans

Re: [PATCH 0/4] ACPI: EC: Various cleanups

[Rafael J. Wysocki]

On Mon, Jun 20, 2022 at 11:26 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi All,
>
> Here is a set of cleanups / removal of no longer necessary
> quirks (or so I believe please review carefully). These are all
> things which I noticed while working on my:
> "[RFC 0/4] ACPI[CA]: fix ECDT EC probe ordering issues" series.
>
> Regards,
>
> Hans
>
> p.s.
> Talking about my "[RFC 0/4] ACPI[CA]: fix ECDT EC probe ordering issues"
> series, it would be great if someone can take a look at this and let me
> know if that series seems sane. Then I can convert the ACPICA changes
> from kernel patches to upstream github acpica patches and submit a
> pull-req for those at github.
>
>
> Hans de Goede (4):
>   ACPI: EC: Remove duplicate ThinkPad X1 Carbon 6th entry from DMI
>     quirks
>   ACPI: EC: Drop the EC_FLAGS_IGNORE_DSDT_GPE quirk
>   ACPI: EC: Re-use boot_ec when possible even when
>     EC_FLAGS_TRUST_DSDT_GPE is set
>   ACPI: EC: Drop unused ident initializers from dmi_system_id tables
>
>  drivers/acpi/ec.c | 140 ++++++++++++++++------------------------------
>  1 file changed, 48 insertions(+), 92 deletions(-)
>
> --

All four patches applied as 5.20 material, thanks!

[Devel] Re: [PATCH 0/4] ACPI: EC: Various cleanups

[Rafael J. Wysocki]

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

On Mon, Jun 20, 2022 at 11:26 AM Hans de Goede <hdegoede(a)redhat.com> wrote:
>
> Hi All,
>
> Here is a set of cleanups / removal of no longer necessary
> quirks (or so I believe please review carefully). These are all
> things which I noticed while working on my:
> "[RFC 0/4] ACPI[CA]: fix ECDT EC probe ordering issues" series.
>
> Regards,
>
> Hans
>
> p.s.
> Talking about my "[RFC 0/4] ACPI[CA]: fix ECDT EC probe ordering issues"
> series, it would be great if someone can take a look at this and let me
> know if that series seems sane. Then I can convert the ACPICA changes
> from kernel patches to upstream github acpica patches and submit a
> pull-req for those at github.
>
>
> Hans de Goede (4):
>   ACPI: EC: Remove duplicate ThinkPad X1 Carbon 6th entry from DMI
>     quirks
>   ACPI: EC: Drop the EC_FLAGS_IGNORE_DSDT_GPE quirk
>   ACPI: EC: Re-use boot_ec when possible even when
>     EC_FLAGS_TRUST_DSDT_GPE is set
>   ACPI: EC: Drop unused ident initializers from dmi_system_id tables
>
>  drivers/acpi/ec.c | 140 ++++++++++++++++------------------------------
>  1 file changed, 48 insertions(+), 92 deletions(-)
>
> --

All four patches applied as 5.20 material, thanks!