StarLabsLtd / firmware

73 stars 5 forks source link

[Starbook MkVI - Intel][coreboot] Don't wake-up from suspend on lid close #139

Closed divico closed 10 months ago

divico commented 10 months ago

Closing the lid when the laptop is in suspend will wake it up.

I've observed this on Coreboot 8.99 and ITE 1.21

This sequence reproduces the issue:

  1. With the lid open, put the machine in suspend
  2. blue LED is dimming, machine is in suspend
  3. Close the lid
  4. The machine wakes up (blue LED is on)

Below some more details. Note that the machine is configured to suspend on power key press (works fine).

logind events

journalctl -fu systemd-logind
systemd-logind[1035]: Power key pressed short.
systemd-logind[1035]: The system will suspend now!
systemd-logind[1035]: Lid closed.
systemd-logind[1035]: Operation 'sleep' finished.
systemd-logind[1035]: Lid opened.

Same sequence running acpi_listen

acpi_listen
battery PNP0C0A:00 00000080 00000001
battery PNP0C0A:00 00000080 00000001
battery PNP0C0A:00 00000080 00000001
battery PNP0C0A:00 00000080 00000001
battery PNP0C0A:00 00000080 00000001
button/power PBTN 00000080 00000000
button/power LNXPWRBN:00 00000080 00000005
battery PNP0C0A:00 00000080 00000001
button/lid LID close
battery PNP0C0A:00 00000080 00000001
battery PNP0C0A:00 00000080 00000001
battery PNP0C0A:00 00000080 00000001
battery PNP0C0A:00 00000080 00000001
battery PNP0C0A:00 00000080 00000001
button/lid LID open
button/right RIGHT 00000080 00000000 K
battery PNP0C0A:00 00000080 00000001
battery PNP0C0A:00 00000080 00000001
battery PNP0C0A:00 00000080 00000001
battery PNP0C0A:00 00000080 00000001
battery PNP0C0A:00 00000080 00000001
battery PNP0C0A:00 00000080 00000001
Sean-StarLabs commented 10 months ago

It can be observed on any release we've ever done - it's how it should be. Why would you not want it to?

divico commented 10 months ago

Because if there are two consecutive actions indicating that the machine won't be used for a while, I don't expect the machine to wake up.

If I send the machine to suspend, it means I won't be using it for a while. If I close the lid, it most likely means I won't be using it for a while, unless some peripherals are connected.

It is not just for being picky about the interpretation of actions. I do have a real use case that is hindered by the current behavior.

I don't want to rely on the lid to put the machine to sleep, because I often do long calculations that I don't want to interrupt when moving the laptop from one meeting room to another. Therefore, I always explicitly put the machine into suspend by pressing the power button. To access the button, the lid must be opened. Once in suspend, I often put the laptop in a bag or have to carry it around, so I have to close the lid, which makes the machine up.

The current behavior basically prevents any other method than closing the lid to put the laptop into suspend. This also holds true if you suspend from the graphical environment (e.g. gnome menu). If the lid is closed later, the machine will wake up.

Sean-StarLabs commented 10 months ago

I'm not against switching it off, unless someone complains - but I don't get this:

I don't want to rely on the lid to put the machine to sleep, because I often do long calculations that I don't want to interrupt when moving the laptop from one meeting room to another. Therefore, I always explicitly put the machine into suspend by pressing the power button.

End result is S3 - why are you saying via the Power Button is different?

divico commented 10 months ago

Bad English, sorry about that, let me rephrase.

I often do long calculations that I don't want to interrupt when I need to carry the laptop with the lid closed for a short time (e.g. switch meeting room). Therefore, I don't want to rely on lid events for entering suspend (S3). When I need to carry the laptop around for a longer time, I close the lid and put it in a bag. If it was in suspend before, it will wake up when the lid is closed and stay on in the bag.

Could you perhapes make this configurable in the coreboot menu?

Sean-StarLabs commented 10 months ago

Wouldn't that mean you've set the OS to ignore the lid?

divico commented 10 months ago

Wouldn't that mean you've set the OS to ignore the lid?

Yes in /etc/systemd/logind.conf I have

HandleLidSwitch=ignore
HandleLidSwitchExternalPower=ignore
HandleLidSwitchDocked=ignore

I can close the lid and the machine stays on, as expected. However, once in S3, closing the lid will wake it up.

Sean-StarLabs commented 10 months ago

Cool - so it's a software problem. What distro + de?

divico commented 10 months ago

