| Message ID | adf1b81c44806e73a391aa6bb49a9fddca95550c.1718095377.git.perry.yuan@amd.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mario Limonciello |
| Headers | show |
| Series | AMD Pstate Driver Fixes and Improvements | expand |
On 6/11/2024 03:52, Perry Yuan wrote: > If CPPC feature is supported by the CPU however the CPUID flag bit is not > set by SBIOS, the `amd_pstate` will be failed to load while system > booting. > So adding one more debug message to inform user to check the SBIOS setting, > The change also can help maintainers to debug why amd_pstate driver failed > to be loaded at system booting if the processor support CPPC. > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218686 > Signed-off-by: Perry Yuan <perry.yuan@amd.com> > --- > drivers/cpufreq/amd-pstate.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > index f166b3b94091..6b9fc24001f2 100644 > --- a/drivers/cpufreq/amd-pstate.c > +++ b/drivers/cpufreq/amd-pstate.c > @@ -1746,11 +1746,36 @@ static int __init amd_pstate_set_driver(int mode_idx) > */ > static bool amd_cppc_supported(void) > { > + struct cpuinfo_x86 *c = &cpu_data(0); > + > if ((boot_cpu_data.x86 == 0x17) && (boot_cpu_data.x86_model < 0x30)) { > pr_debug_once("CPPC feature is not supported by the processor\n"); > return false; > } > > + /* > + * If the CPPC feature is disabled in the BIOS for processors that support MSR-based CPPC, > + * the AMD Pstate driver may not function correctly. > + * Check the CPPC flag and display a warning message if the platform supports CPPC. > + * Notice: below checking code will not abort the driver registeration process. > + */ > + if (!cpu_feature_enabled(X86_FEATURE_CPPC)) { > + if (cpu_feature_enabled(X86_FEATURE_ZEN1) || cpu_feature_enabled(X86_FEATURE_ZEN2)) { > + if (c->x86_model > 0x60 && c->x86_model < 0xaf) > + goto warn; > + } else if (cpu_feature_enabled(X86_FEATURE_ZEN3) || cpu_feature_enabled(X86_FEATURE_ZEN4)) { > + if ((c->x86_model > 0x10 && c->x86_model < 0x1F) || (c->x86_model > 0x40 && c->x86_model < 0xaf)) > + goto warn; > + } else if (cpu_feature_enabled(X86_FEATURE_ZEN5)) { > + goto warn; > + } > + } > + > + return true; > + > +warn: > + pr_debug_once("The CPPC feature is supported but currently disabled by the BIOS.\n" > + "Please enable it if your BIOS has the CPPC option.\n"); This should be pr_warn_once() or pr_notice_once() IMO otherwise people won't really be able to know there is a problem. > return true; I don't really like the goto and warn label with two identical return statements. Instead how about something like this? static bool amd_cppc_supported(void) { struct cpuinfo_x86 *c = &cpu_data(0); bool warn = false; . . . if (checks) { . . warn = TRUE; } if (warn) pr_warn_once(); return TRUE; That would be a better design pattern, especially if we end up having different classes of warnings to show to users in this function in the future.
Perry Yuan <perry.yuan@amd.com> writes: > If CPPC feature is supported by the CPU however the CPUID flag bit is not > set by SBIOS, the `amd_pstate` will be failed to load while system > booting. > So adding one more debug message to inform user to check the SBIOS setting, > The change also can help maintainers to debug why amd_pstate driver failed > to be loaded at system booting if the processor support CPPC. > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218686 > Signed-off-by: Perry Yuan <perry.yuan@amd.com> Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com> -- Thanks and Regards gautham
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index f166b3b94091..6b9fc24001f2 100644 --- a/drivers/cpufreq/amd-pstate.c +++ b/drivers/cpufreq/amd-pstate.c @@ -1746,11 +1746,36 @@ static int __init amd_pstate_set_driver(int mode_idx) */ static bool amd_cppc_supported(void) { + struct cpuinfo_x86 *c = &cpu_data(0); + if ((boot_cpu_data.x86 == 0x17) && (boot_cpu_data.x86_model < 0x30)) { pr_debug_once("CPPC feature is not supported by the processor\n"); return false; } + /* + * If the CPPC feature is disabled in the BIOS for processors that support MSR-based CPPC, + * the AMD Pstate driver may not function correctly. + * Check the CPPC flag and display a warning message if the platform supports CPPC. + * Notice: below checking code will not abort the driver registeration process. + */ + if (!cpu_feature_enabled(X86_FEATURE_CPPC)) { + if (cpu_feature_enabled(X86_FEATURE_ZEN1) || cpu_feature_enabled(X86_FEATURE_ZEN2)) { + if (c->x86_model > 0x60 && c->x86_model < 0xaf) + goto warn; + } else if (cpu_feature_enabled(X86_FEATURE_ZEN3) || cpu_feature_enabled(X86_FEATURE_ZEN4)) { + if ((c->x86_model > 0x10 && c->x86_model < 0x1F) || (c->x86_model > 0x40 && c->x86_model < 0xaf)) + goto warn; + } else if (cpu_feature_enabled(X86_FEATURE_ZEN5)) { + goto warn; + } + } + + return true; + +warn: + pr_debug_once("The CPPC feature is supported but currently disabled by the BIOS.\n" + "Please enable it if your BIOS has the CPPC option.\n"); return true; }
If CPPC feature is supported by the CPU however the CPUID flag bit is not set by SBIOS, the `amd_pstate` will be failed to load while system booting. So adding one more debug message to inform user to check the SBIOS setting, The change also can help maintainers to debug why amd_pstate driver failed to be loaded at system booting if the processor support CPPC. Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218686 Signed-off-by: Perry Yuan <perry.yuan@amd.com> --- drivers/cpufreq/amd-pstate.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)