| Message ID | 12466682.O9o76ZdvQC@kreacher |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] ACPI: EC: Evaluate orphan _REG under EC device | expand |
Hi, On 6/12/24 4:15 PM, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > After starting to install the EC address space handler at the ACPI > namespace root, if there is an "orphan" _REG method in the EC device's > scope, it will not be evaluated any more. This breaks EC operation > regions on some systems, like Asus gu605. > > To address this, use a wrapper around an existing ACPICA function to > look for an "orphan" _REG method in the EC device scope and evaluate > it if present. > > Fixes: 60fa6ae6e6d0 ("ACPI: EC: Install address space handler at the namespace root") > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218945 > Reported-by: VitaliiT <vitaly.torshyn@gmail.com> > Tested-by: VitaliiT <vitaly.torshyn@gmail.com> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > > Yes, this includes ACPICA changes that are obviously not upstream > and I am going to take care of pusing them to upstream, but for > now there is a regression to fix and it is relatively late in the > cycle. > > --- > drivers/acpi/acpica/acevents.h | 4 +++ > drivers/acpi/acpica/evregion.c | 6 ---- > drivers/acpi/acpica/evxfregn.c | 54 +++++++++++++++++++++++++++++++++++++++++ > drivers/acpi/ec.c | 3 ++ > include/acpi/acpixf.h | 4 +++ > 5 files changed, 66 insertions(+), 5 deletions(-) > > Index: linux-pm/drivers/acpi/acpica/evxfregn.c > =================================================================== > --- linux-pm.orig/drivers/acpi/acpica/evxfregn.c > +++ linux-pm/drivers/acpi/acpica/evxfregn.c > @@ -306,3 +306,57 @@ acpi_execute_reg_methods(acpi_handle dev > } > > ACPI_EXPORT_SYMBOL(acpi_execute_reg_methods) > + > +/******************************************************************************* > + * > + * FUNCTION: acpi_execute_orphan_reg_method > + * > + * PARAMETERS: device - Handle for the device > + * space_id - The address space ID > + * > + * RETURN: Status > + * > + * DESCRIPTION: Execute an "orphan" _REG method that appears under an ACPI > + * device. This is a _REG method that has no corresponding region > + * within the device's scope. > + * > + ******************************************************************************/ > +acpi_status > +acpi_execute_orphan_reg_method(acpi_handle device, acpi_adr_space_type space_id) > +{ > + struct acpi_namespace_node *node; > + acpi_status status; > + > + ACPI_FUNCTION_TRACE(acpi_execute_orphan_reg_method); > + > + /* Parameter validation */ > + > + if (!device) { > + return_ACPI_STATUS(AE_BAD_PARAMETER); > + } > + > + status = acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE); > + if (ACPI_FAILURE(status)) { > + return_ACPI_STATUS(status); > + } > + > + /* Convert and validate the device handle */ > + > + node = acpi_ns_validate_handle(device); > + if (node) { > + > + /* > + * If an "orphan" _REG method is present in the device's scope > + * for the given address space ID, run it. > + */ > + > + acpi_ev_execute_orphan_reg_method(node, space_id); > + } else { > + status = AE_BAD_PARAMETER; > + } > + > + (void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE); > + return_ACPI_STATUS(status); > +} > + > +ACPI_EXPORT_SYMBOL(acpi_execute_orphan_reg_method) > Index: linux-pm/include/acpi/acpixf.h > =================================================================== > --- linux-pm.orig/include/acpi/acpixf.h > +++ linux-pm/include/acpi/acpixf.h > @@ -663,6 +663,10 @@ ACPI_EXTERNAL_RETURN_STATUS(acpi_status > acpi_adr_space_type > space_id)) > ACPI_EXTERNAL_RETURN_STATUS(acpi_status > + acpi_execute_orphan_reg_method(acpi_handle device, > + acpi_adr_space_type > + space_id)) > +ACPI_EXTERNAL_RETURN_STATUS(acpi_status > acpi_remove_address_space_handler(acpi_handle > device, > acpi_adr_space_type > Index: linux-pm/drivers/acpi/acpica/acevents.h > =================================================================== > --- linux-pm.orig/drivers/acpi/acpica/acevents.h > +++ linux-pm/drivers/acpi/acpica/acevents.h > @@ -191,6 +191,10 @@ void > acpi_ev_execute_reg_methods(struct acpi_namespace_node *node, > acpi_adr_space_type space_id, u32 function); > > +void > +acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node *node, > + acpi_adr_space_type space_id); > + > acpi_status > acpi_ev_execute_reg_method(union acpi_operand_object *region_obj, u32 function); > > Index: linux-pm/drivers/acpi/acpica/evregion.c > =================================================================== > --- linux-pm.orig/drivers/acpi/acpica/evregion.c > +++ linux-pm/drivers/acpi/acpica/evregion.c > @@ -20,10 +20,6 @@ extern u8 acpi_gbl_default_address_space > > /* Local prototypes */ > > -static void > -acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node *device_node, > - acpi_adr_space_type space_id); > - > static acpi_status > acpi_ev_reg_run(acpi_handle obj_handle, > u32 level, void *context, void **return_value); > @@ -818,7 +814,7 @@ acpi_ev_reg_run(acpi_handle obj_handle, > * > ******************************************************************************/ > > -static void > +void > acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node *device_node, > acpi_adr_space_type space_id) > { > Index: linux-pm/drivers/acpi/ec.c > =================================================================== > --- linux-pm.orig/drivers/acpi/ec.c > +++ linux-pm/drivers/acpi/ec.c > @@ -1507,6 +1507,9 @@ static int ec_install_handlers(struct ac > > if (call_reg && !test_bit(EC_FLAGS_EC_REG_CALLED, &ec->flags)) { > acpi_execute_reg_methods(scope_handle, ACPI_ADR_SPACE_EC); > + if (scope_handle != ec->handle) > + acpi_execute_orphan_reg_method(ec->handle, ACPI_ADR_SPACE_EC); > + > set_bit(EC_FLAGS_EC_REG_CALLED, &ec->flags); > } > TL;DR: this change made me wonder about a possible issue, but all is fine, except that the code flow leading to "fine" is a bit convoluted. I noticed this landing in Linus' tree and this got me thinking about if the "if (scope_handle != ec->handle)" would not cause the orphan _REG method to not get called for the case where the EC is created by parsing the ECDT early on, since then we set ec->handle = ACPI_ROOT_OBJECT for the ec object. So I checked and acpi_ec_ecdt_probe() calls acpi_ec_setup(ec, NULL, false) with the false making call_reg above false, so the _REG methods do not get executed at this point. Instead they should get executed when parsing the DSDT finds the EC ACPI device by acpi_ec_add() calling acpi_ec_setup(ec, device, true). acpi_ec_add() updated ec->handle away from ACPI_ROOT_OBJECT to the actual acpi_device's handle before calling acpi_ec_setup(ec, device, true) so all should be well. But while checkimg this I noticed a pre-existing issue where if the boot_ec object is created from the ECDT, so with ec->handle == NULL, acpi_ec_add() does not update ec->handle, which means an orphan _REG method will not get executed. Specifically if this if condition evaluated to true, because the !strcmp() evaluates to true: if (boot_ec && (boot_ec->handle == device->handle || !strcmp(acpi_device_hid(device), ACPI_ECDT_HID))) { /* Fast path: this device corresponds to the boot EC. */ ec = boot_ec; } else { then [boot_]ec->handle does not get set to device->handle . So at a first glance it looks like we need something like this to make sure that any orphan _REG methods are also run in the described scenario: diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index e7793ee9e649..af61b9bb3749 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -1635,6 +1635,7 @@ static int acpi_ec_add(struct acpi_device *device) !strcmp(acpi_device_hid(device), ACPI_ECDT_HID))) { /* Fast path: this device corresponds to the boot EC. */ ec = boot_ec; + ec->handle = device->handle; } else { acpi_status status; But upon further checking the only place creating an acpi_device with an ACPI_ECDT_HID is acpi_ec_ecdt_start() which does: status = acpi_get_handle(NULL, ecdt_ptr->id, &handle); if (ACPI_SUCCESS(status)) { boot_ec->handle = handle; /* Add a special ACPI device object to represent the boot EC. */ acpi_bus_register_early_device(ACPI_BUS_TYPE_ECDT_EC); } So boot_ec->handle gets updated in this case before acpi_ec_add() runs and everything is fine. ... Still I'm wondering if it would not be better to replace this "boot_ec->handle = handle;" in acpi_ec_ecdt_start() with the change from my proposed diff above, so that we consistently about the [boot_]ec->handle with device->handle for the being added acpi_device in both branches of the if ... else ... in acpi_ec_add() ? Regards, Hans
Hi, On 6/16/24 11:47 AM, Hans de Goede wrote: > Hi, > > On 6/12/24 4:15 PM, Rafael J. Wysocki wrote: >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> >> After starting to install the EC address space handler at the ACPI >> namespace root, if there is an "orphan" _REG method in the EC device's >> scope, it will not be evaluated any more. This breaks EC operation >> regions on some systems, like Asus gu605. >> >> To address this, use a wrapper around an existing ACPICA function to >> look for an "orphan" _REG method in the EC device scope and evaluate >> it if present. >> >> Fixes: 60fa6ae6e6d0 ("ACPI: EC: Install address space handler at the namespace root") >> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218945 >> Reported-by: VitaliiT <vitaly.torshyn@gmail.com> >> Tested-by: VitaliiT <vitaly.torshyn@gmail.com> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> --- >> >> Yes, this includes ACPICA changes that are obviously not upstream >> and I am going to take care of pusing them to upstream, but for >> now there is a regression to fix and it is relatively late in the >> cycle. >> >> --- >> drivers/acpi/acpica/acevents.h | 4 +++ >> drivers/acpi/acpica/evregion.c | 6 ---- >> drivers/acpi/acpica/evxfregn.c | 54 +++++++++++++++++++++++++++++++++++++++++ >> drivers/acpi/ec.c | 3 ++ >> include/acpi/acpixf.h | 4 +++ >> 5 files changed, 66 insertions(+), 5 deletions(-) >> >> Index: linux-pm/drivers/acpi/acpica/evxfregn.c >> =================================================================== >> --- linux-pm.orig/drivers/acpi/acpica/evxfregn.c >> +++ linux-pm/drivers/acpi/acpica/evxfregn.c >> @@ -306,3 +306,57 @@ acpi_execute_reg_methods(acpi_handle dev >> } >> >> ACPI_EXPORT_SYMBOL(acpi_execute_reg_methods) >> + >> +/******************************************************************************* >> + * >> + * FUNCTION: acpi_execute_orphan_reg_method >> + * >> + * PARAMETERS: device - Handle for the device >> + * space_id - The address space ID >> + * >> + * RETURN: Status >> + * >> + * DESCRIPTION: Execute an "orphan" _REG method that appears under an ACPI >> + * device. This is a _REG method that has no corresponding region >> + * within the device's scope. >> + * >> + ******************************************************************************/ >> +acpi_status >> +acpi_execute_orphan_reg_method(acpi_handle device, acpi_adr_space_type space_id) >> +{ >> + struct acpi_namespace_node *node; >> + acpi_status status; >> + >> + ACPI_FUNCTION_TRACE(acpi_execute_orphan_reg_method); >> + >> + /* Parameter validation */ >> + >> + if (!device) { >> + return_ACPI_STATUS(AE_BAD_PARAMETER); >> + } >> + >> + status = acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE); >> + if (ACPI_FAILURE(status)) { >> + return_ACPI_STATUS(status); >> + } >> + >> + /* Convert and validate the device handle */ >> + >> + node = acpi_ns_validate_handle(device); >> + if (node) { >> + >> + /* >> + * If an "orphan" _REG method is present in the device's scope >> + * for the given address space ID, run it. >> + */ >> + >> + acpi_ev_execute_orphan_reg_method(node, space_id); >> + } else { >> + status = AE_BAD_PARAMETER; >> + } >> + >> + (void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE); >> + return_ACPI_STATUS(status); >> +} >> + >> +ACPI_EXPORT_SYMBOL(acpi_execute_orphan_reg_method) >> Index: linux-pm/include/acpi/acpixf.h >> =================================================================== >> --- linux-pm.orig/include/acpi/acpixf.h >> +++ linux-pm/include/acpi/acpixf.h >> @@ -663,6 +663,10 @@ ACPI_EXTERNAL_RETURN_STATUS(acpi_status >> acpi_adr_space_type >> space_id)) >> ACPI_EXTERNAL_RETURN_STATUS(acpi_status >> + acpi_execute_orphan_reg_method(acpi_handle device, >> + acpi_adr_space_type >> + space_id)) >> +ACPI_EXTERNAL_RETURN_STATUS(acpi_status >> acpi_remove_address_space_handler(acpi_handle >> device, >> acpi_adr_space_type >> Index: linux-pm/drivers/acpi/acpica/acevents.h >> =================================================================== >> --- linux-pm.orig/drivers/acpi/acpica/acevents.h >> +++ linux-pm/drivers/acpi/acpica/acevents.h >> @@ -191,6 +191,10 @@ void >> acpi_ev_execute_reg_methods(struct acpi_namespace_node *node, >> acpi_adr_space_type space_id, u32 function); >> >> +void >> +acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node *node, >> + acpi_adr_space_type space_id); >> + >> acpi_status >> acpi_ev_execute_reg_method(union acpi_operand_object *region_obj, u32 function); >> >> Index: linux-pm/drivers/acpi/acpica/evregion.c >> =================================================================== >> --- linux-pm.orig/drivers/acpi/acpica/evregion.c >> +++ linux-pm/drivers/acpi/acpica/evregion.c >> @@ -20,10 +20,6 @@ extern u8 acpi_gbl_default_address_space >> >> /* Local prototypes */ >> >> -static void >> -acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node *device_node, >> - acpi_adr_space_type space_id); >> - >> static acpi_status >> acpi_ev_reg_run(acpi_handle obj_handle, >> u32 level, void *context, void **return_value); >> @@ -818,7 +814,7 @@ acpi_ev_reg_run(acpi_handle obj_handle, >> * >> ******************************************************************************/ >> >> -static void >> +void >> acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node *device_node, >> acpi_adr_space_type space_id) >> { >> Index: linux-pm/drivers/acpi/ec.c >> =================================================================== >> --- linux-pm.orig/drivers/acpi/ec.c >> +++ linux-pm/drivers/acpi/ec.c >> @@ -1507,6 +1507,9 @@ static int ec_install_handlers(struct ac >> >> if (call_reg && !test_bit(EC_FLAGS_EC_REG_CALLED, &ec->flags)) { >> acpi_execute_reg_methods(scope_handle, ACPI_ADR_SPACE_EC); >> + if (scope_handle != ec->handle) >> + acpi_execute_orphan_reg_method(ec->handle, ACPI_ADR_SPACE_EC); >> + >> set_bit(EC_FLAGS_EC_REG_CALLED, &ec->flags); >> } >> > > TL;DR: this change made me wonder about a possible issue, but all is > fine, except that the code flow leading to "fine" is a bit convoluted. > > I noticed this landing in Linus' tree and this got me thinking about > if the "if (scope_handle != ec->handle)" would not cause the orphan > _REG method to not get called for the case where the EC is created > by parsing the ECDT early on, since then we set > ec->handle = ACPI_ROOT_OBJECT for the ec object. > > So I checked and acpi_ec_ecdt_probe() calls acpi_ec_setup(ec, NULL, false) > with the false making call_reg above false, so the _REG methods do not > get executed at this point. > > Instead they should get executed when parsing the DSDT finds the EC ACPI > device by acpi_ec_add() calling acpi_ec_setup(ec, device, true). > > acpi_ec_add() updated ec->handle away from ACPI_ROOT_OBJECT to the actual > acpi_device's handle before calling acpi_ec_setup(ec, device, true) so > all should be well. > > But while checkimg this I noticed a pre-existing issue where if the boot_ec > object is created from the ECDT, so with ec->handle == NULL, acpi_ec_add() > does not update ec->handle, which means an orphan _REG method will not get > executed. > > Specifically if this if condition evaluated to true, because the !strcmp() > evaluates to true: > > if (boot_ec && (boot_ec->handle == device->handle || > !strcmp(acpi_device_hid(device), ACPI_ECDT_HID))) { > /* Fast path: this device corresponds to the boot EC. */ > ec = boot_ec; > } else { > > then [boot_]ec->handle does not get set to device->handle . > > So at a first glance it looks like we need something like this to make sure > that any orphan _REG methods are also run in the described scenario: > > diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c > index e7793ee9e649..af61b9bb3749 100644 > --- a/drivers/acpi/ec.c > +++ b/drivers/acpi/ec.c > @@ -1635,6 +1635,7 @@ static int acpi_ec_add(struct acpi_device *device) > !strcmp(acpi_device_hid(device), ACPI_ECDT_HID))) { > /* Fast path: this device corresponds to the boot EC. */ > ec = boot_ec; > + ec->handle = device->handle; > } else { > acpi_status status; > > > > But upon further checking the only place creating an acpi_device > with an ACPI_ECDT_HID is acpi_ec_ecdt_start() which does: > > status = acpi_get_handle(NULL, ecdt_ptr->id, &handle); > if (ACPI_SUCCESS(status)) { > boot_ec->handle = handle; > > /* Add a special ACPI device object to represent the boot EC. */ > acpi_bus_register_early_device(ACPI_BUS_TYPE_ECDT_EC); > } > > So boot_ec->handle gets updated in this case before acpi_ec_add() runs > and everything is fine. > > ... > > Still I'm wondering if it would not be better to replace this > "boot_ec->handle = handle;" in acpi_ec_ecdt_start() with the change > from my proposed diff above, so that we consistently about the > [boot_]ec->handle with device->handle for the being added acpi_device > in both branches of the if ... else ... in acpi_ec_add() ? Never mind that will not work because then we would set boot_ec->handle to the handle of the special / fake ACPI device object added to represent the boot EC, rather then to the handle corresponding to ecdt_ptr->id, so this is a bad idea. 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 @@ -306,3 +306,57 @@ acpi_execute_reg_methods(acpi_handle dev } ACPI_EXPORT_SYMBOL(acpi_execute_reg_methods) + +/******************************************************************************* + * + * FUNCTION: acpi_execute_orphan_reg_method + * + * PARAMETERS: device - Handle for the device + * space_id - The address space ID + * + * RETURN: Status + * + * DESCRIPTION: Execute an "orphan" _REG method that appears under an ACPI + * device. This is a _REG method that has no corresponding region + * within the device's scope. + * + ******************************************************************************/ +acpi_status +acpi_execute_orphan_reg_method(acpi_handle device, acpi_adr_space_type space_id) +{ + struct acpi_namespace_node *node; + acpi_status status; + + ACPI_FUNCTION_TRACE(acpi_execute_orphan_reg_method); + + /* Parameter validation */ + + if (!device) { + return_ACPI_STATUS(AE_BAD_PARAMETER); + } + + status = acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE); + if (ACPI_FAILURE(status)) { + return_ACPI_STATUS(status); + } + + /* Convert and validate the device handle */ + + node = acpi_ns_validate_handle(device); + if (node) { + + /* + * If an "orphan" _REG method is present in the device's scope + * for the given address space ID, run it. + */ + + acpi_ev_execute_orphan_reg_method(node, space_id); + } else { + status = AE_BAD_PARAMETER; + } + + (void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE); + return_ACPI_STATUS(status); +} + +ACPI_EXPORT_SYMBOL(acpi_execute_orphan_reg_method) Index: linux-pm/include/acpi/acpixf.h =================================================================== --- linux-pm.orig/include/acpi/acpixf.h +++ linux-pm/include/acpi/acpixf.h @@ -663,6 +663,10 @@ ACPI_EXTERNAL_RETURN_STATUS(acpi_status acpi_adr_space_type space_id)) ACPI_EXTERNAL_RETURN_STATUS(acpi_status + acpi_execute_orphan_reg_method(acpi_handle device, + acpi_adr_space_type + space_id)) +ACPI_EXTERNAL_RETURN_STATUS(acpi_status acpi_remove_address_space_handler(acpi_handle device, acpi_adr_space_type Index: linux-pm/drivers/acpi/acpica/acevents.h =================================================================== --- linux-pm.orig/drivers/acpi/acpica/acevents.h +++ linux-pm/drivers/acpi/acpica/acevents.h @@ -191,6 +191,10 @@ void acpi_ev_execute_reg_methods(struct acpi_namespace_node *node, acpi_adr_space_type space_id, u32 function); +void +acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node *node, + acpi_adr_space_type space_id); + acpi_status acpi_ev_execute_reg_method(union acpi_operand_object *region_obj, u32 function); Index: linux-pm/drivers/acpi/acpica/evregion.c =================================================================== --- linux-pm.orig/drivers/acpi/acpica/evregion.c +++ linux-pm/drivers/acpi/acpica/evregion.c @@ -20,10 +20,6 @@ extern u8 acpi_gbl_default_address_space /* Local prototypes */ -static void -acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node *device_node, - acpi_adr_space_type space_id); - static acpi_status acpi_ev_reg_run(acpi_handle obj_handle, u32 level, void *context, void **return_value); @@ -818,7 +814,7 @@ acpi_ev_reg_run(acpi_handle obj_handle, * ******************************************************************************/ -static void +void acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node *device_node, acpi_adr_space_type space_id) { Index: linux-pm/drivers/acpi/ec.c =================================================================== --- linux-pm.orig/drivers/acpi/ec.c +++ linux-pm/drivers/acpi/ec.c @@ -1507,6 +1507,9 @@ static int ec_install_handlers(struct ac if (call_reg && !test_bit(EC_FLAGS_EC_REG_CALLED, &ec->flags)) { acpi_execute_reg_methods(scope_handle, ACPI_ADR_SPACE_EC); + if (scope_handle != ec->handle) + acpi_execute_orphan_reg_method(ec->handle, ACPI_ADR_SPACE_EC); + set_bit(EC_FLAGS_EC_REG_CALLED, &ec->flags); }