| Message ID | 20241017100528.300143-5-Dhananjay.Ugwekar@amd.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Mario Limonciello |
| Headers | show |
| Series | cpufreq/amd-pstate: Refactor amd_pstate_init() | expand |
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 # 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
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);
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>
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);
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(-)