[v1,1/2] ACPI: EC: Install address space handler at the namespace root

Commit Message

Rafael J. Wysocki May 10, 2024, 2:03 p.m. UTC

From: Rafael J. Wysocki <[e-mail]>

It is reported that _DSM evaluation fails in ucsi_acpi_dsm() on Lenovo
IdeaPad Pro 5 due to a missing address space handler for the EC address
space:

 ACPI Error: No handler for Region [ECSI] (000000007b8176ee) [EmbeddedControl] (20230628/evregion-130)

This happens because the EC driver only registers the EC address space
handler for operation regions defined in the EC device scope of the
ACPI namespace while the operation region being accessed by the _DSM
in question is located beyond that scope.

To address this, modify the ACPI EC driver to install the EC address
space handler at the root of the ACPI namespace.

Note that this change is consistent with some examples in the ACPI
specification in which EC operation regions located outside the EC
device scope are used (for example, see Section 9.17.15 in ACPI 6.5),
so the current behavior of the EC driver is arguably questionable.

Reported-by: webcaptcha <[e-mail]>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=218789
Link: https://uefi.org/specs/ACPI/6.5/09_ACPI_Defined_Devices_and_Device_Specific_Objects.html#example-asl-code
Link: https://lore.kernel.org/linux-acpi/[e-mail]
Suggested-by: Heikki Krogerus <[e-mail]>
Signed-off-by: Rafael J. Wysocki <[e-mail]>
---
 drivers/acpi/ec.c       |   10 +++++-----
 drivers/acpi/internal.h |    1 -
 2 files changed, 5 insertions(+), 6 deletions(-)

Comments

Armin Wolf May 10, 2024, 4:10 p.m. UTC | #1

Am 10.05.24 um 16:03 schrieb Rafael J. Wysocki:

> From: Rafael J. Wysocki <[e-mail]>
>
> It is reported that _DSM evaluation fails in ucsi_acpi_dsm() on Lenovo
> IdeaPad Pro 5 due to a missing address space handler for the EC address
> space:
>
>   ACPI Error: No handler for Region [ECSI] (000000007b8176ee) [EmbeddedControl] (20230628/evregion-130)
>
> This happens because the EC driver only registers the EC address space
> handler for operation regions defined in the EC device scope of the
> ACPI namespace while the operation region being accessed by the _DSM
> in question is located beyond that scope.
>
> To address this, modify the ACPI EC driver to install the EC address
> space handler at the root of the ACPI namespace.
>
> Note that this change is consistent with some examples in the ACPI
> specification in which EC operation regions located outside the EC
> device scope are used (for example, see Section 9.17.15 in ACPI 6.5),
> so the current behavior of the EC driver is arguably questionable.

Hi,

the patch itself looks good to me, but i wonder what happens if multiple
ACPI EC devices are present. How would we handle such a situation?

Thanks,
Armin Wolf

> Reported-by: webcaptcha <[e-mail]>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218789
> Link: https://uefi.org/specs/ACPI/6.5/09_ACPI_Defined_Devices_and_Device_Specific_Objects.html#example-asl-code
> Link: https://lore.kernel.org/linux-acpi/[e-mail]
> Suggested-by: Heikki Krogerus <[e-mail]>
> Signed-off-by: Rafael J. Wysocki <[e-mail]>
> ---
>   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;
>
>
>

Rafael J. Wysocki May 10, 2024, 4:41 p.m. UTC | #2

On Fri, May 10, 2024 at 6:10 PM Armin Wolf <[e-mail]> wrote:
>
> Am 10.05.24 um 16:03 schrieb Rafael J. Wysocki:
>
> > From: Rafael J. Wysocki <[e-mail]>
> >
> > It is reported that _DSM evaluation fails in ucsi_acpi_dsm() on Lenovo
> > IdeaPad Pro 5 due to a missing address space handler for the EC address
> > space:
> >
> >   ACPI Error: No handler for Region [ECSI] (000000007b8176ee) [EmbeddedControl] (20230628/evregion-130)
> >
> > This happens because the EC driver only registers the EC address space
> > handler for operation regions defined in the EC device scope of the
> > ACPI namespace while the operation region being accessed by the _DSM
> > in question is located beyond that scope.
> >
> > To address this, modify the ACPI EC driver to install the EC address
> > space handler at the root of the ACPI namespace.
> >
> > Note that this change is consistent with some examples in the ACPI
> > specification in which EC operation regions located outside the EC
> > device scope are used (for example, see Section 9.17.15 in ACPI 6.5),
> > so the current behavior of the EC driver is arguably questionable.
>
> Hi,
>
> the patch itself looks good to me, but i wonder what happens if multiple
> ACPI EC devices are present. How would we handle such a situation?

I'm wondering if this is a theoretical question or do you have any
existing or planned systems in mind?

ec_read(), ec_write() and ec_transaction() use only the first EC that
has been found anyway.

Armin Wolf May 10, 2024, 4:52 p.m. UTC | #3

Am 10.05.24 um 18:41 schrieb Rafael J. Wysocki:

> On Fri, May 10, 2024 at 6:10 PM Armin Wolf <[e-mail]> wrote:
>> Am 10.05.24 um 16:03 schrieb Rafael J. Wysocki:
>>
>>> From: Rafael J. Wysocki <[e-mail]>
>>>
>>> It is reported that _DSM evaluation fails in ucsi_acpi_dsm() on Lenovo
>>> IdeaPad Pro 5 due to a missing address space handler for the EC address
>>> space:
>>>
>>>    ACPI Error: No handler for Region [ECSI] (000000007b8176ee) [EmbeddedControl] (20230628/evregion-130)
>>>
>>> This happens because the EC driver only registers the EC address space
>>> handler for operation regions defined in the EC device scope of the
>>> ACPI namespace while the operation region being accessed by the _DSM
>>> in question is located beyond that scope.
>>>
>>> To address this, modify the ACPI EC driver to install the EC address
>>> space handler at the root of the ACPI namespace.
>>>
>>> Note that this change is consistent with some examples in the ACPI
>>> specification in which EC operation regions located outside the EC
>>> device scope are used (for example, see Section 9.17.15 in ACPI 6.5),
>>> so the current behavior of the EC driver is arguably questionable.
>> Hi,
>>
>> the patch itself looks good to me, but i wonder what happens if multiple
>> ACPI EC devices are present. How would we handle such a situation?
> I'm wondering if this is a theoretical question or do you have any
> existing or planned systems in mind?
>
> ec_read(), ec_write() and ec_transaction() use only the first EC that
> has been found anyway.

Its a theoretical question, i do not know of any systems which have more than
one ACPI EC device.

This patch would prevent any ACPI ECs other than the first one from probing,
since they would fail to register their address space handler.
I am just curious if/how we want to handle such situations.

Thanks,
Armin Wolf

Andy Shevchenko May 10, 2024, 5:29 p.m. UTC | #4

On Fri, May 10, 2024 at 06:52:41PM +0200, Armin Wolf wrote:
> Am 10.05.24 um 18:41 schrieb Rafael J. Wysocki:
> > On Fri, May 10, 2024 at 6:10 PM Armin Wolf <[e-mail]> wrote:
> > > Am 10.05.24 um 16:03 schrieb Rafael J. Wysocki:
> > > 
> > > > From: Rafael J. Wysocki <[e-mail]>
> > > > 
> > > > It is reported that _DSM evaluation fails in ucsi_acpi_dsm() on Lenovo
> > > > IdeaPad Pro 5 due to a missing address space handler for the EC address
> > > > space:
> > > > 
> > > >    ACPI Error: No handler for Region [ECSI] (000000007b8176ee) [EmbeddedControl] (20230628/evregion-130)
> > > > 
> > > > This happens because the EC driver only registers the EC address space
> > > > handler for operation regions defined in the EC device scope of the
> > > > ACPI namespace while the operation region being accessed by the _DSM
> > > > in question is located beyond that scope.
> > > > 
> > > > To address this, modify the ACPI EC driver to install the EC address
> > > > space handler at the root of the ACPI namespace.
> > > > 
> > > > Note that this change is consistent with some examples in the ACPI
> > > > specification in which EC operation regions located outside the EC
> > > > device scope are used (for example, see Section 9.17.15 in ACPI 6.5),
> > > > so the current behavior of the EC driver is arguably questionable.
> > > Hi,
> > > 
> > > the patch itself looks good to me, but i wonder what happens if multiple
> > > ACPI EC devices are present. How would we handle such a situation?
> > I'm wondering if this is a theoretical question or do you have any
> > existing or planned systems in mind?
> > 
> > ec_read(), ec_write() and ec_transaction() use only the first EC that
> > has been found anyway.
> 
> Its a theoretical question, i do not know of any systems which have more than
> one ACPI EC device.

