| Message ID | 1546694240.2072.74.camel@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | ACPI/ACPICA: Run EC _REG explicitly for ECDT | expand |
ca On Sat, Jan 5, 2019 at 2:17 PM Zhang Rui <rui.zhang@intel.com> wrote: > > From 6f2fac0ffcd7fd61036baa0798ab171496cff50f Mon Sep 17 00:00:00 2001 > From: Zhang Rui <rui.zhang@intel.com> > Date: Fri, 4 Jan 2019 14:35:52 +0800 > Subject: [PATCH] ACPI/ACPICA: Run EC _REG explicitly for ECDT > > commit d737f333b211 ("ACPI: probe ECDT before loading AML tables > regardless of module-level code flag") probes ECDT before loading > the AML tables. > > This is the right thing to do according to the ACPI Spec, but > unfortunately, it breaks the current kernel EC/ECDT support, and makes > many devices, including battery, lid, etc, fails to work on a variety > of platforms. > > This is because: > 1. Probing ECDT requires installing EC address space handler > 2. EC _REG can not be evaluated at the time when probing ECDT because AML > tables have not been loaded yet. > 3. Many devices fail to work because EC _REG is not evaluated. > > To fix this, a solution is proposed in this patch to evaluate EC _REG > explicitly in ACPICA, if ECDT has been probed. It would be good to give some more details here as the patch itself appears to be rather convoluted. Also, the description above doesn't actually explain why the problem is there, as it doesn't explain why probing the ECDT early causes the EC _REG to be not evaluated. It looks like the failure is due to the change of the ordering between acpi_load_tables() and acpi_ec_ecdt_probe() in the acpi_gbl_group_module_level_code case which causes the EC to be probed before instantiating the namespace and _REG obviously cannot be evaluated then. Honestly, I don't think that ACPICA is the right place to address this as the failure appears to be Linux-specific. It would be better to fix it in the EC driver (which may require some help from ACPICA, though).
On Mon, Jan 7, 2019 at 12:51 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > ca On Sat, Jan 5, 2019 at 2:17 PM Zhang Rui <rui.zhang@intel.com> wrote: > > > > From 6f2fac0ffcd7fd61036baa0798ab171496cff50f Mon Sep 17 00:00:00 2001 > > From: Zhang Rui <rui.zhang@intel.com> > > Date: Fri, 4 Jan 2019 14:35:52 +0800 > > Subject: [PATCH] ACPI/ACPICA: Run EC _REG explicitly for ECDT > > > > commit d737f333b211 ("ACPI: probe ECDT before loading AML tables > > regardless of module-level code flag") probes ECDT before loading > > the AML tables. > > > > This is the right thing to do according to the ACPI Spec, but > > unfortunately, it breaks the current kernel EC/ECDT support, and makes > > many devices, including battery, lid, etc, fails to work on a variety > > of platforms. > > > > This is because: > > 1. Probing ECDT requires installing EC address space handler > > 2. EC _REG can not be evaluated at the time when probing ECDT because AML > > tables have not been loaded yet. > > 3. Many devices fail to work because EC _REG is not evaluated. > > > > To fix this, a solution is proposed in this patch to evaluate EC _REG > > explicitly in ACPICA, if ECDT has been probed. > > It would be good to give some more details here as the patch itself > appears to be rather convoluted. > > Also, the description above doesn't actually explain why the problem > is there, as it doesn't explain why probing the ECDT early causes the > EC _REG to be not evaluated. > > It looks like the failure is due to the change of the ordering between > acpi_load_tables() and acpi_ec_ecdt_probe() in the > acpi_gbl_group_module_level_code case which causes the EC to be probed > before instantiating the namespace and _REG obviously cannot be > evaluated then. So what happens is: acpi_ec_ecdt_probe() acpi_ec_ecdt_probe() acpi_config_boot_ec(ec, ACPI_ROOT_OBJECT, false, true) ... ec->handle = ACPI_ROOT_OBJECT; ... acpi_ec_setup(ec, false) ec_install_handlers(ec, false) acpi_install_address_space_handler(ACPI_ROOT_OBJECT, ACPI_ADR_SPACE_EC, &acpi_ec_space_handler, NULL, ec) ... set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags) and now it returns, because handle_events is "false". Next time acpi_ec_setup() is called from acpi_ec_add() that can be invoked in two ways, either through acpi_bus_register_driver(&acpi_ec_driver) which runs it for the DSDT EC (if found), or through acpi_ec_ecdt_start() and acpi_bus_register_early_device(). There are two cases in there, the acpi_config_boot_ec() one and the direct invocation of acpi_ec_setup() when acpi_is_boot_ec(ec) returns "false". The former is the failing case AFAICS, because acpi_ec_ecdt_probe() (if successful) has set boot_ec as well as ec->command_addr and ec->data_addr. So acpi_config_boot_ec() runs again and since boot_ec->handle has not been updated, it doesn't call ec_remove_handlers(), and because the EC_FLAGS_EC_HANDLER_INSTALLED is set, the address space handler installed previously is retained and _REG is not evaluated. Since it wasn't evaluated before (as it was not present then), it is never evaluated and the failure occurs. So it looks like clearing ec-> handle before invoking acpi_config_boot_ec() in the is_ecdt case in acpi_ec_add() can help, can't it?
On Monday, January 7, 2019 1:43:29 PM CET Rafael J. Wysocki wrote: > On Mon, Jan 7, 2019 at 12:51 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > ca On Sat, Jan 5, 2019 at 2:17 PM Zhang Rui <rui.zhang@intel.com> wrote: > > > > > > From 6f2fac0ffcd7fd61036baa0798ab171496cff50f Mon Sep 17 00:00:00 2001 > > > From: Zhang Rui <rui.zhang@intel.com> > > > Date: Fri, 4 Jan 2019 14:35:52 +0800 > > > Subject: [PATCH] ACPI/ACPICA: Run EC _REG explicitly for ECDT > > > > > > commit d737f333b211 ("ACPI: probe ECDT before loading AML tables > > > regardless of module-level code flag") probes ECDT before loading > > > the AML tables. > > > > > > This is the right thing to do according to the ACPI Spec, but > > > unfortunately, it breaks the current kernel EC/ECDT support, and makes > > > many devices, including battery, lid, etc, fails to work on a variety > > > of platforms. > > > > > > This is because: > > > 1. Probing ECDT requires installing EC address space handler > > > 2. EC _REG can not be evaluated at the time when probing ECDT because AML > > > tables have not been loaded yet. > > > 3. Many devices fail to work because EC _REG is not evaluated. > > > > > > To fix this, a solution is proposed in this patch to evaluate EC _REG > > > explicitly in ACPICA, if ECDT has been probed. > > > > It would be good to give some more details here as the patch itself > > appears to be rather convoluted. > > > > Also, the description above doesn't actually explain why the problem > > is there, as it doesn't explain why probing the ECDT early causes the > > EC _REG to be not evaluated. > > > > It looks like the failure is due to the change of the ordering between > > acpi_load_tables() and acpi_ec_ecdt_probe() in the > > acpi_gbl_group_module_level_code case which causes the EC to be probed > > before instantiating the namespace and _REG obviously cannot be > > evaluated then. > > So what happens is: > > acpi_ec_ecdt_probe() > acpi_ec_ecdt_probe() > acpi_config_boot_ec(ec, ACPI_ROOT_OBJECT, false, true) > ... > ec->handle = ACPI_ROOT_OBJECT; > ... > acpi_ec_setup(ec, false) > ec_install_handlers(ec, false) > > acpi_install_address_space_handler(ACPI_ROOT_OBJECT, > ACPI_ADR_SPACE_EC, &acpi_ec_space_handler, NULL, ec) > ... > set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags) > > and now it returns, because handle_events is "false". > > Next time acpi_ec_setup() is called from acpi_ec_add() that can be > invoked in two ways, either through > acpi_bus_register_driver(&acpi_ec_driver) which runs it for the DSDT > EC (if found), or through acpi_ec_ecdt_start() and > acpi_bus_register_early_device(). > > There are two cases in there, the acpi_config_boot_ec() one and the > direct invocation of acpi_ec_setup() when acpi_is_boot_ec(ec) returns > "false". The former is the failing case AFAICS, because > acpi_ec_ecdt_probe() (if successful) has set boot_ec as well as > ec->command_addr and ec->data_addr. > > So acpi_config_boot_ec() runs again and since boot_ec->handle has not > been updated, it doesn't call ec_remove_handlers(), and because the > EC_FLAGS_EC_HANDLER_INSTALLED is set, the address space handler > installed previously is retained and _REG is not evaluated. Since it > wasn't evaluated before (as it was not present then), it is never > evaluated and the failure occurs. > > So it looks like clearing ec-> handle before invoking > acpi_config_boot_ec() in the is_ecdt case in acpi_ec_add() can help, > can't it? Well, that wouldn't work, because ec->handle is checked by acpi_config_boot_ec() too. What might work would be to clear it and then pass the original to acpi_config_boot_ec() as 'handle'. Anyway, the patch below should cover all of the different cases just fine, so can you try this one, please? --- drivers/acpi/ec.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) Index: linux-pm/drivers/acpi/ec.c =================================================================== --- linux-pm.orig/drivers/acpi/ec.c +++ linux-pm/drivers/acpi/ec.c @@ -1545,12 +1545,13 @@ static int acpi_config_boot_ec(struct ac int ret; /* - * Changing the ACPI handle results in a re-configuration of the - * boot EC. And if it happens after the namespace initialization, - * it causes _REG evaluations. + * It is necessary to evaluate the _REG method for the EC address space + * handler, but it might not be evaluated when installing that handler + * previously (as that might happen when the namespace was not present + * yet), so remove the handler at this point to force the evaluation of + * _REG when it is installed again by acpi_ec_setup() below. */ - if (boot_ec && boot_ec->handle != handle) - ec_remove_handlers(boot_ec); + ec_remove_handlers(ec); /* Unset old boot EC */ if (boot_ec != ec)
On Monday, January 7, 2019 2:45:14 PM CET Rafael J. Wysocki wrote: > On Monday, January 7, 2019 1:43:29 PM CET Rafael J. Wysocki wrote: > > On Mon, Jan 7, 2019 at 12:51 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > ca On Sat, Jan 5, 2019 at 2:17 PM Zhang Rui <rui.zhang@intel.com> wrote: > > > > > > > > From 6f2fac0ffcd7fd61036baa0798ab171496cff50f Mon Sep 17 00:00:00 2001 > > > > From: Zhang Rui <rui.zhang@intel.com> > > > > Date: Fri, 4 Jan 2019 14:35:52 +0800 > > > > Subject: [PATCH] ACPI/ACPICA: Run EC _REG explicitly for ECDT > > > > > > > > commit d737f333b211 ("ACPI: probe ECDT before loading AML tables > > > > regardless of module-level code flag") probes ECDT before loading > > > > the AML tables. > > > > > > > > This is the right thing to do according to the ACPI Spec, but > > > > unfortunately, it breaks the current kernel EC/ECDT support, and makes > > > > many devices, including battery, lid, etc, fails to work on a variety > > > > of platforms. > > > > > > > > This is because: > > > > 1. Probing ECDT requires installing EC address space handler > > > > 2. EC _REG can not be evaluated at the time when probing ECDT because AML > > > > tables have not been loaded yet. > > > > 3. Many devices fail to work because EC _REG is not evaluated. > > > > > > > > To fix this, a solution is proposed in this patch to evaluate EC _REG > > > > explicitly in ACPICA, if ECDT has been probed. > > > > > > It would be good to give some more details here as the patch itself > > > appears to be rather convoluted. > > > > > > Also, the description above doesn't actually explain why the problem > > > is there, as it doesn't explain why probing the ECDT early causes the > > > EC _REG to be not evaluated. > > > > > > It looks like the failure is due to the change of the ordering between > > > acpi_load_tables() and acpi_ec_ecdt_probe() in the > > > acpi_gbl_group_module_level_code case which causes the EC to be probed > > > before instantiating the namespace and _REG obviously cannot be > > > evaluated then. > > > > So what happens is: > > > > acpi_ec_ecdt_probe() > > acpi_ec_ecdt_probe() > > acpi_config_boot_ec(ec, ACPI_ROOT_OBJECT, false, true) > > ... > > ec->handle = ACPI_ROOT_OBJECT; > > ... > > acpi_ec_setup(ec, false) > > ec_install_handlers(ec, false) > > > > acpi_install_address_space_handler(ACPI_ROOT_OBJECT, > > ACPI_ADR_SPACE_EC, &acpi_ec_space_handler, NULL, ec) > > ... > > set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags) > > > > and now it returns, because handle_events is "false". > > > > Next time acpi_ec_setup() is called from acpi_ec_add() that can be > > invoked in two ways, either through > > acpi_bus_register_driver(&acpi_ec_driver) which runs it for the DSDT > > EC (if found), or through acpi_ec_ecdt_start() and > > acpi_bus_register_early_device(). > > > > There are two cases in there, the acpi_config_boot_ec() one and the > > direct invocation of acpi_ec_setup() when acpi_is_boot_ec(ec) returns > > "false". The former is the failing case AFAICS, because > > acpi_ec_ecdt_probe() (if successful) has set boot_ec as well as > > ec->command_addr and ec->data_addr. > > > > So acpi_config_boot_ec() runs again and since boot_ec->handle has not > > been updated, it doesn't call ec_remove_handlers(), and because the > > EC_FLAGS_EC_HANDLER_INSTALLED is set, the address space handler > > installed previously is retained and _REG is not evaluated. Since it > > wasn't evaluated before (as it was not present then), it is never > > evaluated and the failure occurs. > > > > So it looks like clearing ec-> handle before invoking > > acpi_config_boot_ec() in the is_ecdt case in acpi_ec_add() can help, > > can't it? > > Well, that wouldn't work, because ec->handle is checked by acpi_config_boot_ec() > too. What might work would be to clear it and then pass the original to > acpi_config_boot_ec() as 'handle'. Anyway, the patch below should cover > all of the different cases just fine, so can you try this one, please? > > --- > drivers/acpi/ec.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > Index: linux-pm/drivers/acpi/ec.c > =================================================================== > --- linux-pm.orig/drivers/acpi/ec.c > +++ linux-pm/drivers/acpi/ec.c > @@ -1545,12 +1545,13 @@ static int acpi_config_boot_ec(struct ac > int ret; > > /* > - * Changing the ACPI handle results in a re-configuration of the > - * boot EC. And if it happens after the namespace initialization, > - * it causes _REG evaluations. > + * It is necessary to evaluate the _REG method for the EC address space > + * handler, but it might not be evaluated when installing that handler > + * previously (as that might happen when the namespace was not present > + * yet), so remove the handler at this point to force the evaluation of > + * _REG when it is installed again by acpi_ec_setup() below. > */ > - if (boot_ec && boot_ec->handle != handle) > - ec_remove_handlers(boot_ec); > + ec_remove_handlers(ec); > > /* Unset old boot EC */ > if (boot_ec != ec) On a second thought, this may cause _REG to be executed twice in a row for the EC address space (in the DSDT boot EC case), so it probably is better to only remove the handler in the "ECDT EC" case: --- drivers/acpi/ec.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) Index: linux-pm/drivers/acpi/ec.c =================================================================== --- linux-pm.orig/drivers/acpi/ec.c +++ linux-pm/drivers/acpi/ec.c @@ -1637,7 +1637,18 @@ static int acpi_ec_add(struct acpi_devic if (acpi_is_boot_ec(ec)) { boot_ec_is_ecdt = is_ecdt; - if (!is_ecdt) { + if (is_ecdt) { + /* + * It is necessary to evaluate the _REG method for the + * EC address space handler, but it was not evaluated + * when installing that handler previously (as that + * happened when the namespace was not present yet), so + * remove the handler at this point to force the + * evaluation of _REG when it is installed again by + * acpi_config_boot_ec() below. + */ + ec_remove_handlers(ec); + } else { /* * Trust PNP0C09 namespace location rather than * ECDT ID. But trust ECDT GPE rather than _GPE
On Mon, Jan 7, 2019 at 11:32 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Monday, January 7, 2019 2:45:14 PM CET Rafael J. Wysocki wrote: > > On Monday, January 7, 2019 1:43:29 PM CET Rafael J. Wysocki wrote: > > > On Mon, Jan 7, 2019 at 12:51 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > ca On Sat, Jan 5, 2019 at 2:17 PM Zhang Rui <rui.zhang@intel.com> wrote: > > > > > > > > > > From 6f2fac0ffcd7fd61036baa0798ab171496cff50f Mon Sep 17 00:00:00 2001 > > > > > From: Zhang Rui <rui.zhang@intel.com> > > > > > Date: Fri, 4 Jan 2019 14:35:52 +0800 > > > > > Subject: [PATCH] ACPI/ACPICA: Run EC _REG explicitly for ECDT > > > > > > > > > > commit d737f333b211 ("ACPI: probe ECDT before loading AML tables > > > > > regardless of module-level code flag") probes ECDT before loading > > > > > the AML tables. > > > > > > > > > > This is the right thing to do according to the ACPI Spec, but > > > > > unfortunately, it breaks the current kernel EC/ECDT support, and makes > > > > > many devices, including battery, lid, etc, fails to work on a variety > > > > > of platforms. > > > > > > > > > > This is because: > > > > > 1. Probing ECDT requires installing EC address space handler > > > > > 2. EC _REG can not be evaluated at the time when probing ECDT because AML > > > > > tables have not been loaded yet. > > > > > 3. Many devices fail to work because EC _REG is not evaluated. > > > > > > > > > > To fix this, a solution is proposed in this patch to evaluate EC _REG > > > > > explicitly in ACPICA, if ECDT has been probed. > > > > > > > > It would be good to give some more details here as the patch itself > > > > appears to be rather convoluted. > > > > > > > > Also, the description above doesn't actually explain why the problem > > > > is there, as it doesn't explain why probing the ECDT early causes the > > > > EC _REG to be not evaluated. > > > > > > > > It looks like the failure is due to the change of the ordering between > > > > acpi_load_tables() and acpi_ec_ecdt_probe() in the > > > > acpi_gbl_group_module_level_code case which causes the EC to be probed > > > > before instantiating the namespace and _REG obviously cannot be > > > > evaluated then. > > > > > > So what happens is: > > > > > > acpi_ec_ecdt_probe() > > > acpi_ec_ecdt_probe() > > > acpi_config_boot_ec(ec, ACPI_ROOT_OBJECT, false, true) > > > ... > > > ec->handle = ACPI_ROOT_OBJECT; > > > ... > > > acpi_ec_setup(ec, false) > > > ec_install_handlers(ec, false) > > > > > > acpi_install_address_space_handler(ACPI_ROOT_OBJECT, > > > ACPI_ADR_SPACE_EC, &acpi_ec_space_handler, NULL, ec) > > > ... > > > set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags) > > > > > > and now it returns, because handle_events is "false". > > > > > > Next time acpi_ec_setup() is called from acpi_ec_add() that can be > > > invoked in two ways, either through > > > acpi_bus_register_driver(&acpi_ec_driver) which runs it for the DSDT > > > EC (if found), or through acpi_ec_ecdt_start() and > > > acpi_bus_register_early_device(). > > > > > > There are two cases in there, the acpi_config_boot_ec() one and the > > > direct invocation of acpi_ec_setup() when acpi_is_boot_ec(ec) returns > > > "false". The former is the failing case AFAICS, because > > > acpi_ec_ecdt_probe() (if successful) has set boot_ec as well as > > > ec->command_addr and ec->data_addr. > > > > > > So acpi_config_boot_ec() runs again and since boot_ec->handle has not > > > been updated, it doesn't call ec_remove_handlers(), and because the > > > EC_FLAGS_EC_HANDLER_INSTALLED is set, the address space handler > > > installed previously is retained and _REG is not evaluated. Since it > > > wasn't evaluated before (as it was not present then), it is never > > > evaluated and the failure occurs. [cut] > it probably is better to only remove the handler in the "ECDT EC" case: > > --- > drivers/acpi/ec.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > Index: linux-pm/drivers/acpi/ec.c > =================================================================== > --- linux-pm.orig/drivers/acpi/ec.c > +++ linux-pm/drivers/acpi/ec.c > @@ -1637,7 +1637,18 @@ static int acpi_ec_add(struct acpi_devic > > if (acpi_is_boot_ec(ec)) { > boot_ec_is_ecdt = is_ecdt; > - if (!is_ecdt) { > + if (is_ecdt) { > + /* > + * It is necessary to evaluate the _REG method for the > + * EC address space handler, but it was not evaluated > + * when installing that handler previously (as that > + * happened when the namespace was not present yet), so > + * remove the handler at this point to force the > + * evaluation of _REG when it is installed again by > + * acpi_config_boot_ec() below. > + */ > + ec_remove_handlers(ec); > + } else { > /* > * Trust PNP0C09 namespace location rather than > * ECDT ID. But trust ECDT GPE rather than _GPE > Moreover, I think that installing the address space handler for the ECDT EC in acpi_ec_ecdt_probe() is pointless, because AML is not allowed to use that opregion anyway without evaluating _REG for it. However, this opregion should be made available early, in analogy with the "DSDT EC" case, so it looks like acpi_ec_ecdt_probe() only needs to initialize the addresses and GPE in the ec object, set its handle to ACPI_ROOT_OBJECT, set boot_ec to point to it and return. And analogously for acpi_ec_dsdt_probe(). Then, acpi_ec_setup() can be called right after acpi_ec_dsdt_probe() and it should work regardless of the boot EC type. Let me try to cut some patches to do that.
On 一, 2019-01-07 at 14:45 +0100, Rafael J. Wysocki wrote: > On Monday, January 7, 2019 1:43:29 PM CET Rafael J. Wysocki wrote: > > > > On Mon, Jan 7, 2019 at 12:51 PM Rafael J. Wysocki <rafael@kernel.or > > g> wrote: > > > > > > > > > ca On Sat, Jan 5, 2019 at 2:17 PM Zhang Rui <rui.zhang@intel.c > > > om> wrote: > > > > > > > > > > > > From 6f2fac0ffcd7fd61036baa0798ab171496cff50f Mon Sep 17 > > > > 00:00:00 2001 > > > > From: Zhang Rui <rui.zhang@intel.com> > > > > Date: Fri, 4 Jan 2019 14:35:52 +0800 > > > > Subject: [PATCH] ACPI/ACPICA: Run EC _REG explicitly for ECDT > > > > > > > > commit d737f333b211 ("ACPI: probe ECDT before loading AML > > > > tables > > > > regardless of module-level code flag") probes ECDT before > > > > loading > > > > the AML tables. > > > > > > > > This is the right thing to do according to the ACPI Spec, but > > > > unfortunately, it breaks the current kernel EC/ECDT support, > > > > and makes > > > > many devices, including battery, lid, etc, fails to work on a > > > > variety > > > > of platforms. > > > > > > > > This is because: > > > > 1. Probing ECDT requires installing EC address space handler > > > > 2. EC _REG can not be evaluated at the time when probing ECDT > > > > because AML > > > > tables have not been loaded yet. > > > > 3. Many devices fail to work because EC _REG is not evaluated. > > > > > > > > To fix this, a solution is proposed in this patch to evaluate > > > > EC _REG > > > > explicitly in ACPICA, if ECDT has been probed. > > > It would be good to give some more details here as the patch > > > itself > > > appears to be rather convoluted. > > > > > > Also, the description above doesn't actually explain why the > > > problem > > > is there, as it doesn't explain why probing the ECDT early causes > > > the > > > EC _REG to be not evaluated. > > > > > > It looks like the failure is due to the change of the ordering > > > between > > > acpi_load_tables() and acpi_ec_ecdt_probe() in the > > > acpi_gbl_group_module_level_code case which causes the EC to be > > > probed > > > before instantiating the namespace and _REG obviously cannot be > > > evaluated then. > > So what happens is: > > > > acpi_ec_ecdt_probe() > > acpi_ec_ecdt_probe() > > acpi_config_boot_ec(ec, ACPI_ROOT_OBJECT, false, true) > > ... > > ec->handle = ACPI_ROOT_OBJECT; > > ... > > acpi_ec_setup(ec, false) > > ec_install_handlers(ec, false) > > > > acpi_install_address_space_handler(ACPI_ROOT_OBJECT, > > ACPI_ADR_SPACE_EC, &acpi_ec_space_handler, NULL, ec) > > ... > > set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec- > > >flags) > > > > and now it returns, because handle_events is "false". > > > > Next time acpi_ec_setup() is called from acpi_ec_add() that can be > > invoked in two ways, either through > > acpi_bus_register_driver(&acpi_ec_driver) which runs it for the > > DSDT > > EC (if found), or through acpi_ec_ecdt_start() and > > acpi_bus_register_early_device(). > > > > There are two cases in there, the acpi_config_boot_ec() one and the > > direct invocation of acpi_ec_setup() when acpi_is_boot_ec(ec) > > returns > > "false". The former is the failing case AFAICS, because > > acpi_ec_ecdt_probe() (if successful) has set boot_ec as well as > > ec->command_addr and ec->data_addr. > > > > So acpi_config_boot_ec() runs again and since boot_ec->handle has > > not > > been updated, it doesn't call ec_remove_handlers(), and because the > > EC_FLAGS_EC_HANDLER_INSTALLED is set, the address space handler > > installed previously is retained and _REG is not evaluated. Since > > it > > wasn't evaluated before (as it was not present then), it is never > > evaluated and the failure occurs. > > > > So it looks like clearing ec-> handle before invoking > > acpi_config_boot_ec() in the is_ecdt case in acpi_ec_add() can > > help, > > can't it? > Well, that wouldn't work, because ec->handle is checked by > acpi_config_boot_ec() > too. What might work would be to clear it and then pass the original > to > acpi_config_boot_ec() as 'handle'. Anyway, the patch below should > cover > all of the different cases just fine, so can you try this one, > please? > I was thinking of fixing it in EC driver in the very beginning, and I have a debug patch similar to the patch below https://bugzilla.kernel.org/show_bug.cgi?id=200011#c71 and it indeed fixes the problem. But then _REG is evaluated in the driver probe time, which I think it might be too late, and the best time to evaluate _REG is 1. after ACPI namespace ready (_REG available) 2. before any _STA being evaluated in ACPICA core. acpi_early_init() acpi_ecdt_probe() acpi_bus_init() acpi_load_tables() ... acpi_initialize_objects() acpi_ns_initialize_devices() ... acpi_ev_initialize_op_regions() ... acpi_ns_init_one_device() acpi_ut_execute_STA() That's why I prefer to fix in it acpi_ev_initialize_op_regions(). To avoid _REG being evaluated twice, we can fix EC driver to 1. install EC address space handler for ECDT and never remove it 2. install EC address space handler for DSDT EC only if a) ECDT does not exists, or b) ECDT handle does not equal DSDT handle. what do you think? thanks, rui > --- > drivers/acpi/ec.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > Index: linux-pm/drivers/acpi/ec.c > =================================================================== > --- linux-pm.orig/drivers/acpi/ec.c > +++ linux-pm/drivers/acpi/ec.c > @@ -1545,12 +1545,13 @@ static int acpi_config_boot_ec(struct ac > int ret; > > /* > - * Changing the ACPI handle results in a re-configuration of > the > - * boot EC. And if it happens after the namespace > initialization, > - * it causes _REG evaluations. > + * It is necessary to evaluate the _REG method for the EC > address space > + * handler, but it might not be evaluated when installing > that handler > + * previously (as that might happen when the namespace was > not present > + * yet), so remove the handler at this point to force the > evaluation of > + * _REG when it is installed again by acpi_ec_setup() below. > */ > - if (boot_ec && boot_ec->handle != handle) > - ec_remove_handlers(boot_ec); > + ec_remove_handlers(ec); > > /* Unset old boot EC */ > if (boot_ec != ec) >
"On Tue, Jan 8, 2019 at 2:24 PM Zhang Rui <rui.zhang@intel.com> wrote: > > On 二, 2019-01-08 at 11:15 +0100, Rafael J. Wysocki wrote: > > On Mon, Jan 7, 2019 at 11:32 PM Rafael J. Wysocki <rjw@rjwysocki.net> > > wrote: > > > > > > > > > On Monday, January 7, 2019 2:45:14 PM CET Rafael J. Wysocki wrote: > > > > > > > > On Monday, January 7, 2019 1:43:29 PM CET Rafael J. Wysocki > > > > wrote: > > > > > > > > > > On Mon, Jan 7, 2019 at 12:51 PM Rafael J. Wysocki <rafael@kerne > > > > > l.org> wrote: > > > > > > > > > > > > > > > > > > ca On Sat, Jan 5, 2019 at 2:17 PM Zhang Rui <rui.zhang@int > > > > > > el.com> wrote: > > > > > > > > > > > > > > > > > > > > > From 6f2fac0ffcd7fd61036baa0798ab171496cff50f Mon Sep 17 > > > > > > > 00:00:00 2001 > > > > > > > From: Zhang Rui <rui.zhang@intel.com> > > > > > > > Date: Fri, 4 Jan 2019 14:35:52 +0800 > > > > > > > Subject: [PATCH] ACPI/ACPICA: Run EC _REG explicitly for > > > > > > > ECDT > > > > > > > > > > > > > > commit d737f333b211 ("ACPI: probe ECDT before loading AML > > > > > > > tables > > > > > > > regardless of module-level code flag") probes ECDT before > > > > > > > loading > > > > > > > the AML tables. > > > > > > > > > > > > > > This is the right thing to do according to the ACPI Spec, > > > > > > > but > > > > > > > unfortunately, it breaks the current kernel EC/ECDT > > > > > > > support, and makes > > > > > > > many devices, including battery, lid, etc, fails to work on > > > > > > > a variety > > > > > > > of platforms. > > > > > > > > > > > > > > This is because: > > > > > > > 1. Probing ECDT requires installing EC address space > > > > > > > handler > > > > > > > 2. EC _REG can not be evaluated at the time when probing > > > > > > > ECDT because AML > > > > > > > tables have not been loaded yet. > > > > > > > 3. Many devices fail to work because EC _REG is not > > > > > > > evaluated. > > > > > > > > > > > > > > To fix this, a solution is proposed in this patch to > > > > > > > evaluate EC _REG > > > > > > > explicitly in ACPICA, if ECDT has been probed. > > > > > > It would be good to give some more details here as the patch > > > > > > itself > > > > > > appears to be rather convoluted. > > > > > > > > > > > > Also, the description above doesn't actually explain why the > > > > > > problem > > > > > > is there, as it doesn't explain why probing the ECDT early > > > > > > causes the > > > > > > EC _REG to be not evaluated. > > > > > > > > > > > > It looks like the failure is due to the change of the > > > > > > ordering between > > > > > > acpi_load_tables() and acpi_ec_ecdt_probe() in the > > > > > > acpi_gbl_group_module_level_code case which causes the EC to > > > > > > be probed > > > > > > before instantiating the namespace and _REG obviously cannot > > > > > > be > > > > > > evaluated then. > > > > > So what happens is: > > > > > > > > > > acpi_ec_ecdt_probe() > > > > > acpi_ec_ecdt_probe() > > > > > acpi_config_boot_ec(ec, ACPI_ROOT_OBJECT, false, true) > > > > > ... > > > > > ec->handle = ACPI_ROOT_OBJECT; > > > > > ... > > > > > acpi_ec_setup(ec, false) > > > > > ec_install_handlers(ec, false) > > > > > > > > > > acpi_install_address_space_handler(ACPI_ROOT_OBJECT, > > > > > ACPI_ADR_SPACE_EC, &acpi_ec_space_handler, NULL, ec) > > > > > ... > > > > > set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec- > > > > > >flags) > > > > > > > > > > and now it returns, because handle_events is "false". > > > > > > > > > > Next time acpi_ec_setup() is called from acpi_ec_add() that can > > > > > be > > > > > invoked in two ways, either through > > > > > acpi_bus_register_driver(&acpi_ec_driver) which runs it for the > > > > > DSDT > > > > > EC (if found), or through acpi_ec_ecdt_start() and > > > > > acpi_bus_register_early_device(). > > > > > > > > > > There are two cases in there, the acpi_config_boot_ec() one and > > > > > the > > > > > direct invocation of acpi_ec_setup() when acpi_is_boot_ec(ec) > > > > > returns > > > > > "false". The former is the failing case AFAICS, because > > > > > acpi_ec_ecdt_probe() (if successful) has set boot_ec as well as > > > > > ec->command_addr and ec->data_addr. > > > > > > > > > > So acpi_config_boot_ec() runs again and since boot_ec->handle > > > > > has not > > > > > been updated, it doesn't call ec_remove_handlers(), and because > > > > > the > > > > > EC_FLAGS_EC_HANDLER_INSTALLED is set, the address space handler > > > > > installed previously is retained and _REG is not > > > > > evaluated. Since it > > > > > wasn't evaluated before (as it was not present then), it is > > > > > never > > > > > evaluated and the failure occurs. > > [cut] > > > > > > > > it probably is better to only remove the handler in the "ECDT EC" > > > case: > > > > > > --- > > > drivers/acpi/ec.c | 13 ++++++++++++- > > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > > > Index: linux-pm/drivers/acpi/ec.c > > > =================================================================== > > > --- linux-pm.orig/drivers/acpi/ec.c > > > +++ linux-pm/drivers/acpi/ec.c > > > @@ -1637,7 +1637,18 @@ static int acpi_ec_add(struct acpi_devic > > > > > > if (acpi_is_boot_ec(ec)) { > > > boot_ec_is_ecdt = is_ecdt; > > > - if (!is_ecdt) { > > > + if (is_ecdt) { > > > + /* > > > + * It is necessary to evaluate the _REG > > > method for the > > > + * EC address space handler, but it was not > > > evaluated > > > + * when installing that handler previously > > > (as that > > > + * happened when the namespace was not > > > present yet), so > > > + * remove the handler at this point to > > > force the > > > + * evaluation of _REG when it is installed > > > again by > > > + * acpi_config_boot_ec() below. > > > + */ > > > + ec_remove_handlers(ec); > > > + } else { > > > /* > > > * Trust PNP0C09 namespace location rather > > > than > > > * ECDT ID. But trust ECDT GPE rather than > > > _GPE > > > > > Moreover, I think that installing the address space handler for the > > ECDT EC in acpi_ec_ecdt_probe() is pointless, because AML is not > > allowed to use that opregion anyway without evaluating _REG for it. > > > According to ACPI spec 5.2.15, > "This optional table (ECDT) provides the processor-relative, translated > resources of an Embedded Controller. > The presence of this table allows OSPM to provide Embedded Controller > operation region space access before the namespace has been evaluated." > > And my understanding is that the presence of ECDT means that we can > access EC address space without _REG being evaluated, for example, by > the module level code. That's a good point, but I'm not sure what exactly "before the namespace has been evaluated" means. I think that having an opregion registered at all is only useful when AML can run. Nothing will access the opregion before instantiating the namespace AFAICs.
s On Tue, Jan 8, 2019 at 2:49 PM Zhang Rui <rui.zhang@intel.com> wrote: > > On 一, 2019-01-07 at 14:45 +0100, Rafael J. Wysocki wrote: > > On Monday, January 7, 2019 1:43:29 PM CET Rafael J. Wysocki wrote: > > > > > > On Mon, Jan 7, 2019 at 12:51 PM Rafael J. Wysocki <rafael@kernel.or > > > g> wrote: > > > > > > > > > > > > ca On Sat, Jan 5, 2019 at 2:17 PM Zhang Rui <rui.zhang@intel.c > > > > om> wrote: > > > > > > > > > > > > > > > From 6f2fac0ffcd7fd61036baa0798ab171496cff50f Mon Sep 17 > > > > > 00:00:00 2001 > > > > > From: Zhang Rui <rui.zhang@intel.com> > > > > > Date: Fri, 4 Jan 2019 14:35:52 +0800 > > > > > Subject: [PATCH] ACPI/ACPICA: Run EC _REG explicitly for ECDT > > > > > > > > > > commit d737f333b211 ("ACPI: probe ECDT before loading AML > > > > > tables > > > > > regardless of module-level code flag") probes ECDT before > > > > > loading > > > > > the AML tables. > > > > > > > > > > This is the right thing to do according to the ACPI Spec, but > > > > > unfortunately, it breaks the current kernel EC/ECDT support, > > > > > and makes > > > > > many devices, including battery, lid, etc, fails to work on a > > > > > variety > > > > > of platforms. > > > > > > > > > > This is because: > > > > > 1. Probing ECDT requires installing EC address space handler > > > > > 2. EC _REG can not be evaluated at the time when probing ECDT > > > > > because AML > > > > > tables have not been loaded yet. > > > > > 3. Many devices fail to work because EC _REG is not evaluated. > > > > > > > > > > To fix this, a solution is proposed in this patch to evaluate > > > > > EC _REG > > > > > explicitly in ACPICA, if ECDT has been probed. > > > > It would be good to give some more details here as the patch > > > > itself > > > > appears to be rather convoluted. > > > > > > > > Also, the description above doesn't actually explain why the > > > > problem > > > > is there, as it doesn't explain why probing the ECDT early causes > > > > the > > > > EC _REG to be not evaluated. > > > > > > > > It looks like the failure is due to the change of the ordering > > > > between > > > > acpi_load_tables() and acpi_ec_ecdt_probe() in the > > > > acpi_gbl_group_module_level_code case which causes the EC to be > > > > probed > > > > before instantiating the namespace and _REG obviously cannot be > > > > evaluated then. > > > So what happens is: > > > > > > acpi_ec_ecdt_probe() > > > acpi_ec_ecdt_probe() > > > acpi_config_boot_ec(ec, ACPI_ROOT_OBJECT, false, true) > > > ... > > > ec->handle = ACPI_ROOT_OBJECT; > > > ... > > > acpi_ec_setup(ec, false) > > > ec_install_handlers(ec, false) > > > > > > acpi_install_address_space_handler(ACPI_ROOT_OBJECT, > > > ACPI_ADR_SPACE_EC, &acpi_ec_space_handler, NULL, ec) > > > ... > > > set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec- > > > >flags) > > > > > > and now it returns, because handle_events is "false". > > > > > > Next time acpi_ec_setup() is called from acpi_ec_add() that can be > > > invoked in two ways, either through > > > acpi_bus_register_driver(&acpi_ec_driver) which runs it for the > > > DSDT > > > EC (if found), or through acpi_ec_ecdt_start() and > > > acpi_bus_register_early_device(). > > > > > > There are two cases in there, the acpi_config_boot_ec() one and the > > > direct invocation of acpi_ec_setup() when acpi_is_boot_ec(ec) > > > returns > > > "false". The former is the failing case AFAICS, because > > > acpi_ec_ecdt_probe() (if successful) has set boot_ec as well as > > > ec->command_addr and ec->data_addr. > > > > > > So acpi_config_boot_ec() runs again and since boot_ec->handle has > > > not > > > been updated, it doesn't call ec_remove_handlers(), and because the > > > EC_FLAGS_EC_HANDLER_INSTALLED is set, the address space handler > > > installed previously is retained and _REG is not evaluated. Since > > > it > > > wasn't evaluated before (as it was not present then), it is never > > > evaluated and the failure occurs. > > > > > > So it looks like clearing ec-> handle before invoking > > > acpi_config_boot_ec() in the is_ecdt case in acpi_ec_add() can > > > help, > > > can't it? > > Well, that wouldn't work, because ec->handle is checked by > > acpi_config_boot_ec() > > too. What might work would be to clear it and then pass the original > > to > > acpi_config_boot_ec() as 'handle'. Anyway, the patch below should > > cover > > all of the different cases just fine, so can you try this one, > > please? > > > I was thinking of fixing it in EC driver in the very beginning, and I > have a debug patch similar to the patch below > https://bugzilla.kernel.org/show_bug.cgi?id=200011#c71 > and it indeed fixes the problem. > But then _REG is evaluated in the driver probe time, which I think it > might be too late, and the best time to evaluate _REG is > 1. after ACPI namespace ready (_REG available) > 2. before any _STA being evaluated in ACPICA core. > > acpi_early_init() > acpi_ecdt_probe() > acpi_bus_init() > acpi_load_tables() > ... > acpi_initialize_objects() > acpi_ns_initialize_devices() > ... > acpi_ev_initialize_op_regions() > ... > acpi_ns_init_one_device() > acpi_ut_execute_STA() > That's why I prefer to fix in it acpi_ev_initialize_op_regions(). > To avoid _REG being evaluated twice, we can fix EC driver to > 1. install EC address space handler for ECDT and never remove it That unless we are going to switch over to the DSDT EC later. > 2. install EC address space handler for DSDT EC only if a) ECDT does > not exists, or b) ECDT handle does not equal DSDT handle. Right: use the DSDT EC as a "boot EC" only if the ECDT EC is not there. > what do you think? Something's fishy. :-) AFAICS acpi_gbl_group_module_level_code has been "false" since commit 5a8361f7ecce (ACPICA: Integrate package handling with module-level code) which was way before commit d737f333b211. This means that the case that we regard as failing, i.e. acpi_load_tables() called from acpi_early_init(), which was before acpi_bus_init() that called acpi_ec_ecdt_probe(), in fact was not there at all since commit 5a8361f7ecce. Thus the code removed by commit d737f333b211 from acpi_early_init() was actually dead already at that time and acpi_ec_ecdt_probe() was always called before acpi_load_tables() even before commit d737f333b211 and that commit couldn't break it. Hence, the only other thing done by commit d737f333b211, which was to move the acpi_ec_ecdt_probe() to acpi_early_init() was wrong and so reverting that part of commit d737f333b211 should fix the problem.
. . w f On Tue, Jan 8, 2019 at 6:17 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > s On Tue, Jan 8, 2019 at 2:49 PM Zhang Rui <rui.zhang@intel.com> wrote: > > > > On 一, 2019-01-07 at 14:45 +0100, Rafael J. Wysocki wrote: > > > On Monday, January 7, 2019 1:43:29 PM CET Rafael J. Wysocki wrote: > > > > > > > > On Mon, Jan 7, 2019 at 12:51 PM Rafael J. Wysocki <rafael@kernel.or > > > > g> wrote: > > > > > > > > > > > > > > > ca On Sat, Jan 5, 2019 at 2:17 PM Zhang Rui <rui.zhang@intel.c > > > > > om> wrote: > > > > > > > > > > > > > > > > > > From 6f2fac0ffcd7fd61036baa0798ab171496cff50f Mon Sep 17 > > > > > > 00:00:00 2001 > > > > > > From: Zhang Rui <rui.zhang@intel.com> > > > > > > Date: Fri, 4 Jan 2019 14:35:52 +0800 > > > > > > Subject: [PATCH] ACPI/ACPICA: Run EC _REG explicitly for ECDT > > > > > > > > > > > > commit d737f333b211 ("ACPI: probe ECDT before loading AML > > > > > > tables > > > > > > regardless of module-level code flag") probes ECDT before > > > > > > loading > > > > > > the AML tables. > > > > > > > > > > > > This is the right thing to do according to the ACPI Spec, but > > > > > > unfortunately, it breaks the current kernel EC/ECDT support, > > > > > > and makes > > > > > > many devices, including battery, lid, etc, fails to work on a > > > > > > variety > > > > > > of platforms. > > > > > > > > > > > > This is because: > > > > > > 1. Probing ECDT requires installing EC address space handler > > > > > > 2. EC _REG can not be evaluated at the time when probing ECDT > > > > > > because AML > > > > > > tables have not been loaded yet. > > > > > > 3. Many devices fail to work because EC _REG is not evaluated. > > > > > > > > > > > > To fix this, a solution is proposed in this patch to evaluate > > > > > > EC _REG > > > > > > explicitly in ACPICA, if ECDT has been probed. > > > > > It would be good to give some more details here as the patch > > > > > itself > > > > > appears to be rather convoluted. > > > > > > > > > > Also, the description above doesn't actually explain why the > > > > > problem > > > > > is there, as it doesn't explain why probing the ECDT early causes > > > > > the > > > > > EC _REG to be not evaluated. > > > > > > > > > > It looks like the failure is due to the change of the ordering > > > > > between > > > > > acpi_load_tables() and acpi_ec_ecdt_probe() in the > > > > > acpi_gbl_group_module_level_code case which causes the EC to be > > > > > probed > > > > > before instantiating the namespace and _REG obviously cannot be > > > > > evaluated then. > > > > So what happens is: > > > > > > > > acpi_ec_ecdt_probe() > > > > acpi_ec_ecdt_probe() > > > > acpi_config_boot_ec(ec, ACPI_ROOT_OBJECT, false, true) > > > > ... > > > > ec->handle = ACPI_ROOT_OBJECT; > > > > ... > > > > acpi_ec_setup(ec, false) > > > > ec_install_handlers(ec, false) > > > > > > > > acpi_install_address_space_handler(ACPI_ROOT_OBJECT, > > > > ACPI_ADR_SPACE_EC, &acpi_ec_space_handler, NULL, ec) > > > > ... > > > > set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec- > > > > >flags) > > > > > > > > and now it returns, because handle_events is "false". > > > > > > > > Next time acpi_ec_setup() is called from acpi_ec_add() that can be > > > > invoked in two ways, either through > > > > acpi_bus_register_driver(&acpi_ec_driver) which runs it for the > > > > DSDT > > > > EC (if found), or through acpi_ec_ecdt_start() and > > > > acpi_bus_register_early_device(). > > > > > > > > There are two cases in there, the acpi_config_boot_ec() one and the > > > > direct invocation of acpi_ec_setup() when acpi_is_boot_ec(ec) > > > > returns > > > > "false". The former is the failing case AFAICS, because > > > > acpi_ec_ecdt_probe() (if successful) has set boot_ec as well as > > > > ec->command_addr and ec->data_addr. > > > > > > > > So acpi_config_boot_ec() runs again and since boot_ec->handle has > > > > not > > > > been updated, it doesn't call ec_remove_handlers(), and because the > > > > EC_FLAGS_EC_HANDLER_INSTALLED is set, the address space handler > > > > installed previously is retained and _REG is not evaluated. Since > > > > it > > > > wasn't evaluated before (as it was not present then), it is never > > > > evaluated and the failure occurs. > > > > > > > > So it looks like clearing ec-> handle before invoking > > > > acpi_config_boot_ec() in the is_ecdt case in acpi_ec_add() can > > > > help, > > > > can't it? > > > Well, that wouldn't work, because ec->handle is checked by > > > acpi_config_boot_ec() > > > too. What might work would be to clear it and then pass the original > > > to > > > acpi_config_boot_ec() as 'handle'. Anyway, the patch below should > > > cover > > > all of the different cases just fine, so can you try this one, > > > please? > > > > > I was thinking of fixing it in EC driver in the very beginning, and I > > have a debug patch similar to the patch below > > https://bugzilla.kernel.org/show_bug.cgi?id=200011#c71 > > and it indeed fixes the problem. > > But then _REG is evaluated in the driver probe time, which I think it > > might be too late, and the best time to evaluate _REG is > > 1. after ACPI namespace ready (_REG available) > > 2. before any _STA being evaluated in ACPICA core. > > > > acpi_early_init() > > acpi_ecdt_probe() > > acpi_bus_init() > > acpi_load_tables() > > ... > > acpi_initialize_objects() > > acpi_ns_initialize_devices() > > ... > > acpi_ev_initialize_op_regions() > > ... > > acpi_ns_init_one_device() > > acpi_ut_execute_STA() > > That's why I prefer to fix in it acpi_ev_initialize_op_regions(). > > To avoid _REG being evaluated twice, we can fix EC driver to > > 1. install EC address space handler for ECDT and never remove it > > That unless we are going to switch over to the DSDT EC later. > > > 2. install EC address space handler for DSDT EC only if a) ECDT does > > not exists, or b) ECDT handle does not equal DSDT handle. > > Right: use the DSDT EC as a "boot EC" only if the ECDT EC is not there. > > > what do you think? > > Something's fishy. :-) > > AFAICS acpi_gbl_group_module_level_code has been "false" since commit > 5a8361f7ecce (ACPICA: Integrate package handling with module-level > code) which was way before commit d737f333b211. This means that the > case that we regard as failing, i.e. acpi_load_tables() called from > acpi_early_init(), which was before acpi_bus_init() that called > acpi_ec_ecdt_probe(), in fact was not there at all since commit > 5a8361f7ecce. Thus the code removed by commit d737f333b211 from > acpi_early_init() was actually dead already at that time and > acpi_ec_ecdt_probe() was always called before acpi_load_tables() even > before commit d737f333b211 and that commit couldn't break it. > > Hence, the only other thing done by commit d737f333b211, which was to > move the acpi_ec_ecdt_probe() to acpi_early_init() was wrong and so > reverting that part of commit d737f333b211 should fix the problem. Well, it looks like commit 5a8361f7ecce is the first bad one at least in the first bug listed by your patch, so indeed the case that we assume to be failing here appears to be the failing one on that system. So the problem appears to be that before commit 5a8361f7ecce on some platforms acpi_load_tables() was actually called before acpi_ec_ecdt_probe() despite what the comment next to the latter says. So, on those platforms running acpi_ec_ecdt_probe() after acpi_load_tables() is fine, but question is if it also is fine on the other platforms. Before commit 5a8361f7ecce acpi_gbl_group_module_level_code and acpi_gbl_parse_table_as_term_list were used for deciding whether or not to call acpi_load_tables() from acpi_early_init(). Then, acpi_gbl_group_module_level_code was always "true" and acpi_gbl_parse_table_as_term_list was "false" on all platforms except for the ones included in acpi_quirks_dmi_table[]. Hence, acpi_load_tables() was actually loaded from acpi_early_init(), i.e. before acpi_ec_ecdt_probe(), on all platforms except the ones from acpi_quirks_dmi_table[], so it should be safe to move acpi_ec_ecdt_probe() after acpi_load_tables() on any platform that was not in acpi_quirks_dmi_table[] before commit 5a8361f7ecce. If it turns out to not work on the platforms that were in acpi_quirks_dmi_table[] before commit 5a8361f7ecce, we can address that by adding the DMI quirks table back.
diff --git a/drivers/acpi/acpica/acevents.h b/drivers/acpi/acpica/acevents.h index b412aa9..2f43ab0 100644 --- a/drivers/acpi/acpica/acevents.h +++ b/drivers/acpi/acpica/acevents.h @@ -155,8 +155,8 @@ union acpi_operand_object *acpi_ev_find_region_handler(acpi_adr_space_type *handler_obj); u8 -acpi_ev_has_default_handler(struct acpi_namespace_node *node, - acpi_adr_space_type space_id); +acpi_ev_has_handler(struct acpi_namespace_node *node, + acpi_adr_space_type space_id, bool default_handler); acpi_status acpi_ev_install_region_handlers(void); diff --git a/drivers/acpi/acpica/evhandler.c b/drivers/acpi/acpica/evhandler.c index 4ed1e67..c6d61fd 100644 --- a/drivers/acpi/acpica/evhandler.c +++ b/drivers/acpi/acpica/evhandler.c @@ -102,21 +102,22 @@ acpi_status acpi_ev_install_region_handlers(void) /******************************************************************************* * - * FUNCTION: acpi_ev_has_default_handler + * FUNCTION: acpi_ev_has_handler * * PARAMETERS: node - Namespace node for the device * space_id - The address space ID + * default_handler - check for default handler * - * RETURN: TRUE if default handler is installed, FALSE otherwise + * RETURN: TRUE if handler is installed, FALSE otherwise * - * DESCRIPTION: Check if the default handler is installed for the requested + * DESCRIPTION: Check if the handler is installed for the requested * space ID. * ******************************************************************************/ u8 -acpi_ev_has_default_handler(struct acpi_namespace_node *node, - acpi_adr_space_type space_id) +acpi_ev_has_handler(struct acpi_namespace_node *node, + acpi_adr_space_type space_id, bool default_handler) { union acpi_operand_object *obj_desc; union acpi_operand_object *handler_obj; @@ -131,8 +132,14 @@ acpi_ev_has_default_handler(struct acpi_namespace_node *node, while (handler_obj) { if (handler_obj->address_space.space_id == space_id) { - if (handler_obj->address_space.handler_flags & - ACPI_ADDR_HANDLER_DEFAULT_INSTALLED) { + if (default_handler && + (handler_obj->address_space.handler_flags & + ACPI_ADDR_HANDLER_DEFAULT_INSTALLED)) { + return (TRUE); + } + if (!default_handler && + !(handler_obj->address_space.handler_flags & + ACPI_ADDR_HANDLER_DEFAULT_INSTALLED)) { return (TRUE); } } diff --git a/drivers/acpi/acpica/evregion.c b/drivers/acpi/acpica/evregion.c index 49decca..2b04cf5 100644 --- a/drivers/acpi/acpica/evregion.c +++ b/drivers/acpi/acpica/evregion.c @@ -60,15 +60,21 @@ acpi_status acpi_ev_initialize_op_regions(void) * default, the _REG methods will have already been run (when the * handler was installed) */ - if (acpi_ev_has_default_handler(acpi_gbl_root_node, + if (acpi_ev_has_handler(acpi_gbl_root_node, acpi_gbl_default_address_spaces - [i])) { + [i], true)) { acpi_ev_execute_reg_methods(acpi_gbl_root_node, acpi_gbl_default_address_spaces [i], ACPI_REG_CONNECT); } } + /* Run _REG methods for EC Operation Region if ECDT has been probed */ + if (acpi_ev_has_handler(acpi_gbl_root_node, ACPI_ADR_SPACE_EC, false)) + acpi_ev_execute_reg_methods(acpi_gbl_root_node, + ACPI_ADR_SPACE_EC, + ACPI_REG_CONNECT); + (void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE); return_ACPI_STATUS(status); }