[PATCH] ACPI / EC: handle ECDT EC and DSDT EC simultaneously

[PATCH] ACPI / EC: handle ECDT EC and DSDT EC simultaneously

[Daniel Drake]

The Asus laptops X550VXK/FX502VD/FX502VE have a BIOS bug where the
ECDT (correctly) states that EC events trigger GPE 0x23, but the
DSDT _GPE method (incorrectly) says GPE 0x33.

A cursory glance of the code suggests that this should work regardless
(because it looks like Linux would treat the ECDT and DSDT as two separate
ECs supported simultaneously), but in practice it is not working since the
ECDT is ultimately ignored in favour of the DSDT EC. The sequence of
events is:

1. acpi_ec_ecdt_probe() is called in early boot, and calls
   acpi_config_boot_ec(is_ecdt=true) for the ECDT EC.

acpi_config_boot_ec() sets boot_ec to this new EC, and
boot_ec_is_ecdt = true.

2. Later, acpi_ec_dsdt_probe() is called and creates a new ec to represent
   the DSDT EC. Then:
    /*
     * When the DSDT EC is available, always re-configure boot EC to
     * have _REG evaluated. _REG can only be evaluated after the
     * namespace initialization.
     * At this point, the GPE is not fully initialized, so do not to
     * handle the events.
     */
    ret = acpi_config_boot_ec(ec, ec->handle, false, false);

So the DSDT EC is passed to acpi_config_boot_ec() which does:
    /* Unset old boot EC */
    if (boot_ec != ec)
        acpi_ec_free(boot_ec);

acpi_ec_free() sets boot_ec to NULL.
Further down in acpi_config_boot_ec() we reach:

    /* Set new boot EC */
    if (!boot_ec) {
        boot_ec = ec;
        boot_ec_is_ecdt = is_ecdt;
    }

So now boot_ec points to the DSDT EC and boot_ec_is_ecdt is false.

3. Later, ecpi_ec_ecdt_start() is called and this looks like it would
   enable GPEs on our ECDT, but it bails out because of:

    if (!boot_ec_is_ecdt)
        return -ENODEV;


The comment I pasted above from acpi_ec_dsdt_probe() says that it is going
to reconfigure the boot EC, but it instead calls acpi_config_boot_ec() on
the newly probed DSDT EC. It seems like the code is not following the
comment here.

Adjusting that code to work with boot_ec adjusts the sequence of events so
that both boot EC and DSDT are treated independently and as a result, we
get EC interrupts firing correctly.

This fixes media keys on the mentioned laptop models.
Thanks to Chris Chiu for diagnosis.

Signed-off-by: Daniel Drake <drake@endlessm.com>
---
 drivers/acpi/ec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index c24235d8fb52..78395395e3d9 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1691,7 +1691,7 @@ int __init acpi_ec_dsdt_probe(void)
 	 * At this point, the GPE is not fully initialized, so do not to
 	 * handle the events.
 	 */
-	ret = acpi_config_boot_ec(ec, ec->handle, false, false);
+	ret = acpi_config_boot_ec(boot_ec, boot_ec->handle, false, false);
 error:
 	if (ret)
 		acpi_ec_free(ec);
-- 
2.11.0

Re: [PATCH] ACPI / EC: handle ECDT EC and DSDT EC simultaneously

[Rafael J. Wysocki]

On Thu, Apr 20, 2017 at 10:12 PM, Daniel Drake <drake@endlessm.com> wrote:
> The Asus laptops X550VXK/FX502VD/FX502VE have a BIOS bug where the
> ECDT (correctly) states that EC events trigger GPE 0x23, but the
> DSDT _GPE method (incorrectly) says GPE 0x33.
>
> A cursory glance of the code suggests that this should work regardless
> (because it looks like Linux would treat the ECDT and DSDT as two separate
> ECs supported simultaneously), but in practice it is not working since the
> ECDT is ultimately ignored in favour of the DSDT EC. The sequence of
> events is:
>
> 1. acpi_ec_ecdt_probe() is called in early boot, and calls
>    acpi_config_boot_ec(is_ecdt=true) for the ECDT EC.
>
> acpi_config_boot_ec() sets boot_ec to this new EC, and
> boot_ec_is_ecdt = true.
>
> 2. Later, acpi_ec_dsdt_probe() is called and creates a new ec to represent
>    the DSDT EC. Then:
>     /*
>      * When the DSDT EC is available, always re-configure boot EC to
>      * have _REG evaluated. _REG can only be evaluated after the
>      * namespace initialization.
>      * At this point, the GPE is not fully initialized, so do not to
>      * handle the events.
>      */
>     ret = acpi_config_boot_ec(ec, ec->handle, false, false);
>
> So the DSDT EC is passed to acpi_config_boot_ec() which does:
>     /* Unset old boot EC */
>     if (boot_ec != ec)
>         acpi_ec_free(boot_ec);
>
> acpi_ec_free() sets boot_ec to NULL.
> Further down in acpi_config_boot_ec() we reach:
>
>     /* Set new boot EC */
>     if (!boot_ec) {
>         boot_ec = ec;
>         boot_ec_is_ecdt = is_ecdt;
>     }
>
> So now boot_ec points to the DSDT EC and boot_ec_is_ecdt is false.
>
> 3. Later, ecpi_ec_ecdt_start() is called and this looks like it would
>    enable GPEs on our ECDT, but it bails out because of:
>
>     if (!boot_ec_is_ecdt)
>         return -ENODEV;
>
>
> The comment I pasted above from acpi_ec_dsdt_probe() says that it is going
> to reconfigure the boot EC, but it instead calls acpi_config_boot_ec() on
> the newly probed DSDT EC. It seems like the code is not following the
> comment here.
>
> Adjusting that code to work with boot_ec adjusts the sequence of events so
> that both boot EC and DSDT are treated independently and as a result, we
> get EC interrupts firing correctly.
>
> This fixes media keys on the mentioned laptop models.
> Thanks to Chris Chiu for diagnosis.
>
> Signed-off-by: Daniel Drake <drake@endlessm.com>

