| Message ID | 4426745.BlFkQnxG1M@aspire.rjw.lan (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Rafael Wysocki |
| Headers | show |
| Series | [RFT] ACPI: EC: Look for ECDT EC after calling acpi_load_tables() | expand |
On 三, 2019-01-09 at 00:34 +0100, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Some systems have had functional issues since commit 5a8361f7ecce > (ACPICA: Integrate package handling with module-level code) that, > among other things, changed the initial values of the > acpi_gbl_group_module_level_code and > acpi_gbl_parse_table_as_term_list > global flags in ACPICA which implicitly caused acpi_ec_ecdt_probe() > to > be called before acpi_load_tables() on the vast majority of > platforms. > > Namely, before commit 5a8361f7ecce, acpi_load_tables() was called > from > acpi_early_init() if acpi_gbl_parse_table_as_term_list was FALSE and > acpi_gbl_group_module_level_code was TRUE, which almost always was > the case as FALSE and TRUE were their initial values, respectively. > The acpi_gbl_parse_table_as_term_list value would be changed to TRUE > for a couple of platforms in acpi_quirks_dmi_table[], but it remained > FALSE in the vast majority of cases. > > After commit 5a8361f7ecce, the initial values of the two flags have > been reversed, so in effect acpi_load_tables() has not been called > from acpi_early_init() any more. That, in turn, affects > acpi_ec_ecdt_probe() which is invoked before acpi_load_tables() now > and it is not possible to evaluate the _REG method for the EC address > space handler installed by it. That effectively causes the EC > address > space to be inaccessible to AML on platforms with an ECDT matching > the > EC device definition in the DSDT and functional problems ensue in > there. > > Because the default behavior before commit 5a8361f7ecce was to call > acpi_ec_ecdt_probe() after acpi_load_tables(), it should be safe to > do that again. Moreover, the EC address space handler installed by > acpi_ec_ecdt_probe() is only needed for AML to be able to access the > EC address space and the only AML that can run during > acpi_load_tables() > is module-level code which only is allowed to access address spaces > with default handlers (memory, I/O and PCI config space). > > For this reason, move the acpi_ec_ecdt_probe() invocation back to > acpi_bus_init(), from where it was taken away by commit d737f333b211 > (ACPI: probe ECDT before loading AML tables regardless of module- > level > code flag), and put it after the invocation of acpi_load_tables() to > restore the original code ordering from before commit 5a8361f7ecce. > > Fixes: 5a8361f7ecce (ACPICA: Integrate package handling with module- > level code) > Link: https://bugzilla.kernel.org/show_bug.cgi?id=199981 > Reported-by: step-ali <sunmooon15@gmail.com> > Reported-by: Charles Stanhope <charles.stanhope@gmail.com> > Reported-by: Paulo Nascimento <paulo.ulusu@googlemail.com> > Reported-by: David Purton <dcpurton@marshwiggle.net> > Reported-by: Adam Harvey <adam@adamharvey.name> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> I have tested this on a Lenovo laptops with ECDT, on top of 5.0-rc1, and confirms that EC _REG is evaluated with the patch, and is not evaluated without the patch. Let me update the patch link to the bugzilla to get more test. thanks, rui > --- > drivers/acpi/bus.c | 24 ++++++++++++------------ > 1 file changed, 12 insertions(+), 12 deletions(-) > > Index: linux-pm/drivers/acpi/bus.c > =================================================================== > --- linux-pm.orig/drivers/acpi/bus.c > +++ linux-pm/drivers/acpi/bus.c > @@ -1054,18 +1054,6 @@ void __init acpi_early_init(void) > goto error0; > } > > - /* > - * ACPI 2.0 requires the EC driver to be loaded and work > before > - * the EC device is found in the namespace (i.e. before > - * acpi_load_tables() is called). > - * > - * This is accomplished by looking for the ECDT table, and > getting > - * the EC parameters out of that. > - * > - * Ignore the result. Not having an ECDT is not fatal. > - */ > - status = acpi_ec_ecdt_probe(); > - > #ifdef CONFIG_X86 > if (!acpi_ioapic) { > /* compatible (0) means level (3) */ > @@ -1142,6 +1130,18 @@ static int __init acpi_bus_init(void) > goto error1; > } > > + /* > + * ACPI 2.0 requires the EC driver to be loaded and work > before the EC > + * device is found in the namespace. > + * > + * This is accomplished by looking for the ECDT table and > getting the EC > + * parameters out of that. > + * > + * Do that before calling acpi_initialize_objects() which > may trigger EC > + * address space accesses. > + */ > + acpi_ec_ecdt_probe(); > + > status = acpi_enable_subsystem(ACPI_NO_ACPI_ENABLE); > if (ACPI_FAILURE(status)) { > printk(KERN_ERR PREFIX >
On Thu, Jan 10, 2019 at 10:43 AM Zhang Rui <rui.zhang@intel.com> wrote: > > On 三, 2019-01-09 at 00:34 +0100, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Some systems have had functional issues since commit 5a8361f7ecce > > (ACPICA: Integrate package handling with module-level code) that, > > among other things, changed the initial values of the > > acpi_gbl_group_module_level_code and > > acpi_gbl_parse_table_as_term_list > > global flags in ACPICA which implicitly caused acpi_ec_ecdt_probe() > > to > > be called before acpi_load_tables() on the vast majority of > > platforms. > > > > Namely, before commit 5a8361f7ecce, acpi_load_tables() was called > > from > > acpi_early_init() if acpi_gbl_parse_table_as_term_list was FALSE and > > acpi_gbl_group_module_level_code was TRUE, which almost always was > > the case as FALSE and TRUE were their initial values, respectively. > > The acpi_gbl_parse_table_as_term_list value would be changed to TRUE > > for a couple of platforms in acpi_quirks_dmi_table[], but it remained > > FALSE in the vast majority of cases. > > > > After commit 5a8361f7ecce, the initial values of the two flags have > > been reversed, so in effect acpi_load_tables() has not been called > > from acpi_early_init() any more. That, in turn, affects > > acpi_ec_ecdt_probe() which is invoked before acpi_load_tables() now > > and it is not possible to evaluate the _REG method for the EC address > > space handler installed by it. That effectively causes the EC > > address > > space to be inaccessible to AML on platforms with an ECDT matching > > the > > EC device definition in the DSDT and functional problems ensue in > > there. > > > > Because the default behavior before commit 5a8361f7ecce was to call > > acpi_ec_ecdt_probe() after acpi_load_tables(), it should be safe to > > do that again. Moreover, the EC address space handler installed by > > acpi_ec_ecdt_probe() is only needed for AML to be able to access the > > EC address space and the only AML that can run during > > acpi_load_tables() > > is module-level code which only is allowed to access address spaces > > with default handlers (memory, I/O and PCI config space). > > > > For this reason, move the acpi_ec_ecdt_probe() invocation back to > > acpi_bus_init(), from where it was taken away by commit d737f333b211 > > (ACPI: probe ECDT before loading AML tables regardless of module- > > level > > code flag), and put it after the invocation of acpi_load_tables() to > > restore the original code ordering from before commit 5a8361f7ecce. > > > > Fixes: 5a8361f7ecce (ACPICA: Integrate package handling with module- > > level code) > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=199981 > > Reported-by: step-ali <sunmooon15@gmail.com> > > Reported-by: Charles Stanhope <charles.stanhope@gmail.com> > > Reported-by: Paulo Nascimento <paulo.ulusu@googlemail.com> > > Reported-by: David Purton <dcpurton@marshwiggle.net> > > Reported-by: Adam Harvey <adam@adamharvey.name> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > I have tested this on a Lenovo laptops with ECDT, on top of 5.0-rc1, > and confirms that EC _REG is evaluated with the patch, and is not > evaluated without the patch. > > Let me update the patch link to the bugzilla to get more test. Thank you!
Index: linux-pm/drivers/acpi/bus.c =================================================================== --- linux-pm.orig/drivers/acpi/bus.c +++ linux-pm/drivers/acpi/bus.c @@ -1054,18 +1054,6 @@ void __init acpi_early_init(void) goto error0; } - /* - * ACPI 2.0 requires the EC driver to be loaded and work before - * the EC device is found in the namespace (i.e. before - * acpi_load_tables() is called). - * - * This is accomplished by looking for the ECDT table, and getting - * the EC parameters out of that. - * - * Ignore the result. Not having an ECDT is not fatal. - */ - status = acpi_ec_ecdt_probe(); - #ifdef CONFIG_X86 if (!acpi_ioapic) { /* compatible (0) means level (3) */ @@ -1142,6 +1130,18 @@ static int __init acpi_bus_init(void) goto error1; } + /* + * ACPI 2.0 requires the EC driver to be loaded and work before the EC + * device is found in the namespace. + * + * This is accomplished by looking for the ECDT table and getting the EC + * parameters out of that. + * + * Do that before calling acpi_initialize_objects() which may trigger EC + * address space accesses. + */ + acpi_ec_ecdt_probe(); + status = acpi_enable_subsystem(ACPI_NO_ACPI_ENABLE); if (ACPI_FAILURE(status)) { printk(KERN_ERR PREFIX