| Message ID | 20201025094542.3669-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | ACPICA: Also handle "orphan" _REG methods for GPIO OpRegions | expand |
> -----Original Message----- > From: Hans de Goede <hdegoede@redhat.com> > Sent: Sunday, October 25, 2020 2:46 AM > To: Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>; > Moore, Robert <robert.moore@intel.com>; Kaneda, Erik > <erik.kaneda@intel.com> > Cc: Hans de Goede <hdegoede@redhat.com>; linux-acpi@vger.kernel.org; > devel@acpica.org > Subject: [PATCH] ACPICA: Also handle "orphan" _REG methods for GPIO > OpRegions > > Before this commit acpi_ev_execute_reg_methods() had special handling > to handle "orphan" (no matching OpRegion declared) _REG methods for EC > nodes. > > On Intel Cherry Trail devices there are 2 possible ACPI OpRegions for > accessing GPIOs. The standard GeneralPurposeIo OpRegion and the Cherry > Trail specific UserDefined 0x9X OpRegions. > > Having 2 different types of OpRegions leads to potential issues with > checks for OpRegion availability, or in other words checks if _REG has > been called for the OpRegion which the ACPI code wants to use. > > Except for the "orphan" EC handling, ACPICA core does not call _REG on > an ACPI node which does not define an OpRegion matching the type being > registered; and the reference design DSDT, from which most Cherry Trail > DSDTs are derived, does not define GeneralPurposeIo, nor > UserDefined(0x93) > OpRegions for the GPO2 (UID 3) device, because no pins were assigned ACPI > controlled functions in the reference design. > > Together this leads to the perfect storm, at least on the Cherry Trail > based Medion Akayo E1239T. This design does use a GPO2 pin from its ACPI > code and has added the Cherry Trail specific UserDefined(0x93) opregion > to its GPO2 ACPI node to access this pin. > > But it uses a has _REG been called availability check for the standard > GeneralPurposeIo OpRegion. This clearly is a bug in the DSDT, but this > does work under Windows. This issue leads to the intel_vbtn driver > reporting the device always being in tablet-mode at boot, even if it > is in laptop mode. Which in turn causes userspace to ignore touchpad > events. So iow this issues causes the touchpad to not work at boot. > > This commit fixes this by extending the "orphan" _REG method handling > to also apply to GPIO address-space handlers. > > Note it seems that Windows always calls "orphan" _REG methods so me > may want to consider dropping the space-id check and always do > "orphan" _REG method handling. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/acpi/acpica/evregion.c | 54 +++++++++++++++++----------------- > 1 file changed, 27 insertions(+), 27 deletions(-) > > diff --git a/drivers/acpi/acpica/evregion.c b/drivers/acpi/acpica/evregion.c > index 738d4b231f34..21ff341e34a4 100644 > --- a/drivers/acpi/acpica/evregion.c > +++ b/drivers/acpi/acpica/evregion.c > @@ -21,7 +21,8 @@ extern u8 acpi_gbl_default_address_spaces[]; > /* Local prototypes */ > > static void > -acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node > *ec_device_node); > +acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node > *device_node, > + acpi_adr_space_type space_id); > > static acpi_status > acpi_ev_reg_run(acpi_handle obj_handle, > @@ -684,10 +685,12 @@ acpi_ev_execute_reg_methods(struct > acpi_namespace_node *node, > ACPI_NS_WALK_UNLOCK, > acpi_ev_reg_run, NULL, > &info, NULL); > > - /* Special case for EC: handle "orphan" _REG methods with no region > */ > - > - if (space_id == ACPI_ADR_SPACE_EC) { > - acpi_ev_orphan_ec_reg_method(node); > + /* > + * Special case for EC and GPIO: handle "orphan" _REG methods with > + * no region. > + */ > + if (space_id == ACPI_ADR_SPACE_EC || space_id == > ACPI_ADR_SPACE_GPIO) { > + acpi_ev_execute_orphan_reg_method(node, space_id); > } > > ACPI_DEBUG_PRINT_RAW((ACPI_DB_NAMES, > @@ -760,31 +763,28 @@ acpi_ev_reg_run(acpi_handle obj_handle, > > > /********************************************************** > ********************* > * > - * FUNCTION: acpi_ev_orphan_ec_reg_method > + * FUNCTION: acpi_ev_execute_orphan_reg_method > * > - * PARAMETERS: ec_device_node - Namespace node for an EC device > + * PARAMETERS: device_node - Namespace node for an ACPI device > + * space_id - The address space ID > * > * RETURN: None > * > - * DESCRIPTION: Execute an "orphan" _REG method that appears under the > EC > + * DESCRIPTION: Execute an "orphan" _REG method that appears under an > ACPI > * device. This is a _REG method that has no corresponding region > - * within the EC device scope. The orphan _REG method appears to > - * have been enabled by the description of the ECDT in the ACPI > - * specification: "The availability of the region space can be > - * detected by providing a _REG method object underneath the > - * Embedded Controller device." > - * > - * To quickly access the EC device, we use the ec_device_node used > - * during EC handler installation. Otherwise, we would need to > - * perform a time consuming namespace walk, executing _HID > - * methods to find the EC device. > + * within the device's scope. ACPI tables depending on these > + * "orphan" _REG methods have been seen for both EC and GPIO > + * Operation Regions. Presumably the Windows ACPI implementation > + * always calls the _REG method independent of the presence of > + * an actual Operation Region with the correct address space ID. > * > * MUTEX: Assumes the namespace is locked > * > > ********************************************************** > ********************/ > > static void > -acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node > *ec_device_node) > +acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node > *device_node, > + acpi_adr_space_type space_id) > { > acpi_handle reg_method; > struct acpi_namespace_node *next_node; > @@ -792,9 +792,9 @@ acpi_ev_orphan_ec_reg_method(struct > acpi_namespace_node *ec_device_node) > struct acpi_object_list args; > union acpi_object objects[2]; > > - ACPI_FUNCTION_TRACE(ev_orphan_ec_reg_method); > + ACPI_FUNCTION_TRACE(ev_execute_orphan_reg_method); > > - if (!ec_device_node) { > + if (!device_node) { > return_VOID; > } > > @@ -804,7 +804,7 @@ acpi_ev_orphan_ec_reg_method(struct > acpi_namespace_node *ec_device_node) > > /* Get a handle to a _REG method immediately under the EC device > */ > > - status = acpi_get_handle(ec_device_node, METHOD_NAME__REG, > ®_method); > + status = acpi_get_handle(device_node, METHOD_NAME__REG, > ®_method); > if (ACPI_FAILURE(status)) { > goto exit; /* There is no _REG method present */ > } > @@ -816,23 +816,23 @@ acpi_ev_orphan_ec_reg_method(struct > acpi_namespace_node *ec_device_node) > * with other space IDs to be present; but the code below will then > * execute the _REG method with the embedded_control space_ID > argument. > */ > - next_node = acpi_ns_get_next_node(ec_device_node, NULL); > + next_node = acpi_ns_get_next_node(device_node, NULL); > while (next_node) { > if ((next_node->type == ACPI_TYPE_REGION) && > (next_node->object) && > - (next_node->object->region.space_id == > ACPI_ADR_SPACE_EC)) { > + (next_node->object->region.space_id == space_id)) { > goto exit; /* Do not execute the _REG */ > } > > - next_node = acpi_ns_get_next_node(ec_device_node, > next_node); > + next_node = acpi_ns_get_next_node(device_node, > next_node); > } > > - /* Evaluate the _REG(embedded_control,Connect) method */ > + /* Evaluate the _REG(space_id, Connect) method */ > > args.count = 2; > args.pointer = objects; > objects[0].type = ACPI_TYPE_INTEGER; > - objects[0].integer.value = ACPI_ADR_SPACE_EC; > + objects[0].integer.value = space_id; > objects[1].type = ACPI_TYPE_INTEGER; > objects[1].integer.value = ACPI_REG_CONNECT; > > -- > 2.28.0 This looks good to me. Bob, any thoughts? Erik
Looks OK to me. Bob -----Original Message----- From: Kaneda, Erik <erik.kaneda@intel.com> Sent: Monday, October 26, 2020 1:56 PM To: Hans de Goede <hdegoede@redhat.com>; Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>; Moore, Robert <robert.moore@intel.com> Cc: linux-acpi@vger.kernel.org; devel@acpica.org Subject: RE: [PATCH] ACPICA: Also handle "orphan" _REG methods for GPIO OpRegions > -----Original Message----- > From: Hans de Goede <hdegoede@redhat.com> > Sent: Sunday, October 25, 2020 2:46 AM > To: Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown > <lenb@kernel.org>; Moore, Robert <robert.moore@intel.com>; Kaneda, > Erik <erik.kaneda@intel.com> > Cc: Hans de Goede <hdegoede@redhat.com>; linux-acpi@vger.kernel.org; > devel@acpica.org > Subject: [PATCH] ACPICA: Also handle "orphan" _REG methods for GPIO > OpRegions > > Before this commit acpi_ev_execute_reg_methods() had special handling > to handle "orphan" (no matching OpRegion declared) _REG methods for EC > nodes. > > On Intel Cherry Trail devices there are 2 possible ACPI OpRegions for > accessing GPIOs. The standard GeneralPurposeIo OpRegion and the Cherry > Trail specific UserDefined 0x9X OpRegions. > > Having 2 different types of OpRegions leads to potential issues with > checks for OpRegion availability, or in other words checks if _REG has > been called for the OpRegion which the ACPI code wants to use. > > Except for the "orphan" EC handling, ACPICA core does not call _REG on > an ACPI node which does not define an OpRegion matching the type being > registered; and the reference design DSDT, from which most Cherry > Trail DSDTs are derived, does not define GeneralPurposeIo, nor > UserDefined(0x93) > OpRegions for the GPO2 (UID 3) device, because no pins were assigned > ACPI controlled functions in the reference design. > > Together this leads to the perfect storm, at least on the Cherry Trail > based Medion Akayo E1239T. This design does use a GPO2 pin from its > ACPI code and has added the Cherry Trail specific UserDefined(0x93) > opregion to its GPO2 ACPI node to access this pin. > > But it uses a has _REG been called availability check for the standard > GeneralPurposeIo OpRegion. This clearly is a bug in the DSDT, but this > does work under Windows. This issue leads to the intel_vbtn driver > reporting the device always being in tablet-mode at boot, even if it > is in laptop mode. Which in turn causes userspace to ignore touchpad > events. So iow this issues causes the touchpad to not work at boot. > > This commit fixes this by extending the "orphan" _REG method handling > to also apply to GPIO address-space handlers. > > Note it seems that Windows always calls "orphan" _REG methods so me > may want to consider dropping the space-id check and always do > "orphan" _REG method handling. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/acpi/acpica/evregion.c | 54 > +++++++++++++++++----------------- > 1 file changed, 27 insertions(+), 27 deletions(-) > > diff --git a/drivers/acpi/acpica/evregion.c > b/drivers/acpi/acpica/evregion.c index 738d4b231f34..21ff341e34a4 > 100644 > --- a/drivers/acpi/acpica/evregion.c > +++ b/drivers/acpi/acpica/evregion.c > @@ -21,7 +21,8 @@ extern u8 acpi_gbl_default_address_spaces[]; > /* Local prototypes */ > > static void > -acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node > *ec_device_node); > +acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node > *device_node, > + acpi_adr_space_type space_id); > > static acpi_status > acpi_ev_reg_run(acpi_handle obj_handle, @@ -684,10 +685,12 @@ > acpi_ev_execute_reg_methods(struct > acpi_namespace_node *node, > ACPI_NS_WALK_UNLOCK, > acpi_ev_reg_run, NULL, > &info, NULL); > > - /* Special case for EC: handle "orphan" _REG methods with no region > */ > - > - if (space_id == ACPI_ADR_SPACE_EC) { > - acpi_ev_orphan_ec_reg_method(node); > + /* > + * Special case for EC and GPIO: handle "orphan" _REG methods with > + * no region. > + */ > + if (space_id == ACPI_ADR_SPACE_EC || space_id == > ACPI_ADR_SPACE_GPIO) { > + acpi_ev_execute_orphan_reg_method(node, space_id); > } > > ACPI_DEBUG_PRINT_RAW((ACPI_DB_NAMES, > @@ -760,31 +763,28 @@ acpi_ev_reg_run(acpi_handle obj_handle, > > > /********************************************************** > ********************* > * > - * FUNCTION: acpi_ev_orphan_ec_reg_method > + * FUNCTION: acpi_ev_execute_orphan_reg_method > * > - * PARAMETERS: ec_device_node - Namespace node for an EC device > + * PARAMETERS: device_node - Namespace node for an ACPI device > + * space_id - The address space ID > * > * RETURN: None > * > - * DESCRIPTION: Execute an "orphan" _REG method that appears under > the EC > + * DESCRIPTION: Execute an "orphan" _REG method that appears under an > ACPI > * device. This is a _REG method that has no corresponding region > - * within the EC device scope. The orphan _REG method appears to > - * have been enabled by the description of the ECDT in the ACPI > - * specification: "The availability of the region space can be > - * detected by providing a _REG method object underneath the > - * Embedded Controller device." > - * > - * To quickly access the EC device, we use the ec_device_node used > - * during EC handler installation. Otherwise, we would need to > - * perform a time consuming namespace walk, executing _HID > - * methods to find the EC device. > + * within the device's scope. ACPI tables depending on these > + * "orphan" _REG methods have been seen for both EC and GPIO > + * Operation Regions. Presumably the Windows ACPI implementation > + * always calls the _REG method independent of the presence of > + * an actual Operation Region with the correct address space ID. > * > * MUTEX: Assumes the namespace is locked > * > > ********************************************************** > ********************/ > > static void > -acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node > *ec_device_node) > +acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node > *device_node, > + acpi_adr_space_type space_id) > { > acpi_handle reg_method; > struct acpi_namespace_node *next_node; @@ -792,9 +792,9 @@ > acpi_ev_orphan_ec_reg_method(struct > acpi_namespace_node *ec_device_node) > struct acpi_object_list args; > union acpi_object objects[2]; > > - ACPI_FUNCTION_TRACE(ev_orphan_ec_reg_method); > + ACPI_FUNCTION_TRACE(ev_execute_orphan_reg_method); > > - if (!ec_device_node) { > + if (!device_node) { > return_VOID; > } > > @@ -804,7 +804,7 @@ acpi_ev_orphan_ec_reg_method(struct > acpi_namespace_node *ec_device_node) > > /* Get a handle to a _REG method immediately under the EC device */ > > - status = acpi_get_handle(ec_device_node, METHOD_NAME__REG, > ®_method); > + status = acpi_get_handle(device_node, METHOD_NAME__REG, > ®_method); > if (ACPI_FAILURE(status)) { > goto exit; /* There is no _REG method present */ > } > @@ -816,23 +816,23 @@ acpi_ev_orphan_ec_reg_method(struct > acpi_namespace_node *ec_device_node) > * with other space IDs to be present; but the code below will then > * execute the _REG method with the embedded_control space_ID > argument. > */ > - next_node = acpi_ns_get_next_node(ec_device_node, NULL); > + next_node = acpi_ns_get_next_node(device_node, NULL); > while (next_node) { > if ((next_node->type == ACPI_TYPE_REGION) && > (next_node->object) && > - (next_node->object->region.space_id == > ACPI_ADR_SPACE_EC)) { > + (next_node->object->region.space_id == space_id)) { > goto exit; /* Do not execute the _REG */ > } > > - next_node = acpi_ns_get_next_node(ec_device_node, > next_node); > + next_node = acpi_ns_get_next_node(device_node, > next_node); > } > > - /* Evaluate the _REG(embedded_control,Connect) method */ > + /* Evaluate the _REG(space_id, Connect) method */ > > args.count = 2; > args.pointer = objects; > objects[0].type = ACPI_TYPE_INTEGER; > - objects[0].integer.value = ACPI_ADR_SPACE_EC; > + objects[0].integer.value = space_id; > objects[1].type = ACPI_TYPE_INTEGER; > objects[1].integer.value = ACPI_REG_CONNECT; > > -- > 2.28.0 This looks good to me. Bob, any thoughts? Erik
> -----Original Message----- > From: Moore, Robert <robert.moore@intel.com> > Sent: Tuesday, October 27, 2020 7:17 AM > To: Kaneda, Erik <erik.kaneda@intel.com>; Hans de Goede > <hdegoede@redhat.com>; Rafael J . Wysocki <rjw@rjwysocki.net>; Len > Brown <lenb@kernel.org> > Cc: linux-acpi@vger.kernel.org; devel@acpica.org > Subject: RE: [PATCH] ACPICA: Also handle "orphan" _REG methods for GPIO > OpRegions > > Looks OK to me. Ok, I'll make a pull request of this to ACPICA on behalf of Hans and it will be in the next ACPICA release. Rafael, let me know if you have any issues with this. Thanks, Erik > Bob > > > -----Original Message----- > From: Kaneda, Erik <erik.kaneda@intel.com> > Sent: Monday, October 26, 2020 1:56 PM > To: Hans de Goede <hdegoede@redhat.com>; Rafael J . Wysocki > <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>; Moore, Robert > <robert.moore@intel.com> > Cc: linux-acpi@vger.kernel.org; devel@acpica.org > Subject: RE: [PATCH] ACPICA: Also handle "orphan" _REG methods for GPIO > OpRegions > > > > > -----Original Message----- > > From: Hans de Goede <hdegoede@redhat.com> > > Sent: Sunday, October 25, 2020 2:46 AM > > To: Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown > > <lenb@kernel.org>; Moore, Robert <robert.moore@intel.com>; Kaneda, > > Erik <erik.kaneda@intel.com> > > Cc: Hans de Goede <hdegoede@redhat.com>; linux-acpi@vger.kernel.org; > > devel@acpica.org > > Subject: [PATCH] ACPICA: Also handle "orphan" _REG methods for GPIO > > OpRegions > > > > Before this commit acpi_ev_execute_reg_methods() had special handling > > to handle "orphan" (no matching OpRegion declared) _REG methods for EC > > nodes. > > > > On Intel Cherry Trail devices there are 2 possible ACPI OpRegions for > > accessing GPIOs. The standard GeneralPurposeIo OpRegion and the Cherry > > Trail specific UserDefined 0x9X OpRegions. > > > > Having 2 different types of OpRegions leads to potential issues with > > checks for OpRegion availability, or in other words checks if _REG has > > been called for the OpRegion which the ACPI code wants to use. > > > > Except for the "orphan" EC handling, ACPICA core does not call _REG on > > an ACPI node which does not define an OpRegion matching the type being > > registered; and the reference design DSDT, from which most Cherry > > Trail DSDTs are derived, does not define GeneralPurposeIo, nor > > UserDefined(0x93) > > OpRegions for the GPO2 (UID 3) device, because no pins were assigned > > ACPI controlled functions in the reference design. > > > > Together this leads to the perfect storm, at least on the Cherry Trail > > based Medion Akayo E1239T. This design does use a GPO2 pin from its > > ACPI code and has added the Cherry Trail specific UserDefined(0x93) > > opregion to its GPO2 ACPI node to access this pin. > > > > But it uses a has _REG been called availability check for the standard > > GeneralPurposeIo OpRegion. This clearly is a bug in the DSDT, but this > > does work under Windows. This issue leads to the intel_vbtn driver > > reporting the device always being in tablet-mode at boot, even if it > > is in laptop mode. Which in turn causes userspace to ignore touchpad > > events. So iow this issues causes the touchpad to not work at boot. > > > > This commit fixes this by extending the "orphan" _REG method handling > > to also apply to GPIO address-space handlers. > > > > Note it seems that Windows always calls "orphan" _REG methods so me > > may want to consider dropping the space-id check and always do > > "orphan" _REG method handling. > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > --- > > drivers/acpi/acpica/evregion.c | 54 > > +++++++++++++++++----------------- > > 1 file changed, 27 insertions(+), 27 deletions(-) > > > > diff --git a/drivers/acpi/acpica/evregion.c > > b/drivers/acpi/acpica/evregion.c index 738d4b231f34..21ff341e34a4 > > 100644 > > --- a/drivers/acpi/acpica/evregion.c > > +++ b/drivers/acpi/acpica/evregion.c > > @@ -21,7 +21,8 @@ extern u8 acpi_gbl_default_address_spaces[]; > > /* Local prototypes */ > > > > static void > > -acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node > > *ec_device_node); > > +acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node > > *device_node, > > + acpi_adr_space_type space_id); > > > > static acpi_status > > acpi_ev_reg_run(acpi_handle obj_handle, @@ -684,10 +685,12 @@ > > acpi_ev_execute_reg_methods(struct > > acpi_namespace_node *node, > > ACPI_NS_WALK_UNLOCK, > > acpi_ev_reg_run, NULL, > > &info, NULL); > > > > - /* Special case for EC: handle "orphan" _REG methods with no region > > */ > > - > > - if (space_id == ACPI_ADR_SPACE_EC) { > > - acpi_ev_orphan_ec_reg_method(node); > > + /* > > + * Special case for EC and GPIO: handle "orphan" _REG methods with > > + * no region. > > + */ > > + if (space_id == ACPI_ADR_SPACE_EC || space_id == > > ACPI_ADR_SPACE_GPIO) { > > + acpi_ev_execute_orphan_reg_method(node, space_id); > > } > > > > ACPI_DEBUG_PRINT_RAW((ACPI_DB_NAMES, > > @@ -760,31 +763,28 @@ acpi_ev_reg_run(acpi_handle obj_handle, > > > > > > > /********************************************************** > > ********************* > > * > > - * FUNCTION: acpi_ev_orphan_ec_reg_method > > + * FUNCTION: acpi_ev_execute_orphan_reg_method > > * > > - * PARAMETERS: ec_device_node - Namespace node for an EC device > > + * PARAMETERS: device_node - Namespace node for an ACPI device > > + * space_id - The address space ID > > * > > * RETURN: None > > * > > - * DESCRIPTION: Execute an "orphan" _REG method that appears under > > the EC > > + * DESCRIPTION: Execute an "orphan" _REG method that appears under > an > > ACPI > > * device. This is a _REG method that has no corresponding region > > - * within the EC device scope. The orphan _REG method appears to > > - * have been enabled by the description of the ECDT in the ACPI > > - * specification: "The availability of the region space can be > > - * detected by providing a _REG method object underneath the > > - * Embedded Controller device." > > - * > > - * To quickly access the EC device, we use the ec_device_node used > > - * during EC handler installation. Otherwise, we would need to > > - * perform a time consuming namespace walk, executing _HID > > - * methods to find the EC device. > > + * within the device's scope. ACPI tables depending on these > > + * "orphan" _REG methods have been seen for both EC and GPIO > > + * Operation Regions. Presumably the Windows ACPI > implementation > > + * always calls the _REG method independent of the presence of > > + * an actual Operation Region with the correct address space ID. > > * > > * MUTEX: Assumes the namespace is locked > > * > > > > > ********************************************************** > > ********************/ > > > > static void > > -acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node > > *ec_device_node) > > +acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node > > *device_node, > > + acpi_adr_space_type space_id) > > { > > acpi_handle reg_method; > > struct acpi_namespace_node *next_node; @@ -792,9 +792,9 @@ > > acpi_ev_orphan_ec_reg_method(struct > > acpi_namespace_node *ec_device_node) > > struct acpi_object_list args; > > union acpi_object objects[2]; > > > > - ACPI_FUNCTION_TRACE(ev_orphan_ec_reg_method); > > + ACPI_FUNCTION_TRACE(ev_execute_orphan_reg_method); > > > > - if (!ec_device_node) { > > + if (!device_node) { > > return_VOID; > > } > > > > @@ -804,7 +804,7 @@ acpi_ev_orphan_ec_reg_method(struct > > acpi_namespace_node *ec_device_node) > > > > /* Get a handle to a _REG method immediately under the EC device > */ > > > > - status = acpi_get_handle(ec_device_node, METHOD_NAME__REG, > > ®_method); > > + status = acpi_get_handle(device_node, METHOD_NAME__REG, > > ®_method); > > if (ACPI_FAILURE(status)) { > > goto exit; /* There is no _REG method present */ > > } > > @@ -816,23 +816,23 @@ acpi_ev_orphan_ec_reg_method(struct > > acpi_namespace_node *ec_device_node) > > * with other space IDs to be present; but the code below will then > > * execute the _REG method with the embedded_control space_ID > > argument. > > */ > > - next_node = acpi_ns_get_next_node(ec_device_node, NULL); > > + next_node = acpi_ns_get_next_node(device_node, NULL); > > while (next_node) { > > if ((next_node->type == ACPI_TYPE_REGION) && > > (next_node->object) && > > - (next_node->object->region.space_id == > > ACPI_ADR_SPACE_EC)) { > > + (next_node->object->region.space_id == space_id)) { > > goto exit; /* Do not execute the _REG */ > > } > > > > - next_node = acpi_ns_get_next_node(ec_device_node, > > next_node); > > + next_node = acpi_ns_get_next_node(device_node, > > next_node); > > } > > > > - /* Evaluate the _REG(embedded_control,Connect) method */ > > + /* Evaluate the _REG(space_id, Connect) method */ > > > > args.count = 2; > > args.pointer = objects; > > objects[0].type = ACPI_TYPE_INTEGER; > > - objects[0].integer.value = ACPI_ADR_SPACE_EC; > > + objects[0].integer.value = space_id; > > objects[1].type = ACPI_TYPE_INTEGER; > > objects[1].integer.value = ACPI_REG_CONNECT; > > > > -- > > 2.28.0 > > This looks good to me. Bob, any thoughts? > > Erik
Hi, On 10/27/20 6:43 PM, Kaneda, Erik wrote: > > >> -----Original Message----- >> From: Moore, Robert <robert.moore@intel.com> >> Sent: Tuesday, October 27, 2020 7:17 AM >> To: Kaneda, Erik <erik.kaneda@intel.com>; Hans de Goede >> <hdegoede@redhat.com>; Rafael J . Wysocki <rjw@rjwysocki.net>; Len >> Brown <lenb@kernel.org> >> Cc: linux-acpi@vger.kernel.org; devel@acpica.org >> Subject: RE: [PATCH] ACPICA: Also handle "orphan" _REG methods for GPIO >> OpRegions >> >> Looks OK to me. > > Ok, I'll make a pull request of this to ACPICA on behalf of Hans and it will be in the next ACPICA release. Great, thank you. Regards, Hans >> -----Original Message----- >> From: Kaneda, Erik <erik.kaneda@intel.com> >> Sent: Monday, October 26, 2020 1:56 PM >> To: Hans de Goede <hdegoede@redhat.com>; Rafael J . Wysocki >> <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>; Moore, Robert >> <robert.moore@intel.com> >> Cc: linux-acpi@vger.kernel.org; devel@acpica.org >> Subject: RE: [PATCH] ACPICA: Also handle "orphan" _REG methods for GPIO >> OpRegions >> >> >> >>> -----Original Message----- >>> From: Hans de Goede <hdegoede@redhat.com> >>> Sent: Sunday, October 25, 2020 2:46 AM >>> To: Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown >>> <lenb@kernel.org>; Moore, Robert <robert.moore@intel.com>; Kaneda, >>> Erik <erik.kaneda@intel.com> >>> Cc: Hans de Goede <hdegoede@redhat.com>; linux-acpi@vger.kernel.org; >>> devel@acpica.org >>> Subject: [PATCH] ACPICA: Also handle "orphan" _REG methods for GPIO >>> OpRegions >>> >>> Before this commit acpi_ev_execute_reg_methods() had special handling >>> to handle "orphan" (no matching OpRegion declared) _REG methods for EC >>> nodes. >>> >>> On Intel Cherry Trail devices there are 2 possible ACPI OpRegions for >>> accessing GPIOs. The standard GeneralPurposeIo OpRegion and the Cherry >>> Trail specific UserDefined 0x9X OpRegions. >>> >>> Having 2 different types of OpRegions leads to potential issues with >>> checks for OpRegion availability, or in other words checks if _REG has >>> been called for the OpRegion which the ACPI code wants to use. >>> >>> Except for the "orphan" EC handling, ACPICA core does not call _REG on >>> an ACPI node which does not define an OpRegion matching the type being >>> registered; and the reference design DSDT, from which most Cherry >>> Trail DSDTs are derived, does not define GeneralPurposeIo, nor >>> UserDefined(0x93) >>> OpRegions for the GPO2 (UID 3) device, because no pins were assigned >>> ACPI controlled functions in the reference design. >>> >>> Together this leads to the perfect storm, at least on the Cherry Trail >>> based Medion Akayo E1239T. This design does use a GPO2 pin from its >>> ACPI code and has added the Cherry Trail specific UserDefined(0x93) >>> opregion to its GPO2 ACPI node to access this pin. >>> >>> But it uses a has _REG been called availability check for the standard >>> GeneralPurposeIo OpRegion. This clearly is a bug in the DSDT, but this >>> does work under Windows. This issue leads to the intel_vbtn driver >>> reporting the device always being in tablet-mode at boot, even if it >>> is in laptop mode. Which in turn causes userspace to ignore touchpad >>> events. So iow this issues causes the touchpad to not work at boot. >>> >>> This commit fixes this by extending the "orphan" _REG method handling >>> to also apply to GPIO address-space handlers. >>> >>> Note it seems that Windows always calls "orphan" _REG methods so me >>> may want to consider dropping the space-id check and always do >>> "orphan" _REG method handling. >>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> --- >>> drivers/acpi/acpica/evregion.c | 54 >>> +++++++++++++++++----------------- >>> 1 file changed, 27 insertions(+), 27 deletions(-) >>> >>> diff --git a/drivers/acpi/acpica/evregion.c >>> b/drivers/acpi/acpica/evregion.c index 738d4b231f34..21ff341e34a4 >>> 100644 >>> --- a/drivers/acpi/acpica/evregion.c >>> +++ b/drivers/acpi/acpica/evregion.c >>> @@ -21,7 +21,8 @@ extern u8 acpi_gbl_default_address_spaces[]; >>> /* Local prototypes */ >>> >>> static void >>> -acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node >>> *ec_device_node); >>> +acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node >>> *device_node, >>> + acpi_adr_space_type space_id); >>> >>> static acpi_status >>> acpi_ev_reg_run(acpi_handle obj_handle, @@ -684,10 +685,12 @@ >>> acpi_ev_execute_reg_methods(struct >>> acpi_namespace_node *node, >>> ACPI_NS_WALK_UNLOCK, >>> acpi_ev_reg_run, NULL, >>> &info, NULL); >>> >>> - /* Special case for EC: handle "orphan" _REG methods with no region >>> */ >>> - >>> - if (space_id == ACPI_ADR_SPACE_EC) { >>> - acpi_ev_orphan_ec_reg_method(node); >>> + /* >>> + * Special case for EC and GPIO: handle "orphan" _REG methods with >>> + * no region. >>> + */ >>> + if (space_id == ACPI_ADR_SPACE_EC || space_id == >>> ACPI_ADR_SPACE_GPIO) { >>> + acpi_ev_execute_orphan_reg_method(node, space_id); >>> } >>> >>> ACPI_DEBUG_PRINT_RAW((ACPI_DB_NAMES, >>> @@ -760,31 +763,28 @@ acpi_ev_reg_run(acpi_handle obj_handle, >>> >>> >>> >> /********************************************************** >>> ********************* >>> * >>> - * FUNCTION: acpi_ev_orphan_ec_reg_method >>> + * FUNCTION: acpi_ev_execute_orphan_reg_method >>> * >>> - * PARAMETERS: ec_device_node - Namespace node for an EC device >>> + * PARAMETERS: device_node - Namespace node for an ACPI device >>> + * space_id - The address space ID >>> * >>> * RETURN: None >>> * >>> - * DESCRIPTION: Execute an "orphan" _REG method that appears under >>> the EC >>> + * DESCRIPTION: Execute an "orphan" _REG method that appears under >> an >>> ACPI >>> * device. This is a _REG method that has no corresponding region >>> - * within the EC device scope. The orphan _REG method appears to >>> - * have been enabled by the description of the ECDT in the ACPI >>> - * specification: "The availability of the region space can be >>> - * detected by providing a _REG method object underneath the >>> - * Embedded Controller device." >>> - * >>> - * To quickly access the EC device, we use the ec_device_node used >>> - * during EC handler installation. Otherwise, we would need to >>> - * perform a time consuming namespace walk, executing _HID >>> - * methods to find the EC device. >>> + * within the device's scope. ACPI tables depending on these >>> + * "orphan" _REG methods have been seen for both EC and GPIO >>> + * Operation Regions. Presumably the Windows ACPI >> implementation >>> + * always calls the _REG method independent of the presence of >>> + * an actual Operation Region with the correct address space ID. >>> * >>> * MUTEX: Assumes the namespace is locked >>> * >>> >>> >> ********************************************************** >>> ********************/ >>> >>> static void >>> -acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node >>> *ec_device_node) >>> +acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node >>> *device_node, >>> + acpi_adr_space_type space_id) >>> { >>> acpi_handle reg_method; >>> struct acpi_namespace_node *next_node; @@ -792,9 +792,9 @@ >>> acpi_ev_orphan_ec_reg_method(struct >>> acpi_namespace_node *ec_device_node) >>> struct acpi_object_list args; >>> union acpi_object objects[2]; >>> >>> - ACPI_FUNCTION_TRACE(ev_orphan_ec_reg_method); >>> + ACPI_FUNCTION_TRACE(ev_execute_orphan_reg_method); >>> >>> - if (!ec_device_node) { >>> + if (!device_node) { >>> return_VOID; >>> } >>> >>> @@ -804,7 +804,7 @@ acpi_ev_orphan_ec_reg_method(struct >>> acpi_namespace_node *ec_device_node) >>> >>> /* Get a handle to a _REG method immediately under the EC device >> */ >>> >>> - status = acpi_get_handle(ec_device_node, METHOD_NAME__REG, >>> ®_method); >>> + status = acpi_get_handle(device_node, METHOD_NAME__REG, >>> ®_method); >>> if (ACPI_FAILURE(status)) { >>> goto exit; /* There is no _REG method present */ >>> } >>> @@ -816,23 +816,23 @@ acpi_ev_orphan_ec_reg_method(struct >>> acpi_namespace_node *ec_device_node) >>> * with other space IDs to be present; but the code below will then >>> * execute the _REG method with the embedded_control space_ID >>> argument. >>> */ >>> - next_node = acpi_ns_get_next_node(ec_device_node, NULL); >>> + next_node = acpi_ns_get_next_node(device_node, NULL); >>> while (next_node) { >>> if ((next_node->type == ACPI_TYPE_REGION) && >>> (next_node->object) && >>> - (next_node->object->region.space_id == >>> ACPI_ADR_SPACE_EC)) { >>> + (next_node->object->region.space_id == space_id)) { >>> goto exit; /* Do not execute the _REG */ >>> } >>> >>> - next_node = acpi_ns_get_next_node(ec_device_node, >>> next_node); >>> + next_node = acpi_ns_get_next_node(device_node, >>> next_node); >>> } >>> >>> - /* Evaluate the _REG(embedded_control,Connect) method */ >>> + /* Evaluate the _REG(space_id, Connect) method */ >>> >>> args.count = 2; >>> args.pointer = objects; >>> objects[0].type = ACPI_TYPE_INTEGER; >>> - objects[0].integer.value = ACPI_ADR_SPACE_EC; >>> + objects[0].integer.value = space_id; >>> objects[1].type = ACPI_TYPE_INTEGER; >>> objects[1].integer.value = ACPI_REG_CONNECT; >>> >>> -- >>> 2.28.0 >> >> This looks good to me. Bob, any thoughts? >> >> Erik >
> -----Original Message----- > From: Hans de Goede <hdegoede@redhat.com> > Sent: Tuesday, October 27, 2020 12:00 PM > To: Kaneda, Erik <erik.kaneda@intel.com>; Moore, Robert > <robert.moore@intel.com>; Rafael J . Wysocki <rjw@rjwysocki.net>; Len > Brown <lenb@kernel.org> > Cc: linux-acpi@vger.kernel.org; devel@acpica.org > Subject: Re: [PATCH] ACPICA: Also handle "orphan" _REG methods for GPIO > OpRegions > > Hi, > > On 10/27/20 6:43 PM, Kaneda, Erik wrote: > > > > > >> -----Original Message----- > >> From: Moore, Robert <robert.moore@intel.com> > >> Sent: Tuesday, October 27, 2020 7:17 AM > >> To: Kaneda, Erik <erik.kaneda@intel.com>; Hans de Goede > >> <hdegoede@redhat.com>; Rafael J . Wysocki <rjw@rjwysocki.net>; Len > >> Brown <lenb@kernel.org> > >> Cc: linux-acpi@vger.kernel.org; devel@acpica.org > >> Subject: RE: [PATCH] ACPICA: Also handle "orphan" _REG methods for > GPIO > >> OpRegions > >> > >> Looks OK to me. > > > > Ok, I'll make a pull request of this to ACPICA on behalf of Hans and it will be > in the next ACPICA release. > > Great, thank you. Pull request is available here: https://github.com/acpica/acpica/pull/644 Once it's merged, it'll be a part of the next ACPICA release. Thanks, Erik > > Regards, > > Hans > > > > >> -----Original Message----- > >> From: Kaneda, Erik <erik.kaneda@intel.com> > >> Sent: Monday, October 26, 2020 1:56 PM > >> To: Hans de Goede <hdegoede@redhat.com>; Rafael J . Wysocki > >> <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>; Moore, Robert > >> <robert.moore@intel.com> > >> Cc: linux-acpi@vger.kernel.org; devel@acpica.org > >> Subject: RE: [PATCH] ACPICA: Also handle "orphan" _REG methods for > GPIO > >> OpRegions > >> > >> > >> > >>> -----Original Message----- > >>> From: Hans de Goede <hdegoede@redhat.com> > >>> Sent: Sunday, October 25, 2020 2:46 AM > >>> To: Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown > >>> <lenb@kernel.org>; Moore, Robert <robert.moore@intel.com>; Kaneda, > >>> Erik <erik.kaneda@intel.com> > >>> Cc: Hans de Goede <hdegoede@redhat.com>; linux- > acpi@vger.kernel.org; > >>> devel@acpica.org > >>> Subject: [PATCH] ACPICA: Also handle "orphan" _REG methods for GPIO > >>> OpRegions > >>> > >>> Before this commit acpi_ev_execute_reg_methods() had special > handling > >>> to handle "orphan" (no matching OpRegion declared) _REG methods for > EC > >>> nodes. > >>> > >>> On Intel Cherry Trail devices there are 2 possible ACPI OpRegions for > >>> accessing GPIOs. The standard GeneralPurposeIo OpRegion and the > Cherry > >>> Trail specific UserDefined 0x9X OpRegions. > >>> > >>> Having 2 different types of OpRegions leads to potential issues with > >>> checks for OpRegion availability, or in other words checks if _REG has > >>> been called for the OpRegion which the ACPI code wants to use. > >>> > >>> Except for the "orphan" EC handling, ACPICA core does not call _REG on > >>> an ACPI node which does not define an OpRegion matching the type > being > >>> registered; and the reference design DSDT, from which most Cherry > >>> Trail DSDTs are derived, does not define GeneralPurposeIo, nor > >>> UserDefined(0x93) > >>> OpRegions for the GPO2 (UID 3) device, because no pins were assigned > >>> ACPI controlled functions in the reference design. > >>> > >>> Together this leads to the perfect storm, at least on the Cherry Trail > >>> based Medion Akayo E1239T. This design does use a GPO2 pin from its > >>> ACPI code and has added the Cherry Trail specific UserDefined(0x93) > >>> opregion to its GPO2 ACPI node to access this pin. > >>> > >>> But it uses a has _REG been called availability check for the standard > >>> GeneralPurposeIo OpRegion. This clearly is a bug in the DSDT, but this > >>> does work under Windows. This issue leads to the intel_vbtn driver > >>> reporting the device always being in tablet-mode at boot, even if it > >>> is in laptop mode. Which in turn causes userspace to ignore touchpad > >>> events. So iow this issues causes the touchpad to not work at boot. > >>> > >>> This commit fixes this by extending the "orphan" _REG method handling > >>> to also apply to GPIO address-space handlers. > >>> > >>> Note it seems that Windows always calls "orphan" _REG methods so me > >>> may want to consider dropping the space-id check and always do > >>> "orphan" _REG method handling. > >>> > >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >>> --- > >>> drivers/acpi/acpica/evregion.c | 54 > >>> +++++++++++++++++----------------- > >>> 1 file changed, 27 insertions(+), 27 deletions(-) > >>> > >>> diff --git a/drivers/acpi/acpica/evregion.c > >>> b/drivers/acpi/acpica/evregion.c index 738d4b231f34..21ff341e34a4 > >>> 100644 > >>> --- a/drivers/acpi/acpica/evregion.c > >>> +++ b/drivers/acpi/acpica/evregion.c > >>> @@ -21,7 +21,8 @@ extern u8 acpi_gbl_default_address_spaces[]; > >>> /* Local prototypes */ > >>> > >>> static void > >>> -acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node > >>> *ec_device_node); > >>> +acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node > >>> *device_node, > >>> + acpi_adr_space_type space_id); > >>> > >>> static acpi_status > >>> acpi_ev_reg_run(acpi_handle obj_handle, @@ -684,10 +685,12 @@ > >>> acpi_ev_execute_reg_methods(struct > >>> acpi_namespace_node *node, > >>> ACPI_NS_WALK_UNLOCK, > >>> acpi_ev_reg_run, NULL, > >>> &info, NULL); > >>> > >>> - /* Special case for EC: handle "orphan" _REG methods with no region > >>> */ > >>> - > >>> - if (space_id == ACPI_ADR_SPACE_EC) { > >>> - acpi_ev_orphan_ec_reg_method(node); > >>> + /* > >>> + * Special case for EC and GPIO: handle "orphan" _REG methods with > >>> + * no region. > >>> + */ > >>> + if (space_id == ACPI_ADR_SPACE_EC || space_id == > >>> ACPI_ADR_SPACE_GPIO) { > >>> + acpi_ev_execute_orphan_reg_method(node, space_id); > >>> } > >>> > >>> ACPI_DEBUG_PRINT_RAW((ACPI_DB_NAMES, > >>> @@ -760,31 +763,28 @@ acpi_ev_reg_run(acpi_handle obj_handle, > >>> > >>> > >>> > >> > /********************************************************** > >>> ********************* > >>> * > >>> - * FUNCTION: acpi_ev_orphan_ec_reg_method > >>> + * FUNCTION: acpi_ev_execute_orphan_reg_method > >>> * > >>> - * PARAMETERS: ec_device_node - Namespace node for an EC > device > >>> + * PARAMETERS: device_node - Namespace node for an ACPI device > >>> + * space_id - The address space ID > >>> * > >>> * RETURN: None > >>> * > >>> - * DESCRIPTION: Execute an "orphan" _REG method that appears under > >>> the EC > >>> + * DESCRIPTION: Execute an "orphan" _REG method that appears > under > >> an > >>> ACPI > >>> * device. This is a _REG method that has no corresponding region > >>> - * within the EC device scope. The orphan _REG method appears > to > >>> - * have been enabled by the description of the ECDT in the ACPI > >>> - * specification: "The availability of the region space can be > >>> - * detected by providing a _REG method object underneath the > >>> - * Embedded Controller device." > >>> - * > >>> - * To quickly access the EC device, we use the ec_device_node > used > >>> - * during EC handler installation. Otherwise, we would need to > >>> - * perform a time consuming namespace walk, executing _HID > >>> - * methods to find the EC device. > >>> + * within the device's scope. ACPI tables depending on these > >>> + * "orphan" _REG methods have been seen for both EC and > GPIO > >>> + * Operation Regions. Presumably the Windows ACPI > >> implementation > >>> + * always calls the _REG method independent of the presence of > >>> + * an actual Operation Region with the correct address space ID. > >>> * > >>> * MUTEX: Assumes the namespace is locked > >>> * > >>> > >>> > >> > ********************************************************** > >>> ********************/ > >>> > >>> static void > >>> -acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node > >>> *ec_device_node) > >>> +acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node > >>> *device_node, > >>> + acpi_adr_space_type space_id) > >>> { > >>> acpi_handle reg_method; > >>> struct acpi_namespace_node *next_node; @@ -792,9 +792,9 @@ > >>> acpi_ev_orphan_ec_reg_method(struct > >>> acpi_namespace_node *ec_device_node) > >>> struct acpi_object_list args; > >>> union acpi_object objects[2]; > >>> > >>> - ACPI_FUNCTION_TRACE(ev_orphan_ec_reg_method); > >>> + ACPI_FUNCTION_TRACE(ev_execute_orphan_reg_method); > >>> > >>> - if (!ec_device_node) { > >>> + if (!device_node) { > >>> return_VOID; > >>> } > >>> > >>> @@ -804,7 +804,7 @@ acpi_ev_orphan_ec_reg_method(struct > >>> acpi_namespace_node *ec_device_node) > >>> > >>> /* Get a handle to a _REG method immediately under the EC device > >> */ > >>> > >>> - status = acpi_get_handle(ec_device_node, METHOD_NAME__REG, > >>> ®_method); > >>> + status = acpi_get_handle(device_node, METHOD_NAME__REG, > >>> ®_method); > >>> if (ACPI_FAILURE(status)) { > >>> goto exit; /* There is no _REG method present */ > >>> } > >>> @@ -816,23 +816,23 @@ acpi_ev_orphan_ec_reg_method(struct > >>> acpi_namespace_node *ec_device_node) > >>> * with other space IDs to be present; but the code below will then > >>> * execute the _REG method with the embedded_control space_ID > >>> argument. > >>> */ > >>> - next_node = acpi_ns_get_next_node(ec_device_node, NULL); > >>> + next_node = acpi_ns_get_next_node(device_node, NULL); > >>> while (next_node) { > >>> if ((next_node->type == ACPI_TYPE_REGION) && > >>> (next_node->object) && > >>> - (next_node->object->region.space_id == > >>> ACPI_ADR_SPACE_EC)) { > >>> + (next_node->object->region.space_id == space_id)) { > >>> goto exit; /* Do not execute the _REG */ > >>> } > >>> > >>> - next_node = acpi_ns_get_next_node(ec_device_node, > >>> next_node); > >>> + next_node = acpi_ns_get_next_node(device_node, > >>> next_node); > >>> } > >>> > >>> - /* Evaluate the _REG(embedded_control,Connect) method */ > >>> + /* Evaluate the _REG(space_id, Connect) method */ > >>> > >>> args.count = 2; > >>> args.pointer = objects; > >>> objects[0].type = ACPI_TYPE_INTEGER; > >>> - objects[0].integer.value = ACPI_ADR_SPACE_EC; > >>> + objects[0].integer.value = space_id; > >>> objects[1].type = ACPI_TYPE_INTEGER; > >>> objects[1].integer.value = ACPI_REG_CONNECT; > >>> > >>> -- > >>> 2.28.0 > >> > >> This looks good to me. Bob, any thoughts? > >> > >> Erik > >
Hi, On 10/28/20 11:33 PM, Kaneda, Erik wrote: > > >> -----Original Message----- >> From: Hans de Goede <hdegoede@redhat.com> >> Sent: Tuesday, October 27, 2020 12:00 PM >> To: Kaneda, Erik <erik.kaneda@intel.com>; Moore, Robert >> <robert.moore@intel.com>; Rafael J . Wysocki <rjw@rjwysocki.net>; Len >> Brown <lenb@kernel.org> >> Cc: linux-acpi@vger.kernel.org; devel@acpica.org >> Subject: Re: [PATCH] ACPICA: Also handle "orphan" _REG methods for GPIO >> OpRegions >> >> Hi, >> >> On 10/27/20 6:43 PM, Kaneda, Erik wrote: >>> >>> >>>> -----Original Message----- >>>> From: Moore, Robert <robert.moore@intel.com> >>>> Sent: Tuesday, October 27, 2020 7:17 AM >>>> To: Kaneda, Erik <erik.kaneda@intel.com>; Hans de Goede >>>> <hdegoede@redhat.com>; Rafael J . Wysocki <rjw@rjwysocki.net>; Len >>>> Brown <lenb@kernel.org> >>>> Cc: linux-acpi@vger.kernel.org; devel@acpica.org >>>> Subject: RE: [PATCH] ACPICA: Also handle "orphan" _REG methods for >> GPIO >>>> OpRegions >>>> >>>> Looks OK to me. >>> >>> Ok, I'll make a pull request of this to ACPICA on behalf of Hans and it will be >> in the next ACPICA release. >> >> Great, thank you. > > Pull request is available here: https://github.com/acpica/acpica/pull/644 > > Once it's merged, it'll be a part of the next ACPICA release. Thank you for porting this to the ACPICA upstream code, unfortunately you missed one change when porting this. I've added a review pointing the missing bit out on github. Specifically you missed this bit: if ((next_node->type == ACPI_TYPE_REGION) && (next_node->object) && - (next_node->object->region.space_id == ACPI_ADR_SPACE_EC)) { + (next_node->object->region.space_id == space_id)) { goto exit; /* Do not execute the _REG */ } Regards, Hans
> -----Original Message----- > From: Hans de Goede <hdegoede@redhat.com> > Sent: Thursday, October 29, 2020 2:49 AM > To: Kaneda, Erik <erik.kaneda@intel.com>; Moore, Robert > <robert.moore@intel.com>; Rafael J . Wysocki <rjw@rjwysocki.net>; Len > Brown <lenb@kernel.org> > Cc: linux-acpi@vger.kernel.org; devel@acpica.org > Subject: Re: [PATCH] ACPICA: Also handle "orphan" _REG methods for GPIO > OpRegions > > Hi, > > On 10/28/20 11:33 PM, Kaneda, Erik wrote: > > > > > >> -----Original Message----- > >> From: Hans de Goede <hdegoede@redhat.com> > >> Sent: Tuesday, October 27, 2020 12:00 PM > >> To: Kaneda, Erik <erik.kaneda@intel.com>; Moore, Robert > >> <robert.moore@intel.com>; Rafael J . Wysocki <rjw@rjwysocki.net>; Len > >> Brown <lenb@kernel.org> > >> Cc: linux-acpi@vger.kernel.org; devel@acpica.org > >> Subject: Re: [PATCH] ACPICA: Also handle "orphan" _REG methods for > GPIO > >> OpRegions > >> > >> Hi, > >> > >> On 10/27/20 6:43 PM, Kaneda, Erik wrote: > >>> > >>> > >>>> -----Original Message----- > >>>> From: Moore, Robert <robert.moore@intel.com> > >>>> Sent: Tuesday, October 27, 2020 7:17 AM > >>>> To: Kaneda, Erik <erik.kaneda@intel.com>; Hans de Goede > >>>> <hdegoede@redhat.com>; Rafael J . Wysocki <rjw@rjwysocki.net>; > Len > >>>> Brown <lenb@kernel.org> > >>>> Cc: linux-acpi@vger.kernel.org; devel@acpica.org > >>>> Subject: RE: [PATCH] ACPICA: Also handle "orphan" _REG methods for > >> GPIO > >>>> OpRegions > >>>> > >>>> Looks OK to me. > >>> > >>> Ok, I'll make a pull request of this to ACPICA on behalf of Hans and it will > be > >> in the next ACPICA release. > >> > >> Great, thank you. > > > > Pull request is available here: https://github.com/acpica/acpica/pull/644 > > > > Once it's merged, it'll be a part of the next ACPICA release. > > Thank you for porting this to the ACPICA upstream code, > unfortunately you missed one change when porting this. I've added > a review pointing the missing bit out on github. > > Specifically you missed this bit: > > if ((next_node->type == ACPI_TYPE_REGION) && > (next_node->object) && > - (next_node->object->region.space_id == > ACPI_ADR_SPACE_EC)) { > + (next_node->object->region.space_id == space_id)) { > goto exit; /* Do not execute the _REG */ > } > Updated the PR. Thanks for catching this, Erik > > Regards, > > Hans
I've merged this one. -----Original Message----- From: Kaneda, Erik <erik.kaneda@intel.com> Sent: Wednesday, October 28, 2020 3:34 PM To: Hans de Goede <hdegoede@redhat.com>; Moore, Robert <robert.moore@intel.com>; Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org> Cc: linux-acpi@vger.kernel.org; devel@acpica.org Subject: RE: [PATCH] ACPICA: Also handle "orphan" _REG methods for GPIO OpRegions > -----Original Message----- > From: Hans de Goede <hdegoede@redhat.com> > Sent: Tuesday, October 27, 2020 12:00 PM > To: Kaneda, Erik <erik.kaneda@intel.com>; Moore, Robert > <robert.moore@intel.com>; Rafael J . Wysocki <rjw@rjwysocki.net>; Len > Brown <lenb@kernel.org> > Cc: linux-acpi@vger.kernel.org; devel@acpica.org > Subject: Re: [PATCH] ACPICA: Also handle "orphan" _REG methods for > GPIO OpRegions > > Hi, > > On 10/27/20 6:43 PM, Kaneda, Erik wrote: > > > > > >> -----Original Message----- > >> From: Moore, Robert <robert.moore@intel.com> > >> Sent: Tuesday, October 27, 2020 7:17 AM > >> To: Kaneda, Erik <erik.kaneda@intel.com>; Hans de Goede > >> <hdegoede@redhat.com>; Rafael J . Wysocki <rjw@rjwysocki.net>; Len > >> Brown <lenb@kernel.org> > >> Cc: linux-acpi@vger.kernel.org; devel@acpica.org > >> Subject: RE: [PATCH] ACPICA: Also handle "orphan" _REG methods for > GPIO > >> OpRegions > >> > >> Looks OK to me. > > > > Ok, I'll make a pull request of this to ACPICA on behalf of Hans and > > it will be > in the next ACPICA release. > > Great, thank you. Pull request is available here: https://github.com/acpica/acpica/pull/644 Once it's merged, it'll be a part of the next ACPICA release. Thanks, Erik > > Regards, > > Hans > > > > >> -----Original Message----- > >> From: Kaneda, Erik <erik.kaneda@intel.com> > >> Sent: Monday, October 26, 2020 1:56 PM > >> To: Hans de Goede <hdegoede@redhat.com>; Rafael J . Wysocki > >> <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>; Moore, Robert > >> <robert.moore@intel.com> > >> Cc: linux-acpi@vger.kernel.org; devel@acpica.org > >> Subject: RE: [PATCH] ACPICA: Also handle "orphan" _REG methods for > GPIO > >> OpRegions > >> > >> > >> > >>> -----Original Message----- > >>> From: Hans de Goede <hdegoede@redhat.com> > >>> Sent: Sunday, October 25, 2020 2:46 AM > >>> To: Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown > >>> <lenb@kernel.org>; Moore, Robert <robert.moore@intel.com>; Kaneda, > >>> Erik <erik.kaneda@intel.com> > >>> Cc: Hans de Goede <hdegoede@redhat.com>; linux- > acpi@vger.kernel.org; > >>> devel@acpica.org > >>> Subject: [PATCH] ACPICA: Also handle "orphan" _REG methods for > >>> GPIO OpRegions > >>> > >>> Before this commit acpi_ev_execute_reg_methods() had special > handling > >>> to handle "orphan" (no matching OpRegion declared) _REG methods > >>> for > EC > >>> nodes. > >>> > >>> On Intel Cherry Trail devices there are 2 possible ACPI OpRegions > >>> for accessing GPIOs. The standard GeneralPurposeIo OpRegion and > >>> the > Cherry > >>> Trail specific UserDefined 0x9X OpRegions. > >>> > >>> Having 2 different types of OpRegions leads to potential issues > >>> with checks for OpRegion availability, or in other words checks if > >>> _REG has been called for the OpRegion which the ACPI code wants to use. > >>> > >>> Except for the "orphan" EC handling, ACPICA core does not call > >>> _REG on an ACPI node which does not define an OpRegion matching > >>> the type > being > >>> registered; and the reference design DSDT, from which most Cherry > >>> Trail DSDTs are derived, does not define GeneralPurposeIo, nor > >>> UserDefined(0x93) > >>> OpRegions for the GPO2 (UID 3) device, because no pins were > >>> assigned ACPI controlled functions in the reference design. > >>> > >>> Together this leads to the perfect storm, at least on the Cherry > >>> Trail based Medion Akayo E1239T. This design does use a GPO2 pin > >>> from its ACPI code and has added the Cherry Trail specific > >>> UserDefined(0x93) opregion to its GPO2 ACPI node to access this pin. > >>> > >>> But it uses a has _REG been called availability check for the > >>> standard GeneralPurposeIo OpRegion. This clearly is a bug in the > >>> DSDT, but this does work under Windows. This issue leads to the > >>> intel_vbtn driver reporting the device always being in tablet-mode > >>> at boot, even if it is in laptop mode. Which in turn causes > >>> userspace to ignore touchpad events. So iow this issues causes the touchpad to not work at boot. > >>> > >>> This commit fixes this by extending the "orphan" _REG method > >>> handling to also apply to GPIO address-space handlers. > >>> > >>> Note it seems that Windows always calls "orphan" _REG methods so > >>> me may want to consider dropping the space-id check and always do > >>> "orphan" _REG method handling. > >>> > >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >>> --- > >>> drivers/acpi/acpica/evregion.c | 54 > >>> +++++++++++++++++----------------- > >>> 1 file changed, 27 insertions(+), 27 deletions(-) > >>> > >>> diff --git a/drivers/acpi/acpica/evregion.c > >>> b/drivers/acpi/acpica/evregion.c index 738d4b231f34..21ff341e34a4 > >>> 100644 > >>> --- a/drivers/acpi/acpica/evregion.c > >>> +++ b/drivers/acpi/acpica/evregion.c > >>> @@ -21,7 +21,8 @@ extern u8 acpi_gbl_default_address_spaces[]; > >>> /* Local prototypes */ > >>> > >>> static void > >>> -acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node > >>> *ec_device_node); > >>> +acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node > >>> *device_node, > >>> + acpi_adr_space_type space_id); > >>> > >>> static acpi_status > >>> acpi_ev_reg_run(acpi_handle obj_handle, @@ -684,10 +685,12 @@ > >>> acpi_ev_execute_reg_methods(struct > >>> acpi_namespace_node *node, > >>> ACPI_NS_WALK_UNLOCK, > >>> acpi_ev_reg_run, NULL, > >>> &info, NULL); > >>> > >>> - /* Special case for EC: handle "orphan" _REG methods with no region > >>> */ > >>> - > >>> - if (space_id == ACPI_ADR_SPACE_EC) { > >>> - acpi_ev_orphan_ec_reg_method(node); > >>> + /* > >>> + * Special case for EC and GPIO: handle "orphan" _REG methods with > >>> + * no region. > >>> + */ > >>> + if (space_id == ACPI_ADR_SPACE_EC || space_id == > >>> ACPI_ADR_SPACE_GPIO) { > >>> + acpi_ev_execute_orphan_reg_method(node, space_id); > >>> } > >>> > >>> ACPI_DEBUG_PRINT_RAW((ACPI_DB_NAMES, > >>> @@ -760,31 +763,28 @@ acpi_ev_reg_run(acpi_handle obj_handle, > >>> > >>> > >>> > >> > /********************************************************** > >>> ********************* > >>> * > >>> - * FUNCTION: acpi_ev_orphan_ec_reg_method > >>> + * FUNCTION: acpi_ev_execute_orphan_reg_method > >>> * > >>> - * PARAMETERS: ec_device_node - Namespace node for an EC > device > >>> + * PARAMETERS: device_node - Namespace node for an ACPI device > >>> + * space_id - The address space ID > >>> * > >>> * RETURN: None > >>> * > >>> - * DESCRIPTION: Execute an "orphan" _REG method that appears > >>> under the EC > >>> + * DESCRIPTION: Execute an "orphan" _REG method that appears > under > >> an > >>> ACPI > >>> * device. This is a _REG method that has no corresponding region > >>> - * within the EC device scope. The orphan _REG method appears > to > >>> - * have been enabled by the description of the ECDT in the ACPI > >>> - * specification: "The availability of the region space can be > >>> - * detected by providing a _REG method object underneath the > >>> - * Embedded Controller device." > >>> - * > >>> - * To quickly access the EC device, we use the ec_device_node > used > >>> - * during EC handler installation. Otherwise, we would need to > >>> - * perform a time consuming namespace walk, executing _HID > >>> - * methods to find the EC device. > >>> + * within the device's scope. ACPI tables depending on these > >>> + * "orphan" _REG methods have been seen for both EC and > GPIO > >>> + * Operation Regions. Presumably the Windows ACPI > >> implementation > >>> + * always calls the _REG method independent of the presence of > >>> + * an actual Operation Region with the correct address space ID. > >>> * > >>> * MUTEX: Assumes the namespace is locked > >>> * > >>> > >>> > >> > ********************************************************** > >>> ********************/ > >>> > >>> static void > >>> -acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node > >>> *ec_device_node) > >>> +acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node > >>> *device_node, > >>> + acpi_adr_space_type space_id) > >>> { > >>> acpi_handle reg_method; > >>> struct acpi_namespace_node *next_node; @@ -792,9 +792,9 @@ > >>> acpi_ev_orphan_ec_reg_method(struct > >>> acpi_namespace_node *ec_device_node) > >>> struct acpi_object_list args; > >>> union acpi_object objects[2]; > >>> > >>> - ACPI_FUNCTION_TRACE(ev_orphan_ec_reg_method); > >>> + ACPI_FUNCTION_TRACE(ev_execute_orphan_reg_method); > >>> > >>> - if (!ec_device_node) { > >>> + if (!device_node) { > >>> return_VOID; > >>> } > >>> > >>> @@ -804,7 +804,7 @@ acpi_ev_orphan_ec_reg_method(struct > >>> acpi_namespace_node *ec_device_node) > >>> > >>> /* Get a handle to a _REG method immediately under the EC device > >> */ > >>> > >>> - status = acpi_get_handle(ec_device_node, METHOD_NAME__REG, > >>> ®_method); > >>> + status = acpi_get_handle(device_node, METHOD_NAME__REG, > >>> ®_method); > >>> if (ACPI_FAILURE(status)) { > >>> goto exit; /* There is no _REG method present */ > >>> } > >>> @@ -816,23 +816,23 @@ acpi_ev_orphan_ec_reg_method(struct > >>> acpi_namespace_node *ec_device_node) > >>> * with other space IDs to be present; but the code below will then > >>> * execute the _REG method with the embedded_control space_ID > >>> argument. > >>> */ > >>> - next_node = acpi_ns_get_next_node(ec_device_node, NULL); > >>> + next_node = acpi_ns_get_next_node(device_node, NULL); > >>> while (next_node) { > >>> if ((next_node->type == ACPI_TYPE_REGION) && > >>> (next_node->object) && > >>> - (next_node->object->region.space_id == > >>> ACPI_ADR_SPACE_EC)) { > >>> + (next_node->object->region.space_id == space_id)) { > >>> goto exit; /* Do not execute the _REG */ > >>> } > >>> > >>> - next_node = acpi_ns_get_next_node(ec_device_node, > >>> next_node); > >>> + next_node = acpi_ns_get_next_node(device_node, > >>> next_node); > >>> } > >>> > >>> - /* Evaluate the _REG(embedded_control,Connect) method */ > >>> + /* Evaluate the _REG(space_id, Connect) method */ > >>> > >>> args.count = 2; > >>> args.pointer = objects; > >>> objects[0].type = ACPI_TYPE_INTEGER; > >>> - objects[0].integer.value = ACPI_ADR_SPACE_EC; > >>> + objects[0].integer.value = space_id; > >>> objects[1].type = ACPI_TYPE_INTEGER; > >>> objects[1].integer.value = ACPI_REG_CONNECT; > >>> > >>> -- > >>> 2.28.0 > >> > >> This looks good to me. Bob, any thoughts? > >> > >> Erik > >
diff --git a/drivers/acpi/acpica/evregion.c b/drivers/acpi/acpica/evregion.c index 738d4b231f34..21ff341e34a4 100644 --- a/drivers/acpi/acpica/evregion.c +++ b/drivers/acpi/acpica/evregion.c @@ -21,7 +21,8 @@ extern u8 acpi_gbl_default_address_spaces[]; /* Local prototypes */ static void -acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node *ec_device_node); +acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node *device_node, + acpi_adr_space_type space_id); static acpi_status acpi_ev_reg_run(acpi_handle obj_handle, @@ -684,10 +685,12 @@ acpi_ev_execute_reg_methods(struct acpi_namespace_node *node, ACPI_NS_WALK_UNLOCK, acpi_ev_reg_run, NULL, &info, NULL); - /* Special case for EC: handle "orphan" _REG methods with no region */ - - if (space_id == ACPI_ADR_SPACE_EC) { - acpi_ev_orphan_ec_reg_method(node); + /* + * Special case for EC and GPIO: handle "orphan" _REG methods with + * no region. + */ + if (space_id == ACPI_ADR_SPACE_EC || space_id == ACPI_ADR_SPACE_GPIO) { + acpi_ev_execute_orphan_reg_method(node, space_id); } ACPI_DEBUG_PRINT_RAW((ACPI_DB_NAMES, @@ -760,31 +763,28 @@ acpi_ev_reg_run(acpi_handle obj_handle, /******************************************************************************* * - * FUNCTION: acpi_ev_orphan_ec_reg_method + * FUNCTION: acpi_ev_execute_orphan_reg_method * - * PARAMETERS: ec_device_node - Namespace node for an EC device + * PARAMETERS: device_node - Namespace node for an ACPI device + * space_id - The address space ID * * RETURN: None * - * DESCRIPTION: Execute an "orphan" _REG method that appears under the EC + * DESCRIPTION: Execute an "orphan" _REG method that appears under an ACPI * device. This is a _REG method that has no corresponding region - * within the EC device scope. The orphan _REG method appears to - * have been enabled by the description of the ECDT in the ACPI - * specification: "The availability of the region space can be - * detected by providing a _REG method object underneath the - * Embedded Controller device." - * - * To quickly access the EC device, we use the ec_device_node used - * during EC handler installation. Otherwise, we would need to - * perform a time consuming namespace walk, executing _HID - * methods to find the EC device. + * within the device's scope. ACPI tables depending on these + * "orphan" _REG methods have been seen for both EC and GPIO + * Operation Regions. Presumably the Windows ACPI implementation + * always calls the _REG method independent of the presence of + * an actual Operation Region with the correct address space ID. * * MUTEX: Assumes the namespace is locked * ******************************************************************************/ static void -acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node *ec_device_node) +acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node *device_node, + acpi_adr_space_type space_id) { acpi_handle reg_method; struct acpi_namespace_node *next_node; @@ -792,9 +792,9 @@ acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node *ec_device_node) struct acpi_object_list args; union acpi_object objects[2]; - ACPI_FUNCTION_TRACE(ev_orphan_ec_reg_method); + ACPI_FUNCTION_TRACE(ev_execute_orphan_reg_method); - if (!ec_device_node) { + if (!device_node) { return_VOID; } @@ -804,7 +804,7 @@ acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node *ec_device_node) /* Get a handle to a _REG method immediately under the EC device */ - status = acpi_get_handle(ec_device_node, METHOD_NAME__REG, ®_method); + status = acpi_get_handle(device_node, METHOD_NAME__REG, ®_method); if (ACPI_FAILURE(status)) { goto exit; /* There is no _REG method present */ } @@ -816,23 +816,23 @@ acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node *ec_device_node) * with other space IDs to be present; but the code below will then * execute the _REG method with the embedded_control space_ID argument. */ - next_node = acpi_ns_get_next_node(ec_device_node, NULL); + next_node = acpi_ns_get_next_node(device_node, NULL); while (next_node) { if ((next_node->type == ACPI_TYPE_REGION) && (next_node->object) && - (next_node->object->region.space_id == ACPI_ADR_SPACE_EC)) { + (next_node->object->region.space_id == space_id)) { goto exit; /* Do not execute the _REG */ } - next_node = acpi_ns_get_next_node(ec_device_node, next_node); + next_node = acpi_ns_get_next_node(device_node, next_node); } - /* Evaluate the _REG(embedded_control,Connect) method */ + /* Evaluate the _REG(space_id, Connect) method */ args.count = 2; args.pointer = objects; objects[0].type = ACPI_TYPE_INTEGER; - objects[0].integer.value = ACPI_ADR_SPACE_EC; + objects[0].integer.value = space_id; objects[1].type = ACPI_TYPE_INTEGER; objects[1].integer.value = ACPI_REG_CONNECT;
Before this commit acpi_ev_execute_reg_methods() had special handling to handle "orphan" (no matching OpRegion declared) _REG methods for EC nodes. On Intel Cherry Trail devices there are 2 possible ACPI OpRegions for accessing GPIOs. The standard GeneralPurposeIo OpRegion and the Cherry Trail specific UserDefined 0x9X OpRegions. Having 2 different types of OpRegions leads to potential issues with checks for OpRegion availability, or in other words checks if _REG has been called for the OpRegion which the ACPI code wants to use. Except for the "orphan" EC handling, ACPICA core does not call _REG on an ACPI node which does not define an OpRegion matching the type being registered; and the reference design DSDT, from which most Cherry Trail DSDTs are derived, does not define GeneralPurposeIo, nor UserDefined(0x93) OpRegions for the GPO2 (UID 3) device, because no pins were assigned ACPI controlled functions in the reference design. Together this leads to the perfect storm, at least on the Cherry Trail based Medion Akayo E1239T. This design does use a GPO2 pin from its ACPI code and has added the Cherry Trail specific UserDefined(0x93) opregion to its GPO2 ACPI node to access this pin. But it uses a has _REG been called availability check for the standard GeneralPurposeIo OpRegion. This clearly is a bug in the DSDT, but this does work under Windows. This issue leads to the intel_vbtn driver reporting the device always being in tablet-mode at boot, even if it is in laptop mode. Which in turn causes userspace to ignore touchpad events. So iow this issues causes the touchpad to not work at boot. This commit fixes this by extending the "orphan" _REG method handling to also apply to GPIO address-space handlers. Note it seems that Windows always calls "orphan" _REG methods so me may want to consider dropping the space-id check and always do "orphan" _REG method handling. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/acpi/acpica/evregion.c | 54 +++++++++++++++++----------------- 1 file changed, 27 insertions(+), 27 deletions(-)