| Message ID | 20220620092546.8298-3-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Commit | f7090e0ef360d674f08a22fab90e4e209fb1f658 |
| Headers | show |
| Series | ACPI: EC: Various cleanups | expand |
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
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
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
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.
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
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"),
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(-)