Details
- Reviewers
olce
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 71148 Build 68031: arc lint + arc unit
Event Timeline
Some minor stuff to change.
I also added some notes about pre-existing problems as reminders (not necessarily to be fixed in this change).
| sys/x86/cpufreq/hwpstate_intel.c | ||
|---|---|---|
| 5 | Probably missing an additional line like: Portions of this software were developed by ShengYi Hung <aokblast@FreeBSD.org> under sponsorship from the FreeBSD Foundation. ? | |
| 138–140 | Ideally, I'd like some fine-grained error reporting like I did for hwpstate_amd(4), especially as this is a debug knob. At least a single return value would already make a big difference. | |
| 150–153 | Same as in D55604: Let's keep this debug dumper free from all cached value, and always re-read it from the MSR. (The cached value is accessible from the sysctl knob.) | |
| 326–328 | Side note: Pre-existing problem: There's nothing preventing a concurrent update of sc->req, and this operation is not atomic (note that, in hwpstate_amd(4), I've taken care of such situations by having read-updates execute on the target CPU in a critical section (actually, with interrupts disabled)). | |
| 394–395 | Error is signed, so I'd rather have it in a separate field. But I think it's even better to remove these macros completely. There is not need to pass an error, as *msr_safe() can only return 0 or EFAULT. | |
| 431 | Pre-existing problem needing the same change as in hwpstate_amd(4): Once enabled, CPPC cannot be disabled, so we must not bail out on any error after the write to MSR_IA32_PM_ENABLE succeeded. Mentioning this because, in this change, HWP_ERROR_CPPC_* are not set in an independent manner (i.e., when bailing out, bits for all steps not performed should also be set), which is fine for the time being, but will need to be changed when fixing the above-mentioned problem. | |
| 505–543 | Some pre-existing problems: As a return value, we shouldn't return EFAULT but rather ENXIO on HWP_ERROR_CPPC_ENABLE, and 0 in *all* other cases (and the driver must cope with specific MSR errors once CPPC is enabled). For the second part, please see enable_cppc() in hwpstate_amd(4), in particular, commits bd58239d3653d and 7f36d7a9505a. | |