Hi Rafael, On 5/8/24 2:50 PM, Rafael J. Wysocki 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. > > 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). +Cc Armin for WMI EC handler No objections from me against registering the EC handler at the root, when I saw that the WMI driver was registering its own handler I was already wondering why we did not just register the acpi/ec.c handler at the root level but I did not have time to pursue this further. One question which I have is does the drivers/acpi/ec.c version handle read/writes of a width other then 8 bits ? Armin recently added support for other widths to the WMI version of the OpRegion handler to fix issues on some laptop models: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c663b26972eae7d2a614f584c92a266fe9a2d44c Regards, Hans >> 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; > >