The specification is clear about this case in the "ACPI Embedded Controller
Interface Specification":

 "The ACPI standard supports multiple embedded controllers in a system,
  each with its own resources. Each embedded controller has a flat
  byte-addressable I/O space, currently defined as 256 bytes."

However, I haven't checked deeper, so it might be a leftover in the documentation.

The OperationRegion() has no reference to the EC (or in general, device) which
we need to speak to. The only possibility to declare OpRegion() for the second+
EC is to use vendor specific RegionSpace, AFAIU. So, even if ACPI specification
supports 2+ ECs, it doesn't support OpRegion():s for them under the same
RegionSpace.

That said, the commit message might be extended to summarize this, but at
the same time I see no way how this series can break anything even in 2+ ECs
environments.

> This patch would prevent any ACPI ECs other than the first one from probing,
> since they would fail to register their address space handler.
> I am just curious if/how we want to handle such situations.

Armin Wolf May 10, 2024, 5:38 p.m. UTC | #5

Am 10.05.24 um 19:29 schrieb Andy Shevchenko:

> On Fri, May 10, 2024 at 06:52:41PM +0200, Armin Wolf wrote:
>> Am 10.05.24 um 18:41 schrieb Rafael J. Wysocki:
>>> On Fri, May 10, 2024 at 6:10 PM Armin Wolf <[e-mail]> wrote:
>>>> Am 10.05.24 um 16:03 schrieb Rafael J. Wysocki:
>>>>
>>>>> From: Rafael J. Wysocki <[e-mail]>
>>>>>
>>>>> It is reported that _DSM evaluation fails in ucsi_acpi_dsm() on Lenovo
>>>>> IdeaPad Pro 5 due to a missing address space handler for the EC address
>>>>> space:
>>>>>
>>>>>     ACPI Error: No handler for Region [ECSI] (000000007b8176ee) [EmbeddedControl] (20230628/evregion-130)
>>>>>
>>>>> This happens because the EC driver only registers the EC address space
>>>>> handler for operation regions defined in the EC device scope of the
>>>>> ACPI namespace while the operation region being accessed by the _DSM
>>>>> in question is located beyond that scope.
>>>>>
>>>>> To address this, modify the ACPI EC driver to install the EC address
>>>>> space handler at the root of the ACPI namespace.
>>>>>
>>>>> Note that this change is consistent with some examples in the ACPI
>>>>> specification in which EC operation regions located outside the EC
>>>>> device scope are used (for example, see Section 9.17.15 in ACPI 6.5),
>>>>> so the current behavior of the EC driver is arguably questionable.
>>>> Hi,
>>>>
>>>> the patch itself looks good to me, but i wonder what happens if multiple
>>>> ACPI EC devices are present. How would we handle such a situation?
>>> I'm wondering if this is a theoretical question or do you have any
>>> existing or planned systems in mind?
>>>
>>> ec_read(), ec_write() and ec_transaction() use only the first EC that
>>> has been found anyway.
>> Its a theoretical question, i do not know of any systems which have more than
>> one ACPI EC device.
> The specification is clear about this case in the "ACPI Embedded Controller
> Interface Specification":
>
>   "The ACPI standard supports multiple embedded controllers in a system,
>    each with its own resources. Each embedded controller has a flat
>    byte-addressable I/O space, currently defined as 256 bytes."
>
> However, I haven't checked deeper, so it might be a leftover in the documentation.
>
> The OperationRegion() has no reference to the EC (or in general, device) which
> we need to speak to. The only possibility to declare OpRegion() for the second+
> EC is to use vendor specific RegionSpace, AFAIU. So, even if ACPI specification
> supports 2+ ECs, it doesn't support OpRegion():s for them under the same
> RegionSpace.
>
> That said, the commit message might be extended to summarize this, but at
> the same time I see no way how this series can break anything even in 2+ ECs
> environments.

Consider the following execution flow when the second EC probes:

1. acpi_install_address_space_handler_no_reg() fails with AE_ALREADY_EXISTS since the first EC
has already installed a handler at ACPI_ROOT_OBJECT.

2. ec_install_handlers() fails with -ENODEV.

3. acpi_ec_setup() fails with -ENODEV.

4. acpi_ec_add() fails with -ENODEV.

5. Probe of seconds EC fails with -ENODEV.

This might cause problems if the second EC is supposed to for example handle EC query events.
Of course if we only support a single EC, then this situation cannot happen.

>> This patch would prevent any ACPI ECs other than the first one from probing,
>> since they would fail to register their address space handler.
>> I am just curious if/how we want to handle such situations.

Mario Limonciello May 10, 2024, 5:40 p.m. UTC | #6

On 5/10/2024 12:29, Andy Shevchenko wrote:
> On Fri, May 10, 2024 at 06:52:41PM +0200, Armin Wolf wrote:
>> Am 10.05.24 um 18:41 schrieb Rafael J. Wysocki:
>>> On Fri, May 10, 2024 at 6:10 PM Armin Wolf <[e-mail]> wrote:
>>>> Am 10.05.24 um 16:03 schrieb Rafael J. Wysocki:
>>>>
>>>>> From: Rafael J. Wysocki <[e-mail]>
>>>>>
>>>>> It is reported that _DSM evaluation fails in ucsi_acpi_dsm() on Lenovo
>>>>> IdeaPad Pro 5 due to a missing address space handler for the EC address
>>>>> space:
>>>>>
>>>>>     ACPI Error: No handler for Region [ECSI] (000000007b8176ee) [EmbeddedControl] (20230628/evregion-130)
>>>>>
>>>>> This happens because the EC driver only registers the EC address space
>>>>> handler for operation regions defined in the EC device scope of the
>>>>> ACPI namespace while the operation region being accessed by the _DSM
>>>>> in question is located beyond that scope.
>>>>>
>>>>> To address this, modify the ACPI EC driver to install the EC address
>>>>> space handler at the root of the ACPI namespace.
>>>>>
>>>>> Note that this change is consistent with some examples in the ACPI
>>>>> specification in which EC operation regions located outside the EC
>>>>> device scope are used (for example, see Section 9.17.15 in ACPI 6.5),
>>>>> so the current behavior of the EC driver is arguably questionable.
>>>> Hi,
>>>>
>>>> the patch itself looks good to me, but i wonder what happens if multiple
>>>> ACPI EC devices are present. How would we handle such a situation?
>>> I'm wondering if this is a theoretical question or do you have any
>>> existing or planned systems in mind?
>>>
>>> ec_read(), ec_write() and ec_transaction() use only the first EC that
>>> has been found anyway.
>>
>> Its a theoretical question, i do not know of any systems which have more than
>> one ACPI EC device.
> 
> The specification is clear about this case in the "ACPI Embedded Controller
> Interface Specification":
> 
>   "The ACPI standard supports multiple embedded controllers in a system,
>    each with its own resources. Each embedded controller has a flat
>    byte-addressable I/O space, currently defined as 256 bytes."
> 
> However, I haven't checked deeper, so it might be a leftover in the documentation.
> 
> The OperationRegion() has no reference to the EC (or in general, device) which
> we need to speak to. The only possibility to declare OpRegion() for the second+
> EC is to use vendor specific RegionSpace, AFAIU. So, even if ACPI specification
> supports 2+ ECs, it doesn't support OpRegion():s for them under the same
> RegionSpace.
> 
> That said, the commit message might be extended to summarize this, but at
> the same time I see no way how this series can break anything even in 2+ ECs
> environments.

It's deviating from the patch, but in practice /why/ would you even want 
to have a design with two ECs?  In general that is going to mean a much 
more complex state machine with synchronizing the interaction between 
both of them and the host.

Understanding the benefit of such a design might make it easier to 
hypothesize impacts.

> 
>> This patch would prevent any ACPI ECs other than the first one from probing,
>> since they would fail to register their address space handler.
>> I am just curious if/how we want to handle such situations.
>

Armin Wolf May 10, 2024, 5:47 p.m. UTC | #7

Am 10.05.24 um 19:40 schrieb Mario Limonciello:

