Details

Reviewers

olce
imp
mhorne

Commits

rGaba599a6cc55: zzz: Rewrite to use new power device

Summary

Previous script called acpiconf(8) (or apm(8) if ACPI wasn't supported,
although this was anyway redundant because APMIO just uses ACPI now).

Since a new generic power management interface was introduced, this isn't
sufficient, as this would only work for ACPI systems and for ACPI S3 suspend
(so no way to select suspend-to-idle). Rewrite in C to take advantage of the
new power interface.

We may want to add a switch to manually override the kern.power.suspend sysctl,
which is otherwise what the power device uses to decide which suspend type to
switch to (suspend-to-idle or firmware suspend), but this will require us to
amend the power interface.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 72964
Build 69847: arc lint + arc unit

Event Timeline

obiwac created this revision.Sun, May 10, 8:54 AM

Herald added a subscriber: imp. · View Herald TranscriptSun, May 10, 8:54 AM

obiwac requested review of this revision.Sun, May 10, 8:54 AM

Harbormaster completed remote builds in B72964: Diff 177529.Sun, May 10, 8:54 AM

also, TODO, update manpage

usr.sbin/zzz/Makefile
3	probably don't want this to be in the acpi package! but I don't know what the most appropriate thing here is.

obiwac edited the summary of this revision. (Show Details)Sun, May 10, 8:59 AM

obiwac added inline comments.Sun, May 10, 12:29 PM

usr.sbin/zzz/zzz.c
40–42	I don't know if it's worth adding fallback support to APCIIO or APMIO?

obiwac mentioned this in D55508: power: Power device and ioctl for state transitions.Mon, May 11, 9:24 PM

rebase

Harbormaster completed remote builds in B72999: Diff 177613.Mon, May 11, 9:29 PM

imp accepted this revision.Mon, May 11, 9:37 PM

This revision is now accepted and ready to land.Mon, May 11, 9:37 PM

obiwac edited the test plan for this revision. (Show Details)Tue, May 12, 9:45 AM

I think my laptop works with ACPI S3; I will try this out!

usr.sbin/zzz/zzz.c
21	I learned about this new style(9) convention recently: Global pathnames are defined in <paths.h>. Pathnames local to the program go in "pathnames.h" in the local directory. #include <paths.h> Even for such a small utility, it seems to apply here. See e.g. `usr.sbin/pciconf/pathnames.h`.
42–43	I believe this is equivalent.
56

I tried it out. The machine didn't suspend, instead it just froze :/

I haven't looked at the pathways for this new power device yet, but I will try to familiarize myself with it. That's all I can report today.

usr.sbin/zzz/Makefile
3	Right now it defaults to -utilities. I suggest keeping it there if there is no an actual dependency on ACPI but just the power interface.
usr.sbin/zzz/zzz.c
37	This is necessary here, as argv contains the name of the utility. I forget if getopt() adjusts for this.

respond to mhorne@'s suggestions

This revision now requires review to proceed.Wed, May 20, 10:22 PM

Harbormaster completed remote builds in B73299: Diff 178292.Wed, May 20, 10:22 PM

In D56918#1309166, @mhorne wrote:

I tried it out. The machine didn't suspend, instead it just froze :/

I haven't looked at the pathways for this new power device yet, but I will try to familiarize myself with it. That's all I can report today.

aie, can you confirm this works with acpiconf -s 3? Is this what you were using previously? Also, could you show me your kern.power sysctl tree?

thanks for trying this out and reviewing!

In D56918#1309427, @obiwac wrote:

In D56918#1309166, @mhorne wrote:

I tried it out. The machine didn't suspend, instead it just froze :/

I haven't looked at the pathways for this new power device yet, but I will try to familiarize myself with it. That's all I can report today.

aie, can you confirm this works with acpiconf -s 3? Is this what you were using previously? Also, could you show me your kern.power sysctl tree?

thanks for trying this out and reviewing!

I tried a few more tests today. It seems that the new zzz hangs when I have a graphical desktop loaded (sddm/kde), but it succeeds in suspend/resume when I tried with just the vt(4) console.

Strangely, acpiconf -s 3 succeeds in both cases. I believe this is what I was using before, based on the previous implementation of zzz.

Here is some of the relevant sysctl info:

hw.acpi.handle_reboot: 1
hw.acpi.disable_on_reboot: 0
hw.acpi.verbose: 0
hw.acpi.s4bios_supported: 0
hw.acpi.s4bios: 0
hw.acpi.sleep_delay: 1
hw.acpi.standby_state: NONE
hw.acpi.suspend_state: S3
hw.acpi.lid_switch_state: NONE
hw.acpi.sleep_button_state: fw_suspen
hw.acpi.power_button_state: poweroff
hw.acpi.supported_sleep_state: S3 S4 S5
kern.power.hibernate: fw_hibern
kern.power.suspend: fw_suspen
kern.power.standby: NONE
kern.power.supported_stype: awake fw_suspend suspend_to_idle fw_hibernate poweroff

One thing: I think you need to bump the length of the input character arrays in these sysctls, to accommodate for the new name lengths.

include/paths.h
56 ↗	(On Diff #178292)	It should go here, alphabetically.

obiwac marked an inline comment as done.Fri, May 22, 12:15 AM

obiwac added inline comments.

include/paths.h
56 ↗	(On Diff #178292)	yup i had called it just `_PATH_POWER` initially and forgot to move. thanks for catching that

fix path ordering

Harbormaster completed remote builds in B73328: Diff 178358.Fri, May 22, 12:18 AM

Strangely, acpiconf -s 3 succeeds in both cases. I believe this is what I was using before, based on the previous implementation of zzz.

I will look into this more tomorrow but maybe the power code is opting to do suspend-to-idle on your machine rather than FW suspend. that may explain why you're hanging

One thing: I think you need to bump the length of the input character arrays in these sysctls, to accommodate for the new name lengths.

Indeed, will commit a fix tomorrow :)

@mhorne just committed f814650aaf788323b3d485d96996fce6cd7b2d7f, does it work with that? I'm surprised it was hanging considering if the suspend stype is unknown it should just not do anything. If it's still hanging I'll send you a patch to try out to exit out of acpi_EnterSleepState() early and see what's going on there.

This change looks OK per se, but before committing we have to confirm that we can get the same behavior as before. The divergence in behavior with acpiconf -s 3 should be fixed by D57239. Could you give it a try?

usr.sbin/zzz/zzz.c
35–37	Nitpick, but nothing a priori prevents someone from launching this executable with an empty `argv[]`, and although unlikely that shouldn't be a problem.

This revision is now accepted and ready to land.Mon, May 25, 4:22 PM

LGTM. Indeed, D57239 was the key difference.

An update to the manual page would also be welcome, as its content is now essentially stale (except on the essential point that zzz serves to suspend :-)).

olce added inline comments.Tue, May 26, 1:24 PM