[PATCH] ACPI: EC: Fix EC address space handler unregistration

[Hans de Goede]

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 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).

> 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.

> 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()
2. acpi_run_address_space_handler__reg()

?

Regards,

Hans


> 
>>>               }
>>>       }
>>>
>>> Index: linux-pm/drivers/acpi/acpica/evxfregn.c
>>> ===================================================================
>>> --- linux-pm.orig/drivers/acpi/acpica/evxfregn.c
>>> +++ linux-pm/drivers/acpi/acpica/evxfregn.c
>>> @@ -78,6 +78,18 @@ acpi_install_address_space_handler(acpi_
>>>               goto unlock_and_exit;
>>>       }
>>>
>>> +     /*
>>> +      * Avoid evaluating _REG methods if an EC address space handler is
>>> +      * installed for acpi_gbl_root_node, because this is done in order to
>>> +      * make Embedded Controller operation regions, accessed via the Embedded
>>> +      * Controllers described in ECDT, available early (see ACPI 6.4, Section
>>> +      * 6.5.4, exception 2).
>>> +      */
>>> +
>>> +     if (node == acpi_gbl_root_node || space_id == ACPI_ADR_SPACE_EC) {
>>> +             goto unlock_and_exit;
>>> +     }
>>> +
>>
>> Hmm, I like this in that it is KISS. But OTOH this does mean that
>> acpi_install_address_space_handler() now behaves differently depending on its
>> parameters in a possibly surprising way. So IMHO this feels a bit too clever
>> for our own good, since it may surprise the callers of this function.
>>
>> My biggest problem is, that as indicated above I believe that instead
>> of uninstalling + re-installing the handler we really need to have a way
>> to just call _REG later; and that in turn requires the caller to know if
>> _REG has run or not.
> 
> Well, as stated above, I think it would be prudent to move the handler
> under the EC object proper once it has been discovered.
> 
>> I've posted a new RFC patch series which adds flags to
>> acpi_install_address_space_handler() to not run / only run _REG :
>>
>> https://lore.kernel.org/linux-acpi/20220706201410.88244-1-hdegoede@xxxxxxxxxx/
>>
>> this then gets used in the drivers/acpi/ec.c patch to defer calling _REG when
>> registering the handler based on the ECDT until the DSDT EC entry is parsed.
>> I personally like how this turns out and IMHO this is cleaner (less hackish)
>> then the proposed solution with calling ec_remove_handlers(ec) :
>>
>> https://lore.kernel.org/linux-acpi/20220706201410.88244-3-hdegoede@xxxxxxxxxx/
> 
> Overall, I think that we'll need a new "no _REG" variant of
> acpi_install_address_space_handler(), at least for backward
> compatibility with other OSes using ACPICA, but I would call it
> acpi_install_address_space_handler_no_reg() and do all of the flags
> (or BOOL for that matter) passing internally in evxfregn.c.
> 
> Thanks!
>