[PATCH] drivers/cpufreq: Warn user when powernow-k8 tries to fall back to acpi-cpufreq and it is unavailable.

[PATCH] drivers/cpufreq: Warn user when powernow-k8 tries to fall back to acpi-cpufreq and it is unavailable.

[Aravind Gopalakrishnan]

This patch is in reference to bug#:51741. (https://bugzilla.kernel.org/show_bug.cgi?id=51741)
powernow-k8 falls back to acpi-cpufreq if CPU is not supported. However, it states that acpi-cpufreq
has taken over even if acpi-cpufreq is not compiled in. This patch rewords the warning message to
clarify that the CPU is unsupported and prints a warning message when there is no acpi-cpufreq
present.

Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
---
 drivers/cpufreq/powernow-k8.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/powernow-k8.c b/drivers/cpufreq/powernow-k8.c
index 056faf6..6fa58b4 100644
--- a/drivers/cpufreq/powernow-k8.c
+++ b/drivers/cpufreq/powernow-k8.c
@@ -1256,7 +1256,15 @@ static int __cpuinit powernowk8_init(void)
 	int rv;
 
 	if (static_cpu_has(X86_FEATURE_HW_PSTATE)) {
-		pr_warn(PFX "this CPU is not supported anymore, using acpi-cpufreq instead.\n");
+		pr_warn(PFX
+			"this CPU is not supported anymore, use acpi-cpufreq instead"
+			"Look for message from acpi-cpufreq to ensure it is loaded."
+			".\n");
+#ifndef CONFIG_X86_ACPI_CPUFREQ
+	pr_warn(PFX "acpi-cpufreq is disabled."
+		"Enable it in the config options to get frequency scaling.\n");
+	return -ENODEV;
+#endif
 		request_module("acpi-cpufreq");
 		return -ENODEV;
 	}
-- 
1.7.10.4

Re: [PATCH] drivers/cpufreq: Warn user when powernow-k8 tries to fall back to acpi-cpufreq and it is unavailable.

[Borislav Petkov]

+ Andre.

On Wed, Jan 09, 2013 at 07:09:21PM -0600, Aravind Gopalakrishnan wrote:
> This patch is in reference to bug#:51741. (https://bugzilla.kernel.org/show_bug.cgi?id=51741)
> powernow-k8 falls back to acpi-cpufreq if CPU is not supported. However, it states that acpi-cpufreq
> has taken over even if acpi-cpufreq is not compiled in. This patch rewords the warning message to
> clarify that the CPU is unsupported and prints a warning message when there is no acpi-cpufreq
> present.
> 
> Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> ---
>  drivers/cpufreq/powernow-k8.c |   10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/powernow-k8.c b/drivers/cpufreq/powernow-k8.c
> index 056faf6..6fa58b4 100644
> --- a/drivers/cpufreq/powernow-k8.c
> +++ b/drivers/cpufreq/powernow-k8.c
> @@ -1256,7 +1256,15 @@ static int __cpuinit powernowk8_init(void)
>  	int rv;
>  
>  	if (static_cpu_has(X86_FEATURE_HW_PSTATE)) {
> -		pr_warn(PFX "this CPU is not supported anymore, using acpi-cpufreq instead.\n");
> +		pr_warn(PFX
> +			"this CPU is not supported anymore, use acpi-cpufreq instead"
> +			"Look for message from acpi-cpufreq to ensure it is loaded."
> +			".\n");
> +#ifndef CONFIG_X86_ACPI_CPUFREQ
> +	pr_warn(PFX "acpi-cpufreq is disabled."
> +		"Enable it in the config options to get frequency scaling.\n");
> +	return -ENODEV;
> +#endif
>  		request_module("acpi-cpufreq");
>  		return -ENODEV;

Ok, the suggestion in that BZ is valid and something needs to be done
for that case but I don't think that simply warning the user about it is
enough.

First of all, CONFIG_X86_POWERNOW_K8 should depend on
CONFIG_X86_ACPI_CPUFREQ since powernow-k8.ko requests that module.

Then, having that dependency out of the way, we can almost safely
request_module("acpi-cpufreq"). However, we can also check the return
value of that function to make sure loading went fine.

And finally, we should check that acpi-cpufreq actually registered
properly and wasn't unloaded later for some other reason which wasn't
signalled through request_module retval.

Andre, I'm not sure about the details of that last one but the first
two are easy. Any ideas, since you've been looking at acpi-cpufreq code
lately?

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] drivers/cpufreq: Warn user when powernow-k8 tries to fall back to acpi-cpufreq and it is unavailable.

[Borislav Petkov]

Adding bugreporter from BZ to CC.