> On 5/10/2024 12:29, Andy Shevchenko wrote:
>> On Fri, May 10, 2024 at 06:52:41PM +0200, Armin Wolf wrote:
>>> Am 10.05.24 um 18:41 schrieb Rafael J. Wysocki:
>>>> On Fri, May 10, 2024 at 6:10 PM Armin Wolf <[e-mail]> wrote:
>>>>> Am 10.05.24 um 16:03 schrieb Rafael J. Wysocki:
>>>>>
>>>>>> From: Rafael J. Wysocki <[e-mail]>
>>>>>>
>>>>>> It is reported that _DSM evaluation fails in ucsi_acpi_dsm() on
>>>>>> Lenovo
>>>>>> IdeaPad Pro 5 due to a missing address space handler for the EC
>>>>>> address
>>>>>> space:
>>>>>>
>>>>>>     ACPI Error: No handler for Region [ECSI] (000000007b8176ee)
>>>>>> [EmbeddedControl] (20230628/evregion-130)
>>>>>>
>>>>>> This happens because the EC driver only registers the EC address
>>>>>> space
>>>>>> handler for operation regions defined in the EC device scope of the
>>>>>> ACPI namespace while the operation region being accessed by the _DSM
>>>>>> in question is located beyond that scope.
>>>>>>
>>>>>> To address this, modify the ACPI EC driver to install the EC address
>>>>>> space handler at the root of the ACPI namespace.
>>>>>>
>>>>>> Note that this change is consistent with some examples in the ACPI
>>>>>> specification in which EC operation regions located outside the EC
>>>>>> device scope are used (for example, see Section 9.17.15 in ACPI
>>>>>> 6.5),
>>>>>> so the current behavior of the EC driver is arguably questionable.
>>>>> Hi,
>>>>>
>>>>> the patch itself looks good to me, but i wonder what happens if
>>>>> multiple
>>>>> ACPI EC devices are present. How would we handle such a situation?
>>>> I'm wondering if this is a theoretical question or do you have any
>>>> existing or planned systems in mind?
>>>>
>>>> ec_read(), ec_write() and ec_transaction() use only the first EC that
>>>> has been found anyway.
>>>
>>> Its a theoretical question, i do not know of any systems which have
>>> more than
>>> one ACPI EC device.
>>
>> The specification is clear about this case in the "ACPI Embedded
>> Controller
>> Interface Specification":
>>
>>   "The ACPI standard supports multiple embedded controllers in a system,
>>    each with its own resources. Each embedded controller has a flat
>>    byte-addressable I/O space, currently defined as 256 bytes."
>>
>> However, I haven't checked deeper, so it might be a leftover in the
>> documentation.
>>
>> The OperationRegion() has no reference to the EC (or in general,
>> device) which
>> we need to speak to. The only possibility to declare OpRegion() for
>> the second+
>> EC is to use vendor specific RegionSpace, AFAIU. So, even if ACPI
>> specification
>> supports 2+ ECs, it doesn't support OpRegion():s for them under the same
>> RegionSpace.
>>
>> That said, the commit message might be extended to summarize this,
>> but at
>> the same time I see no way how this series can break anything even in
>> 2+ ECs
>> environments.
>
> It's deviating from the patch, but in practice /why/ would you even
> want to have a design with two ECs?  In general that is going to mean
> a much more complex state machine with synchronizing the interaction
> between both of them and the host.
>
> Understanding the benefit of such a design might make it easier to
> hypothesize impacts.
>
I am not saying that such designs would make sense, it was a theoretical question only.

Maybe we can just add a small note to the Linux ACPI documentation saying that we only
support a single EC device?

Thanks,
Armin Wolf

>>
>>> This patch would prevent any ACPI ECs other than the first one from
>>> probing,
>>> since they would fail to register their address space handler.
>>> I am just curious if/how we want to handle such situations.
>>
>

Andy Shevchenko May 10, 2024, 5:50 p.m. UTC | #8

On Fri, May 10, 2024 at 12:40:05PM -0500, Mario Limonciello wrote:
> On 5/10/2024 12:29, Andy Shevchenko wrote:
> > On Fri, May 10, 2024 at 06:52:41PM +0200, Armin Wolf wrote:
> > > Am 10.05.24 um 18:41 schrieb Rafael J. Wysocki:
> > > > On Fri, May 10, 2024 at 6:10 PM Armin Wolf <[e-mail]> wrote:
> > > > > Am 10.05.24 um 16:03 schrieb Rafael J. Wysocki:
> > > > > 
> > > > > > From: Rafael J. Wysocki <[e-mail]>
> > > > > > 
> > > > > > It is reported that _DSM evaluation fails in ucsi_acpi_dsm() on Lenovo
> > > > > > IdeaPad Pro 5 due to a missing address space handler for the EC address
> > > > > > space:
> > > > > > 
> > > > > >     ACPI Error: No handler for Region [ECSI] (000000007b8176ee) [EmbeddedControl] (20230628/evregion-130)
> > > > > > 
> > > > > > This happens because the EC driver only registers the EC address space
> > > > > > handler for operation regions defined in the EC device scope of the
> > > > > > ACPI namespace while the operation region being accessed by the _DSM
> > > > > > in question is located beyond that scope.
> > > > > > 
> > > > > > To address this, modify the ACPI EC driver to install the EC address
> > > > > > space handler at the root of the ACPI namespace.
> > > > > > 
> > > > > > Note that this change is consistent with some examples in the ACPI
> > > > > > specification in which EC operation regions located outside the EC
> > > > > > device scope are used (for example, see Section 9.17.15 in ACPI 6.5),
> > > > > > so the current behavior of the EC driver is arguably questionable.
> > > > > Hi,
> > > > > 
> > > > > the patch itself looks good to me, but i wonder what happens if multiple
> > > > > ACPI EC devices are present. How would we handle such a situation?
> > > > I'm wondering if this is a theoretical question or do you have any
> > > > existing or planned systems in mind?
> > > > 
> > > > ec_read(), ec_write() and ec_transaction() use only the first EC that
> > > > has been found anyway.
> > > 
> > > Its a theoretical question, i do not know of any systems which have more than
> > > one ACPI EC device.
> > 
> > The specification is clear about this case in the "ACPI Embedded Controller
> > Interface Specification":
> > 
> >   "The ACPI standard supports multiple embedded controllers in a system,
> >    each with its own resources. Each embedded controller has a flat
> >    byte-addressable I/O space, currently defined as 256 bytes."
> > 
> > However, I haven't checked deeper, so it might be a leftover in the documentation.
> > 
> > The OperationRegion() has no reference to the EC (or in general, device) which
> > we need to speak to. The only possibility to declare OpRegion() for the second+
> > EC is to use vendor specific RegionSpace, AFAIU. So, even if ACPI specification
> > supports 2+ ECs, it doesn't support OpRegion():s for them under the same
> > RegionSpace.
> > 
> > That said, the commit message might be extended to summarize this, but at
> > the same time I see no way how this series can break anything even in 2+ ECs
> > environments.
> 
> It's deviating from the patch, but in practice /why/ would you even want to
> have a design with two ECs?  In general that is going to mean a much more
> complex state machine with synchronizing the interaction between both of
> them and the host.
> 
> Understanding the benefit of such a design might make it easier to
> hypothesize impacts.

First that comes to my mind (but hypothetical), is the separate CPU/EC add-on
cards. If the main firmware somehow supports all of these add-on platforms,
it might need to handle 2+ ECs.

Again, it might be ACPI specification issue. For instance, the cited piece
doesn't tell about 16-bit EC accesses.

> > > This patch would prevent any ACPI ECs other than the first one from probing,
> > > since they would fail to register their address space handler.
> > > I am just curious if/how we want to handle such situations.

Andy Shevchenko May 10, 2024, 5:54 p.m. UTC | #9

