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@xxxxxxxxxx> 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@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. > > 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@xxxxxxxxxx> >> 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@xxxxxxxxxx> >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> >> --- >> 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
