Re: [PATCH] ACPICA: Also handle "orphan" _REG methods for GPIO OpRegions

[Hans de Goede <hdegoede@redhat.com>]

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,
>>> &reg_method);
>>> +	status = acpi_get_handle(device_node, METHOD_NAME__REG,
>>> &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
>