Message-ID: <85cd941e-c892-677b-1582-c294c7efe770@redhat.com>
Date:   Thu, 4 Aug 2022 15:57:46 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Linux ACPI <linux-acpi@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC][PATCH] ACPI: EC: Make evaluate acpi_ec_add() _REG for EC
 operation regions

Hi,

On 8/4/22 15:51, Rafael J. Wysocki wrote:
> Hi Hans,
> 
> On Thu, Aug 4, 2022 at 1:57 PM Hans de Goede <hdegoede@...hat.com> wrote:
>>
>> Hi Rafael,
>>
>> Sorry for the slow response...
> 
> No sweat.
> 
>> On 7/7/22 21:31, Rafael J. Wysocki wrote:
>>> On Wed, Jul 6, 2022 at 10:26 PM Hans de Goede <hdegoede@...hat.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 7/6/22 14:37, Rafael J. Wysocki wrote:
>>>>> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>>>>>
>>>>> 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@...el.com>
>>>>> ---
>>>>>
>>>>> 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.
> 
> That's until the proper EC object is discovered, though.
> 
>>> 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:
>>
>> From fceb436703bc8f0e29b7613246a83c039b631cb4 Mon Sep 17 00:00:00 2001
>> From: Hans de Goede <hdegoede@...hat.com>
>> Date: Thu, 4 Aug 2022 13:38:35 +0200
>> Subject: [PATCH] ACPI: EC: Fix EC address space handler unregistration
>>
>> When an ECDT table is present the EC address space handler gets registered
>> on the root node. So to unregister it properly the unregister call also
>> must be done on the root node.
>>
>> Store the ACPI handle used for the acpi_install_address_space_handler()
>> call and use te same handle for the acpi_remove_address_space_handler()
>> call.
>>
>> Reported-by: Rafael J. Wysocki <rafael@...nel.org>
>> Signed-off-by: Hans de Goede <hdegoede@...hat.com>
>> ---
>>  drivers/acpi/ec.c       | 4 +++-
>>  drivers/acpi/internal.h | 1 +
>>  2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
>> index 1e93677e4b82..6aa8210501d3 100644
>> --- a/drivers/acpi/ec.c
>> +++ b/drivers/acpi/ec.c
>> @@ -1483,6 +1483,7 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device,
>>                         return -ENODEV;
>>                 }
>>                 set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
>> +               ec->address_space_handler_handle = ec->handle;
>>         }
>>
>>         if (call_reg && !test_bit(EC_FLAGS_EC_REG_CALLED, &ec->flags)) {
>> @@ -1543,7 +1544,8 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device,
>>  static void ec_remove_handlers(struct acpi_ec *ec)
>>  {
>>         if (test_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags)) {
>> -               if (ACPI_FAILURE(acpi_remove_address_space_handler(ec->handle,
>> +               if (ACPI_FAILURE(acpi_remove_address_space_handler(
>> +                                       ec->address_space_handler_handle,
>>                                         ACPI_ADR_SPACE_EC, &acpi_ec_space_handler)))
>>                         pr_err("failed to remove space handler\n");
>>                 clear_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
>> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
>> index 628bf8f18130..140af11d0c39 100644
>> --- a/drivers/acpi/internal.h
>> +++ b/drivers/acpi/internal.h
>> @@ -173,6 +173,7 @@ enum acpi_ec_event_state {
>>
>>  struct acpi_ec {
>>         acpi_handle handle;
>> +       acpi_handle address_space_handler_handle;
>>         int gpe;
>>         int irq;
>>         unsigned long command_addr;
>> --
> 
> This works.
> 
> I would rename address_space_handler_handle to something like
> address_space_handler_holder.

Ok, I'll rename this for the official upstream submission.

>> This fixes ec_remove_handlers() without requiring (IMHO) risky changes
>> where we call _REG() multiple times.
>>
>> Also note that ec_remove_handlers() is only ever called from
>> acpi_ec_driver.remove which in practice never runs since the EC never
>> gets hot unplugged (arguably the entire remove code could be removed).
> 
> Indeed.
> 
>>> But in order to move the EC address space handler under the EC object,
>>> it needs to be uninstalled and for this purpose AML needs to be told
>>> that it's not there, so evaluating _REG(EC, 0) seems reasonable to me
>>> even though I agree that it is somewhat risky.
>>
>> I'm pretty worried that calling _REG(EC, 0) will cause problems
>> the _REG handlers run pretty early on and various BIOS/ACPI table
>> authors seem to (ab)use this to do some sort of early setup
>> of some things in _REG, That is pretty much how this whole thread
>> has started. Given all the weirdness some ACPI tables do in their
>> _REG handling running _REG 3 times:
>>
>> 1. _REG(EC, 1)
>> 2. _REG(EC, 0)
>> 3. _REG(EC, 1)
>>
>> really seems like a bad idea to me. I have the feeling that this is
>> asking for trouble.
> 
> OK, fair enough.
> 
>>> Second, the spec is kind of suggesting doing it (cf. the "These
>>> operation regions may become inaccessible after OSPM runs
>>> _REG(EmbeddedControl, 0)" comment in the _REG definition section).
>>
>> That is boilerplate documentation copy and pasted from all the
>> other address space handlers the spec defines. I'm not sure if
>> Windows ever actually calls _REG(EmbeddedControl, 0) and I would
>> not be surprised if Windows does not do this.
>>
>>> Moreover, I don't quite like the ACPI_NO_INSTALL_SPACE_HANDLER flag,
>>> because it causes the "handler installation" to actually do something
>>> else.
>>
>> As mentioned before (IIRC) I would be more then happy to respin both
>> the ACPICA as well as the Linux ACPI bits to introduce / use 2 new
>> functions for this, lets say:
>>
>> 1. acpi_install_address_space_handler_no__reg()
> 
> So we need this in ACPICA, because it doesn't make sense to drop and
> re-acquire the namespace mutex around _REG evaluation in the non-EC
> case.

Right, just like the flags changes in this RFC getting this fixed
will require some work on the ACPICA side + then Linux changes
using the new ACPICA functions.

> But as stated before I would prefer to introduce an
> acpi_install_address_space_handler_internal() taking an additional
> BOOL run__reg argument  and I would define
> acpi_install_address_space_handler() and the new
> acpi_install_address_space_handler_no__reg() as wrappers around it.

Right, that is how it will look like inside ACPICA, but API consumers
will just see a new acpi_install_address_space_handler_no__reg()
getting introduced.

> 
>> 2. acpi_run_address_space_handler__reg()
> 
> So this would just be a wrapper around acpi_ev_execute_reg_methods()
> that would acquire the namespace mutex around it, right?  [I think
> that it should also acquire acpi_gbl_namespace_rw_lock along the lines
> of acpi_walk_namespace(), though.]

Ack.

> I would call it acpi_execute_reg_methods() then.

acpi_execute_reg_methods() works for me.

I'll try to prepare a new ACPICA pull-req with the discussed
changes + a Linux series on top sometime the coming few weeks.

Regards,

Hans

Powered by blists - more mailing lists