Re: [RFC][PATCH] ACPI: EC: Make evaluate acpi_ec_add() _REG for EC operation regions

[Hans de Goede]

Hi Rafael,

Sorry for the slow response...

On 7/7/22 21:31, Rafael J. Wysocki wrote:
> On Wed, Jul 6, 2022 at 10:26 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>>
>> Hi,
>>
>> On 7/6/22 14:37, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>>>
>>> acpi_ec_ecdt_probe() is called between acpi_load_tables() and
>>> acpi_enable_subsystem().  It passes ACPI_ROOT_OBJECT as ec->handle
>>> to acpi_ec_setup() and so ACPI_ROOT_OBJECT is passed to
>>> acpi_install_address_space_handler() via ec_install_handlers().
>>>
>>> Next, acpi_ns_validate_handle() converts it to acpi_gbl_root_node
>>> which is passed to acpi_ev_install_space_handler() and the handler is
>>> installed for acpi_gbl_root_node.
>>>
>>> Now, acpi_gbl_root_node is passed to acpi_ev_execute_reg_methods() which
>>> evaluates _REG for any ACPI_ADR_SPACE_EC regions it can find in the
>>> namespace which should not be necessary, because the OS is expected to
>>> make the ECDT operation regions available before evaluating any AML, so
>>> in particular AML is not expected to check the evaluation of _REG before
>>> it accesses these operation regions (see ACPI 6.4, Section 6.5.4,
>>> exception 2 [1]).  Doing that is also problematic, because the _REG
>>> methods for the ACPI_ADR_SPACE_EC regions may depend on various _INI, so
>>> they should be be evaluated before running acpi_initialize_objects() [2].
>>>
>>> Address this problem by modifying acpi_install_address_space_handler()
>>> to avoid evaluating _REG for ACPI_ADR_SPACE_EC regions when the handler
>>> is installed for acpi_gbl_root_node which indicates the ECDT case.
>>>
>>> However, this needs to be accompanied by an EC driver change to
>>> actually trigger the evaluation of _REG for the ACPI_ADR_SPACE_EC
>>> regions when it finds the EC object in the namespace.
>>>
>>> Link: https://uefi.org/specs/ACPI/6.4/06_Device_Configuration/Device_Configuration.html#reg-region # [1]
>>> Link: https://github.com/acpica/acpica/pull/786 # [2]
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>>> ---
>>>
>>> Note: This change doesn't make any practical difference on any of the systems
>>> in my office.
>>>
>>> ---
>>>  drivers/acpi/acpica/evxfregn.c |   12 ++++++++++++
>>>  drivers/acpi/ec.c              |    7 +++++++
>>>  2 files changed, 19 insertions(+)
>>>
>>> Index: linux-pm/drivers/acpi/ec.c
>>> ===================================================================
>>> --- linux-pm.orig/drivers/acpi/ec.c
>>> +++ linux-pm/drivers/acpi/ec.c
>>> @@ -1632,6 +1632,13 @@ static int acpi_ec_add(struct acpi_devic
>>>                       acpi_handle_debug(ec->handle, "duplicated.\n");
>>>                       acpi_ec_free(ec);
>>>                       ec = boot_ec;
>>> +                     /*
>>> +                      * Uninstall the EC address space handler and let
>>> +                      * acpi_ec_setup() install it again along with
>>> +                      * evaluating _REG methogs associated with
>>> +                      * ACPI_ADR_SPACE_EC operation regions.
>>> +                      */
>>> +                     ec_remove_handlers(ec);
>>
>> This will call the _REG method to get called with ACPI_REG_DISCONNECT (0)
>> as second argument which may lead to unexpected consequences so I'm not
>> in favor of doing things this way.
>>
>> IMHO it would be much better to instead have flags; or if flags are
>> disliked a separate function to only call _REG later on.
> 
> I'm aware of the _REG(EC, 0) part, but I thought that it might be the
> right thing to do.
> 
> First off, I'm a bit concerned about leaving the EC address space
> handler attached to the root node after we have discovered the proper
> EC object in the namespace, because that's inconsistent with the "no
> ECDT" case.

True, but in the ECDT case the EC opregion should work anywhere
according to the spec, so I believe it is consistent with the spec.

> It leaves a potential problem on the table too, because acpi_ec_add()
> changes boot_ec->handle from ACPI_ROOT_OBJECT to ec->handle and if
> ec_remove_handlers() is called for it after that, it will fail to
> remove the handler, but it will clear the
> EC_FLAGS_EC_HANDLER_INSTALLED flag (so the change above is actually
> incorrect, because it should remove the handler before changing
> boot_ec->handle).

You are right, but this can be fixed by keeping track of the handle
used when registering the handler, e.g. something like this: