mbox series

[00/11] AMD Pstate Driver Fixes and Improvements

Message ID cover.1715065568.git.perry.yuan@amd.com
Headers show
Series AMD Pstate Driver Fixes and Improvements | expand

Message

Perry Yuan May 7, 2024, 7:15 a.m. UTC
Hello everyone,

This patchset addresses critical issues and enhances performance settings for CPUs
with heterogeneous core types in the AMD pstate driver. 
Specifically, it resolves problems related to calculating the highest performance
and frequency on the latest CPUs with preferred cores. 
Additionally, the patchset includes documentation improvements in amd-pstate.rst,
offering a comprehensive guide covering topics such as recommended reboot requirements
during driver switching, debugging procedures for driver loading failures.

Your feedback and suggestions for improvement are highly appreciated. 
Please review the patches and provide your valuable input.

Thank you.

Best regards,
Perry.

Perry Yuan (11):
  cpufreq: amd-pstate: optimiza the initial frequency values
    verification
  cpufreq: amd-pstate: show CPPC debug message if CPPC is not supported
  cpufreq: amd-pstate: add debug message while CPPC is supported and
    disabled by SBIOS
  Documentation: PM: amd-pstate: introducing recommended reboot
    requirement during driver switch
  Documentation: PM: amd-pstate: add debugging section for driver
    loading failure
  Documentation: PM: amd-pstate: add guide mode to the Operation mode
  cpufreq: amd-pstate: switch boot_cpu_has() to cpu_feature_enabled()
  x86/cpufeatures: Add feature bits for AMD heterogeneous processor
  cpufreq: amd-pstate: implement heterogeneous core topology for highest
    performance initialization
  cpufreq: amd-pstate: fix the highest frequency issue which limit
    performance
  cpufreq: amd-pstate: automatically load pstate driver by default

 Documentation/admin-guide/pm/amd-pstate.rst |  19 +-
 arch/x86/include/asm/cpufeatures.h          |   1 +
 arch/x86/include/asm/processor.h            |   2 +
 arch/x86/kernel/cpu/amd.c                   |  19 ++
 arch/x86/kernel/cpu/scattered.c             |   1 +
 drivers/cpufreq/amd-pstate.c                | 196 ++++++++++++++------
 include/linux/amd-pstate.h                  |   8 +
 7 files changed, 185 insertions(+), 61 deletions(-)

Comments

Mario Limonciello May 7, 2024, 2:41 p.m. UTC | #1
On 5/7/2024 02:15, Perry Yuan wrote:
> If the `amd-pstate` driver is not loaded automatically by default,
> it is because the kernel command line parameter has not been added.
> To resolve this issue, it is necessary to call the `amd_pstate_set_driver()`
> function to enable the desired mode (passive/active/guided) before registering
> the driver instance.
> This ensures that the driver is loaded correctly without relying on the kernel
> command line parameter.
> 
> [    0.917789] usb usb6: Manufacturer: Linux 6.9.0-rc6-amd-pstate-new-fix-v1 xhci-hcd
> [    0.982579] amd_pstate: failed to register with return -22
> 
> Reported-by: Andrei Amuraritei <andamu@posteo.net>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218705
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> ---
>   drivers/cpufreq/amd-pstate.c | 39 +++++++++++++++++++-----------------
>   1 file changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 3ff381c4edf7..f5368497ee79 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -66,7 +66,7 @@
>   static struct cpufreq_driver *current_pstate_driver;
>   static struct cpufreq_driver amd_pstate_driver;
>   static struct cpufreq_driver amd_pstate_epp_driver;
> -static int cppc_state = AMD_PSTATE_UNDEFINED;
> +static int cppc_state;
>   static bool cppc_enabled;
>   static bool amd_pstate_prefcore = true;
>   static struct quirk_entry *quirks;
> @@ -1762,6 +1762,16 @@ static int __init amd_pstate_init(void)
>   	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
>   		return -ENODEV;
>   
> +	/* Disable on the following configs by default:
> +	 * 1. Undefined platforms
> +	 * 2. Server platforms
> +	 */
> +	if (amd_pstate_acpi_pm_profile_undefined() ||
> +		amd_pstate_acpi_pm_profile_server()) {
> +		pr_info("driver load is disabled for server or undefined platform\n");
> +		return -ENODEV;
> +	}
> +
>   	/* show debug message only if CPPC is not supported */
>   	if (!amd_cppc_supported())
>   		return -EOPNOTSUPP;
> @@ -1781,28 +1791,21 @@ static int __init amd_pstate_init(void)
>   	/* check if this machine need CPPC quirks */
>   	dmi_check_system(amd_pstate_quirks_table);
>   
> +	/* get default driver mode for loading*/
> +	cppc_state = CONFIG_X86_AMD_PSTATE_DEFAULT_MODE;

