Received-SPF: pass (google.com: domain of linux-kernel+bounces-216227-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <3e902b05-40be-4f59-bafa-9f1d7b415064@redhat.com>
Date: Sun, 16 Jun 2024 12:00:24 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1] ACPI: EC: Evaluate orphan _REG under EC device
From: Hans de Goede <hdegoede@redhat.com>
To: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
 Linux ACPI <linux-acpi@vger.kernel.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
 Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
 VitaliiT <vitaly.torshyn@gmail.com>, Armin Wolf <w_armin@gmx.de>
References: <12466682.O9o76ZdvQC@kreacher>
 <533a5778-3d68-4b61-b4ca-e82c37d65b33@redhat.com>
Content-Language: en-US, nl
In-Reply-To: <533a5778-3d68-4b61-b4ca-e82c37d65b33@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

Hi,

On 6/16/24 11:47 AM, Hans de Goede wrote:
> Hi,
> 
> On 6/12/24 4:15 PM, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> After starting to install the EC address space handler at the ACPI
>> namespace root, if there is an "orphan" _REG method in the EC device's
>> scope, it will not be evaluated any more.  This breaks EC operation
>> regions on some systems, like Asus gu605.
>>
>> To address this, use a wrapper around an existing ACPICA function to
>> look for an "orphan" _REG method in the EC device scope and evaluate
>> it if present.
>>
>> Fixes: 60fa6ae6e6d0 ("ACPI: EC: Install address space handler at the namespace root")
>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218945
>> Reported-by: VitaliiT <vitaly.torshyn@gmail.com>
>> Tested-by: VitaliiT <vitaly.torshyn@gmail.com>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> ---
>>
>> Yes, this includes ACPICA changes that are obviously not upstream
>> and I am going to take care of pusing them to upstream, but for
>> now there is a regression to fix and it is relatively late in the
>> cycle.
>>
>> ---
>>  drivers/acpi/acpica/acevents.h |    4 +++
>>  drivers/acpi/acpica/evregion.c |    6 ----
>>  drivers/acpi/acpica/evxfregn.c |   54 +++++++++++++++++++++++++++++++++++++++++
>>  drivers/acpi/ec.c              |    3 ++
>>  include/acpi/acpixf.h          |    4 +++
>>  5 files changed, 66 insertions(+), 5 deletions(-)
>>
>> Index: linux-pm/drivers/acpi/acpica/evxfregn.c
>> ===================================================================
>> --- linux-pm.orig/drivers/acpi/acpica/evxfregn.c
>> +++ linux-pm/drivers/acpi/acpica/evxfregn.c
>> @@ -306,3 +306,57 @@ acpi_execute_reg_methods(acpi_handle dev
>>  }
>>  
>>  ACPI_EXPORT_SYMBOL(acpi_execute_reg_methods)
>> +
>> +/*******************************************************************************
>> + *
>> + * FUNCTION:    acpi_execute_orphan_reg_method
>> + *
>> + * PARAMETERS:  device          - Handle for the device
>> + *              space_id        - The address space ID
>> + *
>> + * RETURN:      Status
>> + *
>> + * DESCRIPTION: Execute an "orphan" _REG method that appears under an ACPI
>> + *              device. This is a _REG method that has no corresponding region
>> + *              within the device's scope.
>> + *
>> + ******************************************************************************/
>> +acpi_status
>> +acpi_execute_orphan_reg_method(acpi_handle device, acpi_adr_space_type space_id)
>> +{
>> +	struct acpi_namespace_node *node;
>> +	acpi_status status;
>> +
>> +	ACPI_FUNCTION_TRACE(acpi_execute_orphan_reg_method);
>> +
>> +	/* Parameter validation */
>> +
>> +	if (!device) {
>> +		return_ACPI_STATUS(AE_BAD_PARAMETER);
>> +	}
>> +
>> +	status = acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE);
>> +	if (ACPI_FAILURE(status)) {
>> +		return_ACPI_STATUS(status);
>> +	}
>> +
>> +	/* Convert and validate the device handle */
>> +
>> +	node = acpi_ns_validate_handle(device);
>> +	if (node) {
>> +
>> +		/*
>> +		 * If an "orphan" _REG method is present in the device's scope
>> +		 * for the given address space ID, run it.
>> +		 */
>> +
>> +		acpi_ev_execute_orphan_reg_method(node, space_id);
>> +	} else {
>> +		status = AE_BAD_PARAMETER;
>> +	}
>> +
>> +	(void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE);
>> +	return_ACPI_STATUS(status);
>> +}
>> +
>> +ACPI_EXPORT_SYMBOL(acpi_execute_orphan_reg_method)
>> Index: linux-pm/include/acpi/acpixf.h
>> ===================================================================
>> --- linux-pm.orig/include/acpi/acpixf.h
>> +++ linux-pm/include/acpi/acpixf.h
>> @@ -663,6 +663,10 @@ ACPI_EXTERNAL_RETURN_STATUS(acpi_status
>>  						     acpi_adr_space_type
>>  						     space_id))
>>  ACPI_EXTERNAL_RETURN_STATUS(acpi_status
>> +			    acpi_execute_orphan_reg_method(acpi_handle device,
>> +							   acpi_adr_space_type
>> +							   space_id))
>> +ACPI_EXTERNAL_RETURN_STATUS(acpi_status
>>  			    acpi_remove_address_space_handler(acpi_handle
>>  							      device,
>>  							      acpi_adr_space_type
>> Index: linux-pm/drivers/acpi/acpica/acevents.h
>> ===================================================================
>> --- linux-pm.orig/drivers/acpi/acpica/acevents.h
>> +++ linux-pm/drivers/acpi/acpica/acevents.h
>> @@ -191,6 +191,10 @@ void
>>  acpi_ev_execute_reg_methods(struct acpi_namespace_node *node,
>>  			    acpi_adr_space_type space_id, u32 function);
>>  
>> +void
>> +acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node *node,
>> +				  acpi_adr_space_type space_id);
>> +
>>  acpi_status
>>  acpi_ev_execute_reg_method(union acpi_operand_object *region_obj, u32 function);
>>  
>> Index: linux-pm/drivers/acpi/acpica/evregion.c
>> ===================================================================
>> --- linux-pm.orig/drivers/acpi/acpica/evregion.c
>> +++ linux-pm/drivers/acpi/acpica/evregion.c
>> @@ -20,10 +20,6 @@ extern u8 acpi_gbl_default_address_space
>>  
>>  /* Local prototypes */
>>  
>> -static void
>> -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,
>>  		u32 level, void *context, void **return_value);
>> @@ -818,7 +814,7 @@ acpi_ev_reg_run(acpi_handle obj_handle,
>>   *
>>   ******************************************************************************/
>>  
>> -static void
>> +void
>>  acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node *device_node,
>>  				  acpi_adr_space_type space_id)
>>  {
>> Index: linux-pm/drivers/acpi/ec.c
>> ===================================================================
>> --- linux-pm.orig/drivers/acpi/ec.c
>> +++ linux-pm/drivers/acpi/ec.c
>> @@ -1507,6 +1507,9 @@ static int ec_install_handlers(struct ac
>>  
>>  	if (call_reg && !test_bit(EC_FLAGS_EC_REG_CALLED, &ec->flags)) {
>>  		acpi_execute_reg_methods(scope_handle, ACPI_ADR_SPACE_EC);
>> +		if (scope_handle != ec->handle)
>> +			acpi_execute_orphan_reg_method(ec->handle, ACPI_ADR_SPACE_EC);
>> +
>>  		set_bit(EC_FLAGS_EC_REG_CALLED, &ec->flags);
>>  	}
>>  
> 
> TL;DR: this change made me wonder about a possible issue, but all is
> fine, except that the code flow leading to "fine" is a bit convoluted.
> 
> I noticed this landing in Linus' tree and this got me thinking about
> if the "if (scope_handle != ec->handle)" would not cause the orphan
> _REG method to not get called for the case where the EC is created
> by parsing the ECDT early on, since then we set
> ec->handle = ACPI_ROOT_OBJECT for the ec object.
> 
> So I checked and acpi_ec_ecdt_probe() calls acpi_ec_setup(ec, NULL, false)
> with the false making call_reg above false, so the _REG methods do not
> get executed at this point.
> 
> Instead they should get executed when parsing the DSDT finds the EC ACPI
> device by acpi_ec_add() calling acpi_ec_setup(ec, device, true).
> 
> acpi_ec_add() updated ec->handle away from ACPI_ROOT_OBJECT to the actual
> acpi_device's handle before calling acpi_ec_setup(ec, device, true) so
> all should be well.
> 
> But while checkimg this I noticed a pre-existing issue where if the boot_ec
> object is created from the ECDT, so with ec->handle == NULL, acpi_ec_add()
> does not update ec->handle, which means an orphan _REG method will not get
> executed.
> 
> Specifically if this if condition evaluated to true, because the !strcmp()
> evaluates to true:
> 
>         if (boot_ec && (boot_ec->handle == device->handle ||
>             !strcmp(acpi_device_hid(device), ACPI_ECDT_HID))) {
>                 /* Fast path: this device corresponds to the boot EC. */
>                 ec = boot_ec;
>         } else {
> 
> then [boot_]ec->handle does not get set to device->handle .
> 
> So at a first glance it looks like we need something like this to make sure
> that any orphan _REG methods are also run in the described scenario:
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index e7793ee9e649..af61b9bb3749 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -1635,6 +1635,7 @@ static int acpi_ec_add(struct acpi_device *device)
>  	    !strcmp(acpi_device_hid(device), ACPI_ECDT_HID))) {
>  		/* Fast path: this device corresponds to the boot EC. */
>  		ec = boot_ec;
> +		ec->handle = device->handle;
>  	} else {
>  		acpi_status status;
>  
> 
> 
> But upon further checking the only place creating an acpi_device
> with an ACPI_ECDT_HID is acpi_ec_ecdt_start() which does:
> 
>         status = acpi_get_handle(NULL, ecdt_ptr->id, &handle);
>         if (ACPI_SUCCESS(status)) {
>                 boot_ec->handle = handle;
> 
>                 /* Add a special ACPI device object to represent the boot EC. */
>                 acpi_bus_register_early_device(ACPI_BUS_TYPE_ECDT_EC);
>         }
> 
> So boot_ec->handle gets updated in this case before acpi_ec_add() runs
> and everything is fine.
> 
> ...
> 
> Still I'm wondering if it would not be better to replace this
> "boot_ec->handle = handle;" in acpi_ec_ecdt_start() with the change
> from my proposed diff above, so that we consistently about the
> [boot_]ec->handle with device->handle for the being added acpi_device
> in both branches of the if ... else ... in acpi_ec_add() ?

Never mind that will not work because then we would set boot_ec->handle
to the handle of the special / fake ACPI device object added to
represent the boot EC, rather then to the handle corresponding to
ecdt_ptr->id, so this is a bad idea.

Regards,

Hans