* [RFC][PATCH] ACPI: EC: Make evaluate acpi_ec_add() _REG for EC operation regions
@ 2022-07-06 12:37 Rafael J. Wysocki
2022-07-06 20:26 ` Hans de Goede
0 siblings, 1 reply; 10+ messages in thread
From: Rafael J. Wysocki @ 2022-07-06 12:37 UTC (permalink / raw)
To: Hans de Goede, Linux ACPI; +Cc: LKML
From: Rafael J. Wysocki <rafael.j.wysocki@intel.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@intel.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);
}
}
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;
+ }
+
/* Run all _REG methods for this address space */
acpi_ev_execute_reg_methods(node, space_id, ACPI_REG_CONNECT);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC][PATCH] ACPI: EC: Make evaluate acpi_ec_add() _REG for EC operation regions
2022-07-06 12:37 [RFC][PATCH] ACPI: EC: Make evaluate acpi_ec_add() _REG for EC operation regions Rafael J. Wysocki
@ 2022-07-06 20:26 ` Hans de Goede
2022-07-07 19:31 ` Rafael J. Wysocki
0 siblings, 1 reply; 10+ messages in thread
From: Hans de Goede @ 2022-07-06 20:26 UTC (permalink / raw)
To: Rafael J. Wysocki, Linux ACPI; +Cc: LKML
Hi,
On 7/6/22 14:37, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.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@intel.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.
> }
> }
>
> 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.
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@redhat.com/
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@redhat.com/
Regards,
Hans
> /* Run all _REG methods for this address space */
>
> acpi_ev_execute_reg_methods(node, space_id, ACPI_REG_CONNECT);
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC][PATCH] ACPI: EC: Make evaluate acpi_ec_add() _REG for EC operation regions
2022-07-06 20:26 ` Hans de Goede
@ 2022-07-07 19:31 ` Rafael J. Wysocki
2022-08-04 11:57 ` Hans de Goede
0 siblings, 1 reply; 10+ messages in thread
From: Rafael J. Wysocki @ 2022-07-07 19:31 UTC (permalink / raw)
To: Hans de Goede; +Cc: Rafael J. Wysocki, Linux ACPI, LKML
On Wed, Jul 6, 2022 at 10:26 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 7/6/22 14:37, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.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@intel.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.
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).
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.
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).
Moreover, I don't quite like the ACPI_NO_INSTALL_SPACE_HANDLER flag,
because it causes the "handler installation" to actually do something
else.
> > }
> > }
> >
> > 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@redhat.com/
>
> 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@redhat.com/
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!
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC][PATCH] ACPI: EC: Make evaluate acpi_ec_add() _REG for EC operation regions
2022-07-07 19:31 ` Rafael J. Wysocki
@ 2022-08-04 11:57 ` Hans de Goede
2022-08-04 13:51 ` Rafael J. Wysocki
0 siblings, 1 reply; 10+ messages in thread
From: Hans de Goede @ 2022-08-04 11:57 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Rafael J. Wysocki, Linux ACPI, LKML
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@redhat.com> wrote:
>>
>> Hi,
>>
>> On 7/6/22 14:37, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.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@intel.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.
> 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@redhat.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@kernel.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.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 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@redhat.com/
>>
>> 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@redhat.com/
>
> 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!
>
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC][PATCH] ACPI: EC: Make evaluate acpi_ec_add() _REG for EC operation regions
2022-08-04 11:57 ` Hans de Goede
@ 2022-08-04 13:51 ` Rafael J. Wysocki
2022-08-04 13:57 ` Hans de Goede
0 siblings, 1 reply; 10+ messages in thread
From: Rafael J. Wysocki @ 2022-08-04 13:51 UTC (permalink / raw)
To: Hans de Goede; +Cc: Rafael J. Wysocki, Rafael J. Wysocki, Linux ACPI, LKML
Hi Hans,
On Thu, Aug 4, 2022 at 1:57 PM Hans de Goede <hdegoede@redhat.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@redhat.com> wrote:
> >>
> >> Hi,
> >>
> >> On 7/6/22 14:37, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.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@intel.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@redhat.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@kernel.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.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.
> 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.
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.
> 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.]
I would call it acpi_execute_reg_methods() then.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC][PATCH] ACPI: EC: Make evaluate acpi_ec_add() _REG for EC operation regions
2022-08-04 13:51 ` Rafael J. Wysocki
@ 2022-08-04 13:57 ` Hans de Goede
2022-08-04 14:08 ` Rafael J. Wysocki
0 siblings, 1 reply; 10+ messages in thread
From: Hans de Goede @ 2022-08-04 13:57 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Rafael J. Wysocki, Linux ACPI, LKML
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@redhat.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@redhat.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 7/6/22 14:37, Rafael J. Wysocki wrote:
>>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.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@intel.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@redhat.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@kernel.org>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC][PATCH] ACPI: EC: Make evaluate acpi_ec_add() _REG for EC operation regions
2022-08-04 13:57 ` Hans de Goede
@ 2022-08-04 14:08 ` Rafael J. Wysocki
2022-08-04 14:11 ` Rafael J. Wysocki
0 siblings, 1 reply; 10+ messages in thread
From: Rafael J. Wysocki @ 2022-08-04 14:08 UTC (permalink / raw)
To: Hans de Goede; +Cc: Rafael J. Wysocki, Rafael J. Wysocki, Linux ACPI, LKML
On Thu, Aug 4, 2022 at 3:57 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> 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@redhat.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@redhat.com> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> On 7/6/22 14:37, Rafael J. Wysocki wrote:
> >>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.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@intel.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@redhat.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@kernel.org>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.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.
Well, one more thing about it.
This would be a very generic interface with a very specific use case.
Moreover, the use case in question is already detectable in
acpi_install_address_space_handler().
Namely, the _REG evaluation can be skipped automatically if an
ACPI_ADR_SPACE_EC handler is installed at the root of the ACPI
namespace (because it doesn't even make sense to evaluate _REG then).
If this is done, we don't need the extra argument.
Hmm?
> >
> >> 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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC][PATCH] ACPI: EC: Make evaluate acpi_ec_add() _REG for EC operation regions
2022-08-04 14:08 ` Rafael J. Wysocki
@ 2022-08-04 14:11 ` Rafael J. Wysocki
2022-08-04 15:10 ` Hans de Goede
0 siblings, 1 reply; 10+ messages in thread
From: Rafael J. Wysocki @ 2022-08-04 14:11 UTC (permalink / raw)
To: Hans de Goede; +Cc: Rafael J. Wysocki, Linux ACPI, LKML
On Thu, Aug 4, 2022 at 4:08 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Aug 4, 2022 at 3:57 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > 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@redhat.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@redhat.com> wrote:
> > >>>>
> > >>>> Hi,
> > >>>>
> > >>>> On 7/6/22 14:37, Rafael J. Wysocki wrote:
> > >>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.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@intel.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@redhat.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@kernel.org>
> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.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.
>
> Well, one more thing about it.
>
> This would be a very generic interface with a very specific use case.
> Moreover, the use case in question is already detectable in
> acpi_install_address_space_handler().
>
> Namely, the _REG evaluation can be skipped automatically if an
> ACPI_ADR_SPACE_EC handler is installed at the root of the ACPI
> namespace (because it doesn't even make sense to evaluate _REG then).
> If this is done, we don't need the extra argument.
More specifically, bail out of acpi_ev_execute_reg_methods() early if
the space ID is ACPI_ADR_SPACE_EC and node is the namespace root, in
which case the EC address space can be regarded as a "must always be
accessible" one.
> Hmm?
>
> > >
> > >> 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
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC][PATCH] ACPI: EC: Make evaluate acpi_ec_add() _REG for EC operation regions
2022-08-04 14:11 ` Rafael J. Wysocki
@ 2022-08-04 15:10 ` Hans de Goede
2022-08-04 15:19 ` Rafael J. Wysocki
0 siblings, 1 reply; 10+ messages in thread
From: Hans de Goede @ 2022-08-04 15:10 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Rafael J. Wysocki, Linux ACPI, LKML
Hi,
On 8/4/22 16:11, Rafael J. Wysocki wrote:
> On Thu, Aug 4, 2022 at 4:08 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>
>> On Thu, Aug 4, 2022 at 3:57 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> 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@redhat.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@redhat.com> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 7/6/22 14:37, Rafael J. Wysocki wrote:
>>>>>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.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@intel.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@redhat.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@kernel.org>
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.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.
>>
>> Well, one more thing about it.
>>
>> This would be a very generic interface with a very specific use case.
>> Moreover, the use case in question is already detectable in
>> acpi_install_address_space_handler().
>>
>> Namely, the _REG evaluation can be skipped automatically if an
>> ACPI_ADR_SPACE_EC handler is installed at the root of the ACPI
>> namespace (because it doesn't even make sense to evaluate _REG then).
>> If this is done, we don't need the extra argument.
>
> More specifically, bail out of acpi_ev_execute_reg_methods() early if
> the space ID is ACPI_ADR_SPACE_EC and node is the namespace root, in
> which case the EC address space can be regarded as a "must always be
> accessible" one.
I'm not really in favor of hiding the conditions under which _REG
calling is skipped in this way.
If you look at this RFC patch int introduces a EC_FLAGS_EC_REG_CALLED
flag in drivers/acpi/ec.c and then later on uses that flag to
determine that _REG still needs to be called when ec_install_handlers()
is called the second time when actually probing/parsing the ACPI EC
object.
If we hide the conditions under which _REG is skipped inside
ACPICA, then determining when to call the new
acpi_execute_reg_methods() method is going to be somewhat tricky and
more over anyone reading the code then needs to also figure out that
acpica originally skipped this and under which conditions it was
orignally skipped.
IMHO having drivers/acpi/ec.c in full control over when to skip
(and thus also when to run _REG later) is cleaner then splitting
this over 2 different code bases.
Regards,
Hans
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC][PATCH] ACPI: EC: Make evaluate acpi_ec_add() _REG for EC operation regions
2022-08-04 15:10 ` Hans de Goede
@ 2022-08-04 15:19 ` Rafael J. Wysocki
0 siblings, 0 replies; 10+ messages in thread
From: Rafael J. Wysocki @ 2022-08-04 15:19 UTC (permalink / raw)
To: Hans de Goede; +Cc: Rafael J. Wysocki, Rafael J. Wysocki, Linux ACPI, LKML
On Thu, Aug 4, 2022 at 5:11 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 8/4/22 16:11, Rafael J. Wysocki wrote:
> > On Thu, Aug 4, 2022 at 4:08 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >>
> >> On Thu, Aug 4, 2022 at 3:57 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>>
> >>> 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@redhat.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@redhat.com> wrote:
> >>>>>>>
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> On 7/6/22 14:37, Rafael J. Wysocki wrote:
> >>>>>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.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@intel.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@redhat.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@kernel.org>
> >>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.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.
> >>
> >> Well, one more thing about it.
> >>
> >> This would be a very generic interface with a very specific use case.
> >> Moreover, the use case in question is already detectable in
> >> acpi_install_address_space_handler().
> >>
> >> Namely, the _REG evaluation can be skipped automatically if an
> >> ACPI_ADR_SPACE_EC handler is installed at the root of the ACPI
> >> namespace (because it doesn't even make sense to evaluate _REG then).
> >> If this is done, we don't need the extra argument.
> >
> > More specifically, bail out of acpi_ev_execute_reg_methods() early if
> > the space ID is ACPI_ADR_SPACE_EC and node is the namespace root, in
> > which case the EC address space can be regarded as a "must always be
> > accessible" one.
>
> I'm not really in favor of hiding the conditions under which _REG
> calling is skipped in this way.
>
> If you look at this RFC patch int introduces a EC_FLAGS_EC_REG_CALLED
> flag in drivers/acpi/ec.c and then later on uses that flag to
> determine that _REG still needs to be called when ec_install_handlers()
> is called the second time when actually probing/parsing the ACPI EC
> object.
>
> If we hide the conditions under which _REG is skipped inside
> ACPICA, then determining when to call the new
> acpi_execute_reg_methods() method is going to be somewhat tricky and
> more over anyone reading the code then needs to also figure out that
> acpica originally skipped this and under which conditions it was
> orignally skipped.
>
> IMHO having drivers/acpi/ec.c in full control over when to skip
> (and thus also when to run _REG later) is cleaner then splitting
> this over 2 different code bases.
OK
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-08-04 15:19 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-06 12:37 [RFC][PATCH] ACPI: EC: Make evaluate acpi_ec_add() _REG for EC operation regions Rafael J. Wysocki
2022-07-06 20:26 ` Hans de Goede
2022-07-07 19:31 ` Rafael J. Wysocki
2022-08-04 11:57 ` Hans de Goede
2022-08-04 13:51 ` Rafael J. Wysocki
2022-08-04 13:57 ` Hans de Goede
2022-08-04 14:08 ` Rafael J. Wysocki
2022-08-04 14:11 ` Rafael J. Wysocki
2022-08-04 15:10 ` Hans de Goede
2022-08-04 15:19 ` Rafael J. Wysocki
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.