Bug 106941

Summary: LID init state - Surface 3 Lid Switch state not properly set at boot
Product: ACPI Reporter: Stephen Just (stephenjust)
Component: BIOSAssignee: Lv Zheng (lv.zheng)
Status: CLOSED CODE_FIX    
Severity: normal CC: aaron.lu, andrea, lenb, lv.zheng, matt, mika.westerberg, nathanel.titane, rui.zhang, yu.c.chen
Priority: P1    
Hardware: Intel   
OS: Linux   
Kernel Version: 4.3.0-rc6+acpi/bleeding-edge 6368074ac Subsystem:
Regression: No Bisected commit-id:
Attachments: acpidump output
dmesg with ec debug
dmesg after 191911
Quirk for lid
quirk to provide the cached lid state for broken bios
dmi quirk for broken lid state

Description Stephen Just 2015-10-31 06:28:16 UTC
Created attachment 191671 [details]
acpidump output

On the Surface 3, the state of the lid switch is set to Closed when the computer starts, irrespective of the position of the Surface 3 Type Cover. With the patch from Bug 106571, the lid switch state does correctly update after you "open" or "close" the "lid" of this device, but the initial state is still incorrect.

This seems similar to the discussion in http://thread.gmane.org/gmane.linux.acpi.devel/77428 except that I think that this thread was talking about devices with the keyboard cover detached, whereas in this case it is attached.

I will attach the acpidump and dmesg (with ec debug enabled) to this issue - hopefully I've collected this correctly.
Comment 1 Stephen Just 2015-10-31 06:28:45 UTC
Created attachment 191681 [details]
dmesg with ec debug
Comment 2 Lv Zheng 2015-11-05 00:34:13 UTC
You can try attachment 191911 [details], the patch tries to synchronize the LID state using only when the LID status change events arrive. And it will set the LID state to open as default if there is no events arrived after reboot/resume.

