public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] cpufreq/amd-pstate: Refactor amd_pstate_init()
@ 2024-10-17 10:05 Dhananjay Ugwekar
  2024-10-17 10:05 ` [PATCH 1/4] cpufreq/amd-pstate: Call amd_pstate_register() in amd_pstate_init() Dhananjay Ugwekar
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Dhananjay Ugwekar @ 2024-10-17 10:05 UTC (permalink / raw)
  To: gautham.shenoy, mario.limonciello, perry.yuan, rafael,
	viresh.kumar
  Cc: linux-pm, linux-kernel, Dhananjay Ugwekar

Replace code chunks with function calls, improve readability and
reduce redundancy.

Base: superm1/linux-next + https://lore.kernel.org/linux-pm/20241017053927.25285-1-Dhananjay.Ugwekar@amd.com/

Dhananjay Ugwekar (4):
  cpufreq/amd-pstate: Call amd_pstate_register() in amd_pstate_init()
  cpufreq/amd-pstate: Call amd_pstate_set_driver() in
    amd_pstate_register_driver()
  cpufreq/amd-pstate: Remove the switch case in amd_pstate_init()
  cpufreq/amd-pstate: Remove the redundant amd_pstate_set_driver() call

 drivers/cpufreq/amd-pstate.c | 79 +++++++++++++-----------------------
 1 file changed, 29 insertions(+), 50 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/4] cpufreq/amd-pstate: Call amd_pstate_register() in amd_pstate_init()
  2024-10-17 10:05 [PATCH 0/4] cpufreq/amd-pstate: Refactor amd_pstate_init() Dhananjay Ugwekar
@ 2024-10-17 10:05 ` Dhananjay Ugwekar
  2024-10-17 10:05 ` [PATCH 2/4] cpufreq/amd-pstate: Call amd_pstate_set_driver() in amd_pstate_register_driver() Dhananjay Ugwekar
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Dhananjay Ugwekar @ 2024-10-17 10:05 UTC (permalink / raw)
  To: gautham.shenoy, mario.limonciello, perry.yuan, rafael,
	viresh.kumar
  Cc: linux-pm, linux-kernel, Dhananjay Ugwekar

Replace a similar chunk of code in amd_pstate_init() with
amd_pstate_register() call.

Suggested-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index b0a1c472cc3b..856f0d11983e 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -1879,17 +1879,10 @@ static int __init amd_pstate_init(void)
 			return ret;
 	}
 
-	/* enable amd pstate feature */
-	ret = amd_pstate_enable(true);
-	if (ret) {
-		pr_err("failed to enable driver mode(%d)\n", cppc_state);
-		return ret;
-	}
-
-	ret = cpufreq_register_driver(current_pstate_driver);
+	ret = amd_pstate_register_driver(cppc_state);
 	if (ret) {
 		pr_err("failed to register with return %d\n", ret);
-		goto disable_driver;
+		return ret;
 	}
 
 	dev_root = bus_get_dev_root(&cpu_subsys);
@@ -1906,7 +1899,6 @@ static int __init amd_pstate_init(void)
 
 global_attr_free:
 	cpufreq_unregister_driver(current_pstate_driver);
-disable_driver:
 	amd_pstate_enable(false);
 	return ret;
 }
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/4] cpufreq/amd-pstate: Call amd_pstate_set_driver() in amd_pstate_register_driver()
  2024-10-17 10:05 [PATCH 0/4] cpufreq/amd-pstate: Refactor amd_pstate_init() Dhananjay Ugwekar
  2024-10-17 10:05 ` [PATCH 1/4] cpufreq/amd-pstate: Call amd_pstate_register() in amd_pstate_init() Dhananjay Ugwekar
@ 2024-10-17 10:05 ` Dhananjay Ugwekar
  2024-10-17 10:05 ` [PATCH 3/4] cpufreq/amd-pstate: Remove the switch case in amd_pstate_init() Dhananjay Ugwekar
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Dhananjay Ugwekar @ 2024-10-17 10:05 UTC (permalink / raw)
  To: gautham.shenoy, mario.limonciello, perry.yuan, rafael,
	viresh.kumar
  Cc: linux-pm, linux-kernel, Dhananjay Ugwekar

