Re: Missing default handler for the EmbeddedControl OpRegion

[Hans de Goede <hdegoede@redhat.com>]

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@linux.intel.com> 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;
> 
>