Make AddressSpaceHandler Install and _REG execution 2 separate steps #786

jwrdegoede · 2022-07-04T13:32:08Z

ACPI-2.0 says that the EC OpRegion handler must be available immediately
(like the standard default OpRegion handlers):

Quoting from the ACPI spec version 6.3: "6.5.4 _REG (Region) ...
2. OSPM must make Embedded Controller operation regions, accessed via
the Embedded Controllers described in ECDT, available before executing
any control method. These operation regions may become inaccessible
after OSPM runs _REG(EmbeddedControl, 0)."

So the OS must probe the ECDT described EC and install the OpRegion handler
before calling AcpiEnableSubsystem() and AcpiInitializeObjects().

This is a problem because calling AcpiInstallAddressSpaceHandler()
does not just install the OpRegion handler, it also runs the EC's _REG
method. This _REG method may rely on initialization done by the _INI
methods of one of the PCI / _SB root devices.

For the other early/default OpRegion handlers the OpRegion handler
install and the _REG execution is split into 2 separate steps:

AcpiEvInstallRegionHandlers(), called early from AcpiLoadTables()
AcpiEvInitializeOpRegions(), called from AcpiInitializeObjects()

To fix the EC OpRegion issue, add a new flags parameter to
AcpiInstallAddressSpaceHandler() to allow doing things in
2 steps for other OpRegion handlers, like the EC handler, too.

To avoid having to modify all AcpiInstallAddressSpaceHandler() callers,
the function is renamed to AcpiInstallAddressSpaceHandlerFlags() and
a static inline AcpiInstallAddressSpaceHandler() is provided.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=214899

…ate steps ACPICA pull-req: acpica/acpica#786 ACPI-2.0 says that the EC op_region handler must be available immediately (like the standard default op_region handlers): Quoting from the ACPI spec version 6.3: "6.5.4 _REG (Region) ... 2. OSPM must make Embedded Controller operation regions, accessed via the Embedded Controllers described in ECDT, available before executing any control method. These operation regions may become inaccessible after OSPM runs _REG(EmbeddedControl, 0)." So the OS must probe the ECDT described EC and install the op_region handler before calling acpi_enable_subsystem() and acpi_initialize_objects(). This is a problem because calling acpi_install_address_space_handler() does not just install the op_region handler, it also runs the EC's _REG method. This _REG method may rely on initialization done by the _INI methods of one of the PCI / _SB root devices. For the other early/default op_region handlers the op_region handler install and the _REG execution is split into 2 separate steps: 1. acpi_ev_install_region_handlers(), called early from acpi_load_tables() 2. acpi_ev_initialize_op_regions(), called from acpi_initialize_objects() To fix the EC op_region issue, add a new flags parameter to acpi_install_address_space_handler() to allow doing things in 2 steps for other op_region handlers, like the EC handler, too. To avoid having to modify all acpi_install_address_space_handler() callers, the function is renamed to acpi_install_address_space_handler_flags() and a static inline acpi_install_address_space_handler() is provided. bug_link: https://bugzilla.kernel.org/show_bug.cgi?id=214899 Link: acpica/acpica@a44f29d0 Reported-and-tested-by: Johannes Penßel <johannespenssel@posteo.net> Signed-off-by: Hans de Goede <hdegoede@redhat.com> Signed-off-by: Bob Moore <robert.moore@intel.com> Signed-off-by: <hdegoede@redhat.com>

rafaeljw

Linux calls acpi_ec_ecdt_probe() between acpi_load_tables() and acpi_enable_subsystem() (first invocation). It passes ACPI_ROOT_OBJECT as ec->handle to acpi_ec_setup() and so that handle is passed to acpi_install_address_space_handler() via ec_install_handlers().

Next, acpi_ns_validate_handle() converts that 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 executes _REG for any ACPI_ADR_SPACE_EC regions it can find in the namespace which should not be necessary, because AML may assume these regions to be always present until _REG(EC, 0) is evaluated for them (apart from being dependent on various _INI).

To me, AcpiInstallAddressSpaceHandler() could be made skip the _REG evaluation if the node is AcpiGblRootNode and the handler is not a default one, or at least when node is AcpiGblRootNode and the address space is ACPI_ADR_SPACE_EC.

