Patchwork ACPI/ACPICA: Run EC _REG explicitly for ECDT

login
register
mail settings
Submitter Zhang, Rui
Date Jan. 5, 2019, 1:17 p.m.
Message ID <1546694240.2072.74.camel@intel.com>
Download mbox | patch
Permalink /patch/693325/
State New
Headers show

Comments

Zhang, Rui - Jan. 5, 2019, 1:17 p.m.
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.

Fixes: d737f333b211 ("ACPI: probe ECDT before loading AML tables regardless of module-level code flag")
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199981
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=201351
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=200011
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=200141
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=201679
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/acpica/acevents.h  |  4 ++--
 drivers/acpi/acpica/evhandler.c | 21 ++++++++++++++-------
 drivers/acpi/acpica/evregion.c  | 10 ++++++++--
 3 files changed, 24 insertions(+), 11 deletions(-)

-- 
2.7.4
Rafael J. Wysocki - Jan. 7, 2019, 11:51 a.m.
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).
Rafael J. Wysocki - Jan. 7, 2019, 12:43 p.m.
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?

Patch

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);
 }