Received-SPF: pass (google.com: domain of linux-kernel+bounces-176057-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <22937f20-93fd-4ae2-a5cb-31e5a477ac37@gmx.de>
Date: Fri, 10 May 2024 19:38:56 +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: 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>,
 Mario Limonciello <mario.limonciello@amd.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>
Content-Language: en-US
From: Armin Wolf <W_Armin@gmx.de>
In-Reply-To: <Zj5ZdcQeaTo9ImT4@smile.fi.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable
UI-OutboundReport: notjunk:1;M01:P0:1K6PtlAGIfw=;2BcAVPpb1fvsHbnQxxUrenpuOAx
 EQ5BFX3d+fsO+xwVpINiKqlaB8f4dQSg7CZPlEqsBOcisrYkhn7IuCaqaAvchgH9lWcur2J5B
 EYEGuGse3/4cBxkLzsaiqsCanyfnvE+hHqTeXYrTTDNJS5/9tIpydZH0kU1uOshWcDYR4kS2k
 w9DYDqo6KiiqU0fJlnn4RiafWCuHcJM5LF11E56YWWhrWP9VoeTzqOZLPm9USfrP0qt0DMRyM
 1QZMohlboSAf/sfZDqu0vB0dCNCH6owBUhP7PnW1WSs2mIDXZBlIaHbDSbHKzTtdyihymAmm3
 xYhQYOWMVebi4zlpotf8/k7Q/CBt/F74al35DdRmzN2+6iG0+CqSJWOQOY5sgRzjImKJLNOnG
 JE52jAUZQq2zutaFFtaDmGPrf1DlXtf/siFBluH6eFTJhAlPS4aP6+M9pxUKEnetVbQUfWiA1
 BZ1KXcw2VLLKhymPVbvcHO6mFcvUOscmaX0RdmLdTeW72OGigCbgKpRG+d7UalA57ruuYMm/K
 SKHwnd1CdxW3AbD8ul8bmFYDOqTNRLf7GaFYE7XVHSUqjvevA85u+UZAxo0RU+kHlIW620h5y
 K6fHnacNELWzasFy/e4O/LC2iPjXYx3PCUEGTgxPzthvlEcDHCryJT+Uu5+LebUu1Pb+iKzjB
 Wt/C/ytF89IWXk32ns5lJ6ExoETiQ6T+DxPe77IwvQ+Fniy7tsSLlKrjFalzYF7Tv+uWjDvYW
 qP7eukc7oQBwGhpFAjUIjVNpMZxDKR8DR3JNDwhuGbXJ/kmKZNIi3Z9hiT8QIZP53coWHTGp2
 IB6icSkdXNb0cifCCU8lqvxK62J86SXHsCX2UhxDpJsO4=

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=E2=80=AFPM Armin Wolf <W_Armin@gmx.de> wr=
ote:
>>>> 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 Leno=
vo
>>>>> IdeaPad Pro 5 due to a missing address space handler for the EC addr=
ess
>>>>> space:
>>>>>
>>>>>     ACPI Error: No handler for Region [ECSI] (000000007b8176ee) [Emb=
eddedControl] (20230628/evregion-130)
>>>>>
>>>>> This happens because the EC driver only registers the EC address spa=
ce
>>>>> 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 multi=
ple
>>>> 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 mor=
e than
>> one ACPI EC device.
> The specification is clear about this case in the "ACPI Embedded Control=
ler
> 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 docu=
mentation.
>
> 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 specif=
ication
> 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 a=
t
> 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_EXIST=
S 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 hand=
le EC query events.
Of course if we only support a single EC, then this situation cannot happe=
n.

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