EFI wake time

[jo_bruelltuete.com]

Summary

Expose EFI wake time API

Test Plan

Use the included efiwake tool.

Get the current wake time:

$ /usr/sbin/efiwake
Current EFI time: 2023-04-24T15:56:29
EFI wake time: 2023-04-23T22:00:00; enabled=1, pending=0

Set a valid wake time:

$ /usr/sbin/efiwake -e 2023-10-24T22:00:00
Current EFI time: 2023-04-24T16:00:32
EFI wake time: 2023-04-24T22:00:00; enabled=1, pending=0

Set an invalid wake time (31st of Apr does not exist):

$ /usr/sbin/efiwake -e 2023-04-31T22:00:00
Current EFI time: 2023-04-24T16:01:22
efiwake: cannot enable EFI wake time: Invalid argument

Disable wake time:

$ /usr/sbin/efiwake -d
Current EFI time: 2023-04-24T16:02:06
EFI wake time: 2023-04-24T16:02:06; enabled=0, pending=0

[john.grafton_runbox.com]

I was unable to test this patch. efi.h patch fails against CURRENT. D28620 abstracted the EFI public interface functions to support Xen VMs.

[imp]

The patch looks good.... for an older system. I like the concepts here, and maybe you could rebase to -current?

[jo_bruelltuete.com]

hi @imp & @john.grafton_runbox.com, thanks for looking!
this patch was against stable-13. I don't have spare hardware to test on current...
how useful do you think this patch is? it did not actually work for me... i suspect a dodgy bios in the nas box, as it wakes from s5 but not from s3.

[john.grafton_runbox.com]

Yeah, I think supporting EFI wake time could be useful to some folks. I've got a box running current I'll test with if you're able to rebase the patch.

[jo_bruelltuete.com]

rebase onto current.
(compiles but not runtime tested)

[john.grafton_runbox.com]

I tested this patch on a Lenovo T480 laptop and a generic AMD desktop running CURRENT. Both reported that they supported EFI wake time.

