Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

suspend-then-hibernate: Add support for ACPI _BTP so that the bios/hw will trigger a wakeup when battery is low #23895

Merged
merged 1 commit into from Aug 23, 2022
Merged

suspend-then-hibernate: Add support for ACPI _BTP so that the bios/hw will trigger a wakeup when battery is low #23895

merged 1 commit into from Aug 23, 2022

Conversation

cerebro1
Copy link
Contributor

@cerebro1 cerebro1 commented Jul 4, 2022

No description provided.

poettering
poettering reviewed Jul 4, 2022
View reviewed changes
src/sleep/sleep.c Outdated Show resolved Hide resolved
@cerebro1
Copy link
Contributor Author

cerebro1 commented Jul 6, 2022

Sorry. Just saw the review here. I was checking only this #23640 . Thanks for review @poettering . These suggested changes will be part of #23640 so will make changes and push. This PR will be build on the success of #23640 . I should have waited for that to merge before raising this.

@cerebro1 cerebro1 marked this pull request as draft July 6, 2022 07:28
@cerebro1 cerebro1 mentioned this pull request Jul 6, 2022
Merged
@cerebro1 cerebro1 force-pushed the week6 branch 3 times, most recently from 85903d6 to 836f528 Compare July 26, 2022 08:24
@lgtm-com

This comment was marked as resolved.

Sign in to view
@bluca bluca linked an issue Jul 26, 2022 that may be closed by this pull request
Improve Suspend-Then-Hibernate functionality on Linux #21165
Closed
8 tasks
@cerebro1 cerebro1 force-pushed the week6 branch 2 times, most recently from e08b333 to 61acb7e Compare July 27, 2022 02:28
@cerebro1 cerebro1 force-pushed the week6 branch 9 times, most recently from 3ebd75d to d386b14 Compare August 10, 2022 05:48
bluca
bluca reviewed Aug 10, 2022
View reviewed changes
src/shared/sleep-config.c Outdated
return SMBIOS_WAKEUP_BIT_SET;
}
log_debug("DMI BIOS System Information does not indicate wakeup type bit is set.");
return SMBIOS_WAKEUP_BIT_UNSET;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified: we are not interest in the exact bit set, only if it was timer or not. So instead of the enum, simply make this function return 1 if it was timer, 0 if it wasn't, and negative error at usual on errors.

Copy link
Contributor Author

@cerebro1 cerebro1 Aug 11, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In systems like Ubuntu 22.04 on Lenovo Thinqpad, which have btp support so alarm file exists but the architecture does not expose smbios dmi entries. so this will need to return 0 if wakeup bit is not set. If any system exposes dmi entries and wakeup bit is set no matter what value, we need to check its set at the time of suspend.
if bit corresponds to APM, timer then hibernate. So the bit has to be returned to check on suspend as well as on wakeup.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@poettering do you know what's the story with these dmi entries not being available?

bluca
bluca reviewed Aug 10, 2022
View reviewed changes
src/shared/sleep-config.c Outdated Show resolved Hide resolved
yuwata
yuwata reviewed Aug 10, 2022
View reviewed changes
src/shared/sleep-config.c Outdated Show resolved Hide resolved
@cerebro1 cerebro1 force-pushed the week6 branch 2 times, most recently from 4347e0e to 54702b1 Compare August 13, 2022 10:19
@cerebro1 cerebro1 marked this pull request as ready for review August 13, 2022 10:19
yuwata
yuwata approved these changes Aug 20, 2022
View reviewed changes
Copy link
Member

@yuwata yuwata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Only several minor comments.

src/shared/sleep-config.c Outdated Show resolved Hide resolved
src/shared/sleep-config.c Outdated Show resolved Hide resolved
src/sleep/sleep.c Outdated Show resolved Hide resolved
src/sleep/sleep.c Outdated Show resolved Hide resolved
@bluca bluca added please-review and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Aug 20, 2022
@bluca
Copy link
Member

bluca commented Aug 20, 2022

Also, the alarm should be enabled explicitly (by writing to the sysfs attribute) if it isn't available yet.

Missed this: the kernel enables the alarm by default if it's supported. I don't think we should override it - a user might want to disable it (eg: hardware is broken but kernel doesn't blocklist it yet), and they can do so. If we always override it, they can't.

@bluca bluca added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed please-review labels Aug 22, 2022
DaanDeMeyer
DaanDeMeyer approved these changes Aug 22, 2022
View reviewed changes
Copy link
Contributor

@DaanDeMeyer DaanDeMeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a few nits, please address before merging

src/shared/sleep-config.c Outdated Show resolved Hide resolved
src/shared/sleep-config.c Show resolved Hide resolved
src/shared/sleep-config.c Outdated Show resolved Hide resolved
src/sleep/sleep.c Outdated Show resolved Hide resolved
@bluca bluca added good-to-merge/with-minor-suggestions and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels Aug 22, 2022
@bluca bluca added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed good-to-merge/with-minor-suggestions labels Aug 22, 2022
@cerebro1 cerebro1 mentioned this pull request Aug 23, 2022
Merged
@cerebro1
Copy link
Contributor Author

Please review this. I have moved the while loop as a seperate helper now.

bluca
bluca reviewed Aug 23, 2022
View reviewed changes
src/sleep/sleep.c Outdated Show resolved Hide resolved
@bluca bluca added good-to-merge/with-minor-suggestions and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels Aug 23, 2022
bluca
bluca reviewed Aug 23, 2022
View reviewed changes
src/sleep/sleep.c Outdated Show resolved Hide resolved
@bluca bluca added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed good-to-merge/with-minor-suggestions labels Aug 23, 2022
@bluca bluca merged commit 1afe3d7 into systemd:main Aug 23, 2022
@dtardon dtardon mentioned this pull request Aug 26, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bluca bluca bluca approved these changes

@DaanDeMeyer DaanDeMeyer DaanDeMeyer approved these changes

@yuwata yuwata yuwata approved these changes

@poettering poettering Awaiting requested review from poettering

Assignees
No one assigned
Labels
good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed sleep
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve Suspend-Then-Hibernate functionality on Linux
5 participants
@cerebro1 @bluca @poettering @DaanDeMeyer @yuwata