On Fri, Jan 11, 2013 at 03:49:40PM +0100, Borislav Petkov wrote:
> + Andre.
> 
> On Wed, Jan 09, 2013 at 07:09:21PM -0600, Aravind Gopalakrishnan wrote:
> > This patch is in reference to bug#:51741. (https://bugzilla.kernel.org/show_bug.cgi?id=51741)
> > powernow-k8 falls back to acpi-cpufreq if CPU is not supported. However, it states that acpi-cpufreq
> > has taken over even if acpi-cpufreq is not compiled in. This patch rewords the warning message to
> > clarify that the CPU is unsupported and prints a warning message when there is no acpi-cpufreq
> > present.
> > 
> > Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> > ---
> >  drivers/cpufreq/powernow-k8.c |   10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/cpufreq/powernow-k8.c b/drivers/cpufreq/powernow-k8.c
> > index 056faf6..6fa58b4 100644
> > --- a/drivers/cpufreq/powernow-k8.c
> > +++ b/drivers/cpufreq/powernow-k8.c
> > @@ -1256,7 +1256,15 @@ static int __cpuinit powernowk8_init(void)
> >  	int rv;
> >  
> >  	if (static_cpu_has(X86_FEATURE_HW_PSTATE)) {
> > -		pr_warn(PFX "this CPU is not supported anymore, using acpi-cpufreq instead.\n");
> > +		pr_warn(PFX
> > +			"this CPU is not supported anymore, use acpi-cpufreq instead"
> > +			"Look for message from acpi-cpufreq to ensure it is loaded."
> > +			".\n");
> > +#ifndef CONFIG_X86_ACPI_CPUFREQ
> > +	pr_warn(PFX "acpi-cpufreq is disabled."
> > +		"Enable it in the config options to get frequency scaling.\n");
> > +	return -ENODEV;
> > +#endif
> >  		request_module("acpi-cpufreq");
> >  		return -ENODEV;
> 
> Ok, the suggestion in that BZ is valid and something needs to be done
> for that case but I don't think that simply warning the user about it is
> enough.
> 
> First of all, CONFIG_X86_POWERNOW_K8 should depend on
> CONFIG_X86_ACPI_CPUFREQ since powernow-k8.ko requests that module.
> 
> Then, having that dependency out of the way, we can almost safely
> request_module("acpi-cpufreq"). However, we can also check the return
> value of that function to make sure loading went fine.
> 
> And finally, we should check that acpi-cpufreq actually registered
> properly and wasn't unloaded later for some other reason which wasn't
> signalled through request_module retval.
> 
> Andre, I'm not sure about the details of that last one but the first
> two are easy. Any ideas, since you've been looking at acpi-cpufreq code
> lately?

Ok,

this turned out to be not that hard, here's what I came up with. I
haven't checked the case where acpi-cpufreq and powernow-k8 are built in
and what happens then.

This still doesn't deal with pathological situations when acpi-cpufreq
f*cks up loading later and errors out. For that, we probably need to
export some cpufreq internals like cpufreq_driver and look at its ->name
member and decide whether it is properly set. I'm not sure that is
really warranted though...

In the meantime, here's a first stab at it, which should cover the major
holes.

--
From: Borislav Petkov <bp@suse.de>
Date: Fri, 11 Jan 2013 17:36:58 +0100
Subject: [PATCH] powernow-k8: Robustify loading of acpi-cpufreq

Andreas says in https://bugzilla.kernel.org/show_bug.cgi?id=51741 that
with his Gentoo config, acpi-cpufreq wasn't enabled and powernow-k8
couldn't handoff properly to acpi-cpufreq leading to running without
P-state support (i.e., cores are constantly in P0).

To alleaviate that, we need to make powernow-k8 depend on acpi-cpufreq
so that acpi-cpufreq is always present as a module and is thus loadable.
Also, request_module waits until acpi-cpufreq is loaded so we check its
retval whether the loading succeeded or not. If not, we warn the user so
that she can take precaution and fix the situation.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 drivers/cpufreq/Kconfig.x86   | 2 +-
 drivers/cpufreq/powernow-k8.c | 8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/Kconfig.x86 b/drivers/cpufreq/Kconfig.x86
index 934854ae5eb4..7227cd734042 100644
--- a/drivers/cpufreq/Kconfig.x86
+++ b/drivers/cpufreq/Kconfig.x86
@@ -106,7 +106,7 @@ config X86_POWERNOW_K7_ACPI
 config X86_POWERNOW_K8
 	tristate "AMD Opteron/Athlon64 PowerNow!"
 	select CPU_FREQ_TABLE
-	depends on ACPI && ACPI_PROCESSOR
+	depends on ACPI && ACPI_PROCESSOR && X86_ACPI_CPUFREQ
 	help
 	  This adds the CPUFreq driver for K8/early Opteron/Athlon64 processors.
 	  Support for K10 and newer processors is now in acpi-cpufreq.
diff --git a/drivers/cpufreq/powernow-k8.c b/drivers/cpufreq/powernow-k8.c
index 056faf6af1a9..1adbe86d0d3b 100644
--- a/drivers/cpufreq/powernow-k8.c
+++ b/drivers/cpufreq/powernow-k8.c
@@ -1256,8 +1256,12 @@ static int __cpuinit powernowk8_init(void)
 	int rv;
 
 	if (static_cpu_has(X86_FEATURE_HW_PSTATE)) {
-		pr_warn(PFX "this CPU is not supported anymore, using acpi-cpufreq instead.\n");
-		request_module("acpi-cpufreq");
+		pr_warn(PFX "This CPU is now supported by acpi-cpufreq, loading it.\n");
+
+		if (request_module("acpi-cpufreq"))
+			pr_warn(PFX "Error loading acpi-cpufreq, make sure you "
+				"have it enabled, else you won't have any "
+				"Pstates support on this CPU!\n");
 		return -ENODEV;
 	}
 
-- 
1.8.1.rc3

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

RE: [PATCH] drivers/cpufreq: Warn user when powernow-k8 tries to fall back to acpi-cpufreq and it is unavailable.

[Gopalakrishnan, Aravind]

So, I had tried out the case when acpi-cpufreq was compiled into the kernel and looked at the return values from request_module(); it returns a positive value (255) both when acpi-cpufreq was compiled-in and when not compiled-in.  (Please do let me know if I am missing something here...)
(This was the case Andreas had mentioned in the bug report too)

It was due to this that I had decided to just check the CONFIG option and print out a warning to the user.

-----Original Message-----
From: Borislav Petkov [mailto:bp@alien8.de] 
Sent: Friday, January 11, 2013 10:51 AM
To: Gopalakrishnan, Aravind; Andre Przywara; rjw@sisk.pl; cpufreq@vger.kernel.org; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
Cc: Andreas
Subject: Re: [PATCH] drivers/cpufreq: Warn user when powernow-k8 tries to fall back to acpi-cpufreq and it is unavailable.

Adding bugreporter from BZ to CC.

On Fri, Jan 11, 2013 at 03:49:40PM +0100, Borislav Petkov wrote:
> + Andre.
> 
> On Wed, Jan 09, 2013 at 07:09:21PM -0600, Aravind Gopalakrishnan wrote:
> > This patch is in reference to bug#:51741. 
> > (https://bugzilla.kernel.org/show_bug.cgi?id=51741)
> > powernow-k8 falls back to acpi-cpufreq if CPU is not supported. 
> > However, it states that acpi-cpufreq has taken over even if 
> > acpi-cpufreq is not compiled in. This patch rewords the warning 
> > message to clarify that the CPU is unsupported and prints a warning message when there is no acpi-cpufreq present.
> > 
> > Signed-off-by: Aravind Gopalakrishnan 
> > <Aravind.Gopalakrishnan@amd.com>
> > ---
> >  drivers/cpufreq/powernow-k8.c |   10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/cpufreq/powernow-k8.c 
> > b/drivers/cpufreq/powernow-k8.c index 056faf6..6fa58b4 100644
> > --- a/drivers/cpufreq/powernow-k8.c
> > +++ b/drivers/cpufreq/powernow-k8.c
> > @@ -1256,7 +1256,15 @@ static int __cpuinit powernowk8_init(void)
> >  	int rv;
> >  
> >  	if (static_cpu_has(X86_FEATURE_HW_PSTATE)) {
> > -		pr_warn(PFX "this CPU is not supported anymore, using acpi-cpufreq instead.\n");
> > +		pr_warn(PFX
> > +			"this CPU is not supported anymore, use acpi-cpufreq instead"
> > +			"Look for message from acpi-cpufreq to ensure it is loaded."
> > +			".\n");
> > +#ifndef CONFIG_X86_ACPI_CPUFREQ
> > +	pr_warn(PFX "acpi-cpufreq is disabled."
> > +		"Enable it in the config options to get frequency scaling.\n");
> > +	return -ENODEV;
> > +#endif
> >  		request_module("acpi-cpufreq");
> >  		return -ENODEV;
> 
> Ok, the suggestion in that BZ is valid and something needs to be done 
> for that case but I don't think that simply warning the user about it 
> is enough.
> 
> First of all, CONFIG_X86_POWERNOW_K8 should depend on 
> CONFIG_X86_ACPI_CPUFREQ since powernow-k8.ko requests that module.
> 
> Then, having that dependency out of the way, we can almost safely 
> request_module("acpi-cpufreq"). However, we can also check the return 
> value of that function to make sure loading went fine.
> 
> And finally, we should check that acpi-cpufreq actually registered 
> properly and wasn't unloaded later for some other reason which wasn't 
> signalled through request_module retval.
> 
> Andre, I'm not sure about the details of that last one but the first 
> two are easy. Any ideas, since you've been looking at acpi-cpufreq 
> code lately?

Ok,

this turned out to be not that hard, here's what I came up with. I haven't checked the case where acpi-cpufreq and powernow-k8 are built in and what happens then.

This still doesn't deal with pathological situations when acpi-cpufreq f*cks up loading later and errors out. For that, we probably need to export some cpufreq internals like cpufreq_driver and look at its ->name member and decide whether it is properly set. I'm not sure that is really warranted though...

In the meantime, here's a first stab at it, which should cover the major holes.

--
From: Borislav Petkov <bp@suse.de>
Date: Fri, 11 Jan 2013 17:36:58 +0100
Subject: [PATCH] powernow-k8: Robustify loading of acpi-cpufreq

Andreas says in https://bugzilla.kernel.org/show_bug.cgi?id=51741 that with his Gentoo config, acpi-cpufreq wasn't enabled and powernow-k8 couldn't handoff properly to acpi-cpufreq leading to running without P-state support (i.e., cores are constantly in P0).

To alleaviate that, we need to make powernow-k8 depend on acpi-cpufreq so that acpi-cpufreq is always present as a module and is thus loadable.
Also, request_module waits until acpi-cpufreq is loaded so we check its retval whether the loading succeeded or not. If not, we warn the user so that she can take precaution and fix the situation.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 drivers/cpufreq/Kconfig.x86   | 2 +-
 drivers/cpufreq/powernow-k8.c | 8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/Kconfig.x86 b/drivers/cpufreq/Kconfig.x86 index 934854ae5eb4..7227cd734042 100644
--- a/drivers/cpufreq/Kconfig.x86
+++ b/drivers/cpufreq/Kconfig.x86
@@ -106,7 +106,7 @@ config X86_POWERNOW_K7_ACPI  config X86_POWERNOW_K8
 	tristate "AMD Opteron/Athlon64 PowerNow!"
 	select CPU_FREQ_TABLE
-	depends on ACPI && ACPI_PROCESSOR
+	depends on ACPI && ACPI_PROCESSOR && X86_ACPI_CPUFREQ
 	help
 	  This adds the CPUFreq driver for K8/early Opteron/Athlon64 processors.
 	  Support for K10 and newer processors is now in acpi-cpufreq.
diff --git a/drivers/cpufreq/powernow-k8.c b/drivers/cpufreq/powernow-k8.c index 056faf6af1a9..1adbe86d0d3b 100644
--- a/drivers/cpufreq/powernow-k8.c
+++ b/drivers/cpufreq/powernow-k8.c
@@ -1256,8 +1256,12 @@ static int __cpuinit powernowk8_init(void)
 	int rv;
 
 	if (static_cpu_has(X86_FEATURE_HW_PSTATE)) {
-		pr_warn(PFX "this CPU is not supported anymore, using acpi-cpufreq instead.\n");
-		request_module("acpi-cpufreq");
+		pr_warn(PFX "This CPU is now supported by acpi-cpufreq, loading 
+it.\n");
+
+		if (request_module("acpi-cpufreq"))
+			pr_warn(PFX "Error loading acpi-cpufreq, make sure you "
+				"have it enabled, else you won't have any "
+				"Pstates support on this CPU!\n");
 		return -ENODEV;
 	}
 
--
1.8.1.rc3

--
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

RE: [PATCH] drivers/cpufreq: Warn user when powernow-k8 tries to fall back to acpi-cpufreq and it is unavailable.

[Gopalakrishnan, Aravind]

So, I had tried out the case when acpi-cpufreq was compiled into the kernel and looked at the return values from request_module(); it returns a positive value (255) both when acpi-cpufreq was compiled-in and when not compiled-in.  (Please do let me know if I am missing something here...) (This was the case Andreas had mentioned in the bug report too)

It was due to this that I had decided to just check the CONFIG option and print out a warning to the user.

-----Original Message-----
From: Borislav Petkov [mailto:bp@alien8.de] 
Sent: Friday, January 11, 2013 10:51 AM
To: Gopalakrishnan, Aravind; Andre Przywara; rjw@sisk.pl; cpufreq@vger.kernel.org; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
Cc: Andreas
Subject: Re: [PATCH] drivers/cpufreq: Warn user when powernow-k8 tries to fall back to acpi-cpufreq and it is unavailable.

Adding bugreporter from BZ to CC.

On Fri, Jan 11, 2013 at 03:49:40PM +0100, Borislav Petkov wrote:
> + Andre.
> 
> On Wed, Jan 09, 2013 at 07:09:21PM -0600, Aravind Gopalakrishnan wrote:
> > This patch is in reference to bug#:51741. 
> > (https://bugzilla.kernel.org/show_bug.cgi?id=51741)
> > powernow-k8 falls back to acpi-cpufreq if CPU is not supported. 
> > However, it states that acpi-cpufreq has taken over even if 
> > acpi-cpufreq is not compiled in. This patch rewords the warning 
> > message to clarify that the CPU is unsupported and prints a warning message when there is no acpi-cpufreq present.
> > 
> > Signed-off-by: Aravind Gopalakrishnan 
> > <Aravind.Gopalakrishnan@amd.com>
> > ---
> >  drivers/cpufreq/powernow-k8.c |   10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/cpufreq/powernow-k8.c 
> > b/drivers/cpufreq/powernow-k8.c index 056faf6..6fa58b4 100644
> > --- a/drivers/cpufreq/powernow-k8.c
> > +++ b/drivers/cpufreq/powernow-k8.c
> > @@ -1256,7 +1256,15 @@ static int __cpuinit powernowk8_init(void)
> >  	int rv;
> >  
> >  	if (static_cpu_has(X86_FEATURE_HW_PSTATE)) {
> > -		pr_warn(PFX "this CPU is not supported anymore, using acpi-cpufreq instead.\n");
> > +		pr_warn(PFX
> > +			"this CPU is not supported anymore, use acpi-cpufreq instead"
> > +			"Look for message from acpi-cpufreq to ensure it is loaded."
> > +			".\n");
> > +#ifndef CONFIG_X86_ACPI_CPUFREQ
> > +	pr_warn(PFX "acpi-cpufreq is disabled."
> > +		"Enable it in the config options to get frequency scaling.\n");
> > +	return -ENODEV;
> > +#endif
> >  		request_module("acpi-cpufreq");
> >  		return -ENODEV;
> 
> Ok, the suggestion in that BZ is valid and something needs to be done 
> for that case but I don't think that simply warning the user about it 
> is enough.
> 
> First of all, CONFIG_X86_POWERNOW_K8 should depend on 
> CONFIG_X86_ACPI_CPUFREQ since powernow-k8.ko requests that module.
> 
> Then, having that dependency out of the way, we can almost safely 
> request_module("acpi-cpufreq"). However, we can also check the return 
> value of that function to make sure loading went fine.
> 
> And finally, we should check that acpi-cpufreq actually registered 
> properly and wasn't unloaded later for some other reason which wasn't 
> signalled through request_module retval.
> 
> Andre, I'm not sure about the details of that last one but the first 
> two are easy. Any ideas, since you've been looking at acpi-cpufreq 
> code lately?

Ok,

this turned out to be not that hard, here's what I came up with. I haven't checked the case where acpi-cpufreq and powernow-k8 are built in and what happens then.

This still doesn't deal with pathological situations when acpi-cpufreq f*cks up loading later and errors out. For that, we probably need to export some cpufreq internals like cpufreq_driver and look at its ->name member and decide whether it is properly set. I'm not sure that is really warranted though...

In the meantime, here's a first stab at it, which should cover the major holes.

--
From: Borislav Petkov <bp@suse.de>
Date: Fri, 11 Jan 2013 17:36:58 +0100
Subject: [PATCH] powernow-k8: Robustify loading of acpi-cpufreq

Andreas says in https://bugzilla.kernel.org/show_bug.cgi?id=51741 that with his Gentoo config, acpi-cpufreq wasn't enabled and powernow-k8 couldn't handoff properly to acpi-cpufreq leading to running without P-state support (i.e., cores are constantly in P0).

To alleaviate that, we need to make powernow-k8 depend on acpi-cpufreq so that acpi-cpufreq is always present as a module and is thus loadable.
Also, request_module waits until acpi-cpufreq is loaded so we check its retval whether the loading succeeded or not. If not, we warn the user so that she can take precaution and fix the situation.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 drivers/cpufreq/Kconfig.x86   | 2 +-
 drivers/cpufreq/powernow-k8.c | 8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/Kconfig.x86 b/drivers/cpufreq/Kconfig.x86 index 934854ae5eb4..7227cd734042 100644
--- a/drivers/cpufreq/Kconfig.x86
+++ b/drivers/cpufreq/Kconfig.x86
@@ -106,7 +106,7 @@ config X86_POWERNOW_K7_ACPI  config X86_POWERNOW_K8
 	tristate "AMD Opteron/Athlon64 PowerNow!"
 	select CPU_FREQ_TABLE
-	depends on ACPI && ACPI_PROCESSOR
+	depends on ACPI && ACPI_PROCESSOR && X86_ACPI_CPUFREQ
 	help
 	  This adds the CPUFreq driver for K8/early Opteron/Athlon64 processors.
 	  Support for K10 and newer processors is now in acpi-cpufreq.
diff --git a/drivers/cpufreq/powernow-k8.c b/drivers/cpufreq/powernow-k8.c index 056faf6af1a9..1adbe86d0d3b 100644
--- a/drivers/cpufreq/powernow-k8.c
+++ b/drivers/cpufreq/powernow-k8.c
@@ -1256,8 +1256,12 @@ static int __cpuinit powernowk8_init(void)
 	int rv;
 
 	if (static_cpu_has(X86_FEATURE_HW_PSTATE)) {
-		pr_warn(PFX "this CPU is not supported anymore, using acpi-cpufreq instead.\n");
-		request_module("acpi-cpufreq");
+		pr_warn(PFX "This CPU is now supported by acpi-cpufreq, loading 
+it.\n");
+
+		if (request_module("acpi-cpufreq"))
+			pr_warn(PFX "Error loading acpi-cpufreq, make sure you "
+				"have it enabled, else you won't have any "
+				"Pstates support on this CPU!\n");
 		return -ENODEV;
 	}
 
--
1.8.1.rc3

--
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] drivers/cpufreq: Warn user when powernow-k8 tries to fall back to acpi-cpufreq and it is unavailable.

[Borislav Petkov]

On Fri, Jan 11, 2013 at 07:03:35PM +0000, Gopalakrishnan, Aravind wrote:
> So, I had tried out the case when acpi-cpufreq was compiled into the
> kernel and looked at the return values from request_module(); it
> returns a positive value (255) both when acpi-cpufreq was compiled-in
> and when not compiled-in. (Please do let me know if I am missing
> something here...) (This was the case Andreas had mentioned in the bug
> report too)
>
> It was due to this that I had decided to just check the CONFIG option
> and print out a warning to the user.

Well, when both are built-in, I get -2 from request_module which is most
probably the retval of modprobe with a missing module (I delved deep
into the do_execve bowels but didn't go deep enough).

So, handoff to acpi-cpufreq still has some issues. When both are
built-in, the module_init functions turn into normal initcalls and
in that case, they're executed in link order and it can happen that
powernowk8_init() runs before acpi_cpufreq_init().

In that case, we get -2 (-ENOENT, see above) and even though there's an
error, acpi-cpufreq gets loaded so the error is bogus:

[    2.225413] powernow-k8: This CPU is now supported by acpi-cpufreq, loading it.
[    2.227266] powernow-k8: Error(-2) loading acpi-cpufreq, make sure you have it enabled, else you won't have any Pstates support on this CPU!
[    2.227868] acpi-cpufreq: overriding BIOS provided _PSD data

I still need to think hard about this case and how to fix it.

In the meantime, here's fix for Andreas' issue (as a reply to this
message). Rafael, the patch is trivial, you might want to send it now
and for stable.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

[PATCH] powernow-k8: Add a kconfig dependency on acpi-cpufreq

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>
Date: Thu, 17 Jan 2013 12:44:20 +0100
Subject: [PATCH] powernow-k8: Add a kconfig dependency on acpi-cpufreq

Andreas reports in https://bugzilla.kernel.org/show_bug.cgi?id=51741
that with his Gentoo config, acpi-cpufreq wasn't enabled and powernow-k8
couldn't handoff properly to acpi-cpufreq leading to running without
P-state support (i.e., cores are constantly in P0).

To alleaviate that, we need to make powernow-k8 depend on acpi-cpufreq
so that acpi-cpufreq is always present.

Cc: Andre Przywara <andre@andrep.de>
Reported-by: Andreas <linuxuser330250@gmx.net>
Signed-off-by: Borislav Petkov <bp@suse.de>
---
 drivers/cpufreq/Kconfig.x86 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/Kconfig.x86 b/drivers/cpufreq/Kconfig.x86
index 934854ae5eb4..7227cd734042 100644
--- a/drivers/cpufreq/Kconfig.x86
+++ b/drivers/cpufreq/Kconfig.x86
@@ -106,7 +106,7 @@ config X86_POWERNOW_K7_ACPI
 config X86_POWERNOW_K8
 	tristate "AMD Opteron/Athlon64 PowerNow!"
 	select CPU_FREQ_TABLE
-	depends on ACPI && ACPI_PROCESSOR
+	depends on ACPI && ACPI_PROCESSOR && X86_ACPI_CPUFREQ
 	help
 	  This adds the CPUFreq driver for K8/early Opteron/Athlon64 processors.
 	  Support for K10 and newer processors is now in acpi-cpufreq.
-- 
1.8.1.rc3


-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] powernow-k8: Add a kconfig dependency on acpi-cpufreq

[Rafael J. Wysocki]

On Thursday, January 17, 2013 12:58:44 PM Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> Date: Thu, 17 Jan 2013 12:44:20 +0100
> Subject: [PATCH] powernow-k8: Add a kconfig dependency on acpi-cpufreq
> 
> Andreas reports in https://bugzilla.kernel.org/show_bug.cgi?id=51741
> that with his Gentoo config, acpi-cpufreq wasn't enabled and powernow-k8
> couldn't handoff properly to acpi-cpufreq leading to running without
> P-state support (i.e., cores are constantly in P0).
> 
> To alleaviate that, we need to make powernow-k8 depend on acpi-cpufreq
> so that acpi-cpufreq is always present.

OK

Do we need that in -stable?

Rafael


> Cc: Andre Przywara <andre@andrep.de>
> Reported-by: Andreas <linuxuser330250@gmx.net>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  drivers/cpufreq/Kconfig.x86 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/Kconfig.x86 b/drivers/cpufreq/Kconfig.x86
> index 934854ae5eb4..7227cd734042 100644
> --- a/drivers/cpufreq/Kconfig.x86
> +++ b/drivers/cpufreq/Kconfig.x86
> @@ -106,7 +106,7 @@ config X86_POWERNOW_K7_ACPI
>  config X86_POWERNOW_K8
>  	tristate "AMD Opteron/Athlon64 PowerNow!"
>  	select CPU_FREQ_TABLE
> -	depends on ACPI && ACPI_PROCESSOR
> +	depends on ACPI && ACPI_PROCESSOR && X86_ACPI_CPUFREQ
>  	help
>  	  This adds the CPUFreq driver for K8/early Opteron/Athlon64 processors.
>  	  Support for K10 and newer processors is now in acpi-cpufreq.
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] powernow-k8: Add a kconfig dependency on acpi-cpufreq

[Borislav Petkov]

On Thu, Jan 17, 2013 at 01:53:55PM +0100, Rafael J. Wysocki wrote:
> Do we need that in -stable?

Yes, please.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] powernow-k8: Add a kconfig dependency on acpi-cpufreq

[Rafael J. Wysocki]

On Thursday, January 17, 2013 03:17:58 PM Borislav Petkov wrote:
> On Thu, Jan 17, 2013 at 01:53:55PM +0100, Rafael J. Wysocki wrote:
> > Do we need that in -stable?
> 
> Yes, please.

3.7+ I suppose?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] powernow-k8: Add a kconfig dependency on acpi-cpufreq

[Borislav Petkov]

On Thu, Jan 17, 2013 at 11:10:56PM +0100, Rafael J. Wysocki wrote:
> 3.7+ I suppose?

Yes, the patch adding the request_module() call went in in 3.7:

$ git tag --contains e1f0b8e9b04a2
v3.7
v3.7-rc1
v3.7-rc2
v3.7-rc3
v3.7-rc4
v3.7-rc5
v3.7-rc6
v3.7-rc7
v3.7-rc8
v3.7.1
v3.8-rc1
v3.8-rc2
v3.8-rc3
$

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] drivers/cpufreq: Warn user when powernow-k8 tries to fall back to acpi-cpufreq and it is unavailable.

[Matthew Garrett]

On Thu, Jan 17, 2013 at 12:54:37PM +0100, Borislav Petkov wrote:

> So, handoff to acpi-cpufreq still has some issues. When both are
> built-in, the module_init functions turn into normal initcalls and
> in that case, they're executed in link order and it can happen that
> powernowk8_init() runs before acpi_cpufreq_init().

Just flip the link order? It's only the way it is because in the past we 
wanted to try hardware-specific drivers before more generic ones, and I 
don't think that's a concern in this case now.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] drivers/cpufreq: Warn user when powernow-k8 tries to fall back to acpi-cpufreq and it is unavailable.

[Borislav Petkov]

On Fri, Jan 18, 2013 at 04:23:47PM +0000, Matthew Garrett wrote:
> On Thu, Jan 17, 2013 at 12:54:37PM +0100, Borislav Petkov wrote:
> 
> > So, handoff to acpi-cpufreq still has some issues. When both are
> > built-in, the module_init functions turn into normal initcalls and
> > in that case, they're executed in link order and it can happen that
> > powernowk8_init() runs before acpi_cpufreq_init().
> 
> Just flip the link order? It's only the way it is because in the past we 
> wanted to try hardware-specific drivers before more generic ones, and I 
> don't think that's a concern in this case now.

Yeah, I heard that the acpi-idle and intel-idle drivers do that and also
heard that it was a hack. It doesn't look too ugly IMHO:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=2671717265ae6e720a9ba5f13fbec3a718983b65

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] drivers/cpufreq: Warn user when powernow-k8 tries to fall back to acpi-cpufreq and it is unavailable.