Lv, can you have a look at this, please?

> ---
>  drivers/acpi/ec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index c24235d8fb52..78395395e3d9 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -1691,7 +1691,7 @@ int __init acpi_ec_dsdt_probe(void)
>          * At this point, the GPE is not fully initialized, so do not to
>          * handle the events.
>          */
> -       ret = acpi_config_boot_ec(ec, ec->handle, false, false);
> +       ret = acpi_config_boot_ec(boot_ec, boot_ec->handle, false, false);
>  error:
>         if (ret)
>                 acpi_ec_free(ec);
> --
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] ACPI / EC: handle ECDT EC and DSDT EC simultaneously

[Zheng, Lv]

Hi,

> From: Daniel Drake [mailto:drake@endlessm.com]
> Sent: Friday, April 21, 2017 4:12 AM
> To: rjw@rjwysocki.net; lenb@kernel.org
> Cc: linux-acpi@vger.kernel.org; Zheng, Lv <lv.zheng@intel.com>; chiu@endlessm.com; linux@endlessm.com
> Subject: [PATCH] ACPI / EC: handle ECDT EC and DSDT EC simultaneously
> 
> The Asus laptops X550VXK/FX502VD/FX502VE have a BIOS bug where the
> ECDT (correctly) states that EC events trigger GPE 0x23, but the
> DSDT _GPE method (incorrectly) says GPE 0x33.
> 
> A cursory glance of the code suggests that this should work regardless
> (because it looks like Linux would treat the ECDT and DSDT as two separate
> ECs supported simultaneously), but in practice it is not working since the
> ECDT is ultimately ignored in favour of the DSDT EC.

IMO, your observation is wrong.

Linux never thought there should be 2 ACPI ECs provided by BIOS :). How
would it be useful to put 2 ECs in your platforms and export both of them
as ACPI EmbeddedControl operation region? EC operation region itself by
design is not able to allow OS to detect which EC opregion belongs to which
EC device as we did see some platforms defined EC opregion under root but
EC device under some bus nodes.

ACPI spec thinks ECDT EC is meant to be provided to work since early stages
where the ACPI namespace hasn't been prepared.