Thanks and best regards
-Lv
Comment 3 Stephen Just 2015-11-05 04:19:31 UTC
Unfortunately, applying attachment 191911 [details] on top of the patch that made the lid switch report changes didn't work - and in fact the lid switch state stopped changing at all.
Comment 4 Stephen Just 2015-11-05 04:20:19 UTC
Created attachment 192161 [details]
dmesg after 191911
Comment 5 Lv Zheng 2015-11-05 05:39:32 UTC
(In reply to Stephen Just from comment #3)
I checked the old bug.

        Device (LID)
        {
            Name (_HID, EisaId ("PNP0C0D"))
            Name (LIDB, Zero)
            Method (_LID, 0, NotSerialized)
            {
                Return (LIDB)
            }
        }

        Scope (GPO0)
        {
            Method (_E4C, 0, Serialized)
            {
                If ((HELD == One))
                {
                    ^^LID.LIDB = One
                }
                Else
                {
                    ^^LID.LIDB = Zero
                    Notify (LID, 0x80)
                }
                Notify (^^PCI0.SPI1.NTRG, One) // Device Check
            }
        }

Looks the default value of LIDB is "closed" unless a GPIO event arrived to update it.
Maybe you still need to ask Mika to confirm if there is a GPIO IRQ lost during boot.

> Unfortunately, applying attachment 191911 [details] on top of the patch that
> made the lid switch report changes didn't work - and in fact the lid switch
> state stopped changing at all.

From the DSDT, LID status change is only generated by the BIOS when the LID status is changed to "closed". That's why it is not updated.
So the fix is not correct for you platform.

Thanks and best regards
-Lv
Comment 6 Aaron Lu 2015-11-09 03:04:56 UTC
Add Mika.

Mika,
Please take a look at comment #5. Is it possible the _E4C could happen during boot but we somehow lost it?

BTW, this bug has nothing to do with EC, but I don't know which category to put it either.
Comment 7 Mika Westerberg 2015-11-09 08:54:58 UTC
My guess is that the GPIO event is triggered before GPIO driver is loaded and that's why we lose the initial state.
Comment 8 Mika Westerberg 2015-11-09 09:27:50 UTC
But that would require some hardware to do so.

Does Windows report the initial state correct BTW?
Comment 9 Stephen Just 2015-11-09 16:14:53 UTC
I'm not sure if it is possible to directly query that on Windows. I didn't see anything interesting in device manager to do that. But the tablet does respond correctly when the lid is closed, which to me would seem to indicate a state change.
Comment 10 Mika Westerberg 2015-11-11 10:37:15 UTC
In Linux, what fails when lid initial state is wrong? Is the UI somehow misbehaving because of that?
Comment 11 Aaron Lu 2015-11-12 02:32:04 UTC
systemd-sleep will trigger suspend once it found the LID status is closed, that seems to happen at some random time after boot.
Comment 12 Mika Westerberg 2015-11-12 13:32:47 UTC
OK, I see. However, I'm not sure how we can fix this from the kernel side. If Windows waits for the initial event and only then decides what to do, it explains why it works. At least from the ACPI ASL code I'm not able to find out any place where the initial value would be set to anything else than 0.

One option is to modify systemd-sleep so that it does not read the initial status (why would it suspend the device in the first place if it did not receive an event about lid being closed).
Comment 13 Lv Zheng 2015-11-13 00:27:11 UTC
Hi, Mika

Sorry for interrupting.

(In reply to Mika Westerberg from comment #12)
> OK, I see. However, I'm not sure how we can fix this from the kernel side.
> If Windows waits for the initial event and only then decides what to do, it
> explains why it works. At least from the ACPI ASL code I'm not able to find
> out any place where the initial value would be set to anything else than 0.

There simply won't be the initial event.
On Samsung platforms, Linux drain events using a quirk. So there is no worry that the event will be lost. We can see that there is no such a event on some Samsung platforms.

The LID event should have already been handled by the BIOS, and OSPM was just invoked via the waking vector.
We can also see,
1. There are many BIOS tables resetting LID state in _WAK because of this.
2. There are BIOS tables never initiating LID open event.

> 
> One option is to modify systemd-sleep so that it does not read the initial
> status (why would it suspend the device in the first place if it did not
> receive an event about lid being closed).

If above mentioned platforms can all work with Windows, I don't know how this can be fixed.
Just an idea that, userspace may not suspend system by reading the LID status, it probably should only wait for the "LID close" event.
If there is no such a event, but LID is closed, it might be OK for Windows to stay awake.

Thanks and best regards
-Lv
Comment 14 Aaron Lu 2015-11-13 02:25:48 UTC
(In reply to Lv Zheng from comment #13)
> Hi, Mika
> 
> Sorry for interrupting.
> 
> (In reply to Mika Westerberg from comment #12)
> > OK, I see. However, I'm not sure how we can fix this from the kernel side.
> > If Windows waits for the initial event and only then decides what to do, it
> > explains why it works. At least from the ACPI ASL code I'm not able to find
> > out any place where the initial value would be set to anything else than 0.
> 
> There simply won't be the initial event.
> On Samsung platforms, Linux drain events using a quirk. So there is no worry
> that the event will be lost. We can see that there is no such a event on
> some Samsung platforms.
> 
> The LID event should have already been handled by the BIOS, and OSPM was
> just invoked via the waking vector.
> We can also see,
> 1. There are many BIOS tables resetting LID state in _WAK because of this.
> 2. There are BIOS tables never initiating LID open event.

Hi Lv,

The problem here is about things after system boot, not system resume :-)

> 
> > 
> > One option is to modify systemd-sleep so that it does not read the initial
> > status (why would it suspend the device in the first place if it did not
> > receive an event about lid being closed).
> 
> If above mentioned platforms can all work with Windows, I don't know how
> this can be fixed.
> Just an idea that, userspace may not suspend system by reading the LID
> status, it probably should only wait for the "LID close" event.

I think this is a good idea, we probably need to raise this to systemd.
Comment 15 Aaron Lu 2015-11-13 02:35:49 UTC
I just reported this to systemd, let's see what they have to say:
https://github.com/systemd/systemd/issues/1874
Comment 16 Lv Zheng 2015-11-16 03:07:28 UTC
> 
> Hi Lv,
> 
> The problem here is about things after system boot, not system resume :-)

They look like same scenario...
It's likely that Windows simply don't check the lid status and only respond to the "LID close" event.

Thanks
-Lv
Comment 17 Stephen Just 2015-11-16 06:29:08 UTC
Lennart responded to the issue in Comment 15. Any suggestions based on his refusal to change the behavior of SystemD? I do get where he's coming from there, even if it may not exactly match windows behavior - which is a bit of a headache.

Is it possible to add a platform driver to just trigger the GPIO event that reads the lid switch state on boot or wake? It's obviously not ideal, but I doubt there's a generic way to work around this.
Comment 18 Lv Zheng 2015-11-16 07:46:58 UTC
2 ACPI LID device related facts here I should emphasized.

1. No "LID open" event on this platform:
If we search LID status change notification in the AML, we only get 1 instance here:
	Line 11619:                     Notify (LID, 0x80)
The whole source code is:
            Method (_E4C, 0, Serialized)  // _Exx: Edge-Triggered GPE
            {
                If (LEqual (HELD, One))
                {
                    Store (One, ^^LID.LIDB)
                }
                Else
                {
                    Store (Zero, ^^LID.LIDB)
                    Notify (LID, 0x80)
                }

                Notify (^^PCI0.SPI1.NTRG, One)
            }
The BIOS only generate "LID close" event for ACPI LID device and never generates "LID open" event on this platform.

2. _LID control method only returns cached value.
And for the _LID control method, it only contains cached value that is updated here in _E4C:
	Line 11551:             Name (LIDB, Zero)
	Line 11554:                 Return (LIDB)
	Line 11614:                     Store (One, ^^LID.LIDB)
	Line 11618:                     Store (Zero, ^^LID.LIDB)

The whole source code is:
            Name (LIDB, Zero)
            Method (_LID, 0, NotSerialized)  // _LID: Lid Status
            {
                Return (LIDB)
            }

If the ACPI LID device can work with Windows, does this imply that "Windows" (defacto standard) is exactly working as what I've imagind?

So it seems systemd need to implement "HandleLidSwitch=CloseOnly" for all such platforms?
It doesn't seem to be a good idea to work it around in ACPI LID device driver.

Thanks and best regards
-Lv
Comment 19 Lv Zheng 2015-11-16 07:55:24 UTC
A similar bug can be seen here:
https://bugzilla.kernel.org/show_bug.cgi?id=89211
Which talks about a resume issue.

Thanks and best regards
-Lv
Comment 20 Chen Yu 2015-11-18 08:43:08 UTC
Created attachment 194801 [details]
Quirk for lid

Can you please help to check with this quirk applied, is the status 'open' or 'close' after boot up?
thx
Comment 21 Chen Yu 2015-11-18 08:44:11 UTC
plz also provide dmesg as well.
thanks
Comment 22 Chen Yu 2015-11-23 05:52:05 UTC
Tested on Surface 3 with #Comment 20 applied, the status shows 'Open' after boot up. This patch is the same as https://bugzilla.kernel.org/show_bug.cgi?id=89211, shall we add quirk for both Surface 3 and Surface Pro 1?
Yu
Comment 23 Chen Yu 2016-01-04 15:04:28 UTC
Hi, guys,
Problem due to incorrect _LID initial value, re-classify as BIOS issue. And since the systemd is driven by input event and systemd also has a mechanism of 'timeout' framework(mentioned at bug 89211), I think a quirk solution would be acceptable at present. If there is no objection I'll send the patch at #Comment 22 for review. thanks.
Yu
Comment 24 Chen Yu 2016-02-01 12:21:52 UTC
Created attachment 202601 [details]
quirk to provide the cached lid state for broken bios

Hi Stephen,
do you have time to check if this patch work for you, I'm planning to send it out for review.
Comment 25 Chen Yu 2016-02-01 12:25:57 UTC
acpi_button=cache_lid in cmdline
Comment 26 Chen Yu 2016-02-02 08:16:18 UTC
Created attachment 202731 [details]
dmi quirk for broken lid state

plz apply this one
Comment 27 Nathanel Titane 2016-02-13 16:35:09 UTC
Confirming same or similar issue using MS Surface Pro 3 and Type covver.

Issue usually expresses itself with a sleep state request automatically setting itself at Login on Gnome GDM.

Device must be woken up using built-in Windows Synaptic trigger key or any other connected peripheral.
Comment 28 Stephen Just 2016-02-13 17:50:05 UTC
I have been unable to test the previous patch. My surface 3 is experiencing hardware problems unrelated to Linux. If someone else can confirm that the patch works, that would be appreciated.
Comment 29 Chen Yu 2016-02-14 06:11:06 UTC
(In reply to Nathanel Titane from comment #27)
> Confirming same or similar issue using MS Surface Pro 3 and Type covver.
> 
> Issue usually expresses itself with a sleep state request automatically
> setting itself at Login on Gnome GDM.
> 
> Device must be woken up using built-in Windows Synaptic trigger key or any
> other connected peripheral.

Hi Nathanel,
I have a surface pro 3 in hand but does not have the problem you described,
have you enabled the surface pro 3 button driver in kernel?
Comment 30 Chen Yu 2016-02-14 06:12:05 UTC
The lastest patch for current thread is at:
https://patchwork.kernel.org/patch/8200351/
Comment 31 Chen Yu 2016-03-07 15:12:31 UTC
Above patch is obsoleted since it is too 'hack' and hard to maintain,
actually there is a problem in previous acpi button driver that has been
introduced long ago, we need to revert that commit and leverage the systemd
developer for a root cause, I've filed a thread on systemd at:
https://github.com/systemd/systemd/issues/2807
Comment 32 Zhang Rui 2016-05-15 08:51:00 UTC
(In reply to Stephen Just from comment #0)
> Created attachment 191671 [details]
> acpidump output
> 
> On the Surface 3, the state of the lid switch is set to Closed when the
> computer starts, irrespective of the position of the Surface 3 Type Cover.
> With the patch from Bug 106571, the lid switch state does correctly update
> after you "open" or "close" the "lid" of this device, but the initial state
> is still incorrect.
> 
Question: why do you care about Lid status upon boot? Does this bring any actual problem to you when using the laptop?

I guess this also happens in windows, but you don't have a way to check the Lid status in windows...
Comment 33 Zhang Rui 2016-06-20 02:14:29 UTC
ping...
Comment 34 Lv Zheng 2016-06-20 02:36:06 UTC
We just forgot to give the feedback on the bugzilla.
Please download the 3 patches:
https://patchwork.kernel.org/patch/9147055/
https://patchwork.kernel.org/patch/9147063/
https://patchwork.kernel.org/patch/9147067/

Apply them and boot the built kernel with following boot parameter:
lid_init_state.lid_init_state=open.

Thanks in advance.
Comment 35 Lv Zheng 2016-06-20 02:39:08 UTC
Sorry, the boot parameter should be:
button.lid_init_state=open.

Thanks
-Lv
Comment 36 Lv Zheng 2016-06-20 02:59:07 UTC
For this bug, Surface 3 is a runtime idle platform.
There is a community patch can also help to bring the GPIO irq back.
I don't know the status of that patch.

Thanks
-Lv
Comment 37 Stephen Just 2016-06-20 03:03:01 UTC
Benjamin Tissoires had also posted a patch that seems to resolve the issue on Surface 3 in https://lkml.org/lkml/2016/5/26/193.

@Zhang Rui: systemd will suspend the device if the reported lid state is wrong - discussion of whether this is right or wrong has been happening in mailing lists.
Comment 38 Lv Zheng 2016-06-20 03:13:55 UTC
(In reply to Stephen Just from comment #37)
> Benjamin Tissoires had also posted a patch that seems to resolve the issue
> on Surface 3 in https://lkml.org/lkml/2016/5/26/193.

This patch still looks like a workaround.
It might be better to fix this from the freeze core. (And doing this may be dangerous IMO because of the unavailability of the drivers).
So I'm not sure if this patch is in the upstream.

> 
> @Zhang Rui: systemd will suspend the device if the reported lid state is
> wrong - discussion of whether this is right or wrong has been happening in
> mailing lists.

Our conclusion is:
We need another key event for ACPI lid close.
And the userspace is going to be tuned to behave correctly for the new event.
During the period the gap hasn't been filled, users may use the above mentioned 3 lid patches to work the issue around with button.lid_init_state=open.

However this issue is not related, it should in fact be fixed in the freeze core.
But it can still be worked around using this quirk mechanism.

-Lv
Comment 39 Lv Zheng 2016-08-23 05:12:34 UTC
As button.lid_init_state=open upstreamed.
Closing this bug for ACPI component.

Thanks
Lv