Unfortunately, I had the same experience you had in 13. Everything *looked* like it should work but neither system powered on or woke from S3. :(

[jo_bruelltuete.com]

Hm... no idea how that is supposed to work then...
I suspect that something something ACPI needs to happen.
For example, for my NAS box I have (sysctl):

dev.acpi_sysresource.3.wake: 0

which is the super-io / power management controller (cf D36424).
But messing around with that wake parameter does not seem to do anything.

[kib]

I suspect the lack of actual wakeup is related to PM1.RTC_EN bit not set properly. Look up it in the section 4.8.3.1.2 PM1Enable Registers of ACPI spec v. 6.5.

[jo_bruelltuete.com]

thanks for the pointer, kib!
spec looks relevant. but i have no idea how to do anything about those flags in the fbsd source code...

[kib]

In D36714#860138, @jo_bruelltuete.com wrote:

thanks for the pointer, kib!
spec looks relevant. but i have no idea how to do anything about those flags in the fbsd source code...

I suspect it is along the lines of

AcpiWriteBitRegister(ACPI_BITREG_RT_CLOCK_ENABLE, 1);

[jo_bruelltuete.com]

sys/dev/efidev/efirt.c

621

I put a AcpiWriteBitRegister(ACPI_BITREG_RT_CLOCK_ENABLE, 1) here and that made S3-wakeup work!
Thanks @kib!

Now question is: should that call to AcpiWriteBitRegister really live here? Feels out of place...

[kib]

sys/dev/efidev/efirt.c

621

Why not? Of course, it must be covered by the #ifdef DEV_ACPI guards. Also, you need to disable RT_CLOCK bit if the set_waketime got enable == false.

[jo_bruelltuete.com]

will update patch shortly.
meanwhile, got this in the console after (first) resume:

ACPI Error: No installed handler for fixed event - RealTimeClock (4), disabling (20201113/evevent-436)

Otherwise seems to be working fine.

[jo_bruelltuete.com]

Patch against current. I've checked that it compiles but cannot runtime test it.
The 13-stable version of this patch works fine on my hardware.

[kib]

sys/dev/efidev/efirt.c

581

605

If efi_call() returned error, the call to ReadBit seems to be useless.

609

Don't you want to inform caller when ReadBit failed?

635

Same note about efi_call() failing.

usr.sbin/efiwake/efiwake.c

49

No need to put tab between type and var declaration.

55

Why not use getopt(3)?

83

Look at the err(3)

141

[jo_bruelltuete.com]

addressing comments.
and found a weird bug in my bios...

[jo_bruelltuete.com]

Thanks for reviewing!
Would be good to get folks to test this out on their hardware. I'm not sure what to make of the BIOS in my mainboard... it allows setting the wake timer, e.g. 2025-10-31T20:50:59, but ignores the month component! So you get a timer date of 2025-04-31T..., a date that does not exist. Or maybe I made a mistake somewhere...

[kib]

In D36714#905228, @jo_bruelltuete.com wrote:

Thanks for reviewing!
Would be good to get folks to test this out on their hardware. I'm not sure what to make of the BIOS in my mainboard... it allows setting the wake timer, e.g. 2025-10-31T20:50:59, but ignores the month component! So you get a timer date of 2025-04-31T..., a date that does not exist. Or maybe I made a mistake somewhere...

Could you try to add printf()s in the kernel code right before SET_WAKETIME and after GET_WAKETIME efi_calls, to see what is the month value passed to/from UEFI RT methods. It might be not bios but some bug in userspace/kernel.

usr.sbin/efiwake/efiwake.c

69

I suggest to add exit(EX_USAGE) to usage() and remove that return's.

78

check that only one of enable/disable flags is set? Also why not use a bool type for them?

[jo_bruelltuete.com]

In D36714#905278, @kib wrote:

In D36714#905228, @jo_bruelltuete.com wrote:

Thanks for reviewing!
Would be good to get folks to test this out on their hardware. I'm not sure what to make of the BIOS in my mainboard... it allows setting the wake timer, e.g. 2025-10-31T20:50:59, but ignores the month component! So you get a timer date of 2025-04-31T..., a date that does not exist. Or maybe I made a mistake somewhere...

Could you try to add printf()s in the kernel code right before SET_WAKETIME and after GET_WAKETIME efi_calls, to see what is the month value passed to/from UEFI RT methods. It might be not bios but some bug in userspace/kernel.

/usr/sbin/efiwake -e 2023-10-23T22:00:00
tail /var/log/messages
Apr 24 16:56:29 fred kernel: DEBUG before efi_set_waketime: 2023-10-23T22:0:0
Apr 24 16:56:29 fred kernel: DEBUG after efi_get_waketime: 2023-4-23T22:0:0

The time makes it through to the kernel alright. Something inside EFI mangles the month.

The machine does actually wake on the mangled date-time, tried it last night.

[jo_bruelltuete.com]

review comments

[john.grafton_runbox.com]

I tested this patch against CURRENT and the update wakes my generic AMD desktop at the specified time using efiwake.

[jo_bruelltuete.com]

In D36714#905557, @john.grafton_runbox.com wrote:

I tested this patch against CURRENT and the update wakes my generic AMD desktop at the specified time using efiwake.

thanks for testing!
Could you try and give efiwake a non-April date and see if it gets set correctly. (I suspect it's only a problem on my hardware, but worth checking)

[kib]

sys/dev/efidev/efirt.c

609

You need to either translate ACPI error code to errno namespace, or just assign some error value there, in case of !ACPI_SUCCESS

638

Same.

usr.sbin/efiwake/efiwake.c

61

case should be indented on the same level as switch. Basically un-indent the switch body by one tab.

[jo_bruelltuete.com]

sys/dev/efidev/efirt.c

609

I would rather not try to translate error codes... how about fixing it to EOPNOTSUPP?

There's a bit of a can of worms: if the EFI call succeeds but ACPI fails, do we need to undo the EFI call?

usr.sbin/efiwake/efiwake.c

61

IDE is forever fighting me on every key stroke... :/

[kib]

sys/dev/efidev/efirt.c

609

As I suggested above, single error code is fine too, e.g. EIO. You might printf something which would include raw value of the ACPI error.

Also I do not think that we need to undo anything if ACPI call failed. If it is failed (i.e. the BIOS and hw are broken) so be it. We informed user about the situation.

[jo_bruelltuete.com]

review comments.
patch against current. compile-tested only.

[kib]

Please send me two git patches for this review, one for all kernel bits, and another for userspace utility addition. Please ensure that the patch metadata is correct, esp. the author' name and email.

[jo_bruelltuete.com]

@kib this came out of git-format-patch, does that look right?

{F60222593}

{F60222592}

[kib]

In D36714#906737, @jo_bruelltuete.com wrote:

@kib this came out of git-format-patch, does that look right?

{F60222593}

{F60222592}

I have no idea what is this and how to get it. Please mail patches to me.