Received-SPF: pass (google.com: domain of linux-kernel+bounces-176066-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <269412ef-13de-4192-9289-52e51507e0b5@gmx.de>
Date: Fri, 10 May 2024 19:47:16 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1 1/2] ACPI: EC: Install address space handler at the
 namespace root
To: Mario Limonciello <mario.limonciello@amd.com>,
 Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
 "Rafael J. Wysocki" <rjw@rjwysocki.net>,
 Linux ACPI <linux-acpi@vger.kernel.org>, LKML
 <linux-kernel@vger.kernel.org>, Hans de Goede <hdegoede@redhat.com>,
 Heikki Krogerus <heikki.krogerus@linux.intel.com>
References: <5787281.DvuYhMxLoT@kreacher> <4926735.31r3eYUQgx@kreacher>
 <ac04c433-b0ac-4b82-b8eb-98ac16f872d8@gmx.de>
 <CAJZ5v0g_NjGHRvhm-N5vQFnOsqnxExSq99v8n_B_6ANoaCga0w@mail.gmail.com>
 <568291fc-fd79-4f08-9eb7-aed7f5a32345@gmx.de>
 <Zj5ZdcQeaTo9ImT4@smile.fi.intel.com>
 <0cdf0af6-851b-4781-83fe-99320c35544f@amd.com>
Content-Language: en-US
From: Armin Wolf <W_Armin@gmx.de>
In-Reply-To: <0cdf0af6-851b-4781-83fe-99320c35544f@amd.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable
UI-OutboundReport: notjunk:1;M01:P0:oHUl/hDCxWo=;AzpTXZuB7plqogmUY/9bxEW6iGX
 Ewm3HzbOe3pmKMFxHG4HntAWL8KH4LWyKlwlIBpKHyJztif8b6flVLJEqo+iFHNh1d473yYgY
 P9//MohkIYqL2yuIJo7YHerdZLqlNYGpgwZdPtyYSbi0nZsAEFisdbtrPfyvLZVhHkiPCvxL/
 zj2ufifdfa+yjHsi8K37Zk6kMY36tlu975kjS4m8l85O90norHuJ5FN8SkP3R2WI2tYofMoWn
 65vJWxR8VucdHwaF97hFtnCXtXwQAX6uK03sfmrYTR7gHKcjnwxJUvNoCNp21TGcI5g0NJLGM
 J3YE1zfln3MbqWS0+6PhEZGsYRsU/gXjbeuYMjP0kgpmhLCrMiPCCSV/C3wJBOix+6laJ8L6n
 b+kZiwHrqOPlSKKM/Jlfhm6kGn1qCIfSSCN2hTy/QykJHAmW+SVCDjS1rV52zmJqv5P21apTU
 rnLdlB+b2rOR06IPU90UjSzl2w4aYA00RzG/o4weFPQVl8FTWQJKNmeqlNxxdYxAHbaxUG0Wg
 WW78hhSZK+C3odfLo78AFIcxwS9YjiFCXrtt05OP75FxMko1ii1w3hEC3I8KIU68Fvy7qy170
 OjYy9/RFWDusfO9Q8ldnQPjLIM1lqHv15Ds0gUo6P058R83M+nCndQID2ZA6UHeXVbLvDwiLf
 PyLDsqk53QGQ3fLKVap0FZz+R8ro6tytW4BhFvFMQ1gS59sv3+7fLzmnV5hmDm6ZKDmfVQXY8
 yk6aOofW58GBiA3nPrS3Z33B1H1uZa7d7ZUHu2/XD53G4VhM5dkBkAw22Tuk07/VcYbXhWUYG
 Sr1ZM6qYPEuFp18VUgOkscJ5RDiruYr+pUzoW53CJmzS4=

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=E2=80=AFPM Armin Wolf <W_Armin@gmx.de> w=
rote:
>>>>> Am 10.05.24 um 16:03 schrieb Rafael J. Wysocki:
>>>>>
>>>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.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:
>>>>>>
>>>>>> =C2=A0=C2=A0=C2=A0 ACPI Error: No handler for Region [ECSI] (000000=
007b8176ee)
>>>>>> [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 _DS=
M
>>>>>> in question is located beyond that scope.
>>>>>>
>>>>>> To address this, modify the ACPI EC driver to install the EC addres=
s
>>>>>> 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":
>>
>> =C2=A0 "The ACPI standard supports multiple embedded controllers in a s=
ystem,
>> =C2=A0=C2=A0 each with its own resources. Each embedded controller has =
a flat
>> =C2=A0=C2=A0 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 sam=
e
>> 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?=C2=A0 In general that is going to me=
an
> 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 q=
uestion 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.
>>
>