Now, obviously Linux will have to evaluate these _REG methods eventually when it finds the EC object in the namespace, but IIUC this can be triggered by calling ec_remove_handlers() from acpi_ec_add() in the case when the "boot EC" is going to be used instead of the newly allocated one.

rafaeljw · 2022-07-06T12:41:05Z

I've just posted an alternative Linux patch: https://patchwork.kernel.org/project/linux-acpi/patch/5592689.DvuYhMxLoT@kreacher/

rafaeljw · 2022-07-06T13:21:03Z

That said IMV it may be beneficial to add AcpiInstallAddressSpaceHandlerFlags() for better backwards compatibility with non-Linux hosts using ACPICA, but the only flag needed for it would be ACPI_NO__REG.

…ate steps ACPICA pull-req: acpica/acpica#786 ACPI-2.0 says that the EC op_region handler must be available immediately (like the standard default op_region handlers): Quoting from the ACPI spec version 6.3: "6.5.4 _REG (Region) ... 2. OSPM must make Embedded Controller operation regions, accessed via the Embedded Controllers described in ECDT, available before executing any control method. These operation regions may become inaccessible after OSPM runs _REG(EmbeddedControl, 0)." So the OS must probe the ECDT described EC and install the op_region handler before calling acpi_enable_subsystem() and acpi_initialize_objects(). This is a problem because calling acpi_install_address_space_handler() does not just install the op_region handler, it also runs the EC's _REG method. This _REG method may rely on initialization done by the _INI methods of one of the PCI / _SB root devices. For the other early/default op_region handlers the op_region handler install and the _REG execution is split into 2 separate steps: 1. acpi_ev_install_region_handlers(), called early from acpi_load_tables() 2. acpi_ev_initialize_op_regions(), called from acpi_initialize_objects() To fix the EC op_region issue, add a new flags parameter to acpi_install_address_space_handler() to allow doing things in 2 steps for other op_region handlers, like the EC handler, too. To avoid having to modify all acpi_install_address_space_handler() callers, the function is renamed to acpi_install_address_space_handler_flags() and a static inline acpi_install_address_space_handler() is provided. bug_link: https://bugzilla.kernel.org/show_bug.cgi?id=214899 Link: acpica/acpica@a44f29d0 Reported-and-tested-by: Johannes Penßel <johannespenssel@posteo.net> Signed-off-by: Hans de Goede <hdegoede@redhat.com> Signed-off-by: Bob Moore <robert.moore@intel.com> Signed-off-by: <hdegoede@redhat.com>

jwrdegoede · 2022-07-06T20:27:41Z

I've just posted an alternative Linux patch: https://patchwork.kernel.org/project/linux-acpi/patch/5592689.DvuYhMxLoT@kreacher/

To keep everyone in the loop let me copy and paste my mailinglist reply to this here:

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

Clickable version of the linked patch-series:
https://lore.kernel.org/linux-acpi/20220706201410.88244-1-hdegoede@redhat.com/
https://lore.kernel.org/linux-acpi/20220706201410.88244-2-hdegoede@redhat.com/
https://lore.kernel.org/linux-acpi/20220706201410.88244-3-hdegoede@redhat.com/

…ate steps ACPICA pull-req: acpica/acpica#786 ACPI-2.0 says that the EC op_region handler must be available immediately (like the standard default op_region handlers): Quoting from the ACPI spec version 6.3: "6.5.4 _REG (Region) ... 2. OSPM must make Embedded Controller operation regions, accessed via the Embedded Controllers described in ECDT, available before executing any control method. These operation regions may become inaccessible after OSPM runs _REG(EmbeddedControl, 0)." So the OS must probe the ECDT described EC and install the op_region handler before calling acpi_enable_subsystem() and acpi_initialize_objects(). This is a problem because calling acpi_install_address_space_handler() does not just install the op_region handler, it also runs the EC's _REG method. This _REG method may rely on initialization done by the _INI methods of one of the PCI / _SB root devices. For the other early/default op_region handlers the op_region handler install and the _REG execution is split into 2 separate steps: 1. acpi_ev_install_region_handlers(), called early from acpi_load_tables() 2. acpi_ev_initialize_op_regions(), called from acpi_initialize_objects() To fix the EC op_region issue, add a new flags parameter to acpi_install_address_space_handler() to allow doing things in 2 steps for other op_region handlers, like the EC handler, too. To avoid having to modify all acpi_install_address_space_handler() callers, the function is renamed to acpi_install_address_space_handler_flags() and a static inline acpi_install_address_space_handler() is provided. bug_link: https://bugzilla.kernel.org/show_bug.cgi?id=214899 Link: acpica/acpica@a44f29d0 Reported-and-tested-by: Johannes Penßel <johannespenssel@posteo.net> Signed-off-by: Hans de Goede <hdegoede@redhat.com> Signed-off-by: Bob Moore <robert.moore@intel.com> Signed-off-by: <hdegoede@redhat.com>