On Fri, May 10, 2024 at 08:50:29PM +0300, Andy Shevchenko wrote:
> On Fri, May 10, 2024 at 12:40:05PM -0500, Mario Limonciello wrote:
> > On 5/10/2024 12:29, Andy Shevchenko wrote:
> > > On Fri, May 10, 2024 at 06:52:41PM +0200, Armin Wolf wrote:
> > > > Am 10.05.24 um 18:41 schrieb Rafael J. Wysocki:
> > > > > On Fri, May 10, 2024 at 6:10 PM Armin Wolf <[e-mail]> wrote:
> > > > > > Am 10.05.24 um 16:03 schrieb Rafael J. Wysocki:
> > > > > > 
> > > > > > > From: Rafael J. Wysocki <[e-mail]>
> > > > > > > 
> > > > > > > It is reported that _DSM evaluation fails in ucsi_acpi_dsm() on Lenovo
> > > > > > > IdeaPad Pro 5 due to a missing address space handler for the EC address
> > > > > > > space:
> > > > > > > 
> > > > > > >     ACPI Error: No handler for Region [ECSI] (000000007b8176ee) [EmbeddedControl] (20230628/evregion-130)
> > > > > > > 
> > > > > > > This happens because the EC driver only registers the EC address space
> > > > > > > handler for operation regions defined in the EC device scope of the
> > > > > > > ACPI namespace while the operation region being accessed by the _DSM
> > > > > > > in question is located beyond that scope.
> > > > > > > 
> > > > > > > To address this, modify the ACPI EC driver to install the EC address
> > > > > > > space handler at the root of the ACPI namespace.
> > > > > > > 
> > > > > > > Note that this change is consistent with some examples in the ACPI
> > > > > > > specification in which EC operation regions located outside the EC
> > > > > > > device scope are used (for example, see Section 9.17.15 in ACPI 6.5),
> > > > > > > so the current behavior of the EC driver is arguably questionable.
> > > > > > Hi,
> > > > > > 
> > > > > > the patch itself looks good to me, but i wonder what happens if multiple
> > > > > > ACPI EC devices are present. How would we handle such a situation?
> > > > > I'm wondering if this is a theoretical question or do you have any
> > > > > existing or planned systems in mind?
> > > > > 
> > > > > ec_read(), ec_write() and ec_transaction() use only the first EC that
> > > > > has been found anyway.
> > > > 
> > > > Its a theoretical question, i do not know of any systems which have more than
> > > > one ACPI EC device.
> > > 
> > > The specification is clear about this case in the "ACPI Embedded Controller
> > > Interface Specification":
> > > 
> > >   "The ACPI standard supports multiple embedded controllers in a system,
> > >    each with its own resources. Each embedded controller has a flat
> > >    byte-addressable I/O space, currently defined as 256 bytes."
> > > 
> > > However, I haven't checked deeper, so it might be a leftover in the documentation.
> > > 
> > > The OperationRegion() has no reference to the EC (or in general, device) which
> > > we need to speak to. The only possibility to declare OpRegion() for the second+
> > > EC is to use vendor specific RegionSpace, AFAIU. So, even if ACPI specification
> > > supports 2+ ECs, it doesn't support OpRegion():s for them under the same
> > > RegionSpace.
> > > 
> > > That said, the commit message might be extended to summarize this, but at
> > > the same time I see no way how this series can break anything even in 2+ ECs
> > > environments.
> > 
> > It's deviating from the patch, but in practice /why/ would you even want to
> > have a design with two ECs?  In general that is going to mean a much more
> > complex state machine with synchronizing the interaction between both of
> > them and the host.
> > 
> > Understanding the benefit of such a design might make it easier to
> > hypothesize impacts.
> 
> First that comes to my mind (but hypothetical), is the separate CPU/EC add-on
> cards. If the main firmware somehow supports all of these add-on platforms,
> it might need to handle 2+ ECs.
> 
> Again, it might be ACPI specification issue. For instance, the cited piece
> doesn't tell about 16-bit EC accesses.

Chapter "4.8.4.2.2 Embedded Controller" among other says:

  "Additionally the embedded controller can support up to 255 generic events
   per embedded controller, referred to as query events."

So, if specification needs an update, it should be very carefully proof-read.

> > > > This patch would prevent any ACPI ECs other than the first one from probing,
> > > > since they would fail to register their address space handler.
> > > > I am just curious if/how we want to handle such situations.

Rafael J. Wysocki May 10, 2024, 5:56 p.m. UTC | #10

On Fri, May 10, 2024 at 6:52 PM Armin Wolf <[e-mail]> wrote:
>
> Am 10.05.24 um 18:41 schrieb Rafael J. Wysocki:
>
> > On Fri, May 10, 2024 at 6:10 PM Armin Wolf <[e-mail]> wrote:
> >> Am 10.05.24 um 16:03 schrieb Rafael J. Wysocki:
> >>
> >>> From: Rafael J. Wysocki <[e-mail]>
> >>>
> >>> It is reported that _DSM evaluation fails in ucsi_acpi_dsm() on Lenovo
> >>> IdeaPad Pro 5 due to a missing address space handler for the EC address
> >>> space:
> >>>
> >>>    ACPI Error: No handler for Region [ECSI] (000000007b8176ee) [EmbeddedControl] (20230628/evregion-130)
> >>>
> >>> This happens because the EC driver only registers the EC address space
> >>> handler for operation regions defined in the EC device scope of the
> >>> ACPI namespace while the operation region being accessed by the _DSM
> >>> in question is located beyond that scope.
> >>>
> >>> To address this, modify the ACPI EC driver to install the EC address
> >>> space handler at the root of the ACPI namespace.
> >>>
> >>> Note that this change is consistent with some examples in the ACPI
> >>> specification in which EC operation regions located outside the EC
> >>> device scope are used (for example, see Section 9.17.15 in ACPI 6.5),
> >>> so the current behavior of the EC driver is arguably questionable.
> >> Hi,
> >>
> >> the patch itself looks good to me, but i wonder what happens if multiple
> >> ACPI EC devices are present. How would we handle such a situation?
> > I'm wondering if this is a theoretical question or do you have any
> > existing or planned systems in mind?
> >
> > ec_read(), ec_write() and ec_transaction() use only the first EC that
> > has been found anyway.
>
> Its a theoretical question, i do not know of any systems which have more than
> one ACPI EC device.
>
> This patch would prevent any ACPI ECs other than the first one from probing,
> since they would fail to register their address space handler.
> I am just curious if/how we want to handle such situations.

I'm not worried until I see a system where that is a problem.

That said, it can be addressed by adding a first_ec check around the
address space handler registration/unregistration.

Rafael J. Wysocki May 10, 2024, 6 p.m. UTC | #11

On Fri, May 10, 2024 at 7:39 PM Armin Wolf <[e-mail]> wrote:
>
> Am 10.05.24 um 19:29 schrieb Andy Shevchenko:
>
> > On Fri, May 10, 2024 at 06:52:41PM +0200, Armin Wolf wrote:
> >> Am 10.05.24 um 18:41 schrieb Rafael J. Wysocki:
> >>> On Fri, May 10, 2024 at 6:10 PM Armin Wolf <[e-mail]> wrote:
> >>>> Am 10.05.24 um 16:03 schrieb Rafael J. Wysocki:
> >>>>
> >>>>> From: Rafael J. Wysocki <[e-mail]>
> >>>>>
> >>>>> It is reported that _DSM evaluation fails in ucsi_acpi_dsm() on Lenovo
> >>>>> IdeaPad Pro 5 due to a missing address space handler for the EC address
> >>>>> space:
> >>>>>
> >>>>>     ACPI Error: No handler for Region [ECSI] (000000007b8176ee) [EmbeddedControl] (20230628/evregion-130)
> >>>>>
> >>>>> This happens because the EC driver only registers the EC address space
> >>>>> handler for operation regions defined in the EC device scope of the
> >>>>> ACPI namespace while the operation region being accessed by the _DSM
> >>>>> in question is located beyond that scope.
> >>>>>
> >>>>> To address this, modify the ACPI EC driver to install the EC address
> >>>>> space handler at the root of the ACPI namespace.
> >>>>>
> >>>>> Note that this change is consistent with some examples in the ACPI
> >>>>> specification in which EC operation regions located outside the EC
> >>>>> device scope are used (for example, see Section 9.17.15 in ACPI 6.5),
> >>>>> so the current behavior of the EC driver is arguably questionable.
> >>>> Hi,
> >>>>
> >>>> the patch itself looks good to me, but i wonder what happens if multiple
> >>>> ACPI EC devices are present. How would we handle such a situation?
> >>> I'm wondering if this is a theoretical question or do you have any
> >>> existing or planned systems in mind?
> >>>
> >>> ec_read(), ec_write() and ec_transaction() use only the first EC that
> >>> has been found anyway.
> >> Its a theoretical question, i do not know of any systems which have more than
> >> one ACPI EC device.
> > The specification is clear about this case in the "ACPI Embedded Controller
> > Interface Specification":
> >
> >   "The ACPI standard supports multiple embedded controllers in a system,
> >    each with its own resources. Each embedded controller has a flat
> >    byte-addressable I/O space, currently defined as 256 bytes."
> >
> > However, I haven't checked deeper, so it might be a leftover in the documentation.
> >
> > The OperationRegion() has no reference to the EC (or in general, device) which
> > we need to speak to. The only possibility to declare OpRegion() for the second+
> > EC is to use vendor specific RegionSpace, AFAIU. So, even if ACPI specification
> > supports 2+ ECs, it doesn't support OpRegion():s for them under the same
> > RegionSpace.
> >
> > That said, the commit message might be extended to summarize this, but at
> > the same time I see no way how this series can break anything even in 2+ ECs
> > environments.
>
> Consider the following execution flow when the second EC probes:
>
> 1. acpi_install_address_space_handler_no_reg() fails with AE_ALREADY_EXISTS since the first EC
> has already installed a handler at ACPI_ROOT_OBJECT.
>
> 2. ec_install_handlers() fails with -ENODEV.
>
> 3. acpi_ec_setup() fails with -ENODEV.
>
> 4. acpi_ec_add() fails with -ENODEV.
>
> 5. Probe of seconds EC fails with -ENODEV.
>
> This might cause problems if the second EC is supposed to for example handle EC query events.
> Of course if we only support a single EC, then this situation cannot happen.

