| Message ID | 20170420201219.21568-1-drake@endlessm.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Thu, Apr 20, 2017 at 10:12 PM, Daniel Drake <drake@endlessm.com> wrote: > The Asus laptops X550VXK/FX502VD/FX502VE have a BIOS bug where the > ECDT (correctly) states that EC events trigger GPE 0x23, but the > DSDT _GPE method (incorrectly) says GPE 0x33. > > A cursory glance of the code suggests that this should work regardless > (because it looks like Linux would treat the ECDT and DSDT as two separate > ECs supported simultaneously), but in practice it is not working since the > ECDT is ultimately ignored in favour of the DSDT EC. The sequence of > events is: > > 1. acpi_ec_ecdt_probe() is called in early boot, and calls > acpi_config_boot_ec(is_ecdt=true) for the ECDT EC. > > acpi_config_boot_ec() sets boot_ec to this new EC, and > boot_ec_is_ecdt = true. > > 2. Later, acpi_ec_dsdt_probe() is called and creates a new ec to represent > the DSDT EC. Then: > /* > * When the DSDT EC is available, always re-configure boot EC to > * have _REG evaluated. _REG can only be evaluated after the > * namespace initialization. > * At this point, the GPE is not fully initialized, so do not to > * handle the events. > */ > ret = acpi_config_boot_ec(ec, ec->handle, false, false); > > So the DSDT EC is passed to acpi_config_boot_ec() which does: > /* Unset old boot EC */ > if (boot_ec != ec) > acpi_ec_free(boot_ec); > > acpi_ec_free() sets boot_ec to NULL. > Further down in acpi_config_boot_ec() we reach: > > /* Set new boot EC */ > if (!boot_ec) { > boot_ec = ec; > boot_ec_is_ecdt = is_ecdt; > } > > So now boot_ec points to the DSDT EC and boot_ec_is_ecdt is false. > > 3. Later, ecpi_ec_ecdt_start() is called and this looks like it would > enable GPEs on our ECDT, but it bails out because of: > > if (!boot_ec_is_ecdt) > return -ENODEV; > > > The comment I pasted above from acpi_ec_dsdt_probe() says that it is going > to reconfigure the boot EC, but it instead calls acpi_config_boot_ec() on > the newly probed DSDT EC. It seems like the code is not following the > comment here. > > Adjusting that code to work with boot_ec adjusts the sequence of events so > that both boot EC and DSDT are treated independently and as a result, we > get EC interrupts firing correctly. > > This fixes media keys on the mentioned laptop models. > Thanks to Chris Chiu for diagnosis. > > Signed-off-by: Daniel Drake <drake@endlessm.com> Lv, can you have a look at this, please? > --- > drivers/acpi/ec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c > index c24235d8fb52..78395395e3d9 100644 > --- a/drivers/acpi/ec.c > +++ b/drivers/acpi/ec.c > @@ -1691,7 +1691,7 @@ int __init acpi_ec_dsdt_probe(void) > * At this point, the GPE is not fully initialized, so do not to > * handle the events. > */ > - ret = acpi_config_boot_ec(ec, ec->handle, false, false); > + ret = acpi_config_boot_ec(boot_ec, boot_ec->handle, false, false); > error: > if (ret) > acpi_ec_free(ec); > -- > 2.11.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, > From: Daniel Drake [mailto:drake@endlessm.com] > Sent: Friday, April 21, 2017 4:12 AM > To: rjw@rjwysocki.net; lenb@kernel.org > Cc: linux-acpi@vger.kernel.org; Zheng, Lv <lv.zheng@intel.com>; chiu@endlessm.com; linux@endlessm.com > Subject: [PATCH] ACPI / EC: handle ECDT EC and DSDT EC simultaneously > > The Asus laptops X550VXK/FX502VD/FX502VE have a BIOS bug where the > ECDT (correctly) states that EC events trigger GPE 0x23, but the > DSDT _GPE method (incorrectly) says GPE 0x33. > > A cursory glance of the code suggests that this should work regardless > (because it looks like Linux would treat the ECDT and DSDT as two separate > ECs supported simultaneously), but in practice it is not working since the > ECDT is ultimately ignored in favour of the DSDT EC. IMO, your observation is wrong. Linux never thought there should be 2 ACPI ECs provided by BIOS :). How would it be useful to put 2 ECs in your platforms and export both of them as ACPI EmbeddedControl operation region? EC operation region itself by design is not able to allow OS to detect which EC opregion belongs to which EC device as we did see some platforms defined EC opregion under root but EC device under some bus nodes. ACPI spec thinks ECDT EC is meant to be provided to work since early stages where the ACPI namespace hasn't been prepared. See spec 6.5.4 _REG (Region). You can find exceptions of _REG execution mentioned for SystemMemory/SystemIo/PCI_Config and ECDT EC. So they are served for the same purpose: Before executing any control method (aka., preparing the ACPI namespace, some hardware components that have already been driven by the BIOS should be allowed to be accessed during the namespace preparation. And hence all DSDT PNP0C09 ECs (actually should only be 1) should always work, in most of the cases, ECDT EC may just be used as a quirk to make some EC opregion accesses working during that special stage - the namespace preparation. For such kind of use case (early stage, loading ACPI namespace), obviously no EC event should be handled, so making ECDT GPE setting correct sounds meaningless to BIOS if the same EC settings can also be provided via DSDT. Thus Linux is always trying to override ECDT EC with DSDT settings and convert the final boot EC to the PNP0C09 driver driven ones. Some code is there not deleted just due to some historical reasons. As such, GPE setting in DSDT should always be correct. There are 2 possibilities for some gray areas: 1. If a BIOS has a wrong EC GPE setting in DSDT, but the OS correctly implemented IRQ polling to handle it, you still couldn't see any problems (you cannot know whether IRQ is handled via interrupt or via polling from user space). 2. If ECDT EC contains a namespace path differing from the DSDT one, the OS may choose to keep both. Or if ECDT EC contains IO addresses differing from the DSDT one, the OS may choose to keep both. I don't have knowledge to know how Windows implement things in the above gray areas. They just sound like some happen-to-work consequences of the unspecified Windows implementation. For gray area 1, your report just means to me that some platforms do have correct ECDT GPE setting but incorrect DSDT GPE setting; and since Linux doesn't implement EC event polling for some stages, for now Linux may need a quirk to favor "correct GPE setting". However for gray area 2, I don't think it means Linux should keep both ECs. I cannot see reasons in this case to support to do so unless seeing your acpidump. Are you able to see 2 ECs in your Windows device tree on these platforms? So could you paste your acpidump here so that we can check if they are different ACPI ECs and make a decision closer to the Windows behavior and tell me your Windows device tree result (you can obtain this using a tool provided in <<Microsoft Windows Internals>>). Maybe for your case, DSDT EC is just invalid and we should ignore it, then if we can find a correct way to ignore the DSDT EC, we needn't change a single line EC driver code. > The sequence of > events is: > > 1. acpi_ec_ecdt_probe() is called in early boot, and calls > acpi_config_boot_ec(is_ecdt=true) for the ECDT EC. > > acpi_config_boot_ec() sets boot_ec to this new EC, and > boot_ec_is_ecdt = true. > > 2. Later, acpi_ec_dsdt_probe() is called and creates a new ec to represent > the DSDT EC. Then: > /* > * When the DSDT EC is available, always re-configure boot EC to > * have _REG evaluated. _REG can only be evaluated after the > * namespace initialization. > * At this point, the GPE is not fully initialized, so do not to > * handle the events. > */ > ret = acpi_config_boot_ec(ec, ec->handle, false, false); > > So the DSDT EC is passed to acpi_config_boot_ec() which does: > /* Unset old boot EC */ > if (boot_ec != ec) > acpi_ec_free(boot_ec); > > acpi_ec_free() sets boot_ec to NULL. > Further down in acpi_config_boot_ec() we reach: > > /* Set new boot EC */ > if (!boot_ec) { > boot_ec = ec; > boot_ec_is_ecdt = is_ecdt; > } > > So now boot_ec points to the DSDT EC and boot_ec_is_ecdt is false. Correct, now boot EC is overridden by the DSDT settings. And the ECDT EC is in fact abandoned. You seem to just need a possibility here to allow OS to abandon the wrong settings and keep the correct settings. However it's hard for OS to determine which settings are correct: ECDT one or DSDT one? No one can answer this, even ACPI spec cannot. So you might need a quirk mechanism here before root causing. As this looks more like a BIOS issue - BIOS developer might have different understanding than the spec and their code happened to work on Windows due to different reasons. > > 3. Later, ecpi_ec_ecdt_start() is called and this looks like it would > enable GPEs on our ECDT, but it bails out because of: > > if (!boot_ec_is_ecdt) > return -ENODEV; > > > The comment I pasted above from acpi_ec_dsdt_probe() says that it is going > to reconfigure the boot EC, but it instead calls acpi_config_boot_ec() on > the newly probed DSDT EC. It seems like the code is not following the > comment here. No, all above means: If the boot EC is now overridden by DSDT EC, no need to enable GPE at this stage. As further PNP0C09 driver probe will do this. You problem is just PNP0C09 GPE setting is wrong and the code here dropped possibly correct setting in ECDT. IMO, 1. We can drop if PNP0C09 GPE driver implements GPE polling for all use cases. But now it doesn't implement that. 2. If we cannot drop wrong GPE setting, we may consider a mechanism here to allow platform quirks to choose the correct one for Linux. > > Adjusting that code to work with boot_ec adjusts the sequence of events so > that both boot EC and DSDT are treated independently and as a result, we > get EC interrupts firing correctly. > > This fixes media keys on the mentioned laptop models. > Thanks to Chris Chiu for diagnosis. > > Signed-off-by: Daniel Drake <drake@endlessm.com> > --- > drivers/acpi/ec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c > index c24235d8fb52..78395395e3d9 100644 > --- a/drivers/acpi/ec.c > +++ b/drivers/acpi/ec.c > @@ -1691,7 +1691,7 @@ int __init acpi_ec_dsdt_probe(void) > * At this point, the GPE is not fully initialized, so do not to > * handle the events. > */ > - ret = acpi_config_boot_ec(ec, ec->handle, false, false); > + ret = acpi_config_boot_ec(boot_ec, boot_ec->handle, false, false); This is a hackish, "ec" created above these lines will never be actually used then. The entire problem looks to me is: When GPE setting differs between ECDT and DSDT, which one should be trusted by OS? The current code chose to always trust DSDT GPE settings as in theory it doesn't make sense to trust the ECDT GPE setting in most of the cases, ECDT GPE is not meant to be used during runtime. So why don't we just add a quirk to favor GPE setting from ECDT rather than the GPE setting from DSDT for these platforms? Thanks Lv > error: > if (ret) > acpi_ec_free(ec); > -- > 2.11.0 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Lv, Thanks for the detailed response. In trying to decode the tricky code flow and seeing all this first_ec / boot_ec / DSDT EC / ECDT EC stuff, I seem to have made the wrong interpretation about how this is designed. On Sun, Apr 23, 2017 at 10:43 PM, Zheng, Lv <lv.zheng@intel.com> wrote: > The entire problem looks to me is: > When GPE setting differs between ECDT and DSDT, which one should be > trusted by OS? This case suggests that Windows uses the ECDT setting, right? > The current code chose to always trust DSDT GPE settings as in theory it > doesn't make sense to trust the ECDT GPE setting in most of the cases, > ECDT GPE is not meant to be used during runtime. So why don't we just add > a quirk to favor GPE setting from ECDT rather than the GPE setting from > DSDT for these platforms? Do you mean a DMI quirk? We have found those to be quite impractical in the past, it is too hard to get 100% coverage of all machines affected by the bug, and usually we would only be able to quirk it late in the process (after the machine has already shipped to users). In a recent case we had an Asus DMI quirks list that grew slowly to over 30 entries over a year, before we found a generic solution. In this case we have asked Asus BIOS engineers to not repeat this issue on future products, but even if they follow our advice there, we can expect a decent number of models affected by this issue. And we just found a 2nd model with the same issue. Here is the "acpidump -b" output: https://www.dropbox.com/s/d3w2xrmrz1oklnw/x580vd_acpi.tgz?dl=0 To see the windows device tree using Microsoft Windows Internals, are you referring to the paperback book? Which of the 2 parts would we have to purchase? Does it come with a binary on CD or would we have to figure out how to compile the tool? Thanks, Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
SGksDQoNCj4gRnJvbTogRGFuaWVsIERyYWtlIFttYWlsdG86ZHJha2VAZW5kbGVzc20uY29tXQ0K PiBTdWJqZWN0OiBSZTogW1BBVENIXSBBQ1BJIC8gRUM6IGhhbmRsZSBFQ0RUIEVDIGFuZCBEU0RU IEVDIHNpbXVsdGFuZW91c2x5DQo+IA0KPiBIaSBMdiwNCj4gDQo+IFRoYW5rcyBmb3IgdGhlIGRl dGFpbGVkIHJlc3BvbnNlLiBJbiB0cnlpbmcgdG8gZGVjb2RlIHRoZSB0cmlja3kgY29kZQ0KPiBm bG93IGFuZCBzZWVpbmcgYWxsIHRoaXMgZmlyc3RfZWMgLyBib290X2VjIC8gRFNEVCBFQyAvIEVD RFQgRUMgc3R1ZmYsDQo+IEkgc2VlbSB0byBoYXZlIG1hZGUgdGhlIHdyb25nIGludGVycHJldGF0 aW9uIGFib3V0IGhvdyB0aGlzIGlzDQo+IGRlc2lnbmVkLg0KPiANCj4gT24gU3VuLCBBcHIgMjMs IDIwMTcgYXQgMTA6NDMgUE0sIFpoZW5nLCBMdiA8bHYuemhlbmdAaW50ZWwuY29tPiB3cm90ZToN Cj4gPiBUaGUgZW50aXJlIHByb2JsZW0gbG9va3MgdG8gbWUgaXM6DQo+ID4gV2hlbiBHUEUgc2V0 dGluZyBkaWZmZXJzIGJldHdlZW4gRUNEVCBhbmQgRFNEVCwgd2hpY2ggb25lIHNob3VsZCBiZQ0K PiA+IHRydXN0ZWQgYnkgT1M/DQo+IA0KPiBUaGlzIGNhc2Ugc3VnZ2VzdHMgdGhhdCBXaW5kb3dz IHVzZXMgdGhlIEVDRFQgc2V0dGluZywgcmlnaHQ/DQoNCkkgY2hlY2tlZCB0aGUgcHJvdmlkZWQg YWNwaSB0YWJsZXMuDQpJbmRlZWQsIHRoZSBFQ0RUIEVDIGlzIGNvcnJlY3QuDQpbMDAwaCAwMDAw ICAgNF0gICAgICAgICAgICAgICAgICAgIFNpZ25hdHVyZSA6ICJFQ0RUIiAgICBbRW1iZWRkZWQg Q29udHJvbGxlciBCb290IFJlc291cmNlcyBUYWJsZV0NClswMDRoIDAwMDQgICA0XSAgICAgICAg ICAgICAgICAgVGFibGUgTGVuZ3RoIDogMDAwMDAwQzENClswMDhoIDAwMDggICAxXSAgICAgICAg ICAgICAgICAgICAgIFJldmlzaW9uIDogMDENClswMDloIDAwMDkgICAxXSAgICAgICAgICAgICAg ICAgICAgIENoZWNrc3VtIDogRUMNClswMEFoIDAwMTAgICA2XSAgICAgICAgICAgICAgICAgICAg ICAgT2VtIElEIDogIl9BU1VTXyINClswMTBoIDAwMTYgICA4XSAgICAgICAgICAgICAgICAgT2Vt IFRhYmxlIElEIDogIk5vdGVib29rIg0KWzAxOGggMDAyNCAgIDRdICAgICAgICAgICAgICAgICBP ZW0gUmV2aXNpb24gOiAwMTA3MjAwOQ0KWzAxQ2ggMDAyOCAgIDRdICAgICAgICAgICAgICBBc2wg Q29tcGlsZXIgSUQgOiAiQU1JLiINClswMjBoIDAwMzIgICA0XSAgICAgICAgQXNsIENvbXBpbGVy IFJldmlzaW9uIDogMDAwMDAwMDUNCg0KDQpbMDI0aCAwMDM2ICAxMl0gICAgICBDb21tYW5kL1N0 YXR1cyBSZWdpc3RlciA6IFtHZW5lcmljIEFkZHJlc3MgU3RydWN0dXJlXQ0KWzAyNGggMDAzNiAg IDFdICAgICAgICAgICAgICAgICAgICAgU3BhY2UgSUQgOiAwMSBbU3lzdGVtSU9dDQpbMDI1aCAw MDM3ICAgMV0gICAgICAgICAgICAgICAgICAgIEJpdCBXaWR0aCA6IDA4DQpbMDI2aCAwMDM4ICAg MV0gICAgICAgICAgICAgICAgICAgQml0IE9mZnNldCA6IDAwDQpbMDI3aCAwMDM5ICAgMV0gICAg ICAgICBFbmNvZGVkIEFjY2VzcyBXaWR0aCA6IDAwIFtVbmRlZmluZWQvTGVnYWN5XQ0KWzAyOGgg MDA0MCAgIDhdICAgICAgICAgICAgICAgICAgICAgIEFkZHJlc3MgOiAwMDAwMDAwMDAwMDAwMDY2 DQoNClswMzBoIDAwNDggIDEyXSAgICAgICAgICAgICAgICBEYXRhIFJlZ2lzdGVyIDogW0dlbmVy aWMgQWRkcmVzcyBTdHJ1Y3R1cmVdDQpbMDMwaCAwMDQ4ICAgMV0gICAgICAgICAgICAgICAgICAg ICBTcGFjZSBJRCA6IDAxIFtTeXN0ZW1JT10NClswMzFoIDAwNDkgICAxXSAgICAgICAgICAgICAg ICAgICAgQml0IFdpZHRoIDogMDgNClswMzJoIDAwNTAgICAxXSAgICAgICAgICAgICAgICAgICBC aXQgT2Zmc2V0IDogMDANClswMzNoIDAwNTEgICAxXSAgICAgICAgIEVuY29kZWQgQWNjZXNzIFdp ZHRoIDogMDAgW1VuZGVmaW5lZC9MZWdhY3ldDQpbMDM0aCAwMDUyICAgOF0gICAgICAgICAgICAg ICAgICAgICAgQWRkcmVzcyA6IDAwMDAwMDAwMDAwMDAwNjINCg0KWzAzQ2ggMDA2MCAgIDRdICAg ICAgICAgICAgICAgICAgICAgICAgICBVSUQgOiAwMDAwMDAwMA0KWzA0MGggMDA2NCAgIDFdICAg ICAgICAgICAgICAgICAgIEdQRSBOdW1iZXIgOiAyMw0KWzA0MWggMDA2NSAgMTldICAgICAgICAg ICAgICAgICAgICAgTmFtZXBhdGggOiAiXF9TQi5QQ0kwLkxQQ0IuRUMwIg0KDQpXaGlsZSB0aGUg RFNEVCBFQyBpcyBpbiBmYWN0IGludmFsaWQgYXMgaXQgcmV0dXJucyAwIGZyb20gaXRzIF9TVEE6 DQogICAgU2NvcGUgKF9TQi5QQ0kwLkxQQ0IpDQogICAgew0KICAgICAgICBEZXZpY2UgKEhfRUMp DQogICAgICAgIHsNCiAgICAgICAgICAgIE5hbWUgKF9ISUQsIEVpc2FJZCAoIlBOUDBDMDkiKSAv KiBFbWJlZGRlZCBDb250cm9sbGVyIERldmljZSAqLykgIC8vIF9ISUQ6IEhhcmR3YXJlIElEDQog ICAgICAgICAgICBOYW1lIChfVUlELCBPbmUpICAvLyBfVUlEOiBVbmlxdWUgSUQNCiAgICAgICAg ICAgIE1ldGhvZCAoX1NUQSwgMCwgTm90U2VyaWFsaXplZCkgIC8vIF9TVEE6IFN0YXR1cw0KICAg ICAgICAgICAgew0KICAgICAgICAgICAgICAgIF5eXkdGWDAuQ0xJRCA9IDB4MDMNCiAgICAgICAg ICAgICAgICBSZXR1cm4gKFplcm8pDQogICAgICAgICAgICB9DQogICAgICAgICAgICAuLi4NCiAg ICAgICAgfQ0KICAgIH0NCg0KSXQgZG9lc24ndCBjb250YWluIF9DUlMsIHNvIGl0J3MgbGlrZWx5 IHRoYXQgaXQgd2lsbCBmYWlsIGluDQphY3BpX2VjX2RzZHRfcHJvYmUoKSBkdWUgdG8gZmFpbHVy ZSBvZiB3YWxrIF9DUlMuDQoNCkFyZSB5b3Ugc3VyZSB0aGUgc2FtZSBwcm9ibGVtIGNhbiBiZSBz ZWVuIG5vIHRoaXMgcGxhdGZvcm0/DQoNCklmIHNvLCBwb3NzaWJseSB5b3Ugb25seSBuZWVkIHRv IGFkZCBzb21lIHNhbml0eSBjaGVja3MgaW4NCmFjcGlfZWNfZHNkdF9wcm9iZSgpLiBBZnRlciBh Y3BpX2dldF9kZXZpY2VzKCkgY2FsbCwgY2hlY2sgaWYNCml0cyBJTyBhZGRyZXNzZXMgYXJlIGlu dmFsaWQuDQpPciBjaGVjayBpdHMgX1NUQSAocHJlc2VudCAmJiByZXR1cm4gdmFsaWQgdmFsdWUp IGFmdGVyIHRoYXQuDQpJdCBtaWdodCBub3QgYmUgcG9zc2libGUgdG8gZG8gc3VjaCBfU1RBIGV4 ZWN1dGlvbi4NCg0KU28gSU1PLCB0aGUgYWNwaV9lY19kc2R0X3Byb2JlKCkgaXMgbm90IGEgZ29v ZCBjaG9pY2UuDQpUaGUgaGlzdG9yeSBpczoNCjEuIEluIEFDUEkgc3BlYyAxLjAsIHRoZXJlIGlz IG5vIEVDRFQsIEJJT1MgZGV2ZWxvcGVycyBoYXZlDQogICB0byBwcm92aWRlIHdyYXBwZXJzIGlu IEVDIHJlYWQvd3JpdGUgZnVuY3Rpb25zIGluIG9yZGVyIHRvDQogICBiZSBhYmxlIHRvIGFjY2Vz cyBFQyBmaXJtd2FyZSBiZWZvcmUgT1MgbG9hZHMgdGhlIEVDDQogICBkcml2ZXIuIEJlZm9yZSBF Qy5fUkVHIGlzIGludm9rZWQsIGFjY2VzcyBzeXN0ZW1pbw0KICAgb3ByZWdpb24gaW5zdGVhZCBv ZiBkaXJlY3RseSBhY2Nlc3NpbmcgRUMgb3ByZWdpb25zLg0KICAgSW4gQUNQSSBzcGVjIDIuMCwg RUNEVCBpcyBwcm92aWRlZCB0byBlbGltaW5hdGUgdGhlDQogICBDb21wbGljYXRpb24gb2YgdGhp cyBuZWVkcy4NCjIuIER1cmluZyBlYXJseSBMaW51eCBFQyBzdXBwb3J0LCBFQ0RUIGNvZGUgaXMg cHJvdmlkZWQNCiAgIGZvciB0aGUgc3BlYyBwdXJwb3NlLiBCdXQgc2V2ZXJhbCB1bi1yb290LWNh dXNlZCBrZXJuZWwNCiAgIGJ1Z3MgbWFkZSB0aGUgZGV2ZWxvcGVycyB0byBtYWtlIHRoZSB3cm9u ZyBjaG9pY2UsIHN0YXJ0aW5nDQogICB0byBhbHdheXMgb3ZlcnJpZGUgRUNEVCBFQyB1c2luZyBE U0RUIEVDLg0KICAgVGhlIGN1bHByaXQgY29tbWl0cyBhcmUgYzZjYjBlODcgYW5kIGE1MDMyYmZk Lg0KICAgVGhlIGN1bHByaXQgaW4gZmFjdCBmaW5hbGx5IGFsc28gcGxheWVkIGFzIG9uZSBvZiB0 aG9zZQ0KICAgcmVhc29ucyBwcmV2ZW50aW5nIHRoZSBBTUwgaW50ZXJwcmV0ZXIgZnJvbSBiZWlu Zw0KICAgY29ycmVjdGx5IGltcGxlbWVudGVkIGFzIFdpbmRvd3MgY29tcGxpYW50Lg0KMy4gSG93 ZXZlciwgYWZ0ZXIgc3dpdGNoaW5nIHRvIHRoZSBvbGQgYmVoYXZpb3IsIGFuZCB0aGUNCiAgIERT RFQgYm9vdCBFQyBtZWNoYW5pc20gaXMgZW50aXJlbHkgYWJhbmRvbmVkLiBCdXQgdXNlcnMNCiAg IFJlcG9ydGVkIHRoYXQgdGhleSBjYW4gc2VlIEVDIG9wcmVnaW9uIGFjY2Vzc2VkIGluIHNvbWUN CiAgIF9TVEEvX0lOSSwgYXMgTGludXggZXhlY3V0ZXMgdGhlc2UgY29udHJvbCBtZXRob2RzIHZl cnkNCiAgIEVhcmx5IChlYXJsaWVyIGJlZm9yZSBwcm9iZSBFQyBkcml2ZXIpIGFuZCB0aGVyZSBp cyBubw0KICAgRUNEVCBvbiB0aG9zZSBwbGF0Zm9ybXMsIHdlIGNhbiBzZWUgZXJyb3JzIGNvbXBs YWluaW5nDQogICBubyBvcHJlZ2lvbiBoYW5kbGVyLiBUaGUgYnVnIGlzIGtlcm5lbCBCdWd6aWxs YSAxMTkyNjEuDQogICBUQkgsIHRoZXNlIGVycm9ycyBhcmUgbm90IHN1Y2ggY3JpdGljYWwgSU1P LCB0aGUgQklPUw0KICAgb2J2aW91c2x5IHZpb2xhdGVzIGZhY3RzIG1lbnRpb25lZCBpbiBoaXN0 b3J5IDEgYW5kIHdlDQogICBvYnZpb3VzbHkgZG8gbm90IGhhdmUga25vd2xlZGdlIG9mIHRoZSBl eGVjdXRpb24gb3JkZXINCiAgIG9mIHRoZXNlIGNvbnRyb2wgbWV0aG9kcyBvbiBXaW5kb3dzIGFu ZCB3ZSBvYnZpb3VzbHkNCiAgIGRvIG5vdCBrbm93IGlmIHN1Y2ggZXJyb3IgY2FuIGFsc28gYmUg c2VlbiBieSBXaW5vZHdzDQogICBhbmQgZG8gbm90IGtub3cgd2hldGhlciBzdWNoIGVycm9yIGNh biBiZSBmdW5jdGlvbmFsDQogICBmYWlsdXJlIGFuZCByZWFjaGVkIEJJT1MgZGV2ZWxvcGVycycg YXR0ZW50aW9uLg0KICAgQnV0IHdlIGZpbmFsbHkgaGF2ZSBubyBjaG9pY2UgYnV0IHJlc3Rvcmlu ZyB0aGUgYm9vdA0KICAgRFNEVCBFQyBoYWNrIGJhY2sgYXMgd2UgZG8gbm90IGtub3cgaG93IExp bnV4IGNhbg0KICAgZXhlY3V0ZSBhbGwgY29udHJvbCBtZXRob2RzIGluIHRoZSBleGFjdCBvcmRl ciBhcw0KICAgV2luZG93cy4uLg0KDQpUaGVuIG5vdywgaXQgYWN0dWFsbHkgbWFrZXMgdGhlIHBy b2JsZW0gbW9yZSBjb21wbGljYXRlZC4NCmFzIGl0IG1pZ2h0IG5vdCBiZSBwb3NzaWJsZSB0byBl eGVjdXRlIF9DUlMvX0dQRS9fU1RBDQphdCB0aGF0IHRpbWUgdG8gc2FuaXRpemUgdGhlIERTRFQg RUMuDQpQcm9iYWJseSB3ZSBjb3VsZCBqdXN0IGFiYW5kb24gaXQgYWdhaW4/DQoNCkFueXdheSB3 ZSBjYW4gZmlyc3RseSBqdXN0IGFkZCAxIGxpbmUgSU8gYWRkcmVzcyBhbmQgX0dQRQ0KZXhpc3Rl bmNlIHNhbml0eSBjaGVjayBpbiBhY3BpX2VjX2RzZHRfcHJvYmUoKSB0byBmaXgNCnlvdXIgaXNz dWUuDQoNCj4gDQo+ID4gVGhlIGN1cnJlbnQgY29kZSBjaG9zZSB0byBhbHdheXMgdHJ1c3QgRFNE VCBHUEUgc2V0dGluZ3MgYXMgaW4gdGhlb3J5IGl0DQo+ID4gZG9lc24ndCBtYWtlIHNlbnNlIHRv IHRydXN0IHRoZSBFQ0RUIEdQRSBzZXR0aW5nIGluIG1vc3Qgb2YgdGhlIGNhc2VzLA0KPiA+IEVD RFQgR1BFIGlzIG5vdCBtZWFudCB0byBiZSB1c2VkIGR1cmluZyBydW50aW1lLiBTbyB3aHkgZG9u J3Qgd2UganVzdCBhZGQNCj4gPiBhIHF1aXJrIHRvIGZhdm9yIEdQRSBzZXR0aW5nIGZyb20gRUNE VCByYXRoZXIgdGhhbiB0aGUgR1BFIHNldHRpbmcgZnJvbQ0KPiA+IERTRFQgZm9yIHRoZXNlIHBs YXRmb3Jtcz8NCj4gDQo+IERvIHlvdSBtZWFuIGEgRE1JIHF1aXJrPyBXZSBoYXZlIGZvdW5kIHRo b3NlIHRvIGJlIHF1aXRlIGltcHJhY3RpY2FsDQo+IGluIHRoZSBwYXN0LCBpdCBpcyB0b28gaGFy ZCB0byBnZXQgMTAwJSBjb3ZlcmFnZSBvZiBhbGwgbWFjaGluZXMNCj4gYWZmZWN0ZWQgYnkgdGhl IGJ1ZywgYW5kIHVzdWFsbHkgd2Ugd291bGQgb25seSBiZSBhYmxlIHRvIHF1aXJrIGl0DQo+IGxh dGUgaW4gdGhlIHByb2Nlc3MgKGFmdGVyIHRoZSBtYWNoaW5lIGhhcyBhbHJlYWR5IHNoaXBwZWQg dG8gdXNlcnMpLg0KPiBJbiBhIHJlY2VudCBjYXNlIHdlIGhhZCBhbiBBc3VzIERNSSBxdWlya3Mg bGlzdCB0aGF0IGdyZXcgc2xvd2x5IHRvDQo+IG92ZXIgMzAgZW50cmllcyBvdmVyIGEgeWVhciwg YmVmb3JlIHdlIGZvdW5kIGEgZ2VuZXJpYyBzb2x1dGlvbi4NCj4gDQo+IEluIHRoaXMgY2FzZSB3 ZSBoYXZlIGFza2VkIEFzdXMgQklPUyBlbmdpbmVlcnMgdG8gbm90IHJlcGVhdCB0aGlzDQo+IGlz c3VlIG9uIGZ1dHVyZSBwcm9kdWN0cywgYnV0IGV2ZW4gaWYgdGhleSBmb2xsb3cgb3VyIGFkdmlj ZSB0aGVyZSwgd2UNCj4gY2FuIGV4cGVjdCBhIGRlY2VudCBudW1iZXIgb2YgbW9kZWxzIGFmZmVj dGVkIGJ5IHRoaXMgaXNzdWUuDQo+IA0KPiBBbmQgd2UganVzdCBmb3VuZCBhIDJuZCBtb2RlbCB3 aXRoIHRoZSBzYW1lIGlzc3VlLiBIZXJlIGlzIHRoZQ0KPiAiYWNwaWR1bXAgLWIiIG91dHB1dDoN Cj4gaHR0cHM6Ly93d3cuZHJvcGJveC5jb20vcy9kM3cyeHJtcnoxb2tsbncveDU4MHZkX2FjcGku dGd6P2RsPTANCj4gDQo+IFRvIHNlZSB0aGUgd2luZG93cyBkZXZpY2UgdHJlZSB1c2luZyBNaWNy b3NvZnQgV2luZG93cyBJbnRlcm5hbHMsIGFyZQ0KPiB5b3UgcmVmZXJyaW5nIHRvIHRoZSBwYXBl cmJhY2sgYm9vaz8gV2hpY2ggb2YgdGhlIDIgcGFydHMgd291bGQgd2UNCj4gaGF2ZSB0byBwdXJj aGFzZT8gRG9lcyBpdCBjb21lIHdpdGggYSBiaW5hcnkgb24gQ0Qgb3Igd291bGQgd2UgaGF2ZSB0 bw0KPiBmaWd1cmUgb3V0IGhvdyB0byBjb21waWxlIHRoZSB0b29sPw0KDQpTb3JyeSBJIGNvbmZ1 c2VkIGl0IHdpdGggc3lzaW50ZXJuYWwgdG9vbHMNCmh0dHBzOi8vdGVjaG5ldC5taWNyb3NvZnQu Y29tL2VuLXVzL3N5c2ludGVybmFscy9iYjg0MjA2Mi5hc3B4DQoNCkhvd2V2ZXIgaXQgc2hvdWxk IGJlIE9TUiBvbmxpbmUgRERLIGRldmVsb3BlciBzdXBwb3J0IHV0aWxpdHk6DQpodHRwOi8vd3d3 Lm9zcm9ubGluZS5jb20vYXJ0aWNsZS5jZm0/aWQ9OTcNCg0KVGhhbmtzDQpMdg0K -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, April 27, 2017 03:18:04 AM Zheng, Lv wrote: > Hi, > > > From: Daniel Drake [mailto:drake@endlessm.com] > > Subject: Re: [PATCH] ACPI / EC: handle ECDT EC and DSDT EC simultaneously > > > > Hi Lv, > > > > Thanks for the detailed response. In trying to decode the tricky code > > flow and seeing all this first_ec / boot_ec / DSDT EC / ECDT EC stuff, > > I seem to have made the wrong interpretation about how this is > > designed. > > > > On Sun, Apr 23, 2017 at 10:43 PM, Zheng, Lv <lv.zheng@intel.com> wrote: > > > The entire problem looks to me is: > > > When GPE setting differs between ECDT and DSDT, which one should be > > > trusted by OS? > > > > This case suggests that Windows uses the ECDT setting, right? > > I checked the provided acpi tables. > Indeed, the ECDT EC is correct. > [000h 0000 4] Signature : "ECDT" [Embedded Controller Boot Resources Table] > [004h 0004 4] Table Length : 000000C1 > [008h 0008 1] Revision : 01 > [009h 0009 1] Checksum : EC > [00Ah 0010 6] Oem ID : "_ASUS_" > [010h 0016 8] Oem Table ID : "Notebook" > [018h 0024 4] Oem Revision : 01072009 > [01Ch 0028 4] Asl Compiler ID : "AMI." > [020h 0032 4] Asl Compiler Revision : 00000005 > > > [024h 0036 12] Command/Status Register : [Generic Address Structure] > [024h 0036 1] Space ID : 01 [SystemIO] > [025h 0037 1] Bit Width : 08 > [026h 0038 1] Bit Offset : 00 > [027h 0039 1] Encoded Access Width : 00 [Undefined/Legacy] > [028h 0040 8] Address : 0000000000000066 > > [030h 0048 12] Data Register : [Generic Address Structure] > [030h 0048 1] Space ID : 01 [SystemIO] > [031h 0049 1] Bit Width : 08 > [032h 0050 1] Bit Offset : 00 > [033h 0051 1] Encoded Access Width : 00 [Undefined/Legacy] > [034h 0052 8] Address : 0000000000000062 > > [03Ch 0060 4] UID : 00000000 > [040h 0064 1] GPE Number : 23 > [041h 0065 19] Namepath : "\_SB.PCI0.LPCB.EC0" > > While the DSDT EC is in fact invalid as it returns 0 from its _STA: > Scope (_SB.PCI0.LPCB) > { > Device (H_EC) > { > Name (_HID, EisaId ("PNP0C09") /* Embedded Controller Device */) // _HID: Hardware ID > Name (_UID, One) // _UID: Unique ID > Method (_STA, 0, NotSerialized) // _STA: Status > { > ^^^GFX0.CLID = 0x03 > Return (Zero) > } > ... > } > } > > It doesn't contain _CRS, so it's likely that it will fail in > acpi_ec_dsdt_probe() due to failure of walk _CRS. > > Are you sure the same problem can be seen no this platform? > > If so, possibly you only need to add some sanity checks in > acpi_ec_dsdt_probe(). Can you suggest a patch, please? Ideally, something that can be tested? Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 27, 2017 at 6:33 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > Can you suggest a patch, please? > > Ideally, something that can be tested? I think I understand Lv's suggestions so we will now test the following change in order to check if either or both of the added DSDT EC checks can help us. https://gist.github.com/dsd/f50a63c9f31779436bd280c76253e37c Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Daniel > From: Daniel Drake [mailto:drake@endlessm.com] > Subject: Re: [PATCH] ACPI / EC: handle ECDT EC and DSDT EC simultaneously > > On Thu, Apr 27, 2017 at 6:33 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > Can you suggest a patch, please? > > > > Ideally, something that can be tested? > > I think I understand Lv's suggestions so we will now test the > following change in order to check if either or both of the added DSDT > EC checks can help us. > > https://gist.github.com/dsd/f50a63c9f31779436bd280c76253e37c We did have a good talk, and left good engineering materials in community around this issue. However in the above debugging commit, I'm sure we shouldn't invoke _STA in ec_parse_device(). As the reasons below. In theory, using DSDT EC as boot EC is not spec compliant. It's just a workaround in Linux for not knowing the Windows device enumeration orders. Especially, the order of executing the control method execution that may contain hardware initialization code. Such control methods are mostly _STA/_INI. While for _HID/_CRS/_GPE/_BBN, it is unlikely to trigger order issues and it might be safe to invoke them such early. If you executes _STA here, you might bring EC._STA execution prior than other _INI/_STA and may break some other platforms. So for now, I think you should only add simple sanity checks for ioports. And since you have the direct accesses to these affected platforms, you can help to provide such working sanity check improvements for us. Thanks and best regards Lv
Hi, On Fri, Apr 28, 2017 at 12:33 AM, Zheng, Lv <lv.zheng@intel.com> wrote: > However in the above debugging commit, I'm sure we shouldn't invoke _STA in ec_parse_device(). > As the reasons below. > > In theory, using DSDT EC as boot EC is not spec compliant. > It's just a workaround in Linux for not knowing the Windows device enumeration orders. > Especially, the order of executing the control method execution that may contain hardware initialization code. > Such control methods are mostly _STA/_INI. > While for _HID/_CRS/_GPE/_BBN, it is unlikely to trigger order issues and it might be safe to invoke them such early. > > If you executes _STA here, you might bring EC._STA execution prior than other _INI/_STA and may break some other platforms. > So for now, I think you should only add simple sanity checks for ioports. > And since you have the direct accesses to these affected platforms, you can help to provide such working sanity check improvements for us. In the DSDT you were looking at the H_EC device, but for whatever reason, there are two ECs in this DSDT and the one that Linux picks up is the 2nd one, \_SB_.PCI0.LPCB.EC0. This device has no _STA but does have valid _CRS, and the debug patch results agree: ACPI : EC: acpi_ec_dsdt_probe: search for DSDT EC ACPI : EC: ec_parse_device: _STA status 5 val 0 ACPI : EC: ec_parse_device: _CRS status 0 command 66 data 62 ACPI : EC: EC stopped ACPI : EC: EC started ACPI: \_SB_.PCI0.LPCB.EC0_: Used as first EC acpidump output is at https://www.dropbox.com/s/d3w2xrmrz1oklnw/x580vd_acpi.tgz?dl=0 Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Lv, On Fri, Apr 28, 2017 at 6:52 AM, Daniel Drake <drake@endlessm.com> wrote: > In the DSDT you were looking at the H_EC device, but for whatever > reason, there are two ECs in this DSDT and the one that Linux picks up > is the 2nd one, \_SB_.PCI0.LPCB.EC0. > > This device has no _STA but does have valid _CRS, and the debug patch > results agree: > > ACPI : EC: acpi_ec_dsdt_probe: search for DSDT EC > ACPI : EC: ec_parse_device: _STA status 5 val 0 > ACPI : EC: ec_parse_device: _CRS status 0 command 66 data 62 > ACPI : EC: EC stopped > ACPI : EC: EC started > ACPI: \_SB_.PCI0.LPCB.EC0_: Used as first EC How do we proceed here? Any other options we can try that don't involve DMI quirking? Thanks Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Daniel > Drake > Subject: Re: [PATCH] ACPI / EC: handle ECDT EC and DSDT EC simultaneously > > Hi, > > On Fri, Apr 28, 2017 at 12:33 AM, Zheng, Lv <lv.zheng@intel.com> wrote: > > However in the above debugging commit, I'm sure we shouldn't invoke _STA in ec_parse_device(). > > As the reasons below. > > > > In theory, using DSDT EC as boot EC is not spec compliant. > > It's just a workaround in Linux for not knowing the Windows device enumeration orders. > > Especially, the order of executing the control method execution that may contain hardware > initialization code. > > Such control methods are mostly _STA/_INI. > > While for _HID/_CRS/_GPE/_BBN, it is unlikely to trigger order issues and it might be safe to invoke > them such early. > > > > If you executes _STA here, you might bring EC._STA execution prior than other _INI/_STA and may > break some other platforms. > > So for now, I think you should only add simple sanity checks for ioports. > > And since you have the direct accesses to these affected platforms, you can help to provide such > working sanity check improvements for us. > > In the DSDT you were looking at the H_EC device, but for whatever > reason, there are two ECs in this DSDT and the one that Linux picks up > is the 2nd one, \_SB_.PCI0.LPCB.EC0. > > This device has no _STA but does have valid _CRS, and the debug patch > results agree: > > ACPI : EC: acpi_ec_dsdt_probe: search for DSDT EC > ACPI : EC: ec_parse_device: _STA status 5 val 0 > ACPI : EC: ec_parse_device: _CRS status 0 command 66 data 62 > ACPI : EC: EC stopped > ACPI : EC: EC started > ACPI: \_SB_.PCI0.LPCB.EC0_: Used as first EC > > acpidump output is at > https://www.dropbox.com/s/d3w2xrmrz1oklnw/x580vd_acpi.tgz?dl=0 Yes, there are 2 ECs. Device (H_EC) { Name (_HID, EisaId ("PNP0C09") /* Embedded Controller Device */) // _HID: Hardware ID Name (_UID, One) // _UID: Unique ID Method (_STA, 0, NotSerialized) // _STA: Status { ^^^GFX0.CLID = 0x03 Return (Zero) } Device (EC0) { Name (_HID, EisaId ("PNP0C09") /* Embedded Controller Device */) // _HID: Hardware ID Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings { IO (Decode16, 0x0062, // Range Minimum 0x0062, // Range Maximum 0x00, // Alignment 0x01, // Length ) IO (Decode16, 0x0066, // Range Minimum 0x0066, // Range Maximum 0x00, // Alignment 0x01, // Length ) }) Method (_GPE, 0, NotSerialized) // _GPE: General Purpose Events { Local0 = 0x33 Return (Local0) } And linux EC driver only tries the 1st one. You probably should add sanity check to skip first one, and return the 2nd one. Thanks Lv > > Daniel > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Daniel > From: Daniel Drake [mailto:drake@endlessm.com] > Subject: Re: [PATCH] ACPI / EC: handle ECDT EC and DSDT EC simultaneously > > Hi Lv, > > On Fri, Apr 28, 2017 at 6:52 AM, Daniel Drake <drake@endlessm.com> wrote: > > In the DSDT you were looking at the H_EC device, but for whatever > > reason, there are two ECs in this DSDT and the one that Linux picks up > > is the 2nd one, \_SB_.PCI0.LPCB.EC0. > > > > This device has no _STA but does have valid _CRS, and the debug patch > > results agree: > > > > ACPI : EC: acpi_ec_dsdt_probe: search for DSDT EC > > ACPI : EC: ec_parse_device: _STA status 5 val 0 > > ACPI : EC: ec_parse_device: _CRS status 0 command 66 data 62 > > ACPI : EC: EC stopped > > ACPI : EC: EC started > > ACPI: \_SB_.PCI0.LPCB.EC0_: Used as first EC > > How do we proceed here? Any other options we can try that don't > involve DMI quirking? Quirk seems to be required: 1. We do not know the order windows invokes _INI/_STA, there are several possible solutions 1. EC opregion handler is installed when EC device is enumerated. Before that, just complains errors. 2. EC opregion access triggers Linux to probe EC device driver. We have tried 1, and as I mentioned before, someone reported a regression for the new errors. 2 also looked scary, as EC._STA may still contain BIOS provided initialization code that cannot be invoked such earlier. 2. Even the above one doesn't matter (for example, 2 is working for all platforms), Linux still don't know the preference of the GPE settings. Maybe I could help to present some demo code. Let's file a kernel Bugzilla entry to work on this issue together: https://bugzilla.kernel.org/show_bug.cgi?id=195651 Thanks and best regards Lv > > Thanks > Daniel
diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index c24235d8fb52..78395395e3d9 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -1691,7 +1691,7 @@ int __init acpi_ec_dsdt_probe(void) * At this point, the GPE is not fully initialized, so do not to * handle the events. */ - ret = acpi_config_boot_ec(ec, ec->handle, false, false); + ret = acpi_config_boot_ec(boot_ec, boot_ec->handle, false, false); error: if (ret) acpi_ec_free(ec);
The Asus laptops X550VXK/FX502VD/FX502VE have a BIOS bug where the ECDT (correctly) states that EC events trigger GPE 0x23, but the DSDT _GPE method (incorrectly) says GPE 0x33. A cursory glance of the code suggests that this should work regardless (because it looks like Linux would treat the ECDT and DSDT as two separate ECs supported simultaneously), but in practice it is not working since the ECDT is ultimately ignored in favour of the DSDT EC. The sequence of events is: 1. acpi_ec_ecdt_probe() is called in early boot, and calls acpi_config_boot_ec(is_ecdt=true) for the ECDT EC. acpi_config_boot_ec() sets boot_ec to this new EC, and boot_ec_is_ecdt = true. 2. Later, acpi_ec_dsdt_probe() is called and creates a new ec to represent the DSDT EC. Then: /* * When the DSDT EC is available, always re-configure boot EC to * have _REG evaluated. _REG can only be evaluated after the * namespace initialization. * At this point, the GPE is not fully initialized, so do not to * handle the events. */ ret = acpi_config_boot_ec(ec, ec->handle, false, false); So the DSDT EC is passed to acpi_config_boot_ec() which does: /* Unset old boot EC */ if (boot_ec != ec) acpi_ec_free(boot_ec); acpi_ec_free() sets boot_ec to NULL. Further down in acpi_config_boot_ec() we reach: /* Set new boot EC */ if (!boot_ec) { boot_ec = ec; boot_ec_is_ecdt = is_ecdt; } So now boot_ec points to the DSDT EC and boot_ec_is_ecdt is false. 3. Later, ecpi_ec_ecdt_start() is called and this looks like it would enable GPEs on our ECDT, but it bails out because of: if (!boot_ec_is_ecdt) return -ENODEV; The comment I pasted above from acpi_ec_dsdt_probe() says that it is going to reconfigure the boot EC, but it instead calls acpi_config_boot_ec() on the newly probed DSDT EC. It seems like the code is not following the comment here. Adjusting that code to work with boot_ec adjusts the sequence of events so that both boot EC and DSDT are treated independently and as a result, we get EC interrupts firing correctly. This fixes media keys on the mentioned laptop models. Thanks to Chris Chiu for diagnosis. Signed-off-by: Daniel Drake <drake@endlessm.com> --- drivers/acpi/ec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)