ACPI-2.0 says that the EC OpRegion handler must be available immediately (like the standard default OpRegion handlers): Quoting from the ACPI spec version 6.3: "6.5.4 _REG (Region) ... 2. OSPM must make Embedded Controller operation regions, accessed via the Embedded Controllers described in ECDT, available before executing any control method. These operation regions may become inaccessible after OSPM runs _REG(EmbeddedControl, 0)." So the OS must probe the ECDT described EC and install the OpRegion handler before calling AcpiEnableSubsystem() and AcpiInitializeObjects(). This is a problem because calling AcpiInstallAddressSpaceHandler() does not just install the OpRegion handler, it also runs the EC's _REG method. This _REG method may rely on initialization done by the _INI methods of one of the PCI / _SB root devices. For the other early/default OpRegion handlers the OpRegion handler install and the _REG execution is split into 2 separate steps: 1. AcpiEvInstallRegionHandlers(), called early from AcpiLoadTables() 2. AcpiEvInitializeOpRegions(), called from AcpiInitializeObjects() To fix the EC OpRegion issue, add 2 bew functions: 1. AcpiInstallAddressSpaceHandlerNo_Reg() 2. AcpiExecuteRegMethods() to allow doing things in 2 steps for other OpRegion handlers, like the EC handler, too. Note that the comment describing AcpiEvInstallRegionHandlers() even has an alinea describing this problem. Using the new methods allows users to avoid this problem. BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=214899 Reported-and-tested-by: Johannes Penßel <johannespenssel@posteo.net> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

jwrdegoede · 2022-10-03T13:38:45Z

I have just pushed a new version which adds 2 new function (wrappers) instead of using the flags approach as discussed on the linux-acpi list: https://lore.kernel.org/linux-acpi/5592689.DvuYhMxLoT@kreacher/T/#u

The 2 new functions added are called:

AcpiInstallAddressSpaceHandlerNo_Reg()
AcpiExecuteRegMethods()

rafaeljw · 2022-10-13T17:11:48Z

Bob, this is part of an EC driver fix in Linux, please prioritize.

rafaeljw · 2022-10-24T16:18:56Z

Bob, any concerns here?

rafaeljw · 2022-10-24T16:19:52Z

@acpibob : Any concerns?

jwrdegoede · 2022-11-15T16:26:46Z

Hello, is anyone following this pull-req ? Can we please get someone to look at this / get this merged ?

rafaeljw · 2022-12-08T12:09:12Z

@jwrdegoede AFAICS this change has no functional impact on the upstream, just some code reorganization needed to fix Linux properly, so I see no reason to wait with merging it into Linux any more.

Please send a Linux counterpart of it with a pointer to this PR in a Link tag to linux-acpi@vger.kernel.org.