This is kind of moot though until a system with 2 ECs is available.
It is hard to say whether or not it is supported until it can be
tested.

Rafael J. Wysocki May 10, 2024, 6:06 p.m. UTC | #12

On Fri, May 10, 2024 at 7:50 PM Andy Shevchenko
<[e-mail]> wrote:
>
> On Fri, May 10, 2024 at 12:40:05PM -0500, Mario Limonciello wrote:
> > On 5/10/2024 12:29, Andy Shevchenko wrote:
> > > On Fri, May 10, 2024 at 06:52:41PM +0200, Armin Wolf wrote:
> > > > Am 10.05.24 um 18:41 schrieb Rafael J. Wysocki:
> > > > > On Fri, May 10, 2024 at 6:10 PM Armin Wolf <[e-mail]> wrote:
> > > > > > Am 10.05.24 um 16:03 schrieb Rafael J. Wysocki:
> > > > > >
> > > > > > > From: Rafael J. Wysocki <[e-mail]>
> > > > > > >
> > > > > > > It is reported that _DSM evaluation fails in ucsi_acpi_dsm() on Lenovo
> > > > > > > IdeaPad Pro 5 due to a missing address space handler for the EC address
> > > > > > > space:
> > > > > > >
> > > > > > >     ACPI Error: No handler for Region [ECSI] (000000007b8176ee) [EmbeddedControl] (20230628/evregion-130)
> > > > > > >
> > > > > > > This happens because the EC driver only registers the EC address space
> > > > > > > handler for operation regions defined in the EC device scope of the
> > > > > > > ACPI namespace while the operation region being accessed by the _DSM
> > > > > > > in question is located beyond that scope.
> > > > > > >
> > > > > > > To address this, modify the ACPI EC driver to install the EC address
> > > > > > > space handler at the root of the ACPI namespace.
> > > > > > >
> > > > > > > Note that this change is consistent with some examples in the ACPI
> > > > > > > specification in which EC operation regions located outside the EC
> > > > > > > device scope are used (for example, see Section 9.17.15 in ACPI 6.5),
> > > > > > > so the current behavior of the EC driver is arguably questionable.
> > > > > > Hi,
> > > > > >
> > > > > > the patch itself looks good to me, but i wonder what happens if multiple
> > > > > > ACPI EC devices are present. How would we handle such a situation?
> > > > > I'm wondering if this is a theoretical question or do you have any
> > > > > existing or planned systems in mind?
> > > > >
> > > > > ec_read(), ec_write() and ec_transaction() use only the first EC that
> > > > > has been found anyway.
> > > >
> > > > Its a theoretical question, i do not know of any systems which have more than
> > > > one ACPI EC device.
> > >
> > > The specification is clear about this case in the "ACPI Embedded Controller
> > > Interface Specification":
> > >
> > >   "The ACPI standard supports multiple embedded controllers in a system,
> > >    each with its own resources. Each embedded controller has a flat
> > >    byte-addressable I/O space, currently defined as 256 bytes."
> > >
> > > However, I haven't checked deeper, so it might be a leftover in the documentation.
> > >
> > > The OperationRegion() has no reference to the EC (or in general, device) which
> > > we need to speak to. The only possibility to declare OpRegion() for the second+
> > > EC is to use vendor specific RegionSpace, AFAIU. So, even if ACPI specification
> > > supports 2+ ECs, it doesn't support OpRegion():s for them under the same
> > > RegionSpace.
> > >
> > > That said, the commit message might be extended to summarize this, but at
> > > the same time I see no way how this series can break anything even in 2+ ECs
> > > environments.
> >
> > It's deviating from the patch, but in practice /why/ would you even want to
> > have a design with two ECs?  In general that is going to mean a much more
> > complex state machine with synchronizing the interaction between both of
> > them and the host.
> >
> > Understanding the benefit of such a design might make it easier to
> > hypothesize impacts.
>
> First that comes to my mind (but hypothetical), is the separate CPU/EC add-on
> cards. If the main firmware somehow supports all of these add-on platforms,
> it might need to handle 2+ ECs.
>
> Again, it might be ACPI specification issue. For instance, the cited piece
> doesn't tell about 16-bit EC accesses.

IMV this is a matter of what is testable.

We can only seriously say that we support 1 EC in the system, because
that's what we can test.

Now, the specification allows (theoretically) multiple ECs to be
supported which does not mean that it will ever be done in practice
and it also does not mean that this is a good idea.

Andy Shevchenko May 10, 2024, 6:17 p.m. UTC | #13

On Fri, May 10, 2024 at 08:06:11PM +0200, Rafael J. Wysocki wrote:
> On Fri, May 10, 2024 at 7:50 PM Andy Shevchenko
> <[e-mail]> wrote:
> > On Fri, May 10, 2024 at 12:40:05PM -0500, Mario Limonciello wrote:
> > > On 5/10/2024 12:29, Andy Shevchenko wrote:
> > > > On Fri, May 10, 2024 at 06:52:41PM +0200, Armin Wolf wrote:
> > > > > Am 10.05.24 um 18:41 schrieb Rafael J. Wysocki:
> > > > > > On Fri, May 10, 2024 at 6:10 PM Armin Wolf <[e-mail]> wrote:
> > > > > > > Am 10.05.24 um 16:03 schrieb Rafael J. Wysocki:
> > > > > > >
> > > > > > > > From: Rafael J. Wysocki <[e-mail]>
> > > > > > > >
> > > > > > > > It is reported that _DSM evaluation fails in ucsi_acpi_dsm() on Lenovo
> > > > > > > > IdeaPad Pro 5 due to a missing address space handler for the EC address
> > > > > > > > space:
> > > > > > > >
> > > > > > > >     ACPI Error: No handler for Region [ECSI] (000000007b8176ee) [EmbeddedControl] (20230628/evregion-130)
> > > > > > > >
> > > > > > > > This happens because the EC driver only registers the EC address space
> > > > > > > > handler for operation regions defined in the EC device scope of the
> > > > > > > > ACPI namespace while the operation region being accessed by the _DSM
> > > > > > > > in question is located beyond that scope.
> > > > > > > >
> > > > > > > > To address this, modify the ACPI EC driver to install the EC address
> > > > > > > > space handler at the root of the ACPI namespace.
> > > > > > > >
> > > > > > > > Note that this change is consistent with some examples in the ACPI
> > > > > > > > specification in which EC operation regions located outside the EC
> > > > > > > > device scope are used (for example, see Section 9.17.15 in ACPI 6.5),
> > > > > > > > so the current behavior of the EC driver is arguably questionable.
> > > > > > > Hi,
> > > > > > >
> > > > > > > the patch itself looks good to me, but i wonder what happens if multiple
> > > > > > > ACPI EC devices are present. How would we handle such a situation?
> > > > > > I'm wondering if this is a theoretical question or do you have any
> > > > > > existing or planned systems in mind?
> > > > > >
> > > > > > ec_read(), ec_write() and ec_transaction() use only the first EC that
> > > > > > has been found anyway.
> > > > >
> > > > > Its a theoretical question, i do not know of any systems which have more than
> > > > > one ACPI EC device.
> > > >
> > > > The specification is clear about this case in the "ACPI Embedded Controller
> > > > Interface Specification":
> > > >
> > > >   "The ACPI standard supports multiple embedded controllers in a system,
> > > >    each with its own resources. Each embedded controller has a flat
> > > >    byte-addressable I/O space, currently defined as 256 bytes."
> > > >
> > > > However, I haven't checked deeper, so it might be a leftover in the documentation.
> > > >
> > > > The OperationRegion() has no reference to the EC (or in general, device) which
> > > > we need to speak to. The only possibility to declare OpRegion() for the second+
> > > > EC is to use vendor specific RegionSpace, AFAIU. So, even if ACPI specification
> > > > supports 2+ ECs, it doesn't support OpRegion():s for them under the same
> > > > RegionSpace.
> > > >
> > > > That said, the commit message might be extended to summarize this, but at
> > > > the same time I see no way how this series can break anything even in 2+ ECs
> > > > environments.
> > >
> > > It's deviating from the patch, but in practice /why/ would you even want to
> > > have a design with two ECs?  In general that is going to mean a much more
> > > complex state machine with synchronizing the interaction between both of
> > > them and the host.
> > >
> > > Understanding the benefit of such a design might make it easier to
> > > hypothesize impacts.
> >
> > First that comes to my mind (but hypothetical), is the separate CPU/EC add-on
> > cards. If the main firmware somehow supports all of these add-on platforms,
> > it might need to handle 2+ ECs.
> >
> > Again, it might be ACPI specification issue. For instance, the cited piece
> > doesn't tell about 16-bit EC accesses.
> 
> IMV this is a matter of what is testable.
> 
> We can only seriously say that we support 1 EC in the system, because
> that's what we can test.
> 
> Now, the specification allows (theoretically) multiple ECs to be
> supported which does not mean that it will ever be done in practice
> and it also does not mean that this is a good idea.

