Page MenuHomeFreeBSD

Add support for _CR3 critical standby (S3) threshold.
ClosedPublic

Authored by crahman_gmail.com on Jul 29 2022, 5:51 AM.
Tags
None
Referenced Files
F102701640: D35980.diff
Sat, Nov 16, 1:38 AM
Unknown Object (File)
Oct 2 2024, 9:37 AM
Unknown Object (File)
Oct 1 2024, 10:47 PM
Unknown Object (File)
Sep 29 2024, 5:59 AM
Unknown Object (File)
Sep 7 2024, 11:33 AM
Unknown Object (File)
Sep 3 2024, 4:50 AM
Unknown Object (File)
Sep 2 2024, 6:16 PM
Unknown Object (File)
Aug 16 2024, 3:23 AM

Details

Summary

Along with _PSV, _HOT, and _CRT, ACPI supports the _CR3 threshold which specifies a
temperature above which a system should transition to the S3 standby state.

On FreeBSD, this is more useful than _HOT, which specifies the S4 transition
threshold temperature (since FreeBSD does not generally support the S4 state), or, in
many cases, _CRT, since after transitioning to S3 the system can cool and then be
resumed.

Test Plan

Stop the cooling fans, run a cpu-intensive job, and/or set _CR3 to a low
temperature. The system will enter standby, cool down, and can be resumed in the usual
fashion.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

That looks quite useful, STR (S3) makes a lot more sense, esp. contrary to unsupported S4.

sys/dev/acpica/acpi_thermal.c
627

Is it right to hardcode ACPI_STATE_S3 here? Should it rather uphold user's hw.acpi.suspend_state setting? (Which defaults to S3.)

This is a good question. According to the specification, we should use S3 unless the FADT LOW_POWER_S0_IDLE_CAPABLE flag is set. If the flag is set, we can use 'an equivalent low power state'. The flag indicates that 'S0 idle achieves savings similar to or better than those typically achieved in S3'.

Rather than using hw.acpi.suspend_state, or hw.acpi.standby_state, perhaps the thing to do is to check the flag, and if it's set use the highest available sleep state; otherwise using S3?

Manual page English LGTM, but holding back until someone approves the design and implementation.

I've given this a little more thought. I don't think it's appropriate to use hw.acpi.suspend_state (or hw.acpi.standby_state) in this case.

The issue is that we're not trying to suspend to a user's preferred state. In general, the manufacturer who has enabled and picked a CR3 threshold temperature has done so after confirming it is safe to transition to that state to prevent further overheating. While there may be users who would set hw.acpi.suspend_state knowing it would affect thermal management, in most cases this will not be true. In other words, I think it would create a problem.

I do think it would be appropriate to detect the FADT LOW_POWER_S0_IDLE_CAPABLE flag and shutdown to a higher state if it's set, but I will not do it now. If anyone is interested and asks, and is willing to test the code, I'll be happy to implement it. But I don't personally have any systems that can do this and I don't want to put out untested code related to something critical to thermal management.

According to the specification, we should use S3 unless the FADT LOW_POWER_S0_IDLE_CAPABLE flag is set. [...] While there may be users who would set hw.acpi.suspend_state knowing it would affect thermal management, in most cases this will not be true. In other words, I think it would create a problem.

Fair enough, I agree that S3 specified by the spec makes sense here (neglecting the S0 idle capability flag corner case).

bcr added a subscriber: bcr.

OK from manpages.

Anything else which prevents this from hitting the tree? It's been cooking for almost three months already.

Adding some of our ACPI experts to give this a further kick.

I agree with hardcoding S3 here. We don't support S0x idle states yet.

My one concern about this is we currently don't enable suspend by default out of the box. (E.g. lid switch state defaults to no change still) There should definitely be a release note for this change.

This revision is now accepted and ready to land.Nov 17 2022, 5:51 PM
@jhb wrote:

My one concern about this is we currently don't enable suspend by default out of the box. (E.g. lid switch state defaults to no change still)

That's true, we don't (and that's a good thing I've always enjoyed), but since this is rarely hit critical condition path it should be okay POLA-wise I think.

I do very much appreciate the thoughtful reviews. Thank you.

It's not clear what further actions I might take to get the code committed, however.

Any suggestions or advice would be helpful.