Page MenuHomeFreeBSD
Differential D36699

acpi: Tell SMM we will handle CPPC notifications
ClosedPublic
Actions

Authored by thj on Sep 25 2022, 3:27 PM.
Tags
None
Referenced Files
F103310748: D36699.id110951.diff
Sat, Nov 23, 9:47 AM
Unknown Object (File)
Wed, Nov 20, 3:17 PM
Unknown Object (File)
Tue, Nov 19, 8:00 PM
Unknown Object (File)
Tue, Nov 19, 12:48 PM
Unknown Object (File)
Sun, Nov 17, 5:44 AM
Unknown Object (File)
Sun, Nov 17, 4:50 AM
Unknown Object (File)
Sun, Nov 17, 4:25 AM
Unknown Object (File)
Sun, Nov 17, 2:38 AM
View All Files
Subscribers
eduardo
emaste
grahamperrin
marco_lordsith.net

Details

Reviewers
imp
jhb
Group Reviewers
Klara
Commits
rG308d3d6be6da: acpi: Tell SMM we will handle CPPC notifications
rG67f2a563bfca: acpi: Tell SMM we will handle CPPC notifications
Summary

Buggy SMM implementations can hang while processing CPPC notifications.
This leads to some laptops (notably Thinkpads) hanging when the
hwpstate_intel driver is loaded.

Tell the SMM that we will handle CPPC notifications as described in:

  • Intel® Processor Vendor-Specific ACPI
  • Intel® 64 and IA-32 Architectures Software Developer’s Manual

CPPC events default to masked (disabled) so while we do not do any
handling right now this does not seem to lead to any issues.

This approach was found via this Linux Kernel patch:
https://lkml.org/lkml/2016/3/17/563

Test Plan

I have a Thinkpad x1 carbon with a 10th Generation i7 (Intel(R) Core(TM)
i7-10610U CPU @ 1.80GHz). Before this patch it would hard lock under load, on
some power events such as inserting or removing power cables or just after some
time.

Once patched I can buildworld/buildkernel and run openssl speed and `povray
--benchmark` without issue.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 47697
Build 44584: arc lint + arc unit

Event Timeline

thj requested review of this revision.Sep 25 2022, 3:27 PM
thj created this revision.
Harbormaster completed remote builds in B47525: Diff 110951.Sep 25 2022, 3:27 PM
grahamperrin added a subscriber: grahamperrin.Sep 25 2022, 3:37 PM
eduardo added a subscriber: eduardo.Sep 25 2022, 7:07 PM
marco_lordsith.net added a subscriber: marco_lordsith.net.Sep 30 2022, 6:20 PM
jhb added a comment.Sep 30 2022, 9:55 PM

Hmmm, are we supposed to be doing something in response to interrupts for this? We may want to have some sort of tunable to opt-in to this if we don't provide full support yet.

I wonder if this "fixes" an issue on my X1 where if I pull the power while suspending it gets stuck in a weird state where it never resumes.

thj updated this revision to Diff 111455.Oct 5 2022, 11:11 AM
  • Add a hint to disable the workaround
Harbormaster completed remote builds in B47697: Diff 111455.Oct 5 2022, 11:11 AM
jhb added inline comments.Oct 5 2022, 6:01 PM
sys/dev/acpica/acpi_cpu.c
406

I would maybe call the hint cppc_notify with the sense being that "1" enables it and "0" disables notifications (we try to avoid "negative" hints/tunables). You would then just want to set the default value to 1 and change the last test to cppcval != 0. I would also perhaps rename cppcval to cppc_notify as well.

thj updated this revision to Diff 111506.Oct 6 2022, 2:05 PM
  • Simplify hint name
Harbormaster completed remote builds in B47713: Diff 111506.Oct 6 2022, 2:05 PM
jhb added a comment.Oct 6 2022, 4:07 PM

Sorry, one more suggestion. It occurred to me that having this be a hint under hint.acpi.0 is probably not really the best approach. For driver-wide default settings like this, the normal practice is to use a tunable exported as a sysctl under 'hw.<driver_name>'. For ACPI things these all live under hw.acpi, e.g.:

hw.acpi.cpu.cx_lowest: C8
hw.acpi.reset_video: 0
hw.acpi.handle_reboot: 1
hw.acpi.disable_on_reboot: 0
hw.acpi.verbose: 0
hw.acpi.s4bios: 0
hw.acpi.sleep_delay: 1
hw.acpi.suspend_state: S3
hw.acpi.standby_state: NONE
hw.acpi.lid_switch_state: NONE
hw.acpi.sleep_button_state: S3
hw.acpi.power_button_state: S5
hw.acpi.supported_sleep_state: S3 S4 S5

Probably this should be a hw.cpu.cppc_notify CTLFLAG_RDTUN sysctl. That also means you will have a way to document it via the sysctl description string, and in the future you can determine if it is enabled on user's machines in case we have future bug reports due to this change.

imp added a comment.Oct 6 2022, 4:27 PM
In D36699#837740, @jhb wrote:

For driver-wide default settings like this, the normal practice is to use a tunable exported as a sysctl under 'hw.<driver_name>'. For ACPI things these all live under hw.acpi, e.g.:

I was going to make a similar suggestion. Otherwise I like this change.

emaste added a subscriber: emaste.Oct 6 2022, 4:36 PM
thj updated this revision to Diff 111554.Oct 7 2022, 2:49 PM
  • use a RDTUN sysctl rather than a hint
  • Use a global control for cppc notifications
Harbormaster completed remote builds in B47735: Diff 111554.Oct 7 2022, 2:49 PM
jhb accepted this revision.Oct 7 2022, 5:56 PM
This revision is now accepted and ready to land.Oct 7 2022, 5:56 PM
imp accepted this revision.Oct 7 2022, 11:34 PM
marco_lordsith.net added a comment.EditedOct 8 2022, 3:47 PM

I'm on stable/13 9168218160 and just had my Carbon X1 gen 7 wedge hard again after rebuilding world and kernel with 111554 patch applied and the hwpstate_intel.0.disabled hint still commented out.
I can't provide any debug info but I rebuild my system this morning and the uptime couldn't have been more than a few hours.
Before applying the 111554 patch I applied 110951 (also whilst on stable/13 9168218160) and my system had been running stable since 30 September ( https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=253288#c67 )
I always do a 'make cleanworld' before rebuilding world and a custom kernel I also was using with the 110951 patch applied.
After building world and kernel I always run beinstall.sh and then immediately reboot.
I use latest and all my packages had been updated before rebuilding world and kernel

  • pkg info -x drm-510-kmod

drm-510-kmod-5.10.113_7

cd /usr/src
sh tools/build/beinstall.sh KERNCONF=HARBINGER

/usr/src/sys/amd64/conf/HARBINGER:

include GENERIC

ident HARBINGER

makeoptions WITH_EXTRA_TCP_STACKS=1
options TCPHPTS

Should I have waited for the patch to officially land?

Closed by commit rG67f2a563bfca: acpi: Tell SMM we will handle CPPC notifications (authored by thj). · Explain WhyOct 10 2022, 1:54 PM
This revision was automatically updated to reflect the committed changes.
thj added a commit: rG67f2a563bfca: acpi: Tell SMM we will handle CPPC notifications.
thj added a commit: rG308d3d6be6da: acpi: Tell SMM we will handle CPPC notifications.Dec 8 2022, 8:04 PM

Revision Contents
Changeset List

PathSize
sys/
dev/
acpica/
11 lines

Diff 111455

View Options

sys/dev/acpica/acpi_cpu.c

Loading...