Received-SPF: pass (google.com: domain of linux-kernel+bounces-176079-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
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>
 <22937f20-93fd-4ae2-a5cb-31e5a477ac37@gmx.de>
In-Reply-To: <22937f20-93fd-4ae2-a5cb-31e5a477ac37@gmx.de>
From: "Rafael J. Wysocki" <rafael@kernel.org>
Date: Fri, 10 May 2024 20:00:57 +0200
Message-ID: <CAJZ5v0iC_DNTj38eCPywgG3v_pkOVOhY9itdgX09AJ7U3yRB4Q@mail.gmail.com>
Subject: Re: [PATCH v1 1/2] ACPI: EC: Install address space handler at the
 namespace root
To: Armin Wolf <W_Armin@gmx.de>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>, 
	"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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, May 10, 2024 at 7:39=E2=80=AFPM Armin Wolf <W_Armin@gmx.de> 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=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 Len=
ovo
> >>>>> IdeaPad Pro 5 due to a missing address space handler for the EC add=
ress
> >>>>> space:
> >>>>>
> >>>>>     ACPI Error: No handler for Region [ECSI] (000000007b8176ee) [Em=
beddedControl] (20230628/evregion-130)
> >>>>>
> >>>>> This happens because the EC driver only registers the EC address sp=
ace
> >>>>> 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 mult=
iple
> >>>> 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 mo=
re than
> >> one ACPI EC device.
> > The specification is clear about this case in the "ACPI Embedded Contro=
ller
> > 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 doc=
umentation.
> >
> > 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 speci=
fication
> > 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.
>
> Consider the following execution flow when the second EC probes:
>
> 1. acpi_install_address_space_handler_no_reg() fails with AE_ALREADY_EXIS=
TS 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 han=
dle EC query events.
> Of course if we only support a single EC, then this situation cannot happ=
en.

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.