Patchwork ACPI/ACPICA: Run EC _REG explicitly for ECDT

login
register
mail settings
Submitter Rafael J. Wysocki
Date Jan. 7, 2019, 10:32 p.m.
Message ID <3442096.OMnbYpCFS6@aspire.rjw.lan>
Download mbox | patch
Permalink /patch/694317/
State New
Headers show

Comments

Rafael J. Wysocki - Jan. 7, 2019, 10:32 p.m.
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(-)
Rafael J. Wysocki - Jan. 8, 2019, 10:15 a.m.
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.
Rafael J. Wysocki - Jan. 8, 2019, 2:20 p.m.
"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.

Patch

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