I briefly read the all mentions of the "Embedded Controller" in the
specification and like 98% implies that the only one is per system. I believe
the specification may be corrected to remove ambiguous plural forms in a couple
(or a few) places. In any case it's a question to ASWG.

Hans de Goede May 11, 2024, 2:22 p.m. UTC | #14

Hi Rafael, Armin, et. al.,

On 5/10/24 8:00 PM, Rafael J. Wysocki wrote:
> On Fri, May 10, 2024 at 7:39 PM Armin Wolf <[e-mail]> wrote:
>>
>> Am 10.05.24 um 19:29 schrieb Andy Shevchenko:
>>
>>> On Fri, May 10, 2024 at 06:52:41PM +0200, Armin Wolf wrote:
>>>> Am 10.05.24 um 18:41 schrieb Rafael J. Wysocki:
>>>>> On Fri, May 10, 2024 at 6:10 PM Armin Wolf <[e-mail]> wrote:
>>>>>> Am 10.05.24 um 16:03 schrieb Rafael J. Wysocki:
>>>>>>
>>>>>>> From: Rafael J. Wysocki <[e-mail]>
>>>>>>>
>>>>>>> It is reported that _DSM evaluation fails in ucsi_acpi_dsm() on Lenovo
>>>>>>> IdeaPad Pro 5 due to a missing address space handler for the EC address
>>>>>>> space:
>>>>>>>
>>>>>>>     ACPI Error: No handler for Region [ECSI] (000000007b8176ee) [EmbeddedControl] (20230628/evregion-130)
>>>>>>>
>>>>>>> This happens because the EC driver only registers the EC address space
>>>>>>> handler for operation regions defined in the EC device scope of the
>>>>>>> ACPI namespace while the operation region being accessed by the _DSM
>>>>>>> in question is located beyond that scope.
>>>>>>>
>>>>>>> To address this, modify the ACPI EC driver to install the EC address
>>>>>>> space handler at the root of the ACPI namespace.
>>>>>>>
>>>>>>> Note that this change is consistent with some examples in the ACPI
>>>>>>> specification in which EC operation regions located outside the EC
>>>>>>> device scope are used (for example, see Section 9.17.15 in ACPI 6.5),
>>>>>>> so the current behavior of the EC driver is arguably questionable.
>>>>>> Hi,
>>>>>>
>>>>>> the patch itself looks good to me, but i wonder what happens if multiple
>>>>>> ACPI EC devices are present. How would we handle such a situation?
>>>>> I'm wondering if this is a theoretical question or do you have any
>>>>> existing or planned systems in mind?
>>>>>
>>>>> ec_read(), ec_write() and ec_transaction() use only the first EC that
>>>>> has been found anyway.
>>>> Its a theoretical question, i do not know of any systems which have more than
>>>> one ACPI EC device.
>>> The specification is clear about this case in the "ACPI Embedded Controller
>>> Interface Specification":
>>>
>>>   "The ACPI standard supports multiple embedded controllers in a system,
>>>    each with its own resources. Each embedded controller has a flat
>>>    byte-addressable I/O space, currently defined as 256 bytes."
>>>
>>> However, I haven't checked deeper, so it might be a leftover in the documentation.
>>>
>>> The OperationRegion() has no reference to the EC (or in general, device) which
>>> we need to speak to. The only possibility to declare OpRegion() for the second+
>>> EC is to use vendor specific RegionSpace, AFAIU. So, even if ACPI specification
>>> supports 2+ ECs, it doesn't support OpRegion():s for them under the same
>>> RegionSpace.
>>>
>>> That said, the commit message might be extended to summarize this, but at
>>> the same time I see no way how this series can break anything even in 2+ ECs
>>> environments.
>>
>> Consider the following execution flow when the second EC probes:
>>
>> 1. acpi_install_address_space_handler_no_reg() fails with AE_ALREADY_EXISTS since the first EC
>> has already installed a handler at ACPI_ROOT_OBJECT.
>>
>> 2. ec_install_handlers() fails with -ENODEV.
>>
>> 3. acpi_ec_setup() fails with -ENODEV.
>>
>> 4. acpi_ec_add() fails with -ENODEV.
>>
>> 5. Probe of seconds EC fails with -ENODEV.
>>
>> This might cause problems if the second EC is supposed to for example handle EC query events.
>> Of course if we only support a single EC, then this situation cannot happen.
> 
> This is kind of moot though until a system with 2 ECs is available.
> It is hard to say whether or not it is supported until it can be
> tested.