I'm on Arch with gnome 45.

Sean-StarLabs commented 10 months ago

Eesh, probably not going to be much help... but, have you checked the schema values match the ones you just posted?

divico commented 10 months ago

I understand that this is not the right place if it is a software issue. Could you please elaborate on how you came to the conclusion that is is a software issue?

Why is there no separate entry for the lid in cat /proc/acpi/wakeup?

Device  S-state   Status   Sysfs node
RP01      S3    *enabled   pci:0000:00:1c.0
XHCI      S3    *enabled   pci:0000:00:14.0
GLAN      S4    *disabled

I was thinking of disabling lid events here.

Sean-StarLabs commented 10 months ago

Correct - either Arch or GNOME forum are probably the "right" place.

I could well be wrong, but I think you've done it in the wrong place - logind.conf is for systemd but it should be GNOME that handles this i.e. change these:

gsettings list-recursively  | grep lid-close

Why is there no separate entry for the lid in cat /proc/acpi/wakeup?

It's not a device, it's just a variable that's tracked.

Could you please elaborate on how you came to the conclusion that is is a software issue?

For your use case, you said it should be ignoring the lid - which it's not.

divico commented 10 months ago

For your use case, you said it should be ignoring the lid - which it's not.

Yes it does ignore the lid when booted in OS, but it does not when in S3.

There are no options anymore in gnome 45 to handle lid switch (neither in settings nor in tweaks). I don't think that the option in previous version would handle the case in S3.

I will dig into this but I don't understand how the OS is supposed to inhibit a lid event while the machine is in S3. Isn't the wake up from S3 happening at a lower level, EC---> CPU or something like this?

Sean-StarLabs commented 10 months ago

There are no options anymore in gnome 45 to handle lid switch (neither in settings nor in tweaks). I don't think that the option in previous version would handle the case in S3.

Weird. It does ignore it on 44.3 (Ubuntu).

how the OS is supposed to inhibit a lid event while the machine is in S3.

It's not meant to inhibit it (it doesn't have that much control), it's just, in your case, not meant to do anything if the value changes. The "lid" is super basic, there is a variable in stored in ACPI - IIRC, 1 is open and 0 is closed.

divico commented 10 months ago

I can reproduce the same behavior on Ubuntu 22.04.3 LTS booted from a USB disk. Closing the lid while in S3 wakes the machine up. I used gnome tweaks to disable suspend on lid close, which worked in the OS. So, it is exactly the same behavior as on my arch install with gnome 45. You should be able to reproduce.

I will further look into how is this ACPI variable handled when in S3.

Sean-StarLabs commented 10 months ago

I did test with 22.04 (installed, not live) - just the ac-action and battery-action set to none, right?

divico commented 10 months ago

just the ac-action and battery-action set to none, right?

These settings have no effect once it is in S3.

I could reproduce the issue with

ubuntu@ubuntu:~$ gsettings list-recursively | grep lid-close
org.gnome.settings-daemon.plugins.power lid-close-ac-action 'suspend'
org.gnome.settings-daemon.plugins.power lid-close-battery-action 'suspend'
org.gnome.settings-daemon.plugins.power lid-close-suspend-with-external-monitor false

as well as with

ubuntu@ubuntu:~$ gsettings list-recursively | grep lid-close
org.gnome.settings-daemon.plugins.power lid-close-ac-action 'nothing'
org.gnome.settings-daemon.plugins.power lid-close-battery-action 'nothing'
org.gnome.settings-daemon.plugins.power lid-close-suspend-with-external-monitor false

Same for the tweak to suspend on lid close, it has no effect when in S3. Screenshot from 2023-11-22 08-19-21

Below a movie that shows the issue. This is on Ubuntu 22.04.3 LTS live.

https://github.com/StarLabsLtd/firmware/assets/11789268/1d3a7b28-6850-430c-a19b-73d2e9add8d5

Sean-StarLabs commented 10 months ago

Have you tried with sudo systemctl suspend?

divico commented 10 months ago

Have you tried with sudo systemctl suspend?

yes same issue

Sean-StarLabs commented 10 months ago

Have you got ectool? Would be interesting to see what clearing the OS flag manually did with sudo ectool -w 04 -z 00

divico commented 10 months ago

So I cleared the flag while booted in the OS, then went into S3 and closed the lid. The issue remains, it woke up when I closed the lid.

This was the output of ectool.

sudo ./ectool -w 04 -z 00

Writing ec 04 = 00
divico commented 10 months ago