Rather than setting it here within amd_pstate_init() I think you can 
just set it at line 67 to CONFIG_X86_AMD_PSTATE_DEFAULT_MODE.

> +	pr_debug("cppc working state set to mode:%d\n", cppc_state);

I think this debug line is going to be unnecessary when it's already 
hardcoded to kernel config.

> +
>   	switch (cppc_state) {
> -	case AMD_PSTATE_UNDEFINED:
> -		/* Disable on the following configs by default:
> -		 * 1. Undefined platforms
> -		 * 2. Server platforms
> -		 * 3. Shared memory designs
> -		 */
> -		if (amd_pstate_acpi_pm_profile_undefined() ||
> -		    amd_pstate_acpi_pm_profile_server() ||
> -		    !cpu_feature_enabled(X86_FEATURE_CPPC)) {
> -			pr_info("driver load is disabled, boot with specific mode to enable this\n");
> -			return -ENODEV;
> -		}
> -		ret = amd_pstate_set_driver(CONFIG_X86_AMD_PSTATE_DEFAULT_MODE);
> -		if (ret)
> -			return ret;
> -		break;
>   	case AMD_PSTATE_DISABLE:
> +		pr_info("driver load is disabled, boot with specific mode to enable this\n");
>   		return -ENODEV;
> +	case AMD_PSTATE_UNDEFINED:

With the intent of this patch I no longer see a need for 
AMD_PSTATE_UNDEFINED in the rest of the driver (or in this case 
statement even).

I feel you can drop it from amd-pstate.h.

>   	case AMD_PSTATE_PASSIVE:
>   	case AMD_PSTATE_ACTIVE:
>   	case AMD_PSTATE_GUIDED:
> +		ret = amd_pstate_set_driver(cppc_state);
> +		if (ret)
> +			return ret;
>   		break;
>   	default:
>   		return -EINVAL;
> @@ -1823,7 +1826,7 @@ static int __init amd_pstate_init(void)
>   	/* enable amd pstate feature */
>   	ret = amd_pstate_enable(true);
>   	if (ret) {
> -		pr_err("failed to enable with return %d\n", ret);
> +		pr_err("failed to enable driver mode(%d) with return %d\n", cppc_state, ret);
>   		return ret;
>   	}
>
Mario Limonciello May 7, 2024, 3:01 p.m. UTC | #2
On 5/7/2024 02:15, Perry Yuan wrote:
> To address issues with the loading of the amd-pstate driver on certain platforms,
> It needs to enable dynamic debugging to capture debug messages during the driver
> loading process. By adding "amd_pstate.dyndbg=+p cppc_acpi.dyndbg=+p  loglevel=4 debug
> amd_pstate=active" to the kernel command line, driver debug logging is enabled.

Here's an attempt at rewording this paragraph to consider.

For debugging issues reported by users with amd-pstate dynamic debugging 
output is often needed to aid in root cause.  amd-pstate does use a 
variety of CPPC functions that also provide dynamic debugging output.

Add documentation showing how to use dynamic debug for both.
> 
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> ---
>   Documentation/admin-guide/pm/amd-pstate.rst | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/Documentation/admin-guide/pm/amd-pstate.rst b/Documentation/admin-guide/pm/amd-pstate.rst
> index 2695feec1baa..230e236796f7 100644
> --- a/Documentation/admin-guide/pm/amd-pstate.rst
> +++ b/Documentation/admin-guide/pm/amd-pstate.rst
> @@ -476,6 +476,19 @@ operations for the new ``amd-pstate`` module with this tool. ::
>   Diagnostics and Tuning
>   =======================
>   
> +Debugging AMD P-State Driver Loading Issues
> +------------------------------------------
> +
> +On some platforms, there may be issues with the loading of the amd-pstate driver.
> +To capture debug messages for issue analysis, users can add below parameter,
> +"amd_pstate.dyndbg=+p cppc_acpi.dyndbg=+p  loglevel=4 debug amd_pstate=active"

Two things:

1. AFAICT you shouldn't need to set the loglevel, this should only 
affect console right?  Most users will share a kernel ring buffer or 
journal.

2. I don't think the debugging suggestion documentation should include 
explicitly setting the mode to active mode.  Active mode is the kernel 
default already.  Users or their distro may have set the mode to a 
different mode by default and this may change things unintentionally for 
them.

> +to the kernel command line. This will enable dynamic debugging and allow better
> +analysis and troubleshooting of the driver loading process.
> +
> +Please note that adding this parameter will only enable debug logging during the
> +driver loading phase and may affect system behavior. Use this option with caution
> +and only for debugging purposes.
> +
>   Trace Events
>   --------------
>
Mario Limonciello May 7, 2024, 3:03 p.m. UTC | #3
On 5/7/2024 02:15, Perry Yuan wrote:
> replace the usage of the deprecated boot_cpu_has() function with

One nit.

Capitalize the "R" in replace.

> the modern cpu_feature_enabled() function. The switch to cpu_feature_enabled()
> ensures compatibility with the latest CPU feature detection mechanisms and
> improves code maintainability.
> 
> Suggested-by: Borislav Petkov (AMD) <bp@alien8.de>
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>

Acked-by: Mario Limonciello <mario.limonciello@amd.com>

> ---
>   drivers/cpufreq/amd-pstate.c | 24 ++++++++++++------------
>   1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index e94b55a7bb59..7145248b38ec 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -124,7 +124,7 @@ static int __init dmi_matched_7k62_bios_bug(const struct dmi_system_id *dmi)
>   	 * broken BIOS lack of nominal_freq and lowest_freq capabilities
>   	 * definition in ACPI tables
>   	 */
> -	if (boot_cpu_has(X86_FEATURE_ZEN2)) {
> +	if (cpu_feature_enabled(X86_FEATURE_ZEN2)) {
>   		quirks = dmi->driver_data;
>   		pr_info("Overriding nominal and lowest frequencies for %s\n", dmi->ident);
>   		return 1;
> @@ -166,7 +166,7 @@ static s16 amd_pstate_get_epp(struct amd_cpudata *cpudata, u64 cppc_req_cached)
>   	u64 epp;
>   	int ret;
>   
> -	if (boot_cpu_has(X86_FEATURE_CPPC)) {
> +	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
>   		if (!cppc_req_cached) {
>   			epp = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ,
>   					&cppc_req_cached);
> @@ -219,7 +219,7 @@ static int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp)
>   	int ret;
>   	struct cppc_perf_ctrls perf_ctrls;
>   
> -	if (boot_cpu_has(X86_FEATURE_CPPC)) {
> +	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
>   		u64 value = READ_ONCE(cpudata->cppc_req_cached);
>   
>   		value &= ~GENMASK_ULL(31, 24);
> @@ -705,7 +705,7 @@ static int amd_pstate_get_highest_perf(int cpu, u32 *highest_perf)
>   {
>   	int ret;
>   
> -	if (boot_cpu_has(X86_FEATURE_CPPC)) {
> +	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
>   		u64 cap1;
>   
>   		ret = rdmsrl_safe_on_cpu(cpu, MSR_AMD_CPPC_CAP1, &cap1);
> @@ -941,7 +941,7 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
>   	/* It will be updated by governor */
>   	policy->cur = policy->cpuinfo.min_freq;
>   
> -	if (boot_cpu_has(X86_FEATURE_CPPC))
> +	if (cpu_feature_enabled(X86_FEATURE_CPPC))
>   		policy->fast_switch_possible = true;
>   
>   	ret = freq_qos_add_request(&policy->constraints, &cpudata->req[0],
> @@ -1174,7 +1174,7 @@ static int amd_pstate_change_mode_without_dvr_change(int mode)
>   
>   	cppc_state = mode;
>   
> -	if (boot_cpu_has(X86_FEATURE_CPPC) || cppc_state == AMD_PSTATE_ACTIVE)
> +	if (cpu_feature_enabled(X86_FEATURE_CPPC) || cppc_state == AMD_PSTATE_ACTIVE)
>   		return 0;
>   
>   	for_each_present_cpu(cpu) {
> @@ -1404,7 +1404,7 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
>   	else
>   		policy->policy = CPUFREQ_POLICY_POWERSAVE;
>   
> -	if (boot_cpu_has(X86_FEATURE_CPPC)) {
> +	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
>   		ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, &value);
>   		if (ret)
>   			return ret;
> @@ -1487,7 +1487,7 @@ static void amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
>   		epp = 0;
>   
>   	/* Set initial EPP value */
> -	if (boot_cpu_has(X86_FEATURE_CPPC)) {
> +	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
>   		value &= ~GENMASK_ULL(31, 24);
>   		value |= (u64)epp << 24;
>   	}
> @@ -1526,7 +1526,7 @@ static void amd_pstate_epp_reenable(struct amd_cpudata *cpudata)
>   	value = READ_ONCE(cpudata->cppc_req_cached);
>   	max_perf = READ_ONCE(cpudata->highest_perf);
>   
> -	if (boot_cpu_has(X86_FEATURE_CPPC)) {
> +	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
>   		wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
>   	} else {
>   		perf_ctrls.max_perf = max_perf;
> @@ -1560,7 +1560,7 @@ static void amd_pstate_epp_offline(struct cpufreq_policy *policy)
>   	value = READ_ONCE(cpudata->cppc_req_cached);
>   
>   	mutex_lock(&amd_pstate_limits_lock);
> -	if (boot_cpu_has(X86_FEATURE_CPPC)) {
> +	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
>   		cpudata->epp_policy = CPUFREQ_POLICY_UNKNOWN;
>   
>   		/* Set max perf same as min perf */
> @@ -1748,7 +1748,7 @@ static int __init amd_pstate_init(void)
>   		 */
>   		if (amd_pstate_acpi_pm_profile_undefined() ||
>   		    amd_pstate_acpi_pm_profile_server() ||
> -		    !boot_cpu_has(X86_FEATURE_CPPC)) {
> +		    !cpu_feature_enabled(X86_FEATURE_CPPC)) {
>   			pr_info("driver load is disabled, boot with specific mode to enable this\n");
>   			return -ENODEV;
>   		}
> @@ -1767,7 +1767,7 @@ static int __init amd_pstate_init(void)
>   	}
>   
>   	/* capability check */
> -	if (boot_cpu_has(X86_FEATURE_CPPC)) {
> +	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
>   		pr_debug("AMD CPPC MSR based functionality is supported\n");
>   		if (cppc_state != AMD_PSTATE_ACTIVE)
>   			current_pstate_driver->adjust_perf = amd_pstate_adjust_perf;
Oleksandr Natalenko May 8, 2024, 3:20 p.m. UTC | #4
Hello.

On úterý 7. května 2024 16:41:29, SELČ Mario Limonciello wrote:
> On 5/7/2024 02:15, Perry Yuan wrote:
> > If the `amd-pstate` driver is not loaded automatically by default,
> > it is because the kernel command line parameter has not been added.
> > To resolve this issue, it is necessary to call the `amd_pstate_set_driver()`
> > function to enable the desired mode (passive/active/guided) before registering
> > the driver instance.
> > This ensures that the driver is loaded correctly without relying on the kernel
> > command line parameter.
> > 
> > [    0.917789] usb usb6: Manufacturer: Linux 6.9.0-rc6-amd-pstate-new-fix-v1 xhci-hcd
> > [    0.982579] amd_pstate: failed to register with return -22
> > 
> > Reported-by: Andrei Amuraritei <andamu@posteo.net>
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218705
> > Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> > ---
> >   drivers/cpufreq/amd-pstate.c | 39 +++++++++++++++++++-----------------
> >   1 file changed, 21 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> > index 3ff381c4edf7..f5368497ee79 100644
> > --- a/drivers/cpufreq/amd-pstate.c
> > +++ b/drivers/cpufreq/amd-pstate.c
> > @@ -66,7 +66,7 @@
> >   static struct cpufreq_driver *current_pstate_driver;
> >   static struct cpufreq_driver amd_pstate_driver;
> >   static struct cpufreq_driver amd_pstate_epp_driver;
> > -static int cppc_state = AMD_PSTATE_UNDEFINED;
> > +static int cppc_state;
> >   static bool cppc_enabled;
> >   static bool amd_pstate_prefcore = true;
> >   static struct quirk_entry *quirks;
> > @@ -1762,6 +1762,16 @@ static int __init amd_pstate_init(void)
> >   	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> >   		return -ENODEV;
> >   
> > +	/* Disable on the following configs by default:
> > +	 * 1. Undefined platforms
> > +	 * 2. Server platforms
> > +	 */
> > +	if (amd_pstate_acpi_pm_profile_undefined() ||
> > +		amd_pstate_acpi_pm_profile_server()) {
> > +		pr_info("driver load is disabled for server or undefined platform\n");
> > +		return -ENODEV;
> > +	}
> > +
> >   	/* show debug message only if CPPC is not supported */
> >   	if (!amd_cppc_supported())
> >   		return -EOPNOTSUPP;
> > @@ -1781,28 +1791,21 @@ static int __init amd_pstate_init(void)
> >   	/* check if this machine need CPPC quirks */
> >   	dmi_check_system(amd_pstate_quirks_table);
> >   
> > +	/* get default driver mode for loading*/
> > +	cppc_state = CONFIG_X86_AMD_PSTATE_DEFAULT_MODE;
> 
> Rather than setting it here within amd_pstate_init() I think you can 
> just set it at line 67 to CONFIG_X86_AMD_PSTATE_DEFAULT_MODE.

To me it seems like setting it here kills a possibility to set the mode via the kernel cmdline. Regardless of what will be set in `amd_pstate=`, `CONFIG_X86_AMD_PSTATE_DEFAULT_MODE` will be used instead.

> 
> > +	pr_debug("cppc working state set to mode:%d\n", cppc_state);
> 
> I think this debug line is going to be unnecessary when it's already 
> hardcoded to kernel config.
> 
> > +
> >   	switch (cppc_state) {
> > -	case AMD_PSTATE_UNDEFINED:
> > -		/* Disable on the following configs by default:
> > -		 * 1. Undefined platforms
> > -		 * 2. Server platforms
> > -		 * 3. Shared memory designs
> > -		 */
> > -		if (amd_pstate_acpi_pm_profile_undefined() ||
> > -		    amd_pstate_acpi_pm_profile_server() ||
> > -		    !cpu_feature_enabled(X86_FEATURE_CPPC)) {
> > -			pr_info("driver load is disabled, boot with specific mode to enable this\n");
> > -			return -ENODEV;
> > -		}
> > -		ret = amd_pstate_set_driver(CONFIG_X86_AMD_PSTATE_DEFAULT_MODE);
> > -		if (ret)
> > -			return ret;
> > -		break;
> >   	case AMD_PSTATE_DISABLE:
> > +		pr_info("driver load is disabled, boot with specific mode to enable this\n");
> >   		return -ENODEV;
> > +	case AMD_PSTATE_UNDEFINED:
> 
> With the intent of this patch I no longer see a need for 
> AMD_PSTATE_UNDEFINED in the rest of the driver (or in this case 
> statement even).
> 
> I feel you can drop it from amd-pstate.h.
> 
> >   	case AMD_PSTATE_PASSIVE:
> >   	case AMD_PSTATE_ACTIVE:
> >   	case AMD_PSTATE_GUIDED:
> > +		ret = amd_pstate_set_driver(cppc_state);
> > +		if (ret)
> > +			return ret;
> >   		break;
> >   	default:
> >   		return -EINVAL;
> > @@ -1823,7 +1826,7 @@ static int __init amd_pstate_init(void)
> >   	/* enable amd pstate feature */
> >   	ret = amd_pstate_enable(true);
> >   	if (ret) {
> > -		pr_err("failed to enable with return %d\n", ret);
> > +		pr_err("failed to enable driver mode(%d) with return %d\n", cppc_state, ret);
> >   		return ret;
> >   	}
> >   
> 
> 
>
Perry Yuan May 8, 2024, 3:24 p.m. UTC | #5
[AMD Official Use Only - General]

> -----Original Message-----
> From: Oleksandr Natalenko <oleksandr@natalenko.name>
> Sent: Wednesday, May 8, 2024 11:21 PM
> To: Yuan, Perry <Perry.Yuan@amd.com>; rafael.j.wysocki@intel.com;
> viresh.kumar@linaro.org; Huang, Ray <Ray.Huang@amd.com>; Shenoy,
> Gautham Ranjal <gautham.shenoy@amd.com>; Petkov, Borislav
> <Borislav.Petkov@amd.com>; Limonciello, Mario
> <Mario.Limonciello@amd.com>
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Shimmer
> <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>; Meng,
> Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 11/11] cpufreq: amd-pstate: automatically load pstate
> driver by default
>
> Hello.
>
> On úterý 7. května 2024 16:41:29, SELČ Mario Limonciello wrote:
> > On 5/7/2024 02:15, Perry Yuan wrote:
> > > If the `amd-pstate` driver is not loaded automatically by default,
> > > it is because the kernel command line parameter has not been added.
> > > To resolve this issue, it is necessary to call the
> > > `amd_pstate_set_driver()` function to enable the desired mode
> > > (passive/active/guided) before registering the driver instance.
> > > This ensures that the driver is loaded correctly without relying on
> > > the kernel command line parameter.
> > >
> > > [    0.917789] usb usb6: Manufacturer: Linux 6.9.0-rc6-amd-pstate-new-
> fix-v1 xhci-hcd
> > > [    0.982579] amd_pstate: failed to register with return -22
> > >
> > > Reported-by: Andrei Amuraritei <andamu@posteo.net>
> > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218705
> > > Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> > > ---
> > >   drivers/cpufreq/amd-pstate.c | 39 +++++++++++++++++++---------------
> --
> > >   1 file changed, 21 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/cpufreq/amd-pstate.c
> > > b/drivers/cpufreq/amd-pstate.c index 3ff381c4edf7..f5368497ee79
> > > 100644
> > > --- a/drivers/cpufreq/amd-pstate.c
> > > +++ b/drivers/cpufreq/amd-pstate.c
> > > @@ -66,7 +66,7 @@
> > >   static struct cpufreq_driver *current_pstate_driver;
> > >   static struct cpufreq_driver amd_pstate_driver;
> > >   static struct cpufreq_driver amd_pstate_epp_driver; -static int
> > > cppc_state = AMD_PSTATE_UNDEFINED;
> > > +static int cppc_state;
> > >   static bool cppc_enabled;
> > >   static bool amd_pstate_prefcore = true;
> > >   static struct quirk_entry *quirks; @@ -1762,6 +1762,16 @@ static
> > > int __init amd_pstate_init(void)
> > >           if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> > >                   return -ENODEV;
> > >
> > > + /* Disable on the following configs by default:
> > > +  * 1. Undefined platforms
> > > +  * 2. Server platforms
> > > +  */
> > > + if (amd_pstate_acpi_pm_profile_undefined() ||
> > > +         amd_pstate_acpi_pm_profile_server()) {
> > > +         pr_info("driver load is disabled for server or undefined
> platform\n");
> > > +         return -ENODEV;
> > > + }
> > > +
> > >           /* show debug message only if CPPC is not supported */
> > >           if (!amd_cppc_supported())
> > >                   return -EOPNOTSUPP;
> > > @@ -1781,28 +1791,21 @@ static int __init amd_pstate_init(void)
> > >           /* check if this machine need CPPC quirks */
> > >           dmi_check_system(amd_pstate_quirks_table);
> > >
> > > + /* get default driver mode for loading*/
> > > + cppc_state = CONFIG_X86_AMD_PSTATE_DEFAULT_MODE;
> >
> > Rather than setting it here within amd_pstate_init() I think you can
> > just set it at line 67 to CONFIG_X86_AMD_PSTATE_DEFAULT_MODE.
>
> To me it seems like setting it here kills a possibility to set the mode via the
> kernel cmdline. Regardless of what will be set in `amd_pstate=`,
> `CONFIG_X86_AMD_PSTATE_DEFAULT_MODE` will be used instead.

You are right,  the kernel command line will be missed by this change,  will fix it in next version.
Thanks

Perry.

>
> >
> > > + pr_debug("cppc working state set to mode:%d\n", cppc_state);
> >
> > I think this debug line is going to be unnecessary when it's already
> > hardcoded to kernel config.
> >
> > > +
> > >           switch (cppc_state) {
> > > - case AMD_PSTATE_UNDEFINED:
> > > -         /* Disable on the following configs by default:
> > > -          * 1. Undefined platforms
> > > -          * 2. Server platforms
> > > -          * 3. Shared memory designs
> > > -          */
> > > -         if (amd_pstate_acpi_pm_profile_undefined() ||
> > > -             amd_pstate_acpi_pm_profile_server() ||
> > > -             !cpu_feature_enabled(X86_FEATURE_CPPC)) {
> > > -                 pr_info("driver load is disabled, boot with specific
> mode to enable this\n");
> > > -                 return -ENODEV;
> > > -         }
> > > -         ret =
> amd_pstate_set_driver(CONFIG_X86_AMD_PSTATE_DEFAULT_MODE);
> > > -         if (ret)
> > > -                 return ret;
> > > -         break;
> > >           case AMD_PSTATE_DISABLE:
> > > +         pr_info("driver load is disabled, boot with specific mode to
> > > +enable this\n");
> > >                   return -ENODEV;
> > > + case AMD_PSTATE_UNDEFINED:
> >
> > With the intent of this patch I no longer see a need for
> > AMD_PSTATE_UNDEFINED in the rest of the driver (or in this case
> > statement even).
> >
> > I feel you can drop it from amd-pstate.h.
> >
> > >           case AMD_PSTATE_PASSIVE:
> > >           case AMD_PSTATE_ACTIVE:
> > >           case AMD_PSTATE_GUIDED:
> > > +         ret = amd_pstate_set_driver(cppc_state);
> > > +         if (ret)
> > > +                 return ret;
> > >                   break;
> > >           default:
> > >                   return -EINVAL;
> > > @@ -1823,7 +1826,7 @@ static int __init amd_pstate_init(void)
> > >           /* enable amd pstate feature */
> > >           ret = amd_pstate_enable(true);
> > >           if (ret) {
> > > -         pr_err("failed to enable with return %d\n", ret);
> > > +         pr_err("failed to enable driver mode(%d) with return %d\n",
> > > +cppc_state, ret);
> > >                   return ret;
> > >           }
> > >
> >
> >
> >
>
>
> --
> Oleksandr Natalenko (post-factum)