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

["Rafael J. Wysocki" <rafael@kernel.org>]

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.

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.

> 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.

Thanks!