Bug 106941

Created attachment 191681 [details]
dmesg with ec debug

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

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.

Created attachment 192161 [details]
dmesg after 191911

(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

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.

My guess is that the GPIO event is triggered before GPIO driver is loaded and that's why we lose the initial state.

But that would require some hardware to do so.

Does Windows report the initial state correct BTW?

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.

In Linux, what fails when lid initial state is wrong? Is the UI somehow misbehaving because of that?

systemd-sleep will trigger suspend once it found the LID status is closed, that seems to happen at some random time after boot.

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

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

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

I just reported this to systemd, let's see what they have to say:
https://github.com/systemd/systemd/issues/1874

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

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.

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

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

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

plz also provide dmesg as well.
thanks

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

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

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.

acpi_button=cache_lid in cmdline

Created attachment 202731 [details]
dmi quirk for broken lid state

plz apply this one

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.

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.

(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?

The lastest patch for current thread is at:
https://patchwork.kernel.org/patch/8200351/

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

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

ping...

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.

Sorry, the boot parameter should be:
button.lid_init_state=open.

Thanks
-Lv

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

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.

(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

As button.lid_init_state=open upstreamed.
Closing this bug for ACPI component.

Thanks
Lv