linux-kernel - Re: [PATCH v1 1/2] ACPI: EC: Install address space handler at the namespace root

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 10 May 2024 19:47:16 +0200
From: Armin Wolf <W_Armin@....de>
To: Mario Limonciello <mario.limonciello@....com>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>,
 "Rafael J. Wysocki" <rjw@...ysocki.net>,
 Linux ACPI <linux-acpi@...r.kernel.org>, LKML
 <linux-kernel@...r.kernel.org>, Hans de Goede <hdegoede@...hat.com>,
 Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Subject: Re: [PATCH v1 1/2] ACPI: EC: Install address space handler at the
 namespace root

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 <W_Armin@....de> wrote:
>>>>> Am 10.05.24 um 16:03 schrieb Rafael J. Wysocki:
>>>>>
>>>>>> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>>>>>>
>>>>>> 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.
>>
>