Re: Missing default handler for the EmbeddedControl OpRegion

[Mario Limonciello <mario.limonciello@amd.com>]

On 5/8/2024 07:50, 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).

Nothing jumps out at me, it looks like a good way to handle this issue.

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