Replace a similar chunk of code in amd_pstate_register_driver() with
amd_pstate_set_driver() call.

Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 47 +++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 25 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 856f0d11983e..5155fc0019df 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -1191,16 +1191,32 @@ static void amd_pstate_driver_cleanup(void)
 	current_pstate_driver = NULL;
 }
 
+static int amd_pstate_set_driver(int mode_idx)
+{
+	if (mode_idx >= AMD_PSTATE_DISABLE && mode_idx < AMD_PSTATE_MAX) {
+		cppc_state = mode_idx;
+		if (cppc_state == AMD_PSTATE_DISABLE)
+			pr_info("driver is explicitly disabled\n");
+
+		if (cppc_state == AMD_PSTATE_ACTIVE)
+			current_pstate_driver = &amd_pstate_epp_driver;
+
+		if (cppc_state == AMD_PSTATE_PASSIVE || cppc_state == AMD_PSTATE_GUIDED)
+			current_pstate_driver = &amd_pstate_driver;
+
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
 static int amd_pstate_register_driver(int mode)
 {
 	int ret;
 
-	if (mode == AMD_PSTATE_PASSIVE || mode == AMD_PSTATE_GUIDED)
-		current_pstate_driver = &amd_pstate_driver;
-	else if (mode == AMD_PSTATE_ACTIVE)
-		current_pstate_driver = &amd_pstate_epp_driver;
-	else
-		return -EINVAL;
+	ret = amd_pstate_set_driver(mode);
+	if (ret)
+		return ret;
 
 	cppc_state = mode;
 
@@ -1726,25 +1742,6 @@ static struct cpufreq_driver amd_pstate_epp_driver = {
 	.attr		= amd_pstate_epp_attr,
 };
 
-static int __init amd_pstate_set_driver(int mode_idx)
-{
-	if (mode_idx >= AMD_PSTATE_DISABLE && mode_idx < AMD_PSTATE_MAX) {
-		cppc_state = mode_idx;
-		if (cppc_state == AMD_PSTATE_DISABLE)
-			pr_info("driver is explicitly disabled\n");
-
-		if (cppc_state == AMD_PSTATE_ACTIVE)
-			current_pstate_driver = &amd_pstate_epp_driver;
-
-		if (cppc_state == AMD_PSTATE_PASSIVE || cppc_state == AMD_PSTATE_GUIDED)
-			current_pstate_driver = &amd_pstate_driver;
-
-		return 0;
-	}
-
-	return -EINVAL;
-}
-
 /*
  * CPPC function is not supported for family ID 17H with model_ID ranging from 0x10 to 0x2F.
  * show the debug message that helps to check if the CPU has CPPC support for loading issue.
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/4] cpufreq/amd-pstate: Remove the switch case in amd_pstate_init()
  2024-10-17 10:05 [PATCH 0/4] cpufreq/amd-pstate: Refactor amd_pstate_init() Dhananjay Ugwekar
  2024-10-17 10:05 ` [PATCH 1/4] cpufreq/amd-pstate: Call amd_pstate_register() in amd_pstate_init() Dhananjay Ugwekar
  2024-10-17 10:05 ` [PATCH 2/4] cpufreq/amd-pstate: Call amd_pstate_set_driver() in amd_pstate_register_driver() Dhananjay Ugwekar
@ 2024-10-17 10:05 ` Dhananjay Ugwekar
  2024-10-17 10:05 ` [PATCH 4/4] cpufreq/amd-pstate: Remove the redundant amd_pstate_set_driver() call Dhananjay Ugwekar
  2024-10-17 18:06 ` [PATCH 0/4] cpufreq/amd-pstate: Refactor amd_pstate_init() Mario Limonciello
  4 siblings, 0 replies; 9+ messages in thread
From: Dhananjay Ugwekar @ 2024-10-17 10:05 UTC (permalink / raw)
  To: gautham.shenoy, mario.limonciello, perry.yuan, rafael,
	viresh.kumar
  Cc: linux-pm, linux-kernel, Dhananjay Ugwekar

Replace the switch case with a more readable if condition.

Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 5155fc0019df..13ee5cac901d 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -1843,21 +1843,15 @@ static int __init amd_pstate_init(void)
 		cppc_state = CONFIG_X86_AMD_PSTATE_DEFAULT_MODE;
 	}
 
-	switch (cppc_state) {
-	case AMD_PSTATE_DISABLE:
+	if (cppc_state == AMD_PSTATE_DISABLE) {
 		pr_info("driver load is disabled, boot with specific mode to enable this\n");
 		return -ENODEV;
-	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;
 	}
 
+	ret = amd_pstate_set_driver(cppc_state);
+	if (ret)
+		return ret;
+
 	/* capability check */
 	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
 		pr_debug("AMD CPPC MSR based functionality is supported\n");
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 4/4] cpufreq/amd-pstate: Remove the redundant amd_pstate_set_driver() call
  2024-10-17 10:05 [PATCH 0/4] cpufreq/amd-pstate: Refactor amd_pstate_init() Dhananjay Ugwekar
                   ` (2 preceding siblings ...)
  2024-10-17 10:05 ` [PATCH 3/4] cpufreq/amd-pstate: Remove the switch case in amd_pstate_init() Dhananjay Ugwekar
@ 2024-10-17 10:05 ` Dhananjay Ugwekar
  2024-10-26 11:58   ` Klara Modin
  2024-10-17 18:06 ` [PATCH 0/4] cpufreq/amd-pstate: Refactor amd_pstate_init() Mario Limonciello
  4 siblings, 1 reply; 9+ messages in thread
From: Dhananjay Ugwekar @ 2024-10-17 10:05 UTC (permalink / raw)
  To: gautham.shenoy, mario.limonciello, perry.yuan, rafael,
	viresh.kumar
  Cc: linux-pm, linux-kernel, Dhananjay Ugwekar

amd_pstate_set_driver() is called twice, once in amd_pstate_init() and once
as part of amd_pstate_register_driver(). Move around code and eliminate
the redundancy.

Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 13ee5cac901d..6f6d961879cc 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -1848,9 +1848,11 @@ static int __init amd_pstate_init(void)
 		return -ENODEV;
 	}
 
-	ret = amd_pstate_set_driver(cppc_state);
-	if (ret)
+	ret = amd_pstate_register_driver(cppc_state);
+	if (ret) {
+		pr_err("failed to register with return %d\n", ret);
 		return ret;
+	}
 
 	/* capability check */
 	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
@@ -1870,12 +1872,6 @@ static int __init amd_pstate_init(void)
 			return ret;
 	}
 
-	ret = amd_pstate_register_driver(cppc_state);
-	if (ret) {
-		pr_err("failed to register with return %d\n", ret);
-		return ret;
-	}
-
 	dev_root = bus_get_dev_root(&cpu_subsys);
 	if (dev_root) {
 		ret = sysfs_create_group(&dev_root->kobj, &amd_pstate_global_attr_group);
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/4] cpufreq/amd-pstate: Refactor amd_pstate_init()
  2024-10-17 10:05 [PATCH 0/4] cpufreq/amd-pstate: Refactor amd_pstate_init() Dhananjay Ugwekar
                   ` (3 preceding siblings ...)
  2024-10-17 10:05 ` [PATCH 4/4] cpufreq/amd-pstate: Remove the redundant amd_pstate_set_driver() call Dhananjay Ugwekar
@ 2024-10-17 18:06 ` Mario Limonciello
  4 siblings, 0 replies; 9+ messages in thread
From: Mario Limonciello @ 2024-10-17 18:06 UTC (permalink / raw)
  To: Dhananjay Ugwekar, gautham.shenoy, perry.yuan, rafael,
	viresh.kumar
  Cc: linux-pm, linux-kernel

On 10/17/2024 05:05, Dhananjay Ugwekar wrote:
> Replace code chunks with function calls, improve readability and
> reduce redundancy.
> 
> Base: superm1/linux-next + https://lore.kernel.org/linux-pm/20241017053927.25285-1-Dhananjay.Ugwekar@amd.com/
> 
> Dhananjay Ugwekar (4):
>    cpufreq/amd-pstate: Call amd_pstate_register() in amd_pstate_init()
>    cpufreq/amd-pstate: Call amd_pstate_set_driver() in
>      amd_pstate_register_driver()
>    cpufreq/amd-pstate: Remove the switch case in amd_pstate_init()
>    cpufreq/amd-pstate: Remove the redundant amd_pstate_set_driver() call
> 
>   drivers/cpufreq/amd-pstate.c | 79 +++++++++++++-----------------------
>   1 file changed, 29 insertions(+), 50 deletions(-)
> 

Thanks for the improvement.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 4/4] cpufreq/amd-pstate: Remove the redundant amd_pstate_set_driver() call
  2024-10-17 10:05 ` [PATCH 4/4] cpufreq/amd-pstate: Remove the redundant amd_pstate_set_driver() call Dhananjay Ugwekar
@ 2024-10-26 11:58   ` Klara Modin
  2024-10-27  4:27     ` Mario Limonciello
  0 siblings, 1 reply; 9+ messages in thread
From: Klara Modin @ 2024-10-26 11:58 UTC (permalink / raw)
  To: Dhananjay Ugwekar, gautham.shenoy, mario.limonciello, perry.yuan,
	rafael, viresh.kumar
  Cc: linux-pm, linux-kernel, klara

[-- Attachment #1: Type: text/plain, Size: 1705 bytes --]

Hi,

On 2024-10-17 12:05, Dhananjay Ugwekar wrote:
> amd_pstate_set_driver() is called twice, once in amd_pstate_init() and once
> as part of amd_pstate_register_driver(). Move around code and eliminate
> the redundancy.
> 
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
> ---
>   drivers/cpufreq/amd-pstate.c | 12 ++++--------
>   1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 13ee5cac901d..6f6d961879cc 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -1848,9 +1848,11 @@ static int __init amd_pstate_init(void)
>   		return -ENODEV;
>   	}
>   
> -	ret = amd_pstate_set_driver(cppc_state);
> -	if (ret)
> +	ret = amd_pstate_register_driver(cppc_state);
> +	if (ret) {
> +		pr_err("failed to register with return %d\n", ret);
>   		return ret;
> +	}
>   
>   	/* capability check */
>   	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
> @@ -1870,12 +1872,6 @@ static int __init amd_pstate_init(void)
>   			return ret;
>   	}
>   
> -	ret = amd_pstate_register_driver(cppc_state);
> -	if (ret) {
> -		pr_err("failed to register with return %d\n", ret);
> -		return ret;
> -	}
> -
>   	dev_root = bus_get_dev_root(&cpu_subsys);
>   	if (dev_root) {
>   		ret = sysfs_create_group(&dev_root->kobj, &amd_pstate_global_attr_group);


This seems to break boot on my Zen 2 desktop (my Zen 4 laptop is fine, 
however). I don't see any messages on the console and nothing shows up 
in the pstore either.

I'll attach the kernel log from a normal boot (with the patch reverted) 
in case it helps.

Please let me know if there's anything else you need.

Regards,
Klara Modin

[-- Attachment #2: no-boot-bisect --]
[-- Type: text/plain, Size: 2861 bytes --]

# bad: [e99058f87395eb9a263bcadca0ae402d34a96326] crypto: rsassa-pkcs1 - Migrate to sig_alg backend
git bisect start 'HEAD'
# status: waiting for good commit(s), bad commit known
# good: [4e46774408d942efe4eb35dc62e5af3af71b9a30] Merge tag 'for-6.12-rc4-tag' of git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux
git bisect good 4e46774408d942efe4eb35dc62e5af3af71b9a30
# bad: [8f803f3cc3ecbdb5ea61dbe6c38461f6ff75adae] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/ath/ath.git
git bisect bad 8f803f3cc3ecbdb5ea61dbe6c38461f6ff75adae
# good: [b5ad19502c85ad4d8c8237d614e383dd31a920b3] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/uml/linux.git
git bisect good b5ad19502c85ad4d8c8237d614e383dd31a920b3
# bad: [ba516a4971bb3ca67dff6dcd13d04dc63fbde292] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git
git bisect bad ba516a4971bb3ca67dff6dcd13d04dc63fbde292
# good: [e4f77835fcb3e30655e8a631358715ace26e3675] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/printk/linux.git
git bisect good e4f77835fcb3e30655e8a631358715ace26e3675
# good: [21b9c329c48d88fb5adc1dbdc34f78b3da0d91a6] Merge branch 'docs-next' of git://git.lwn.net/linux.git
git bisect good 21b9c329c48d88fb5adc1dbdc34f78b3da0d91a6
# good: [e96c6b4a04e1ae12e0144300057af6939dfd35eb] Merge branch 'next' of git://git.linuxtv.org/media
git bisect good e96c6b4a04e1ae12e0144300057af6939dfd35eb
# good: [8a79bf447d8948d136ec64b7070bf7923c60dafd] Merge branch 'thermal-core' into linux-next
git bisect good 8a79bf447d8948d136ec64b7070bf7923c60dafd
# bad: [4ae7df8e2051753c5b6bed657a8d114d9faf02b7] Merge branch 'cpupower' of git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux.git
git bisect bad 4ae7df8e2051753c5b6bed657a8d114d9faf02b7
# bad: [9609386c86eedfb402b6c7d0c49f1b7cbb90729b] Merge tag 'amd-pstate-v6.13-2024-10-22' of ssh://gitolite.kernel.org/pub/scm/linux/kernel/git/superm1/linux
git bisect bad 9609386c86eedfb402b6c7d0c49f1b7cbb90729b
# good: [2af8e3e7d80086b617d4b952bff512fec2b9c909] cpufreq/amd-pstate: Remove the redundant verify() function
git bisect good 2af8e3e7d80086b617d4b952bff512fec2b9c909
# good: [55776e5ca936184bcb6805a7cfabbf5c7c7aaf17] cpufreq/amd-pstate: Remove the switch case in amd_pstate_init()
git bisect good 55776e5ca936184bcb6805a7cfabbf5c7c7aaf17
# bad: [61e9559176a1d7ebe7492f5203259ecf272e52b0] amd-pstate: Set min_perf to nominal_perf for active mode performance gov
git bisect bad 61e9559176a1d7ebe7492f5203259ecf272e52b0
# bad: [e238968a2087a0f7799ba3c5ae5d9a666c52e324] cpufreq/amd-pstate: Remove the redundant amd_pstate_set_driver() call
git bisect bad e238968a2087a0f7799ba3c5ae5d9a666c52e324
# first bad commit: [e238968a2087a0f7799ba3c5ae5d9a666c52e324] cpufreq/amd-pstate: Remove the redundant amd_pstate_set_driver() call

[-- Attachment #3: dmesg.gz --]
[-- Type: application/gzip, Size: 22665 bytes --]

[-- Attachment #4: config.gz --]
[-- Type: application/gzip, Size: 46051 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 4/4] cpufreq/amd-pstate: Remove the redundant amd_pstate_set_driver() call
  2024-10-26 11:58   ` Klara Modin
@ 2024-10-27  4:27     ` Mario Limonciello
  2024-10-27 11:41       ` Klara Modin
  0 siblings, 1 reply; 9+ messages in thread
From: Mario Limonciello @ 2024-10-27  4:27 UTC (permalink / raw)
  To: Klara Modin, Dhananjay Ugwekar, gautham.shenoy, mario.limonciello,
	perry.yuan, rafael, viresh.kumar
  Cc: linux-pm, linux-kernel, klara



On 10/26/24 06:58, Klara Modin wrote:
> Hi,
> 
> On 2024-10-17 12:05, Dhananjay Ugwekar wrote:
>> amd_pstate_set_driver() is called twice, once in amd_pstate_init() and 
>> once
>> as part of amd_pstate_register_driver(). Move around code and eliminate
>> the redundancy.
>>
>> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
>> ---
>>   drivers/cpufreq/amd-pstate.c | 12 ++++--------
>>   1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>> index 13ee5cac901d..6f6d961879cc 100644
>> --- a/drivers/cpufreq/amd-pstate.c
>> +++ b/drivers/cpufreq/amd-pstate.c
>> @@ -1848,9 +1848,11 @@ static int __init amd_pstate_init(void)
>>           return -ENODEV;
>>       }
>> -    ret = amd_pstate_set_driver(cppc_state);
>> -    if (ret)
>> +    ret = amd_pstate_register_driver(cppc_state);
>> +    if (ret) {
>> +        pr_err("failed to register with return %d\n", ret);
>>           return ret;
>> +    }
>>       /* capability check */
>>       if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
>> @@ -1870,12 +1872,6 @@ static int __init amd_pstate_init(void)
>>               return ret;
>>       }
>> -    ret = amd_pstate_register_driver(cppc_state);
>> -    if (ret) {
>> -        pr_err("failed to register with return %d\n", ret);
>> -        return ret;
>> -    }
>> -
>>       dev_root = bus_get_dev_root(&cpu_subsys);
>>       if (dev_root) {
>>           ret = sysfs_create_group(&dev_root->kobj, 
>> &amd_pstate_global_attr_group);
> 
> 
> This seems to break boot on my Zen 2 desktop (my Zen 4 laptop is fine, 
> however). I don't see any messages on the console and nothing shows up 
> in the pstore either.
> 
> I'll attach the kernel log from a normal boot (with the patch reverted) 
> in case it helps.
> 
> Please let me know if there's anything else you need.
> 
> Regards,
> Klara Modin

Hi,

Thanks for reporting.  I think this might be a regression on shared 
memory designs specifically because static calls weren't updated yet.

Can you try this below diff?

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 206725219d8c9..2f0e29b05963d 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -1857,12 +1857,6 @@ static int __init amd_pstate_init(void)
                 return -ENODEV;
         }

