Details
- Reviewers
olce
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 71115 Build 67998: arc lint + arc unit
Event Timeline
Changes themselves look good, except for one mistake (wrong MSR).
We are missing something both to ease configuration and to minimize the disruption due to breaking backwards compatibility: A knob allowing to change the setting of all CPUs at once, to be below dev.hwpstate_amd (instead of under dev.hwpstate_amd.X where X is the CPU number). (That's for a separate revision/commit.) I have been wanting to have a similar change for CPPC, in order to retire machdep.hwpstate_pkg_ctrl for hwpstate_amd(4) (and we would obsolete it for hwpstate_intel(4) by having something similar there).
| sys/x86/cpufreq/hwpstate_amd.c | ||
|---|---|---|
| 639 | Wrong MSR here. | |
| 741–742 | This code to read the status could be factored out (there is another such call in hwpstate_goto_pstate()). | |
| 1240 | Style. | |
| 1265 | msrs seems confusing as a name. Open to other suggestions. | |
Some additional tiny changes and this is good to go.
| sys/x86/cpufreq/hwpstate_amd.c | ||
|---|---|---|
| 591 | When using MSR_OP_READ without MSR_OP_SAFE, only 0 can be returned by x86_msr_op(), so I suggest to just change the return type of the function, and also convert the result of the latter explicitly to void with a comment explaining why. | |
| 594–595 | As explained above (and please add a small comment explaining the why; may be a copy of the above inline comment). | |
| 598–603 | Same here. | |
Looks good.
My previous herald comment perhaps was unclear. I was fearing that this change alone would break powerd(8), because it only operates on dev.cpu.0.freq (first CPU only). While discussing this in person at AsiaBSDCon, though, you pointed out that currently, cpufreq(4) will just call CPUFREQ_SET() on all CPUs (from cpufreq_curr_sysctl()), so in effect powerd(8) should not be affected at this stage (but would be after D55614).