…eparate steps ACPI-2.0 says that the EC op_region handler must be available immediately (like the standard default op_region handlers): Quoting from the ACPI spec version 6.3: "6.5.4 _REG (Region) ... 2. OSPM must make Embedded Controller operation regions, accessed via the Embedded Controllers described in ECDT, available before executing any control method. These operation regions may become inaccessible after OSPM runs _REG(EmbeddedControl, 0)." So the OS must probe the ECDT described EC and install the OpRegion hdlr before calling acpi_enable_subsystem() and acpi_initialize_objects(). This is a problem because calling acpi_install_address_space_handler() does not just install the op_region handler, it also runs the EC's _REG method. This _REG method may rely on initialization done by the _INI methods of one of the PCI / _SB root devices. For the other early/default op_region handlers the op_region handler install and the _REG execution is split into 2 separate steps: 1. acpi_ev_install_region_handlers(), called early from acpi_load_tables() 2. acpi_ev_initialize_op_regions(), called from acpi_initialize_objects() To fix the EC op_region issue, add 2 bew functions: 1. acpi_install_address_space_handler_no_Reg() 2. acpi_execute_reg_methods() to allow doing things in 2 steps for other op_region handlers, like the EC handler, too. Note that the comment describing acpi_ev_install_region_handlers() even has an alinea describing this problem. Using the new methods allows users to avoid this problem. Link: acpica/acpica#786 Link: https://bugzilla.kernel.org/show_bug.cgi?id=214899 Reported-and-tested-by: Johannes Penßel <johannespenssel@posteo.net> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

…eparate steps ACPI-2.0 says that the EC op_region handler must be available immediately (like the standard default op_region handlers): Quoting from the ACPI spec version 6.3: "6.5.4 _REG (Region) ... 2. OSPM must make Embedded Controller operation regions, accessed via the Embedded Controllers described in ECDT, available before executing any control method. These operation regions may become inaccessible after OSPM runs _REG(EmbeddedControl, 0)." So the OS must probe the ECDT described EC and install the OpRegion handler before calling acpi_enable_subsystem() and acpi_initialize_objects(). This is a problem because calling acpi_install_address_space_handler() does not just install the op_region handler, it also runs the EC's _REG method. This _REG method may rely on initialization done by the _INI methods of one of the PCI / _SB root devices. For the other early/default op_region handlers the op_region handler install and the _REG execution is split into 2 separate steps: 1. acpi_ev_install_region_handlers(), called early from acpi_load_tables() 2. acpi_ev_initialize_op_regions(), called from acpi_initialize_objects() To fix the EC op_region issue, add 2 bew functions: 1. acpi_install_address_space_handler_no_reg() 2. acpi_execute_reg_methods() to allow doing things in 2 steps for other op_region handlers, like the EC handler, too. Note that the comment describing acpi_ev_install_region_handlers() even has an alinea describing this problem. Using the new methods allows users to avoid this problem. Link: acpica/acpica#786 Link: https://bugzilla.kernel.org/show_bug.cgi?id=214899 Reported-and-tested-by: Johannes Penßel <johannespenssel@posteo.net> Signed-off-by: Hans de Goede <hdegoede@redhat.com> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

rafaeljw reviewed Jul 6, 2022

View reviewed changes

jwrdegoede force-pushed the split-acpi_install_address_space_handler branch from e427078 to ead772d Compare October 3, 2022 13:35

rafaeljw approved these changes Oct 13, 2022

View reviewed changes

acpibob merged commit 2b00f73 into acpica:master Feb 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make AddressSpaceHandler Install and _REG execution 2 separate steps #786

Make AddressSpaceHandler Install and _REG execution 2 separate steps #786

jwrdegoede commented Jul 4, 2022

rafaeljw left a comment •

edited

rafaeljw commented Jul 6, 2022

rafaeljw commented Jul 6, 2022

jwrdegoede commented Jul 6, 2022

jwrdegoede commented Oct 3, 2022

rafaeljw commented Oct 13, 2022

rafaeljw commented Oct 24, 2022

rafaeljw commented Oct 24, 2022

jwrdegoede commented Nov 15, 2022

rafaeljw commented Dec 8, 2022

Make AddressSpaceHandler Install and _REG execution 2 separate steps #786

Make AddressSpaceHandler Install and _REG execution 2 separate steps #786

Conversation

jwrdegoede commented Jul 4, 2022

rafaeljw left a comment • edited

Choose a reason for hiding this comment

rafaeljw commented Jul 6, 2022

rafaeljw commented Jul 6, 2022

jwrdegoede commented Jul 6, 2022

jwrdegoede commented Oct 3, 2022

rafaeljw commented Oct 13, 2022

rafaeljw commented Oct 24, 2022

rafaeljw commented Oct 24, 2022

jwrdegoede commented Nov 15, 2022

rafaeljw commented Dec 8, 2022

rafaeljw left a comment •

edited