-       ret = amd_pstate_register_driver(cppc_state);
-       if (ret) {
-               pr_err("failed to register with return %d\n", ret);
-               return ret;
-       }
-
         /* capability check */
         if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
                 pr_debug("AMD CPPC MSR based functionality is 
supported\n");
@@ -1881,6 +1875,12 @@ static int __init amd_pstate_init(void)
                         return ret;
         }

+       ret = amd_pstate_register_driver(cppc_state);
+       if (ret) {
+               pr_err("failed to register with return %d\n", ret);
+               return ret;
+       }
+
         dev_root = bus_get_dev_root(&cpu_subsys);
         if (dev_root) {
                 ret = sysfs_create_group(&dev_root->kobj, 
&amd_pstate_global_attr_group);


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 4/4] cpufreq/amd-pstate: Remove the redundant amd_pstate_set_driver() call
  2024-10-27  4:27     ` Mario Limonciello
@ 2024-10-27 11:41       ` Klara Modin
  0 siblings, 0 replies; 9+ messages in thread
From: Klara Modin @ 2024-10-27 11:41 UTC (permalink / raw)
  To: Mario Limonciello, Dhananjay Ugwekar, gautham.shenoy,
	mario.limonciello, perry.yuan, rafael, viresh.kumar
  Cc: linux-pm, linux-kernel, klara