See spec 6.5.4 _REG (Region).
You can find exceptions of _REG execution mentioned for
SystemMemory/SystemIo/PCI_Config and ECDT EC. So they are served for the
same purpose:
 Before executing any control method (aka., preparing the ACPI namespace,
 some hardware components that have already been driven by the BIOS should
 be allowed to be accessed during the namespace preparation.

And hence all DSDT PNP0C09 ECs (actually should only be 1) should always
work, in most of the cases, ECDT EC may just be used as a quirk to make
some EC opregion accesses working during that special stage - the
namespace preparation. For such kind of use case (early stage, loading
ACPI namespace), obviously no EC event should be handled, so making ECDT
GPE setting correct sounds meaningless to BIOS if the same EC settings can
also be provided via DSDT.

Thus Linux is always trying to override ECDT EC with DSDT settings and
convert the final boot EC to the PNP0C09 driver driven ones. Some code is
there not deleted just due to some historical reasons.

As such, GPE setting in DSDT should always be correct.

There are 2 possibilities for some gray areas:
1. If a BIOS has a wrong EC GPE setting in DSDT, but the OS correctly
   implemented IRQ polling to handle it, you still couldn't see any
   problems (you cannot know whether IRQ is handled via interrupt or via
   polling from user space).
2. If ECDT EC contains a namespace path differing from the DSDT one, the
   OS may choose to keep both. Or if ECDT EC contains IO addresses
   differing from the DSDT one, the OS may choose to keep both.
I don't have knowledge to know how Windows implement things in the above
gray areas. They just sound like some happen-to-work consequences of the
unspecified Windows implementation.

For gray area 1, your report just means to me that some platforms do have
correct ECDT GPE setting but incorrect DSDT GPE setting; and since Linux
doesn't implement EC event polling for some stages, for now Linux may need
a quirk to favor "correct GPE setting". 

However for gray area 2, I don't think it means Linux should keep both ECs.
I cannot see reasons in this case to support to do so unless seeing your
acpidump. Are you able to see 2 ECs in your Windows device tree on these
platforms? So could you paste your acpidump here so that we can check if
they are different ACPI ECs and make a decision closer to the Windows
behavior and tell me your Windows device tree result (you can obtain this
using a tool provided in <<Microsoft Windows Internals>>).

Maybe for your case, DSDT EC is just invalid and we should ignore it, then
if we can find a correct way to ignore the DSDT EC, we needn't change a
single line EC driver code.

> The sequence of
> events is:
> 
> 1. acpi_ec_ecdt_probe() is called in early boot, and calls
>    acpi_config_boot_ec(is_ecdt=true) for the ECDT EC.
> 
> acpi_config_boot_ec() sets boot_ec to this new EC, and
> boot_ec_is_ecdt = true.
> 
> 2. Later, acpi_ec_dsdt_probe() is called and creates a new ec to represent
>    the DSDT EC. Then:
>     /*
>      * When the DSDT EC is available, always re-configure boot EC to
>      * have _REG evaluated. _REG can only be evaluated after the
>      * namespace initialization.
>      * At this point, the GPE is not fully initialized, so do not to
>      * handle the events.
>      */
>     ret = acpi_config_boot_ec(ec, ec->handle, false, false);
> 
> So the DSDT EC is passed to acpi_config_boot_ec() which does:
>     /* Unset old boot EC */
>     if (boot_ec != ec)
>         acpi_ec_free(boot_ec);
> 
> acpi_ec_free() sets boot_ec to NULL.
> Further down in acpi_config_boot_ec() we reach:
> 
>     /* Set new boot EC */
>     if (!boot_ec) {
>         boot_ec = ec;
>         boot_ec_is_ecdt = is_ecdt;
>     }
> 
> So now boot_ec points to the DSDT EC and boot_ec_is_ecdt is false.

Correct, now boot EC is overridden by the DSDT settings.
And the ECDT EC is in fact abandoned.

You seem to just need a possibility here to allow OS to abandon the
wrong settings and keep the correct settings.
However it's hard for OS to determine which settings are correct:
ECDT one or DSDT one?
No one can answer this, even ACPI spec cannot.

So you might need a quirk mechanism here before root causing. As this
looks more like a BIOS issue - BIOS developer might have different
understanding than the spec and their code happened to work on Windows
due to different reasons.

> 
> 3. Later, ecpi_ec_ecdt_start() is called and this looks like it would
>    enable GPEs on our ECDT, but it bails out because of:
> 
>     if (!boot_ec_is_ecdt)
>         return -ENODEV;
> 
> 
> The comment I pasted above from acpi_ec_dsdt_probe() says that it is going
> to reconfigure the boot EC, but it instead calls acpi_config_boot_ec() on
> the newly probed DSDT EC. It seems like the code is not following the
> comment here.

No, all above means:
If the boot EC is now overridden by DSDT EC, no need to enable GPE at this
stage. As further PNP0C09 driver probe will do this.

You problem is just PNP0C09 GPE setting is wrong and the code here dropped
possibly correct setting in ECDT.

IMO,
1. We can drop if PNP0C09 GPE driver implements GPE polling for all use
   cases. But now it doesn't implement that.
2. If we cannot drop wrong GPE setting, we may consider a mechanism here
   to allow platform quirks to choose the correct one for Linux.

> 
> Adjusting that code to work with boot_ec adjusts the sequence of events so
> that both boot EC and DSDT are treated independently and as a result, we
> get EC interrupts firing correctly.
> 
> This fixes media keys on the mentioned laptop models.
> Thanks to Chris Chiu for diagnosis.
> 
> Signed-off-by: Daniel Drake <drake@endlessm.com>
> ---
>  drivers/acpi/ec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index c24235d8fb52..78395395e3d9 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -1691,7 +1691,7 @@ int __init acpi_ec_dsdt_probe(void)
>  	 * At this point, the GPE is not fully initialized, so do not to
>  	 * handle the events.
>  	 */
> -	ret = acpi_config_boot_ec(ec, ec->handle, false, false);
> +	ret = acpi_config_boot_ec(boot_ec, boot_ec->handle, false, false);

This is a hackish, "ec" created above these lines will never be actually
used then.

The entire problem looks to me is:
When GPE setting differs between ECDT and DSDT, which one should be
trusted by OS?

The current code chose to always trust DSDT GPE settings as in theory it
doesn't make sense to trust the ECDT GPE setting in most of the cases,
ECDT GPE is not meant to be used during runtime. So why don't we just add
a quirk to favor GPE setting from ECDT rather than the GPE setting from
DSDT for these platforms?

Thanks
Lv

>  error:
>  	if (ret)
>  		acpi_ec_free(ec);
> --
> 2.11.0

Re: [PATCH] ACPI / EC: handle ECDT EC and DSDT EC simultaneously

[Daniel Drake]

Hi Lv,

Thanks for the detailed response. In trying to decode the tricky code
flow and seeing all this first_ec / boot_ec / DSDT EC / ECDT EC stuff,
I seem to have made the wrong interpretation about how this is
designed.

On Sun, Apr 23, 2017 at 10:43 PM, Zheng, Lv <lv.zheng@intel.com> wrote:
> The entire problem looks to me is:
> When GPE setting differs between ECDT and DSDT, which one should be
> trusted by OS?

This case suggests that Windows uses the ECDT setting, right?

> The current code chose to always trust DSDT GPE settings as in theory it
> doesn't make sense to trust the ECDT GPE setting in most of the cases,
> ECDT GPE is not meant to be used during runtime. So why don't we just add
> a quirk to favor GPE setting from ECDT rather than the GPE setting from
> DSDT for these platforms?

Do you mean a DMI quirk? We have found those to be quite impractical
in the past, it is too hard to get 100% coverage of all machines
affected by the bug, and usually we would only be able to quirk it
late in the process (after the machine has already shipped to users).
In a recent case we had an Asus DMI quirks list that grew slowly to
over 30 entries over a year, before we found a generic solution.

In this case we have asked Asus BIOS engineers to not repeat this
issue on future products, but even if they follow our advice there, we
can expect a decent number of models affected by this issue.

And we just found a 2nd model with the same issue. Here is the
"acpidump -b" output:
https://www.dropbox.com/s/d3w2xrmrz1oklnw/x580vd_acpi.tgz?dl=0

To see the windows device tree using Microsoft Windows Internals, are
you referring to the paperback book? Which of the 2 parts would we
have to purchase? Does it come with a binary on CD or would we have to
figure out how to compile the tool?

Thanks,
Daniel

RE: [PATCH] ACPI / EC: handle ECDT EC and DSDT EC simultaneously

[Zheng, Lv]

Hi,

> From: Daniel Drake [mailto:drake@endlessm.com]
> Subject: Re: [PATCH] ACPI / EC: handle ECDT EC and DSDT EC simultaneously
> 
> Hi Lv,
> 
> Thanks for the detailed response. In trying to decode the tricky code
> flow and seeing all this first_ec / boot_ec / DSDT EC / ECDT EC stuff,
> I seem to have made the wrong interpretation about how this is
> designed.
> 
> On Sun, Apr 23, 2017 at 10:43 PM, Zheng, Lv <lv.zheng@intel.com> wrote:
> > The entire problem looks to me is:
> > When GPE setting differs between ECDT and DSDT, which one should be
> > trusted by OS?
> 
> This case suggests that Windows uses the ECDT setting, right?

I checked the provided acpi tables.
Indeed, the ECDT EC is correct.
[000h 0000   4]                    Signature : "ECDT"    [Embedded Controller Boot Resources Table]
[004h 0004   4]                 Table Length : 000000C1
[008h 0008   1]                     Revision : 01
[009h 0009   1]                     Checksum : EC
[00Ah 0010   6]                       Oem ID : "_ASUS_"
[010h 0016   8]                 Oem Table ID : "Notebook"
[018h 0024   4]                 Oem Revision : 01072009
[01Ch 0028   4]              Asl Compiler ID : "AMI."
[020h 0032   4]        Asl Compiler Revision : 00000005


[024h 0036  12]      Command/Status Register : [Generic Address Structure]
[024h 0036   1]                     Space ID : 01 [SystemIO]
[025h 0037   1]                    Bit Width : 08
[026h 0038   1]                   Bit Offset : 00
[027h 0039   1]         Encoded Access Width : 00 [Undefined/Legacy]
[028h 0040   8]                      Address : 0000000000000066

[030h 0048  12]                Data Register : [Generic Address Structure]
[030h 0048   1]                     Space ID : 01 [SystemIO]
[031h 0049   1]                    Bit Width : 08
[032h 0050   1]                   Bit Offset : 00
[033h 0051   1]         Encoded Access Width : 00 [Undefined/Legacy]
[034h 0052   8]                      Address : 0000000000000062

[03Ch 0060   4]                          UID : 00000000
[040h 0064   1]                   GPE Number : 23
[041h 0065  19]                     Namepath : "\_SB.PCI0.LPCB.EC0"

While the DSDT EC is in fact invalid as it returns 0 from its _STA:
    Scope (_SB.PCI0.LPCB)
    {
        Device (H_EC)
        {
            Name (_HID, EisaId ("PNP0C09") /* Embedded Controller Device */)  // _HID: Hardware ID
            Name (_UID, One)  // _UID: Unique ID
            Method (_STA, 0, NotSerialized)  // _STA: Status
            {
                ^^^GFX0.CLID = 0x03
                Return (Zero)
            }
            ...
        }
    }

It doesn't contain _CRS, so it's likely that it will fail in
acpi_ec_dsdt_probe() due to failure of walk _CRS.

Are you sure the same problem can be seen no this platform?

If so, possibly you only need to add some sanity checks in
acpi_ec_dsdt_probe(). After acpi_get_devices() call, check if
its IO addresses are invalid.
Or check its _STA (present && return valid value) after that.
It might not be possible to do such _STA execution.

So IMO, the acpi_ec_dsdt_probe() is not a good choice.
The history is:
1. In ACPI spec 1.0, there is no ECDT, BIOS developers have
   to provide wrappers in EC read/write functions in order to
   be able to access EC firmware before OS loads the EC
   driver. Before EC._REG is invoked, access systemio
   opregion instead of directly accessing EC opregions.
   In ACPI spec 2.0, ECDT is provided to eliminate the
   Complication of this needs.
2. During early Linux EC support, ECDT code is provided
   for the spec purpose. But several un-root-caused kernel
   bugs made the developers to make the wrong choice, starting
   to always override ECDT EC using DSDT EC.
   The culprit commits are c6cb0e87 and a5032bfd.
   The culprit in fact finally also played as one of those
   reasons preventing the AML interpreter from being
   correctly implemented as Windows compliant.
3. However, after switching to the old behavior, and the
   DSDT boot EC mechanism is entirely abandoned. But users
   Reported that they can see EC opregion accessed in some
   _STA/_INI, as Linux executes these control methods very
   Early (earlier before probe EC driver) and there is no
   ECDT on those platforms, we can see errors complaining
   no opregion handler. The bug is kernel Bugzilla 119261.
   TBH, these errors are not such critical IMO, the BIOS
   obviously violates facts mentioned in history 1 and we
   obviously do not have knowledge of the execution order
   of these control methods on Windows and we obviously
   do not know if such error can also be seen by Winodws
   and do not know whether such error can be functional
   failure and reached BIOS developers' attention.
   But we finally have no choice but restoring the boot
   DSDT EC hack back as we do not know how Linux can
   execute all control methods in the exact order as
   Windows...

Then now, it actually makes the problem more complicated.
as it might not be possible to execute _CRS/_GPE/_STA
at that time to sanitize the DSDT EC.
Probably we could just abandon it again?

Anyway we can firstly just add 1 line IO address and _GPE
existence sanity check in acpi_ec_dsdt_probe() to fix
your issue.

> 
> > The current code chose to always trust DSDT GPE settings as in theory it
> > doesn't make sense to trust the ECDT GPE setting in most of the cases,
> > ECDT GPE is not meant to be used during runtime. So why don't we just add
> > a quirk to favor GPE setting from ECDT rather than the GPE setting from
> > DSDT for these platforms?
> 
> Do you mean a DMI quirk? We have found those to be quite impractical
> in the past, it is too hard to get 100% coverage of all machines
> affected by the bug, and usually we would only be able to quirk it
> late in the process (after the machine has already shipped to users).
> In a recent case we had an Asus DMI quirks list that grew slowly to
> over 30 entries over a year, before we found a generic solution.
> 
> In this case we have asked Asus BIOS engineers to not repeat this
> issue on future products, but even if they follow our advice there, we
> can expect a decent number of models affected by this issue.
> 
> And we just found a 2nd model with the same issue. Here is the
> "acpidump -b" output:
> https://www.dropbox.com/s/d3w2xrmrz1oklnw/x580vd_acpi.tgz?dl=0
> 
> To see the windows device tree using Microsoft Windows Internals, are
> you referring to the paperback book? Which of the 2 parts would we
> have to purchase? Does it come with a binary on CD or would we have to
> figure out how to compile the tool?

Sorry I confused it with sysinternal tools
https://technet.microsoft.com/en-us/sysinternals/bb842062.aspx

However it should be OSR online DDK developer support utility:
http://www.osronline.com/article.cfm?id=97

Thanks
Lv

Re: [PATCH] ACPI / EC: handle ECDT EC and DSDT EC simultaneously

[Rafael J. Wysocki]

On Thursday, April 27, 2017 03:18:04 AM Zheng, Lv wrote:
> Hi,
> 
> > From: Daniel Drake [mailto:drake@endlessm.com]
> > Subject: Re: [PATCH] ACPI / EC: handle ECDT EC and DSDT EC simultaneously
> > 
> > Hi Lv,
> > 
> > Thanks for the detailed response. In trying to decode the tricky code
> > flow and seeing all this first_ec / boot_ec / DSDT EC / ECDT EC stuff,
> > I seem to have made the wrong interpretation about how this is
> > designed.
> > 
> > On Sun, Apr 23, 2017 at 10:43 PM, Zheng, Lv <lv.zheng@intel.com> wrote:
> > > The entire problem looks to me is:
> > > When GPE setting differs between ECDT and DSDT, which one should be
> > > trusted by OS?
> > 
> > This case suggests that Windows uses the ECDT setting, right?
> 
> I checked the provided acpi tables.
> Indeed, the ECDT EC is correct.
> [000h 0000   4]                    Signature : "ECDT"    [Embedded Controller Boot Resources Table]
> [004h 0004   4]                 Table Length : 000000C1
> [008h 0008   1]                     Revision : 01
> [009h 0009   1]                     Checksum : EC
> [00Ah 0010   6]                       Oem ID : "_ASUS_"
> [010h 0016   8]                 Oem Table ID : "Notebook"
> [018h 0024   4]                 Oem Revision : 01072009
> [01Ch 0028   4]              Asl Compiler ID : "AMI."
> [020h 0032   4]        Asl Compiler Revision : 00000005
> 
> 
> [024h 0036  12]      Command/Status Register : [Generic Address Structure]
> [024h 0036   1]                     Space ID : 01 [SystemIO]
> [025h 0037   1]                    Bit Width : 08
> [026h 0038   1]                   Bit Offset : 00
> [027h 0039   1]         Encoded Access Width : 00 [Undefined/Legacy]
> [028h 0040   8]                      Address : 0000000000000066
> 
> [030h 0048  12]                Data Register : [Generic Address Structure]
> [030h 0048   1]                     Space ID : 01 [SystemIO]
> [031h 0049   1]                    Bit Width : 08
> [032h 0050   1]                   Bit Offset : 00
> [033h 0051   1]         Encoded Access Width : 00 [Undefined/Legacy]
> [034h 0052   8]                      Address : 0000000000000062
> 
> [03Ch 0060   4]                          UID : 00000000
> [040h 0064   1]                   GPE Number : 23
> [041h 0065  19]                     Namepath : "\_SB.PCI0.LPCB.EC0"
> 
> While the DSDT EC is in fact invalid as it returns 0 from its _STA:
>     Scope (_SB.PCI0.LPCB)
>     {
>         Device (H_EC)
>         {
>             Name (_HID, EisaId ("PNP0C09") /* Embedded Controller Device */)  // _HID: Hardware ID
>             Name (_UID, One)  // _UID: Unique ID
>             Method (_STA, 0, NotSerialized)  // _STA: Status
>             {
>                 ^^^GFX0.CLID = 0x03
>                 Return (Zero)
>             }
>             ...
>         }
>     }
> 
> It doesn't contain _CRS, so it's likely that it will fail in
> acpi_ec_dsdt_probe() due to failure of walk _CRS.
> 
> Are you sure the same problem can be seen no this platform?
> 
> If so, possibly you only need to add some sanity checks in
> acpi_ec_dsdt_probe().

Can you suggest a patch, please?

Ideally, something that can be tested?

Thanks,
Rafael

Re: [PATCH] ACPI / EC: handle ECDT EC and DSDT EC simultaneously

[Daniel Drake]

On Thu, Apr 27, 2017 at 6:33 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> Can you suggest a patch, please?
>
> Ideally, something that can be tested?

I think I understand Lv's suggestions so we will now test the
following change in order to check if either or both of the added DSDT
EC checks can help us.

https://gist.github.com/dsd/f50a63c9f31779436bd280c76253e37c

Daniel

RE: [PATCH] ACPI / EC: handle ECDT EC and DSDT EC simultaneously

[Zheng, Lv]

Hi, Daniel

> From: Daniel Drake [mailto:drake@endlessm.com]
> Subject: Re: [PATCH] ACPI / EC: handle ECDT EC and DSDT EC simultaneously
> 
> On Thu, Apr 27, 2017 at 6:33 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > Can you suggest a patch, please?
> >
> > Ideally, something that can be tested?
> 
> I think I understand Lv's suggestions so we will now test the
> following change in order to check if either or both of the added DSDT
> EC checks can help us.
> 
> https://gist.github.com/dsd/f50a63c9f31779436bd280c76253e37c

We did have a good talk, and left good engineering materials in community around this issue.

However in the above debugging commit, I'm sure we shouldn't invoke _STA in ec_parse_device().
As the reasons below.

In theory, using DSDT EC as boot EC is not spec compliant.
It's just a workaround in Linux for not knowing the Windows device enumeration orders.
Especially, the order of executing the control method execution that may contain hardware initialization code.
Such control methods are mostly _STA/_INI.
While for _HID/_CRS/_GPE/_BBN, it is unlikely to trigger order issues and it might be safe to invoke them such early.

If you executes _STA here, you might bring EC._STA execution prior than other _INI/_STA and may break some other platforms.
So for now, I think you should only add simple sanity checks for ioports.
And since you have the direct accesses to these affected platforms, you can help to provide such working sanity check improvements for us.

Thanks and best regards
Lv

Re: [PATCH] ACPI / EC: handle ECDT EC and DSDT EC simultaneously

[Daniel Drake]

Hi,

On Fri, Apr 28, 2017 at 12:33 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
> However in the above debugging commit, I'm sure we shouldn't invoke _STA in ec_parse_device().
> As the reasons below.
>
> In theory, using DSDT EC as boot EC is not spec compliant.
> It's just a workaround in Linux for not knowing the Windows device enumeration orders.
> Especially, the order of executing the control method execution that may contain hardware initialization code.
> Such control methods are mostly _STA/_INI.
> While for _HID/_CRS/_GPE/_BBN, it is unlikely to trigger order issues and it might be safe to invoke them such early.
>
> If you executes _STA here, you might bring EC._STA execution prior than other _INI/_STA and may break some other platforms.
> So for now, I think you should only add simple sanity checks for ioports.
> And since you have the direct accesses to these affected platforms, you can help to provide such working sanity check improvements for us.

In the DSDT you were looking at the H_EC device, but for whatever
reason, there are two ECs in this DSDT and the one that Linux picks up
is the 2nd one, \_SB_.PCI0.LPCB.EC0.

This device has no _STA but does have valid _CRS, and the debug patch
results agree:

 ACPI : EC: acpi_ec_dsdt_probe: search for DSDT EC
 ACPI : EC: ec_parse_device: _STA status 5 val 0
 ACPI : EC: ec_parse_device: _CRS status 0 command 66 data 62
 ACPI : EC: EC stopped
 ACPI : EC: EC started
 ACPI: \_SB_.PCI0.LPCB.EC0_: Used as first EC

acpidump output is at
https://www.dropbox.com/s/d3w2xrmrz1oklnw/x580vd_acpi.tgz?dl=0

Daniel

Re: [PATCH] ACPI / EC: handle ECDT EC and DSDT EC simultaneously

[Daniel Drake]

Hi Lv,

On Fri, Apr 28, 2017 at 6:52 AM, Daniel Drake <drake@endlessm.com> wrote:
> In the DSDT you were looking at the H_EC device, but for whatever
> reason, there are two ECs in this DSDT and the one that Linux picks up
> is the 2nd one, \_SB_.PCI0.LPCB.EC0.
>
> This device has no _STA but does have valid _CRS, and the debug patch
> results agree:
>
>  ACPI : EC: acpi_ec_dsdt_probe: search for DSDT EC
>  ACPI : EC: ec_parse_device: _STA status 5 val 0
>  ACPI : EC: ec_parse_device: _CRS status 0 command 66 data 62
>  ACPI : EC: EC stopped
>  ACPI : EC: EC started
>  ACPI: \_SB_.PCI0.LPCB.EC0_: Used as first EC

How do we proceed here? Any other options we can try that don't
involve DMI quirking?

Thanks
Daniel

RE: [PATCH] ACPI / EC: handle ECDT EC and DSDT EC simultaneously

[Zheng, Lv]

Hi,

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Daniel
> Drake
> Subject: Re: [PATCH] ACPI / EC: handle ECDT EC and DSDT EC simultaneously
> 
> Hi,
> 
> On Fri, Apr 28, 2017 at 12:33 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
> > However in the above debugging commit, I'm sure we shouldn't invoke _STA in ec_parse_device().
> > As the reasons below.
> >
> > In theory, using DSDT EC as boot EC is not spec compliant.
> > It's just a workaround in Linux for not knowing the Windows device enumeration orders.
> > Especially, the order of executing the control method execution that may contain hardware
> initialization code.
> > Such control methods are mostly _STA/_INI.
> > While for _HID/_CRS/_GPE/_BBN, it is unlikely to trigger order issues and it might be safe to invoke
> them such early.
> >
> > If you executes _STA here, you might bring EC._STA execution prior than other _INI/_STA and may
> break some other platforms.
> > So for now, I think you should only add simple sanity checks for ioports.
> > And since you have the direct accesses to these affected platforms, you can help to provide such
> working sanity check improvements for us.
> 
> In the DSDT you were looking at the H_EC device, but for whatever
> reason, there are two ECs in this DSDT and the one that Linux picks up
> is the 2nd one, \_SB_.PCI0.LPCB.EC0.
> 
> This device has no _STA but does have valid _CRS, and the debug patch
> results agree:
> 
>  ACPI : EC: acpi_ec_dsdt_probe: search for DSDT EC
>  ACPI : EC: ec_parse_device: _STA status 5 val 0
>  ACPI : EC: ec_parse_device: _CRS status 0 command 66 data 62
>  ACPI : EC: EC stopped
>  ACPI : EC: EC started
>  ACPI: \_SB_.PCI0.LPCB.EC0_: Used as first EC
> 
> acpidump output is at
> https://www.dropbox.com/s/d3w2xrmrz1oklnw/x580vd_acpi.tgz?dl=0

Yes, there are 2 ECs.
        Device (H_EC)
        {
            Name (_HID, EisaId ("PNP0C09") /* Embedded Controller Device */)  // _HID: Hardware ID
            Name (_UID, One)  // _UID: Unique ID
            Method (_STA, 0, NotSerialized)  // _STA: Status
            {
                ^^^GFX0.CLID = 0x03
                Return (Zero)
            }

        Device (EC0)
        {
            Name (_HID, EisaId ("PNP0C09") /* Embedded Controller Device */)  // _HID: Hardware ID
            Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
            {
                IO (Decode16,
                    0x0062,             // Range Minimum
                    0x0062,             // Range Maximum
                    0x00,               // Alignment
                    0x01,               // Length
                    )
                IO (Decode16,
                    0x0066,             // Range Minimum
                    0x0066,             // Range Maximum
                    0x00,               // Alignment
                    0x01,               // Length
                    )
            })
            Method (_GPE, 0, NotSerialized)  // _GPE: General Purpose Events
            {
                Local0 = 0x33
                Return (Local0)
            }

And linux EC driver only tries the 1st one.
You probably should add sanity check to skip first one, and return the 2nd one.

Thanks
Lv

> 
> Daniel
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] ACPI / EC: handle ECDT EC and DSDT EC simultaneously

[Zheng, Lv]

Hi, Daniel

> From: Daniel Drake [mailto:drake@endlessm.com]
> Subject: Re: [PATCH] ACPI / EC: handle ECDT EC and DSDT EC simultaneously
> 
> Hi Lv,
> 
> On Fri, Apr 28, 2017 at 6:52 AM, Daniel Drake <drake@endlessm.com> wrote:
> > In the DSDT you were looking at the H_EC device, but for whatever
> > reason, there are two ECs in this DSDT and the one that Linux picks up
> > is the 2nd one, \_SB_.PCI0.LPCB.EC0.
> >
> > This device has no _STA but does have valid _CRS, and the debug patch
> > results agree:
> >
> >  ACPI : EC: acpi_ec_dsdt_probe: search for DSDT EC
> >  ACPI : EC: ec_parse_device: _STA status 5 val 0
> >  ACPI : EC: ec_parse_device: _CRS status 0 command 66 data 62
> >  ACPI : EC: EC stopped
> >  ACPI : EC: EC started
> >  ACPI: \_SB_.PCI0.LPCB.EC0_: Used as first EC
> 
> How do we proceed here? Any other options we can try that don't
> involve DMI quirking?

Quirk seems to be required:
1. We do not know the order windows invokes _INI/_STA, there are several possible solutions
   1. EC opregion handler is installed when EC device is enumerated. Before that, just complains errors.
   2. EC opregion access triggers Linux to probe EC device driver.
   We have tried 1, and as I mentioned before, someone reported a regression for the new errors.
   2 also looked scary, as EC._STA may still contain BIOS provided initialization code that cannot be invoked such earlier.
2. Even the above one doesn't matter (for example, 2 is working for all platforms), Linux still don't know the preference of the GPE settings.

Maybe I could help to present some demo code.
Let's file a kernel Bugzilla entry to work on this issue together:
https://bugzilla.kernel.org/show_bug.cgi?id=195651

Thanks and best regards
Lv

> 
> Thanks
> Daniel