[Borislav Petkov]

On Fri, Jan 18, 2013 at 06:07:55PM +0100, Borislav Petkov wrote:
> > Just flip the link order? It's only the way it is because in the
> > past we wanted to try hardware-specific drivers before more generic
> > ones, and I don't think that's a concern in this case now.
>
> Yeah, I heard that the acpi-idle and intel-idle drivers do that and also
> heard that it was a hack. It doesn't look too ugly IMHO:
>
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=2671717265ae6e720a9ba5f13fbec3a718983b65

Haha, this turns into one of those chicken-or-the-egg problems:

>From <drivers/cpufreq/Makefile>:

##################################################################################
# x86 drivers.
# Link order matters. K8 is preferred to ACPI because of firmware bugs in early
# K8 systems.
...

Great. :(

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] drivers/cpufreq: Warn user when powernow-k8 tries to fall back to acpi-cpufreq and it is unavailable.

[Matthew Garrett]

On Fri, Jan 18, 2013 at 08:00:21PM +0100, Borislav Petkov wrote:

> ##################################################################################
> # x86 drivers.
> # Link order matters. K8 is preferred to ACPI because of firmware bugs in early
> # K8 systems.
> ...
> 
> Great. :(

The only case I can see this hitting would be if the platform is using 
system IO rather than fixed hardware functionality. Easiest thing to do 
there would be something like this:

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 0d048f6..8b466d5 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -762,6 +762,11 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
 
 	switch (perf->control_register.space_id) {
 	case ACPI_ADR_SPACE_SYSTEM_IO:
+		if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
+			pr_debug("Old AMD systems must use native drivers\n");
+			result = -ENODEV;
+			goto err_unreg;
+		}
 		pr_debug("SYSTEM IO addr space\n");
 		data->cpu_feature = SYSTEM_IO_CAPABLE;
 		break;

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] drivers/cpufreq: Warn user when powernow-k8 tries to fall back to acpi-cpufreq and it is unavailable.