On 2024-10-27 05:27, Mario Limonciello wrote:
> 
> 
> On 10/26/24 06:58, Klara Modin wrote:
>> Hi,
>>
>> On 2024-10-17 12:05, Dhananjay Ugwekar wrote:
>>> amd_pstate_set_driver() is called twice, once in amd_pstate_init() 
>>> and once
>>> as part of amd_pstate_register_driver(). Move around code and eliminate
>>> the redundancy.
>>>
>>> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
>>> ---
>>>   drivers/cpufreq/amd-pstate.c | 12 ++++--------
>>>   1 file changed, 4 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>>> index 13ee5cac901d..6f6d961879cc 100644
>>> --- a/drivers/cpufreq/amd-pstate.c
>>> +++ b/drivers/cpufreq/amd-pstate.c
>>> @@ -1848,9 +1848,11 @@ static int __init amd_pstate_init(void)
>>>           return -ENODEV;
>>>       }
>>> -    ret = amd_pstate_set_driver(cppc_state);
>>> -    if (ret)
>>> +    ret = amd_pstate_register_driver(cppc_state);
>>> +    if (ret) {
>>> +        pr_err("failed to register with return %d\n", ret);
>>>           return ret;
>>> +    }
>>>       /* capability check */
>>>       if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
>>> @@ -1870,12 +1872,6 @@ static int __init amd_pstate_init(void)
>>>               return ret;
>>>       }
>>> -    ret = amd_pstate_register_driver(cppc_state);
>>> -    if (ret) {
>>> -        pr_err("failed to register with return %d\n", ret);
>>> -        return ret;
>>> -    }
>>> -
>>>       dev_root = bus_get_dev_root(&cpu_subsys);
>>>       if (dev_root) {
>>>           ret = sysfs_create_group(&dev_root->kobj, 
>>> &amd_pstate_global_attr_group);
>>
>>
>> This seems to break boot on my Zen 2 desktop (my Zen 4 laptop is fine, 
>> however). I don't see any messages on the console and nothing shows up 
>> in the pstore either.
>>
>> I'll attach the kernel log from a normal boot (with the patch 
>> reverted) in case it helps.
>>
>> Please let me know if there's anything else you need.
>>
>> Regards,
>> Klara Modin
> 
> Hi,
> 
> Thanks for reporting.  I think this might be a regression on shared 
> memory designs specifically because static calls weren't updated yet.
> 
> Can you try this below diff?
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 206725219d8c9..2f0e29b05963d 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -1857,12 +1857,6 @@ static int __init amd_pstate_init(void)
>                  return -ENODEV;
>          }
> 
> -       ret = amd_pstate_register_driver(cppc_state);
> -       if (ret) {
> -               pr_err("failed to register with return %d\n", ret);
> -               return ret;
> -       }
> -
>          /* capability check */
>          if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
>                  pr_debug("AMD CPPC MSR based functionality is 
> supported\n");
> @@ -1881,6 +1875,12 @@ static int __init amd_pstate_init(void)
>                          return ret;
>          }
> 
> +       ret = amd_pstate_register_driver(cppc_state);
> +       if (ret) {
> +               pr_err("failed to register with return %d\n", ret);
> +               return ret;
> +       }
> +
>          dev_root = bus_get_dev_root(&cpu_subsys);
>          if (dev_root) {
>                  ret = sysfs_create_group(&dev_root->kobj, 
> &amd_pstate_global_attr_group);
> 

Moving the it afterwards does fix the issue, thanks!

Tested-by: Klara Modin <klarasmodin@gmail.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2024-10-27 11:41 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-17 10:05 [PATCH 0/4] cpufreq/amd-pstate: Refactor amd_pstate_init() Dhananjay Ugwekar
2024-10-17 10:05 ` [PATCH 1/4] cpufreq/amd-pstate: Call amd_pstate_register() in amd_pstate_init() Dhananjay Ugwekar
2024-10-17 10:05 ` [PATCH 2/4] cpufreq/amd-pstate: Call amd_pstate_set_driver() in amd_pstate_register_driver() Dhananjay Ugwekar
2024-10-17 10:05 ` [PATCH 3/4] cpufreq/amd-pstate: Remove the switch case in amd_pstate_init() Dhananjay Ugwekar
2024-10-17 10:05 ` [PATCH 4/4] cpufreq/amd-pstate: Remove the redundant amd_pstate_set_driver() call Dhananjay Ugwekar
2024-10-26 11:58   ` Klara Modin
2024-10-27  4:27     ` Mario Limonciello
2024-10-27 11:41       ` Klara Modin
2024-10-17 18:06 ` [PATCH 0/4] cpufreq/amd-pstate: Refactor amd_pstate_init() Mario Limonciello

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox