| Message ID | 5787281.DvuYhMxLoT@kreacher |
|---|---|
| Headers | show |
| Series | ACPI: EC: Install EC address space handler at the namespace root | expand |
On 5/10/2024 09:01, Rafael J. Wysocki wrote: > Hi Everyone, > > This is a follow up for the discussion in: > > https://lore.kernel.org/linux-acpi/CAJZ5v0hiXdv08PRcop7oSYqgr_g5rwzRTj7HgdNCCGjXeV44zA@mail.gmail.com/T/#t > > Patch [1/2] is a resend of the patch posted in the thread above and patch [2/2] > removes the custom EC address space handler from the WMI driver as it should > not be necessary any more. > > Thanks! > > > For the series: Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
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.
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. > This patch would prevent any ACPI ECs other than the first one from probing, > since they would fail to register their address space handler. > I am just curious if/how we want to handle such situations.
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 patch would prevent any ACPI ECs other than the first one from probing, >> since they would fail to register their address space handler. >> I am just curious if/how we want to handle such situations.
On 5/10/2024 12:29, Andy Shevchenko wrote: > 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. It's deviating from the patch, but in practice /why/ would you even want to have a design with two ECs? In general that is going to mean a much more complex state machine with synchronizing the interaction between both of them and the host. Understanding the benefit of such a design might make it easier to hypothesize impacts. > >> This patch would prevent any ACPI ECs other than the first one from probing, >> since they would fail to register their address space handler. >> I am just curious if/how we want to handle such situations. >
Am 10.05.24 um 19:40 schrieb Mario Limonciello: > On 5/10/2024 12:29, Andy Shevchenko wrote: >> 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. > > It's deviating from the patch, but in practice /why/ would you even > want to have a design with two ECs? In general that is going to mean > a much more complex state machine with synchronizing the interaction > between both of them and the host. > > Understanding the benefit of such a design might make it easier to > hypothesize impacts. > I am not saying that such designs would make sense, it was a theoretical question only. Maybe we can just add a small note to the Linux ACPI documentation saying that we only support a single EC device? Thanks, Armin Wolf >> >>> This patch would prevent any ACPI ECs other than the first one from >>> probing, >>> since they would fail to register their address space handler. >>> I am just curious if/how we want to handle such situations. >> >
On Fri, May 10, 2024 at 12:40:05PM -0500, Mario Limonciello wrote: > On 5/10/2024 12:29, Andy Shevchenko wrote: > > 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. > > It's deviating from the patch, but in practice /why/ would you even want to > have a design with two ECs? In general that is going to mean a much more > complex state machine with synchronizing the interaction between both of > them and the host. > > Understanding the benefit of such a design might make it easier to > hypothesize impacts. First that comes to my mind (but hypothetical), is the separate CPU/EC add-on cards. If the main firmware somehow supports all of these add-on platforms, it might need to handle 2+ ECs. Again, it might be ACPI specification issue. For instance, the cited piece doesn't tell about 16-bit EC accesses. > > > This patch would prevent any ACPI ECs other than the first one from probing, > > > since they would fail to register their address space handler. > > > I am just curious if/how we want to handle such situations.
On Fri, May 10, 2024 at 08:50:29PM +0300, Andy Shevchenko wrote: > On Fri, May 10, 2024 at 12:40:05PM -0500, Mario Limonciello wrote: > > On 5/10/2024 12:29, Andy Shevchenko wrote: > > > 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. > > > > It's deviating from the patch, but in practice /why/ would you even want to > > have a design with two ECs? In general that is going to mean a much more > > complex state machine with synchronizing the interaction between both of > > them and the host. > > > > Understanding the benefit of such a design might make it easier to > > hypothesize impacts. > > First that comes to my mind (but hypothetical), is the separate CPU/EC add-on > cards. If the main firmware somehow supports all of these add-on platforms, > it might need to handle 2+ ECs. > > Again, it might be ACPI specification issue. For instance, the cited piece > doesn't tell about 16-bit EC accesses. Chapter "4.8.4.2.2 Embedded Controller" among other says: "Additionally the embedded controller can support up to 255 generic events per embedded controller, referred to as query events." So, if specification needs an update, it should be very carefully proof-read. > > > > This patch would prevent any ACPI ECs other than the first one from probing, > > > > since they would fail to register their address space handler. > > > > I am just curious if/how we want to handle such situations.
On Fri, May 10, 2024 at 6:52 PM Armin Wolf <W_Armin@gmx.de> 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. > > This patch would prevent any ACPI ECs other than the first one from probing, > since they would fail to register their address space handler. > I am just curious if/how we want to handle such situations. I'm not worried until I see a system where that is a problem. That said, it can be addressed by adding a first_ec check around the address space handler registration/unregistration.
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.
On Fri, May 10, 2024 at 7:50 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, May 10, 2024 at 12:40:05PM -0500, Mario Limonciello wrote: > > On 5/10/2024 12:29, Andy Shevchenko wrote: > > > 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. > > > > It's deviating from the patch, but in practice /why/ would you even want to > > have a design with two ECs? In general that is going to mean a much more > > complex state machine with synchronizing the interaction between both of > > them and the host. > > > > Understanding the benefit of such a design might make it easier to > > hypothesize impacts. > > First that comes to my mind (but hypothetical), is the separate CPU/EC add-on > cards. If the main firmware somehow supports all of these add-on platforms, > it might need to handle 2+ ECs. > > Again, it might be ACPI specification issue. For instance, the cited piece > doesn't tell about 16-bit EC accesses. IMV this is a matter of what is testable. We can only seriously say that we support 1 EC in the system, because that's what we can test. Now, the specification allows (theoretically) multiple ECs to be supported which does not mean that it will ever be done in practice and it also does not mean that this is a good idea.
On Fri, May 10, 2024 at 08:06:11PM +0200, Rafael J. Wysocki wrote: > On Fri, May 10, 2024 at 7:50 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, May 10, 2024 at 12:40:05PM -0500, Mario Limonciello wrote: > > > On 5/10/2024 12:29, Andy Shevchenko wrote: > > > > 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. > > > > > > It's deviating from the patch, but in practice /why/ would you even want to > > > have a design with two ECs? In general that is going to mean a much more > > > complex state machine with synchronizing the interaction between both of > > > them and the host. > > > > > > Understanding the benefit of such a design might make it easier to > > > hypothesize impacts. > > > > First that comes to my mind (but hypothetical), is the separate CPU/EC add-on > > cards. If the main firmware somehow supports all of these add-on platforms, > > it might need to handle 2+ ECs. > > > > Again, it might be ACPI specification issue. For instance, the cited piece > > doesn't tell about 16-bit EC accesses. > > IMV this is a matter of what is testable. > > We can only seriously say that we support 1 EC in the system, because > that's what we can test. > > Now, the specification allows (theoretically) multiple ECs to be > supported which does not mean that it will ever be done in practice > and it also does not mean that this is a good idea. I briefly read the all mentions of the "Embedded Controller" in the specification and like 98% implies that the only one is per system. I believe the specification may be corrected to remove ambiguous plural forms in a couple (or a few) places. In any case it's a question to ASWG.
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
On Fri, May 10, 2024 at 04:01:41PM +0200, Rafael J. Wysocki wrote: > Hi Everyone, > > This is a follow up for the discussion in: > > https://lore.kernel.org/linux-acpi/CAJZ5v0hiXdv08PRcop7oSYqgr_g5rwzRTj7HgdNCCGjXeV44zA@mail.gmail.com/T/#t > > Patch [1/2] is a resend of the patch posted in the thread above and patch [2/2] > removes the custom EC address space handler from the WMI driver as it should > not be necessary any more. For the series: Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> Thanks Rafael!
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!