diff mbox series

[RFT] ACPI: EC: Look for ECDT EC after calling acpi_load_tables()

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

Commit Message

Rafael J. Wysocki Jan. 8, 2019, 11:34 p.m. UTC
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>
---
 drivers/acpi/bus.c |   24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

Comments

Zhang Rui Jan. 10, 2019, 9:43 a.m. UTC | #1
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
>
Rafael J. Wysocki Jan. 10, 2019, 9:48 a.m. UTC | #2
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!
diff mbox series

Patch

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