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

[Hans de Goede <hdegoede@redhat.com>]

Hi Rafael,

On 5/14/24 9:40 PM, Rafael J. Wysocki wrote:
> Hi Hans,
> 
> On Sat, May 11, 2024 at 4:22 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> 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).
> 
> Good point.
> 
>> 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.
> 
> If I'm not mistaken, this can be addressed by using ACPI_ROOT_OBJECT
> to install the EC address space handler for first_ec only and
> ec->handler for the other EC devices found in the ACPI namespace.

Yes that should work and is a nice solution, that would require
moving the setting of first_ec to above the ec_install_handlers()
call in acpi_ec_setup(), with that small change we could do:

                status = acpi_install_address_space_handler_no_reg(
					(ec == first_ec) ? ACPI_ROOT_OBJECT : ec->handle,
                                        ACPI_ADR_SPACE_EC, &acpi_ec_space_handler, NULL, ec);

in ec_install_handlers() and mirror that in ec_remove_handlers().

> 
> This actually is the case when ECDT is present and so (for consistency
> and to address the issue leading to the $subject patch) it can be done
> when there's no ECDT either.
> 
> Note that first_ec is only set once and never cleared (it would be
> cleared during the EC driver removal if it were not equal to boot_ec,
> but the latter is always the case AFAICS), so there can be only one ec
> object with the address equal to first_ec.

Right I was wondering the same, wondering why we have boot boot_ec and
first_ec.

I guess when there is no ECDT we could somehow not find the DSDT defined
EC during acpi_ec_dsdt_probe() due to some weirdness in the DSDT (e.g.
dynamically set _HID) but then find it later, then we would hit a case
where boot_ec is left at NULL and only first_ec is set.

In that case in the theoretical case of acpi_ec_remove() getting called
(which should never happen) then we would clear first_ec. But that clearing
is done by acpi_ec_free() which gets called *after* ec_remove_handlers()
so even then we can still mirror the (ec == first_ec) check in
ec_remove_handlers() and it will do the right thing.

The other way around however, when there is a boot_ec then that will
indeed always be the same as first_ec and first_ec will never get
cleared and this will be the common case.


>> 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.
> 
> Yes, that would be useful IMV.
> 
>> This does mean that we also need to keep acpi_ec.address_space_handler_holder
>> around for when unregistering the opregion handler.
> 
> Well, see above.

Ack.

TL;DR: I like your suggestion of doing something like:

                status = acpi_install_address_space_handler_no_reg(
					(ec == first_ec) ? ACPI_ROOT_OBJECT : ec->handle,
                                        ACPI_ADR_SPACE_EC, &acpi_ec_space_handler, NULL, ec);

And drop acpi_ec.address_space_handler_holder, so lets go with that.

Regards,

Hans