Received-SPF: pass (google.com: domain of linux-kernel+bounces-176573-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <0107bd7c-b4d5-40d3-9147-ed06e1c1f430@redhat.com>
Date: Sat, 11 May 2024 16:22:17 +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: "Rafael J. Wysocki" <rafael@kernel.org>, Armin Wolf <W_Armin@gmx.de>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
 "Rafael J. Wysocki" <rjw@rjwysocki.net>,
 Linux ACPI <linux-acpi@vger.kernel.org>, LKML
 <linux-kernel@vger.kernel.org>, 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>
 <22937f20-93fd-4ae2-a5cb-31e5a477ac37@gmx.de>
 <CAJZ5v0iC_DNTj38eCPywgG3v_pkOVOhY9itdgX09AJ7U3yRB4Q@mail.gmail.com>
Content-Language: en-US, nl
From: Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <CAJZ5v0iC_DNTj38eCPywgG3v_pkOVOhY9itdgX09AJ7U3yRB4Q@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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 <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 PM Armin Wolf <W_Armin@gmx.de> wrote:
>>>>>> 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:
>>>>>>>
>>>>>>>     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