One thing I don't get: how is it actually supposed to be?

Because you said

It can be observed on any release we've ever done - it's how it should be. Why would you not want it to?

but later

Weird. It does ignore it on 44.3 (Ubuntu).

In addition, I tested this on another laptop (thinkpad x230) and it behaves as I expect: when in S3, closing the lid does not wake up.

So is it about fixing a bug or inhibiting a feature?

Sean-StarLabs commented 10 months ago

how is it actually supposed to be?

How is what supposed to be? The lid will update the variable whenever its state changes and Ubuntu does ignore this change when set to ignore it.

thinkpad x230

Too different to be a relevant comparison

So is it about fixing a bug or inhibiting a feature?

Either would do work for you, the former would avoid potentially annoying some of our other users...

tsmithe commented 10 months ago

I have also noticed that, if I suspend my (AMD) StarBook while plugged in with the lid open, and then close the lid, it wakes up. I don't want it always to suspend when I close the lid (when plugged in), but I do want to be able to suspend it and then close the lid, and for it to stay suspended. I don't think this is a KDE bug... and I'm not sure why one would want the laptop to wake up after an explicit instruction to suspend... but there's no accounting for taste!

(I have the system configured only to turn off the screen when the lid is closed while plugged in, by default.)

divico commented 10 months ago

Even unbind the lid switch driver does not prevent the machine from waking up from S3.

echo PNP0C0D:00 | sudo tee /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:02/PNP0C09:00/PNP0C0D:00/driver/unbind

As expected, this removes the lid switch input

evtest /dev/input/event0
evtest: No such file or directory

Disabling wakeup event of the lid switch did not help either.

echo disabled | sudo tee /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:02/PNP0C09:00/PNP0C0D:00/power/wakeup

Before unbinding the driver the lid switch events can be tracked

evtest /dev/input/event0
Input driver version is 1.0.1
Input device ID: bus 0x19 vendor 0x0 product 0x5 version 0x0
Input device name: "Lid Switch"
Supported events:
  Event type 0 (EV_SYN)
  Event type 5 (EV_SW)
    Event code 0 (SW_LID) state 0
Properties:
Testing ... (interrupt to exit)
Event: time 1701421179.257613, type 5 (EV_SW), code 0 (SW_LID), value 1
Event: time 1701421179.257613, -------------- SYN_REPORT ------------
Event: time 1701421180.123577, type 5 (EV_SW), code 0 (SW_LID), value 0
Event: time 1701421180.123577, -------------- SYN_REPORT ------------

What else can be done? In my understanding unbinding the driver acts on the lowest software level possible...i.e. lower than gnome and systemd.

tsmithe commented 9 months ago

Hi @Sean-StarLabs, I've been experiencing this on the AMD variant (or I must be hallucinating?); is it / will it be closed in an upcoming firmware there?

Sean-StarLabs commented 9 months ago

Yup - they all use the same EC so whatever the load behaviour ends up being, it will be the same for all of them

divico commented 8 months ago

Finally I could test the fix in 9.01 and it is indeed solved. Thanks!

divico commented 7 months ago

Today I updated to the stable 24.02 coreboot firmware and the issue is back even though the release note says

Only wake from sleep when the lid is closed with an active PD connection.

I tried with and without PD connection.

Sean-StarLabs commented 7 months ago

Interesting, as coreboot doesn't have anything to do with it.

What's the EC version?

divico commented 7 months ago

Still 1.21

cat /sys/class/dmi/id/ec_firmware_release
1.21

but I did try 1.23 for some time in between, in case this is relevant

Sean-StarLabs commented 7 months ago

It might be worth checking what's changed on your distro, as the lid code in coreboot hasn't changed for quite a while: https://github.com/StarLabsLtd/coreboot/commits/starlabs-4.23/src/ec/starlabs/merlin/acpi/lid.asl

But, I'd recommend using 24.02 to match coreboot. Letting it latch should be enough to update it, just shutdown, disconnect the charger and wait for 12 seconds. Connect the charger and it might automatically go up to 24.02 (long list of requirements...)

divico commented 7 months ago

So I have now also managed to get EC 24.02 and the issue is gone. Thanks for your help.

One more question...How can I figure out whether a coreboot firmware release comes with a new ite or not? Is this for stable releases always the case? Because from the release note it is not clear to me that 24.02 was also supposed to update the ite, that is why I thought 1.21 is the lastest release

Sean-StarLabs commented 7 months ago

No easy way to tell - but they'll all have them going forward so the versions match