Missing "unregister_cpu_notifier" in powernow-k8.c

Missing "unregister_cpu_notifier" in powernow-k8.c

[Neil Brown]

Hi,
 (hope I got the right addressees above....)

 I appears that when powernow-k8 find that 

    No compatible ACPI _PSS objects found.

 and suggests

    Try again with latest BIOS.

 it fails the module load, but does not unregister the cpu_notifier that was 
 registered in powernowk8_init

 This ends up leaving freed memory on the cpu notifier list for some other
 poor module (e.g. md/raid5) to come along and trip over.

 The following might be a partial fix, but I suspect there is probably other
 clean-up that is needed.

 ( https://bugzilla.novell.com/show_bug.cgi?id=655215 has full dmesg traces).

Thanks,
NeilBrown


diff --git a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
index 491977b..812778c 100644
--- a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
+++ b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
@@ -1537,6 +1537,7 @@ static struct notifier_block cpb_nb = {
 static int __cpuinit powernowk8_init(void)
 {
 	unsigned int i, supported_cpus = 0, cpu;
+	int rv;
 
 	for_each_online_cpu(i) {
 		int rc;
@@ -1574,7 +1575,10 @@ static int __cpuinit powernowk8_init(void)
 			(cpb_enabled ? "on" : "off"));
 	}
 
-	return cpufreq_register_driver(&cpufreq_amd64_driver);
+	rv = cpufreq_register_driver(&cpufreq_amd64_driver);
+	if (rv < 0 && boot_cpu_has(X86_FEATURE_CPB))
+		unregister_cpu_notifier(&cpb_nb);
+	return rv;
 }
 
 /* driver entry point for term */

Re: Missing "unregister_cpu_notifier" in powernow-k8.c

[Dave Jones]

On Wed, Nov 24, 2010 at 11:28:01AM +1100, Neil Brown wrote:
 > 
 > Hi,
 >  (hope I got the right addressees above....)
 > 
 >  I appears that when powernow-k8 find that 
 > 
 >     No compatible ACPI _PSS objects found.
 > 
 >  and suggests
 > 
 >     Try again with latest BIOS.
 > 
 >  it fails the module load, but does not unregister the cpu_notifier that was 
 >  registered in powernowk8_init
 > 
 >  This ends up leaving freed memory on the cpu notifier list for some other
 >  poor module (e.g. md/raid5) to come along and trip over.
 > 
 >  The following might be a partial fix, but I suspect there is probably other
 >  clean-up that is needed.
 > 
 >  ( https://bugzilla.novell.com/show_bug.cgi?id=655215 has full dmesg traces).
 
Ouch, I'm surprised that took so long to turn up.
I'll merge this up. It should go to stable too.

thanks,

	Dave

Re: Missing "unregister_cpu_notifier" in powernow-k8.c

[Dave Jones]

On Tue, Nov 23, 2010 at 09:19:38PM -0500, Dave Jones wrote:
 >  The following might be a partial fix, but I suspect there is probably other
 >  clean-up that is needed.

Indeed, looks like another possible leak in the unlikely event that the
percpu msr struct can't be allocated..
Instead of adding an unregister on fail, I think we can just move the registration later.

	Dave

diff --git a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
index 812778c..f0a80f1 100644
--- a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
+++ b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
@@ -1556,14 +1556,14 @@ static int __cpuinit powernowk8_init(void)
 
                cpb_capable = true;
 
-               register_cpu_notifier(&cpb_nb);
-
                msrs = msrs_alloc();
                if (!msrs) {
                        printk(KERN_ERR "%s: Error allocating msrs!\n", __func__);
                        return -ENOMEM;
                }
 
+               register_cpu_notifier(&cpb_nb);
+
                rdmsr_on_cpus(cpu_online_mask, MSR_K7_HWCR, msrs);
 
                for_each_cpu(cpu, cpu_online_mask) {

Re: Missing "unregister_cpu_notifier" in powernow-k8.c

[Borislav Petkov]

On Tue, Nov 23, 2010 at 09:31:24PM -0500, Dave Jones wrote:
> On Tue, Nov 23, 2010 at 09:19:38PM -0500, Dave Jones wrote:
>  >  The following might be a partial fix, but I suspect there is probably other
>  >  clean-up that is needed.
> 
> Indeed, looks like another possible leak in the unlikely event that the
> percpu msr struct can't be allocated..
> Instead of adding an unregister on fail, I think we can just move the registration later.

Oops, that would be partly my fault, sorry. How about the following
instead, with proper exit path labels as it is done in other kernel
code?

  [ It is not yet tested but I'll give it a run later on my X6 to verify ]


Thanks.
--
From: Borislav Petkov <borislav.petkov@amd.com>
Date: Wed, 24 Nov 2010 09:51:09 +0100
Subject: [PATCH] x86, powernow-k8: Fix exit path

When Cool'n'Quiet is disabled in the BIOS we're missing _PSS
objects and powernow-k8 fails loading with a juicy firmware bug
message. However, on machines with boosting cpus, it forgets
to unreg from the cpu notifier chain leading to oops. See also
https://bugzilla.novell.com/show_bug.cgi?id=655215

Fix error path accordingly.

Originally-by: Neil Brown <neilb@suse.de>
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/kernel/cpu/cpufreq/powernow-k8.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
index 491977b..bdeb5f9 100644
--- a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
+++ b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
@@ -1537,6 +1537,7 @@ static struct notifier_block cpb_nb = {
 static int __cpuinit powernowk8_init(void)
 {
 	unsigned int i, supported_cpus = 0, cpu;
+	int err = 0;
 
 	for_each_online_cpu(i) {
 		int rc;
@@ -1557,10 +1558,11 @@ static int __cpuinit powernowk8_init(void)
 
 		register_cpu_notifier(&cpb_nb);
 
+		err = -ENOMEM;
 		msrs = msrs_alloc();
 		if (!msrs) {
 			printk(KERN_ERR "%s: Error allocating msrs!\n", __func__);
-			return -ENOMEM;
+			goto unreg;
 		}
 
 		rdmsr_on_cpus(cpu_online_mask, MSR_K7_HWCR, msrs);
@@ -1574,7 +1576,15 @@ static int __cpuinit powernowk8_init(void)
 			(cpb_enabled ? "on" : "off"));
 	}
 
-	return cpufreq_register_driver(&cpufreq_amd64_driver);
+	err = cpufreq_register_driver(&cpufreq_amd64_driver);
+	if (!err)
+		goto out;
+
+unreg:
+	if (cpb_capable)
+		unregister_cpu_notifier(&cpb_nb);
+out:
+	return err;
 }
 
 /* driver entry point for term */
@@ -1582,7 +1592,7 @@ static void __exit powernowk8_exit(void)
 {
 	dprintk("exit\n");
 
-	if (boot_cpu_has(X86_FEATURE_CPB)) {
+	if (cpb_capable) {
 		msrs_free(msrs);
 		msrs = NULL;
 
-- 
1.7.2.3



-- 
Regards/Gruss,
    Boris.

Re: Missing "unregister_cpu_notifier" in powernow-k8.c

[Borislav Petkov]

On Wed, Nov 24, 2010 at 04:02:28AM -0500, Borislav Petkov wrote:
> On Tue, Nov 23, 2010 at 09:31:24PM -0500, Dave Jones wrote:
> > On Tue, Nov 23, 2010 at 09:19:38PM -0500, Dave Jones wrote:
> >  >  The following might be a partial fix, but I suspect there is probably other
> >  >  clean-up that is needed.
> > 
> > Indeed, looks like another possible leak in the unlikely event that the
> > percpu msr struct can't be allocated..
> > Instead of adding an unregister on fail, I think we can just move the registration later.
> 
> Oops, that would be partly my fault, sorry. How about the following
> instead, with proper exit path labels as it is done in other kernel
> code?
> 
>   [ It is not yet tested but I'll give it a run later on my X6 to verify ]

Ok, I went and fixed up the error message too when Cool'N'Quiet is disabled in
the BIOS. Tested patches coming up...

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

[PATCH 1/2] x86, powernow-k8: Fixup missing _PSS objects message

[Borislav Petkov]

From: Borislav Petkov <borislav.petkov@amd.com>

_PSS objects can also be missing if Cool'N'Quiet is disabled in the
BIOS. Add that to the FW_BUG message for the user to try before updating
her BIOS. Fix formatting.

Cc: Thomas Renninger <trenn@suse.de>
Cc: Mark Langsdorf <mark.langsdorf@amd.com>
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/kernel/cpu/cpufreq/powernow-k8.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
index 491977b..d2e53ac 100644
--- a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
+++ b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
@@ -1247,12 +1247,15 @@ static void __cpuinit powernowk8_cpu_init_on_cpu(void *_init_on_cpu)
 	init_on_cpu->rc = 0;
 }
 
+static const char missing_pss_msg[] =
+	KERN_ERR
+	FW_BUG PFX "No compatible ACPI _PSS objects found.\n"
+	FW_BUG PFX "First, make sure Cool'N'Quiet is enabled in the BIOS.\n"
+	FW_BUG PFX "If that doesn't help, try upgrading your BIOS.\n";
+
 /* per CPU init entry point to the driver */
 static int __cpuinit powernowk8_cpu_init(struct cpufreq_policy *pol)
 {
-	static const char ACPI_PSS_BIOS_BUG_MSG[] =
-		KERN_ERR FW_BUG PFX "No compatible ACPI _PSS objects found.\n"
-		FW_BUG PFX "Try again with latest BIOS.\n";
 	struct powernow_k8_data *data;
 	struct init_on_cpu init_on_cpu;
 	int rc;
@@ -1280,7 +1283,7 @@ static int __cpuinit powernowk8_cpu_init(struct cpufreq_policy *pol)
 		 * an UP version, and is deprecated by AMD.
 		 */
 		if (num_online_cpus() != 1) {
-			printk_once(ACPI_PSS_BIOS_BUG_MSG);
+			printk_once(missing_pss_msg);
 			goto err_out;
 		}
 		if (pol->cpu != 0) {
-- 
1.7.3.1.50.g1e633

[PATCH 2/2] x86, powernow-k8: Fix exit path

[Borislav Petkov]

From: Borislav Petkov <bp@alien8.de>

When Cool'n'Quiet is disabled in the BIOS we're missing _PSS
objects and powernow-k8 fails loading with a juicy firmware bug
message. However, on machines with boosting cpus, it forgets
to unreg from the cpu notifier chain leading to oops. See also
https://bugzilla.novell.com/show_bug.cgi?id=655215

Fix error path accordingly.

Originally-by: Neil Brown <neilb@suse.de>
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/kernel/cpu/cpufreq/powernow-k8.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
index d2e53ac..ccfc68b 100644
--- a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
+++ b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
@@ -1540,6 +1540,7 @@ static struct notifier_block cpb_nb = {
 static int __cpuinit powernowk8_init(void)
 {
 	unsigned int i, supported_cpus = 0, cpu;
+	int err = 0;
 
 	for_each_online_cpu(i) {
 		int rc;
@@ -1560,10 +1561,11 @@ static int __cpuinit powernowk8_init(void)
 
 		register_cpu_notifier(&cpb_nb);
 
+		err = -ENOMEM;
 		msrs = msrs_alloc();
 		if (!msrs) {
 			printk(KERN_ERR "%s: Error allocating msrs!\n", __func__);
-			return -ENOMEM;
+			goto unreg;
 		}
 
 		rdmsr_on_cpus(cpu_online_mask, MSR_K7_HWCR, msrs);
@@ -1577,7 +1579,15 @@ static int __cpuinit powernowk8_init(void)
 			(cpb_enabled ? "on" : "off"));
 	}
 
-	return cpufreq_register_driver(&cpufreq_amd64_driver);
+	err = cpufreq_register_driver(&cpufreq_amd64_driver);
+	if (!err)
+		goto out;
+
+unreg:
+	if (cpb_capable)
+		unregister_cpu_notifier(&cpb_nb);
+out:
+	return err;
 }
 
 /* driver entry point for term */
@@ -1585,7 +1595,7 @@ static void __exit powernowk8_exit(void)
 {
 	dprintk("exit\n");
 
-	if (boot_cpu_has(X86_FEATURE_CPB)) {
+	if (cpb_capable) {
 		msrs_free(msrs);
 		msrs = NULL;
 
-- 
1.7.3.1.50.g1e633