[Borislav Petkov]

On Fri, Jan 18, 2013 at 07:06:59PM +0000, Matthew Garrett wrote:
> On Fri, Jan 18, 2013 at 08:00:21PM +0100, Borislav Petkov wrote:
> 
> > ##################################################################################
> > # x86 drivers.
> > # Link order matters. K8 is preferred to ACPI because of firmware bugs in early
> > # K8 systems.
> > ...
> > 
> > Great. :(
> 
> The only case I can see this hitting would be if the platform is using 
> system IO rather than fixed hardware functionality. Easiest thing to do 
> there would be something like this:
> 
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index 0d048f6..8b466d5 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -762,6 +762,11 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
>  
>  	switch (perf->control_register.space_id) {
>  	case ACPI_ADR_SPACE_SYSTEM_IO:
> +		if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
> +			pr_debug("Old AMD systems must use native drivers\n");
> +			result = -ENODEV;
> +			goto err_unreg;
> +		}
>  		pr_debug("SYSTEM IO addr space\n");
>  		data->cpu_feature = SYSTEM_IO_CAPABLE;
>  		break;

Ok, how much can we rely on ACPI to have this ACPI_ADR_SPACE_SYSTEM_IO
properly set on K8? Because the thing is, we want to use acpi-cpufreq on
F10h onwards and leave powernow-k8 to K8s.

So we're purely relying on ->control_register.space_id being
ACPI_ADR_SPACE_SYSTEM_IO on *all* K8s. And with the history BIOS has, I
don't trust it a single bit. Other than that, this could work...

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] drivers/cpufreq: Warn user when powernow-k8 tries to fall back to acpi-cpufreq and it is unavailable.

