On Fri, May 10, 2024 at 7:39 PM Armin Wolf <W_Armin@xxxxxx> 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@xxxxxx> wrote: > >>>> Am 10.05.24 um 16:03 schrieb Rafael J. Wysocki: > >>>> > >>>>> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx> > >>>>> > >>>>> 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.
