On Wed, May 8, 2024 at 2:50 PM Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote: > > [Resending because it appears to have got lost, sorry for duplicates.] > > On Wednesday, May 8, 2024 2:20:59 PM CEST Heikki Krogerus wrote: > > On Mon, May 06, 2024 at 07:45:07PM +0200, Rafael J. Wysocki wrote: > > > Hi, > > > > > > On Mon, Apr 29, 2024 at 4:55 PM Heikki Krogerus > > > <heikki.krogerus@xxxxxxxxxxxxxxx> wrote: > > > > > > > > Hi, > > > > > > > > There's a bug that is caused by an EmbeddedControl OpRegion which is > > > > declared inside the scope of a specific USB Type-C device (PNP0CA0): > > > > https://bugzilla.kernel.org/show_bug.cgi?id=218789 > > > > > > And in this bug you are essentially proposing to install the EC > > > OpRegion handler at the namespace root instead of the EC device. > > > > > > This sounds reasonable, although AFAICS this is a matter of modifying > > > the EC driver (before the EC OpRegion handler is installed by the EC > > > drvier, ACPICA has no way to handle EC address space accesses anyway). > > > > > > > It looks like that's not the only case where that OpRegion ID is used > > > > outside of the EC device scope. There is at least one driver in Linux > > > > Kernel (drivers/platform/x86/wmi.c) that already has a custom handler > > > > for the EmbeddedControl OpRegion, and based on a quick search, the > > > > problem "Region EmbeddedControl (ID=3) has no handler" has happened > > > > with some other devices too. > > > > > > AFAICS, installing the EC address space handler at the EC device > > > object itself is not based on any sound technical arguments, it's just > > > been always done this way in Linux. It's quite possible that the EC > > > address space handler should have been installed at the namespace root > > > from the outset. > > > > Okay, thank you for the explanation. So can we simply change it like > > this (I may have still misunderstood something)? > > Roughly speaking, yes, but it is missing an analogous change around > the removal. Actually, not around the removal, but around the evaluation of _REG methods and dropping address_space_handler_holder is better IMO, because it will always be ACPI_ROOT_OBJECT now. > Please see the appended patch (which I have created independently in > the meantime). It doesn't break stuff for me and Andy points out that > there are examples of EmbeddedControl OpRegions outside the EC device > scope in the spec (see Section 9.17.15 in ACPI 6.5, for instance). > > So I think that this change can be made relatively safely (but adding Hans and > Mario to the CC in case they know something that might be broken by it). > > > > diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c > > index 02255795b800..6b9dd27171ee 100644 > > --- a/drivers/acpi/ec.c > > +++ b/drivers/acpi/ec.c > > @@ -1488,7 +1488,7 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device, > > > > if (!test_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags)) { > > acpi_ec_enter_noirq(ec); > > - status = acpi_install_address_space_handler_no_reg(ec->handle, > > + status = acpi_install_address_space_handler_no_reg(ACPI_ROOT_OBJECT, > > ACPI_ADR_SPACE_EC, > > &acpi_ec_space_handler, > > NULL, ec); > > @@ -1497,7 +1497,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_holder = ec->handle; > > + ec->address_space_handler_holder = ACPI_ROOT_OBJECT; > > } > > > > if (call_reg && !test_bit(EC_FLAGS_EC_REG_CALLED, &ec->flags)) { > > > > --- > drivers/acpi/ec.c | 10 +++++----- > drivers/acpi/internal.h | 1 - > 2 files changed, 5 insertions(+), 6 deletions(-) > > Index: linux-pm/drivers/acpi/ec.c > =================================================================== > --- linux-pm.orig/drivers/acpi/ec.c > +++ linux-pm/drivers/acpi/ec.c > @@ -1488,7 +1488,7 @@ static int ec_install_handlers(struct ac > > if (!test_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags)) { > acpi_ec_enter_noirq(ec); > - status = acpi_install_address_space_handler_no_reg(ec->handle, > + status = acpi_install_address_space_handler_no_reg(ACPI_ROOT_OBJECT, > ACPI_ADR_SPACE_EC, > &acpi_ec_space_handler, > NULL, ec); > @@ -1497,11 +1497,10 @@ static int ec_install_handlers(struct ac > return -ENODEV; > } > set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags); > - ec->address_space_handler_holder = ec->handle; > } > > if (call_reg && !test_bit(EC_FLAGS_EC_REG_CALLED, &ec->flags)) { > - acpi_execute_reg_methods(ec->handle, ACPI_ADR_SPACE_EC); > + acpi_execute_reg_methods(ACPI_ROOT_OBJECT, ACPI_ADR_SPACE_EC); > set_bit(EC_FLAGS_EC_REG_CALLED, &ec->flags); > } > > @@ -1555,8 +1554,9 @@ static void ec_remove_handlers(struct ac > { > if (test_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags)) { > if (ACPI_FAILURE(acpi_remove_address_space_handler( > - ec->address_space_handler_holder, > - ACPI_ADR_SPACE_EC, &acpi_ec_space_handler))) > + ACPI_ROOT_OBJECT, > + 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); > } > Index: linux-pm/drivers/acpi/internal.h > =================================================================== > --- linux-pm.orig/drivers/acpi/internal.h > +++ linux-pm/drivers/acpi/internal.h > @@ -186,7 +186,6 @@ enum acpi_ec_event_state { > > struct acpi_ec { > acpi_handle handle; > - acpi_handle address_space_handler_holder; > int gpe; > int irq; > unsigned long command_addr; > > >