I do not believe that this is as theoretical as it sounds though.
If the ECDT and the DSDT disagree on the EC-s command_addr or
data_addr, then the check to re-use the boot_ec acpi_ec object
(struct acpi_ec *boot_ec) in acpi_ec_add() around line 1644:

                if (boot_ec && ec->command_addr == boot_ec->command_addr &&
                    ec->data_addr == boot_ec->data_addr) {

will fail and the separately allocated acpi_ec which "ec" points to at this
point will be kept around (rather then free-ed and replaced with the boot_ec).

And then when the below line runs on the newly allocated ec:

        ret = acpi_ec_setup(ec, device, true);

the newly allocated ec obj does not have EC_FLAGS_EC_HANDLER_INSTALLED set in
ec->flags so this acpi_ec_setup() call will call

               status = acpi_install_address_space_handler_no_reg(ec->handle,
                                                                  ACPI_ADR_SPACE_EC,
                                                                  &acpi_ec_space_handler,
                                                                  NULL, ec);

A second time. Now the above is from the old code and if we currently hit this
then the boot_ec acpi_install_address_space_handler_no_reg() call will have been
done with:

	ec->handle = ACPI_ROOT_OBJECT

and the second call for the not boot_ec matching DSDT EC will use the handle from
the DSDT EC.

Given how much quirks we have to deal with ECDT vs DSDT EC mismatches I'm pretty sure
that there is hw out there were we hit this path and atm we essentially treat that
as 2 ECs routing any OpRegion calls outside of the scope of the DSDT EC handle
to the boot_ec object and OpRegion calls any under the scope of the DSDT EC handle
to the regular "ec" object allocated in acpi_ec_add()

For such buggy hardware the old behavior can be preserved by passing which handle
to use for the acpi_install_address_space_handler_no_reg() call to acpi_ec_setup()
and pass ec->handle, rather then ACPI_ROOT_OBJECT when not re-using
the boot_ec in acpi_ec_add().

I think preserving the old behavior when we hit such buggy hw is the best thing
to do here. While at it we should probably also start logging a warning when
we hit this code path.

This does mean that we also need to keep acpi_ec.address_space_handler_holder
around for when unregistering the opregion handler.

Regards,

Hans

Rafael J. Wysocki May 14, 2024, 7:40 p.m. UTC | #15

Hi Hans,

On Sat, May 11, 2024 at 4:22 PM Hans de Goede <[e-mail]> wrote:
>
> Hi Rafael, Armin, et. al.,
>
> On 5/10/24 8:00 PM, Rafael J. Wysocki wrote:
> > On Fri, May 10, 2024 at 7:39 PM Armin Wolf <[e-mail]> wrote:
> >>
> >> Am 10.05.24 um 19:29 schrieb Andy Shevchenko:
> >>
> >>> On Fri, May 10, 2024 at 06:52:41PM +0200, Armin Wolf wrote:
> >>>> Am 10.05.24 um 18:41 schrieb Rafael J. Wysocki:
> >>>>> On Fri, May 10, 2024 at 6:10 PM Armin Wolf <[e-mail]> wrote:
> >>>>>> Am 10.05.24 um 16:03 schrieb Rafael J. Wysocki:
> >>>>>>
> >>>>>>> From: Rafael J. Wysocki <[e-mail]>
> >>>>>>>
> >>>>>>> It is reported that _DSM evaluation fails in ucsi_acpi_dsm() on Lenovo
> >>>>>>> IdeaPad Pro 5 due to a missing address space handler for the EC address
> >>>>>>> space:
> >>>>>>>
> >>>>>>>     ACPI Error: No handler for Region [ECSI] (000000007b8176ee) [EmbeddedControl] (20230628/evregion-130)
> >>>>>>>
> >>>>>>> This happens because the EC driver only registers the EC address space
> >>>>>>> handler for operation regions defined in the EC device scope of the
> >>>>>>> ACPI namespace while the operation region being accessed by the _DSM
> >>>>>>> in question is located beyond that scope.
> >>>>>>>
> >>>>>>> To address this, modify the ACPI EC driver to install the EC address
> >>>>>>> space handler at the root of the ACPI namespace.
> >>>>>>>
> >>>>>>> Note that this change is consistent with some examples in the ACPI
> >>>>>>> specification in which EC operation regions located outside the EC
> >>>>>>> device scope are used (for example, see Section 9.17.15 in ACPI 6.5),
> >>>>>>> so the current behavior of the EC driver is arguably questionable.
> >>>>>> Hi,
> >>>>>>
> >>>>>> the patch itself looks good to me, but i wonder what happens if multiple
> >>>>>> ACPI EC devices are present. How would we handle such a situation?
> >>>>> I'm wondering if this is a theoretical question or do you have any
> >>>>> existing or planned systems in mind?
> >>>>>
> >>>>> ec_read(), ec_write() and ec_transaction() use only the first EC that
> >>>>> has been found anyway.
> >>>> Its a theoretical question, i do not know of any systems which have more than
> >>>> one ACPI EC device.
> >>> The specification is clear about this case in the "ACPI Embedded Controller
> >>> Interface Specification":
> >>>
> >>>   "The ACPI standard supports multiple embedded controllers in a system,
> >>>    each with its own resources. Each embedded controller has a flat
> >>>    byte-addressable I/O space, currently defined as 256 bytes."
> >>>
> >>> However, I haven't checked deeper, so it might be a leftover in the documentation.
> >>>
> >>> The OperationRegion() has no reference to the EC (or in general, device) which
> >>> we need to speak to. The only possibility to declare OpRegion() for the second+
> >>> EC is to use vendor specific RegionSpace, AFAIU. So, even if ACPI specification
> >>> supports 2+ ECs, it doesn't support OpRegion():s for them under the same
> >>> RegionSpace.
> >>>
> >>> That said, the commit message might be extended to summarize this, but at
> >>> the same time I see no way how this series can break anything even in 2+ ECs
> >>> environments.
> >>
> >> Consider the following execution flow when the second EC probes:
> >>
> >> 1. acpi_install_address_space_handler_no_reg() fails with AE_ALREADY_EXISTS since the first EC
> >> has already installed a handler at ACPI_ROOT_OBJECT.
> >>
> >> 2. ec_install_handlers() fails with -ENODEV.
> >>
> >> 3. acpi_ec_setup() fails with -ENODEV.
> >>
> >> 4. acpi_ec_add() fails with -ENODEV.
> >>
> >> 5. Probe of seconds EC fails with -ENODEV.
> >>
> >> This might cause problems if the second EC is supposed to for example handle EC query events.
> >> Of course if we only support a single EC, then this situation cannot happen.
> >
> > This is kind of moot though until a system with 2 ECs is available.
> > It is hard to say whether or not it is supported until it can be
> > tested.
>
> I do not believe that this is as theoretical as it sounds though.
> If the ECDT and the DSDT disagree on the EC-s command_addr or
> data_addr, then the check to re-use the boot_ec acpi_ec object
> (struct acpi_ec *boot_ec) in acpi_ec_add() around line 1644:
>
>                 if (boot_ec && ec->command_addr == boot_ec->command_addr &&
>                     ec->data_addr == boot_ec->data_addr) {
>
> will fail and the separately allocated acpi_ec which "ec" points to at this
> point will be kept around (rather then free-ed and replaced with the boot_ec).

Good point.

> And then when the below line runs on the newly allocated ec:
>
>         ret = acpi_ec_setup(ec, device, true);
>
> the newly allocated ec obj does not have EC_FLAGS_EC_HANDLER_INSTALLED set in
> ec->flags so this acpi_ec_setup() call will call
>
>                status = acpi_install_address_space_handler_no_reg(ec->handle,
>                                                                   ACPI_ADR_SPACE_EC,
>                                                                   &acpi_ec_space_handler,
>                                                                   NULL, ec);
>
> A second time. Now the above is from the old code and if we currently hit this
> then the boot_ec acpi_install_address_space_handler_no_reg() call will have been
> done with:
>
>         ec->handle = ACPI_ROOT_OBJECT
>
> and the second call for the not boot_ec matching DSDT EC will use the handle from
> the DSDT EC.

If I'm not mistaken, this can be addressed by using ACPI_ROOT_OBJECT
to install the EC address space handler for first_ec only and
ec->handler for the other EC devices found in the ACPI namespace.

This actually is the case when ECDT is present and so (for consistency
and to address the issue leading to the $subject patch) it can be done
when there's no ECDT either.

Note that first_ec is only set once and never cleared (it would be
cleared during the EC driver removal if it were not equal to boot_ec,
but the latter is always the case AFAICS), so there can be only one ec
object with the address equal to first_ec.

> Given how much quirks we have to deal with ECDT vs DSDT EC mismatches I'm pretty sure
> that there is hw out there were we hit this path and atm we essentially treat that
> as 2 ECs routing any OpRegion calls outside of the scope of the DSDT EC handle
> to the boot_ec object and OpRegion calls any under the scope of the DSDT EC handle
> to the regular "ec" object allocated in acpi_ec_add()
>
> For such buggy hardware the old behavior can be preserved by passing which handle
> to use for the acpi_install_address_space_handler_no_reg() call to acpi_ec_setup()
> and pass ec->handle, rather then ACPI_ROOT_OBJECT when not re-using
> the boot_ec in acpi_ec_add().
>
> I think preserving the old behavior when we hit such buggy hw is the best thing
> to do here. While at it we should probably also start logging a warning when
> we hit this code path.

Yes, that would be useful IMV.

> This does mean that we also need to keep acpi_ec.address_space_handler_holder
> around for when unregistering the opregion handler.

Well, see above.

Thanks!

Hans de Goede May 15, 2024, 10:05 a.m. UTC | #16

Hi Rafael,

On 5/14/24 9:40 PM, Rafael J. Wysocki wrote:
> Hi Hans,
> 
> On Sat, May 11, 2024 at 4:22 PM Hans de Goede <[e-mail]> wrote:
>>
>> Hi Rafael, Armin, et. al.,
>>
>> On 5/10/24 8:00 PM, Rafael J. Wysocki wrote:
>>> On Fri, May 10, 2024 at 7:39 PM Armin Wolf <[e-mail]> wrote:
>>>>
>>>> Am 10.05.24 um 19:29 schrieb Andy Shevchenko:
>>>>
>>>>> On Fri, May 10, 2024 at 06:52:41PM +0200, Armin Wolf wrote:
>>>>>> Am 10.05.24 um 18:41 schrieb Rafael J. Wysocki:
>>>>>>> On Fri, May 10, 2024 at 6:10 PM Armin Wolf <[e-mail]> wrote:
>>>>>>>> Am 10.05.24 um 16:03 schrieb Rafael J. Wysocki:
>>>>>>>>
>>>>>>>>> From: Rafael J. Wysocki <[e-mail]>
>>>>>>>>>
>>>>>>>>> It is reported that _DSM evaluation fails in ucsi_acpi_dsm() on Lenovo
>>>>>>>>> IdeaPad Pro 5 due to a missing address space handler for the EC address
>>>>>>>>> space:
>>>>>>>>>
>>>>>>>>>     ACPI Error: No handler for Region [ECSI] (000000007b8176ee) [EmbeddedControl] (20230628/evregion-130)
>>>>>>>>>
>>>>>>>>> This happens because the EC driver only registers the EC address space
>>>>>>>>> handler for operation regions defined in the EC device scope of the
>>>>>>>>> ACPI namespace while the operation region being accessed by the _DSM
>>>>>>>>> in question is located beyond that scope.
>>>>>>>>>
>>>>>>>>> To address this, modify the ACPI EC driver to install the EC address
>>>>>>>>> space handler at the root of the ACPI namespace.
>>>>>>>>>
>>>>>>>>> Note that this change is consistent with some examples in the ACPI
>>>>>>>>> specification in which EC operation regions located outside the EC
>>>>>>>>> device scope are used (for example, see Section 9.17.15 in ACPI 6.5),
>>>>>>>>> so the current behavior of the EC driver is arguably questionable.
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> the patch itself looks good to me, but i wonder what happens if multiple
>>>>>>>> ACPI EC devices are present. How would we handle such a situation?
>>>>>>> I'm wondering if this is a theoretical question or do you have any
>>>>>>> existing or planned systems in mind?
>>>>>>>
>>>>>>> ec_read(), ec_write() and ec_transaction() use only the first EC that
>>>>>>> has been found anyway.
>>>>>> Its a theoretical question, i do not know of any systems which have more than
>>>>>> one ACPI EC device.
>>>>> The specification is clear about this case in the "ACPI Embedded Controller
>>>>> Interface Specification":
>>>>>
>>>>>   "The ACPI standard supports multiple embedded controllers in a system,
>>>>>    each with its own resources. Each embedded controller has a flat
>>>>>    byte-addressable I/O space, currently defined as 256 bytes."
>>>>>
>>>>> However, I haven't checked deeper, so it might be a leftover in the documentation.
>>>>>
>>>>> The OperationRegion() has no reference to the EC (or in general, device) which
>>>>> we need to speak to. The only possibility to declare OpRegion() for the second+
>>>>> EC is to use vendor specific RegionSpace, AFAIU. So, even if ACPI specification
>>>>> supports 2+ ECs, it doesn't support OpRegion():s for them under the same
>>>>> RegionSpace.
>>>>>
>>>>> That said, the commit message might be extended to summarize this, but at
>>>>> the same time I see no way how this series can break anything even in 2+ ECs
>>>>> environments.
>>>>
>>>> Consider the following execution flow when the second EC probes:
>>>>
>>>> 1. acpi_install_address_space_handler_no_reg() fails with AE_ALREADY_EXISTS since the first EC
>>>> has already installed a handler at ACPI_ROOT_OBJECT.
>>>>
>>>> 2. ec_install_handlers() fails with -ENODEV.
>>>>
>>>> 3. acpi_ec_setup() fails with -ENODEV.
>>>>
>>>> 4. acpi_ec_add() fails with -ENODEV.
>>>>
>>>> 5. Probe of seconds EC fails with -ENODEV.
>>>>
>>>> This might cause problems if the second EC is supposed to for example handle EC query events.
>>>> Of course if we only support a single EC, then this situation cannot happen.
>>>
>>> This is kind of moot though until a system with 2 ECs is available.
>>> It is hard to say whether or not it is supported until it can be
>>> tested.
>>
>> I do not believe that this is as theoretical as it sounds though.
>> If the ECDT and the DSDT disagree on the EC-s command_addr or
>> data_addr, then the check to re-use the boot_ec acpi_ec object
>> (struct acpi_ec *boot_ec) in acpi_ec_add() around line 1644:
>>
>>                 if (boot_ec && ec->command_addr == boot_ec->command_addr &&
>>                     ec->data_addr == boot_ec->data_addr) {
>>
>> will fail and the separately allocated acpi_ec which "ec" points to at this
>> point will be kept around (rather then free-ed and replaced with the boot_ec).
> 
> Good point.
> 
>> And then when the below line runs on the newly allocated ec:
>>
>>         ret = acpi_ec_setup(ec, device, true);
>>
>> the newly allocated ec obj does not have EC_FLAGS_EC_HANDLER_INSTALLED set in
>> ec->flags so this acpi_ec_setup() call will call
>>
>>                status = acpi_install_address_space_handler_no_reg(ec->handle,
>>                                                                   ACPI_ADR_SPACE_EC,
>>                                                                   &acpi_ec_space_handler,
>>                                                                   NULL, ec);
>>
>> A second time. Now the above is from the old code and if we currently hit this
>> then the boot_ec acpi_install_address_space_handler_no_reg() call will have been
>> done with:
>>
>>         ec->handle = ACPI_ROOT_OBJECT
>>
>> and the second call for the not boot_ec matching DSDT EC will use the handle from
>> the DSDT EC.
> 
> If I'm not mistaken, this can be addressed by using ACPI_ROOT_OBJECT
> to install the EC address space handler for first_ec only and
> ec->handler for the other EC devices found in the ACPI namespace.

Yes that should work and is a nice solution, that would require
moving the setting of first_ec to above the ec_install_handlers()
call in acpi_ec_setup(), with that small change we could do:

                status = acpi_install_address_space_handler_no_reg(
					(ec == first_ec) ? ACPI_ROOT_OBJECT : ec->handle,
                                        ACPI_ADR_SPACE_EC, &acpi_ec_space_handler, NULL, ec);

in ec_install_handlers() and mirror that in ec_remove_handlers().

> 
> This actually is the case when ECDT is present and so (for consistency
> and to address the issue leading to the $subject patch) it can be done
> when there's no ECDT either.
> 
> Note that first_ec is only set once and never cleared (it would be
> cleared during the EC driver removal if it were not equal to boot_ec,
> but the latter is always the case AFAICS), so there can be only one ec
> object with the address equal to first_ec.

Right I was wondering the same, wondering why we have boot boot_ec and
first_ec.

I guess when there is no ECDT we could somehow not find the DSDT defined
EC during acpi_ec_dsdt_probe() due to some weirdness in the DSDT (e.g.
dynamically set _HID) but then find it later, then we would hit a case
where boot_ec is left at NULL and only first_ec is set.

In that case in the theoretical case of acpi_ec_remove() getting called
(which should never happen) then we would clear first_ec. But that clearing
is done by acpi_ec_free() which gets called *after* ec_remove_handlers()
so even then we can still mirror the (ec == first_ec) check in
ec_remove_handlers() and it will do the right thing.

The other way around however, when there is a boot_ec then that will
indeed always be the same as first_ec and first_ec will never get
cleared and this will be the common case.


>> Given how much quirks we have to deal with ECDT vs DSDT EC mismatches I'm pretty sure
>> that there is hw out there were we hit this path and atm we essentially treat that
>> as 2 ECs routing any OpRegion calls outside of the scope of the DSDT EC handle
>> to the boot_ec object and OpRegion calls any under the scope of the DSDT EC handle
>> to the regular "ec" object allocated in acpi_ec_add()
>>
>> For such buggy hardware the old behavior can be preserved by passing which handle
>> to use for the acpi_install_address_space_handler_no_reg() call to acpi_ec_setup()
>> and pass ec->handle, rather then ACPI_ROOT_OBJECT when not re-using
>> the boot_ec in acpi_ec_add().
>>
>> I think preserving the old behavior when we hit such buggy hw is the best thing
>> to do here. While at it we should probably also start logging a warning when
>> we hit this code path.
> 
> Yes, that would be useful IMV.
> 
>> This does mean that we also need to keep acpi_ec.address_space_handler_holder
>> around for when unregistering the opregion handler.
> 
> Well, see above.

Ack.

TL;DR: I like your suggestion of doing something like:

                status = acpi_install_address_space_handler_no_reg(
					(ec == first_ec) ? ACPI_ROOT_OBJECT : ec->handle,
                                        ACPI_ADR_SPACE_EC, &acpi_ec_space_handler, NULL, ec);

And drop acpi_ec.address_space_handler_holder, so lets go with that.

Regards,

Hans

Patch

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;