[Matthew Garrett]

On Fri, Jan 18, 2013 at 08:36:56PM +0100, Borislav Petkov wrote:

> Ok, how much can we rely on ACPI to have this ACPI_ADR_SPACE_SYSTEM_IO
> properly set on K8? Because the thing is, we want to use acpi-cpufreq on
> F10h onwards and leave powernow-k8 to K8s.

SYSTEM_IO only supports single processors and was superceded in ACPI 
2.0. Are there any single-core F10h?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] drivers/cpufreq: Warn user when powernow-k8 tries to fall back to acpi-cpufreq and it is unavailable.

[Borislav Petkov]

On Fri, Jan 18, 2013 at 07:38:34PM +0000, Matthew Garrett wrote:
> On Fri, Jan 18, 2013 at 08:36:56PM +0100, Borislav Petkov wrote:
> 
> > Ok, how much can we rely on ACPI to have this ACPI_ADR_SPACE_SYSTEM_IO
> > properly set on K8? Because the thing is, we want to use acpi-cpufreq on
> > F10h onwards and leave powernow-k8 to K8s.
> 
> SYSTEM_IO only supports single processors and was superceded in ACPI 
> 2.0. Are there any single-core F10h?

Hmm, maybe some downcored creations, who knows? But actually, that
doesn't matter: we want to exit early from acpi-cpufreq loading on K8
only so the check should simply be:

	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
	    boot_cpu_data.x86 == 0xf) {
		pr_debug(...)
		...
	}

Yeah, I think that should work. I'll add your patch to the others and
make a patchset to give it a run. If it looks good, we'll queue it for
3.9 so that it can see more testing.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--