[PATCH] powernow-k8: depend on acpi-processor for SMP systems

[PATCH] powernow-k8: depend on acpi-processor for SMP systems

[Daniel Drake]

powernow-k8 uses PSB BIOS tables to read frequency info on UP systems, but
on SMP it requires the acpi-processor driver. Kconfig should be updated
accordingly to avoid the issues that users are running into.

http://bugzilla.kernel.org/show_bug.cgi?id=8075
https://bugs.gentoo.org/show_bug.cgi?id=178585

Signed-off-by: Daniel Drake <dsd@gentoo.org>

Index: linux/arch/i386/kernel/cpu/cpufreq/Kconfig
===================================================================
--- linux.orig/arch/i386/kernel/cpu/cpufreq/Kconfig
+++ linux/arch/i386/kernel/cpu/cpufreq/Kconfig
@@ -81,6 +81,7 @@ config X86_POWERNOW_K7_ACPI
 config X86_POWERNOW_K8
 	tristate "AMD Opteron/Athlon64 PowerNow!"
 	select CPU_FREQ_TABLE
+	select ACPI_PROCESSOR if SMP
 	depends on EXPERIMENTAL
 	help
 	  This adds the CPUFreq driver for mobile AMD Opteron/Athlon64 processors.

Re: [PATCH] powernow-k8: depend on acpi-processor for SMP systems

[Dave Jones]

On Thu, May 17, 2007 at 12:50:50AM +0100, Daniel Drake wrote:
 > powernow-k8 uses PSB BIOS tables to read frequency info on UP systems, but
 > on SMP it requires the acpi-processor driver. Kconfig should be updated
 > accordingly to avoid the issues that users are running into.
 > 
 > http://bugzilla.kernel.org/show_bug.cgi?id=8075
 > https://bugs.gentoo.org/show_bug.cgi?id=178585

looks ok to me, but I'd like someone who has been seeing problems
to confirm this works first.

	Dave

-- 
http://www.codemonkey.org.uk

Re: [PATCH] powernow-k8: depend on acpi-processor for SMP systems

[Joshua Hoblitt]

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

I don't think this is quiet right either as Ed Sweetman has reported
that this issue doesn't occur on single socket/multi-core systems.

-J

--
On Thu, May 17, 2007 at 12:50:50AM +0100, Daniel Drake wrote:
> powernow-k8 uses PSB BIOS tables to read frequency info on UP systems, but
> on SMP it requires the acpi-processor driver. Kconfig should be updated
> accordingly to avoid the issues that users are running into.
> 
> http://bugzilla.kernel.org/show_bug.cgi?id=8075
> https://bugs.gentoo.org/show_bug.cgi?id=178585
> 
> Signed-off-by: Daniel Drake <dsd@gentoo.org>
> 
> Index: linux/arch/i386/kernel/cpu/cpufreq/Kconfig
> ===================================================================
> --- linux.orig/arch/i386/kernel/cpu/cpufreq/Kconfig
> +++ linux/arch/i386/kernel/cpu/cpufreq/Kconfig
> @@ -81,6 +81,7 @@ config X86_POWERNOW_K7_ACPI
>  config X86_POWERNOW_K8
>  	tristate "AMD Opteron/Athlon64 PowerNow!"
>  	select CPU_FREQ_TABLE
> +	select ACPI_PROCESSOR if SMP
>  	depends on EXPERIMENTAL
>  	help
>  	  This adds the CPUFreq driver for mobile AMD Opteron/Athlon64 processors.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] powernow-k8: depend on acpi-processor for SMP systems

[Dave Jones]

On Wed, May 16, 2007 at 02:26:14PM -1000, Joshua Hoblitt wrote:
 > I don't think this is quiet right either as Ed Sweetman has reported
 > that this issue doesn't occur on single socket/multi-core systems.

I'm not sure why [*], because this should be preventing it..

                if (num_online_cpus() != 1) {
                        printk(KERN_ERR PFX "MP systems not supported by PSB BIOS structure\n");
                        kfree(data);
                        return -ENODEV;
                }

num_online_cpus will return 2 in a dual-core system, even though there's
just one socket.  Given they share a power plane, if there's a valid
PSB structure however, it may be usable.  Though this isn't necessarily
true for all future dual-core AMD CPUs, and the ACPI tables really
should be preferred.

	Dave

[*] unless you have the second core disabled or CONFIG_SMP=n

-- 
http://www.codemonkey.org.uk

Re: [PATCH] powernow-k8: depend on acpi-processor for SMP systems

[Daniel Drake]

Joshua Hoblitt wrote:
> I don't think this is quiet right either as Ed Sweetman has reported
> that this issue doesn't occur on single socket/multi-core systems.

Where did he write that? In an off-list mail, Ed seemed to agree with my 
patch.

Daniel

Re: [PATCH] powernow-k8: depend on acpi-processor for SMP systems

[Ed Sweetman]

Daniel Drake wrote:
> Joshua Hoblitt wrote:
>> I don't think this is quiet right either as Ed Sweetman has reported
>> that this issue doesn't occur on single socket/multi-core systems.
>
> Where did he write that? In an off-list mail, Ed seemed to agree with 
> my patch.
>
> Daniel
>
>
What i didn't agree with was the dependency on the acpi P-state driver 
for single socket multi-core systems, where in the original post of this 
thread, Joshua was stating that smp systems required that driver.   
Later it was found that the acpi p-state driver was only being used to 
enforce the dependency on the acpi_processor driver ...which is the 
actual driver we care about (dependency wise).  

So yes, I do agree with your patch, in so far as my experience with the 
hardware.

Re: [PATCH] powernow-k8: depend on acpi-processor for SMP systems

[Pavel Machek]

Hi!

> powernow-k8 uses PSB BIOS tables to read frequency info on UP systems, but
> on SMP it requires the acpi-processor driver. Kconfig should be updated
> accordingly to avoid the issues that users are running into.
> 
> http://bugzilla.kernel.org/show_bug.cgi?id=8075
> https://bugs.gentoo.org/show_bug.cgi?id=178585
> 
> Signed-off-by: Daniel Drake <dsd@gentoo.org>
> 
> Index: linux/arch/i386/kernel/cpu/cpufreq/Kconfig
> ===================================================================
> --- linux.orig/arch/i386/kernel/cpu/cpufreq/Kconfig
> +++ linux/arch/i386/kernel/cpu/cpufreq/Kconfig
> @@ -81,6 +81,7 @@ config X86_POWERNOW_K7_ACPI
>  config X86_POWERNOW_K8
>  	tristate "AMD Opteron/Athlon64 PowerNow!"
>  	select CPU_FREQ_TABLE
> +	select ACPI_PROCESSOR if SMP
>  	depends on EXPERIMENTAL
>  	help
>  	  This adds the CPUFreq driver for mobile AMD Opteron/Athlon64 processors.

It is still possible to run SMP kernel on UP machine -- so this sounds
wrong.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] powernow-k8: depend on acpi-processor for SMP systems

[Ed Sweetman]

Pavel Machek wrote:
> Hi!
>
>   
>> powernow-k8 uses PSB BIOS tables to read frequency info on UP systems, but
>> on SMP it requires the acpi-processor driver. Kconfig should be updated
>> accordingly to avoid the issues that users are running into.
>>
>> http://bugzilla.kernel.org/show_bug.cgi?id=8075
>> https://bugs.gentoo.org/show_bug.cgi?id=178585
>>
>> Signed-off-by: Daniel Drake <dsd@gentoo.org>
>>
>> Index: linux/arch/i386/kernel/cpu/cpufreq/Kconfig
>> ===================================================================
>> --- linux.orig/arch/i386/kernel/cpu/cpufreq/Kconfig
>> +++ linux/arch/i386/kernel/cpu/cpufreq/Kconfig
>> @@ -81,6 +81,7 @@ config X86_POWERNOW_K7_ACPI
>>  config X86_POWERNOW_K8
>>  	tristate "AMD Opteron/Athlon64 PowerNow!"
>>  	select CPU_FREQ_TABLE
>> +	select ACPI_PROCESSOR if SMP
>>  	depends on EXPERIMENTAL
>>  	help
>>  	  This adds the CPUFreq driver for mobile AMD Opteron/Athlon64 processors.
>>     
>
> It is still possible to run SMP kernel on UP machine -- so this sounds
> wrong.
>
>   

It's possible to run a kernel with drivers for hardware you dont have.  
But not having the dependencies correct for those drivers for that 
hardware you dont have just because it's possible you can run a kernel 
with those drivers without that hardware isn't correct. Got it?

if an SMP K8 system needs ACPI_PROCESSOR for the cpufreq driver, then we 
need to make this dependency,  just because you can configure your 
kernel to include drivers you dont need (like smp on a up system) 
doesn't mean that dependency is wrong,  it just means you've configured 
your system with drivers you dont need.  Looking up the acpi table on a 
UP system probably wont break anything, since the driver should check if 
it's not able to and fallback to non-acpi methods before failing 
completley.  If it doesn't then this patch should be fine.  Otherwise 
we'd need to add a check in the driver to see if we're actually running 
in an smp configuration and use the correct method. 


The Kconfig dependency would still be valid in either case.  If the 
lookup for acpi tables fails with no fallback to the non-acpi way of 
using the cpufreq driver, then we have to fix the driver.

Re: [PATCH] powernow-k8: depend on acpi-processor for SMP systems

[Len Brown]

On Wednesday 16 May 2007 19:50, Daniel Drake wrote:
> powernow-k8 uses PSB BIOS tables to read frequency info on UP systems, but
> on SMP it requires the acpi-processor driver. Kconfig should be updated
> accordingly to avoid the issues that users are running into.
> 
> http://bugzilla.kernel.org/show_bug.cgi?id=8075
> https://bugs.gentoo.org/show_bug.cgi?id=178585
> 
> Signed-off-by: Daniel Drake <dsd@gentoo.org>
> 
> Index: linux/arch/i386/kernel/cpu/cpufreq/Kconfig
> ===================================================================
> --- linux.orig/arch/i386/kernel/cpu/cpufreq/Kconfig
> +++ linux/arch/i386/kernel/cpu/cpufreq/Kconfig
> @@ -81,6 +81,7 @@ config X86_POWERNOW_K7_ACPI
>  config X86_POWERNOW_K8
>  	tristate "AMD Opteron/Athlon64 PowerNow!"
>  	select CPU_FREQ_TABLE
> +	select ACPI_PROCESSOR if SMP
>  	depends on EXPERIMENTAL
>  	help
>  	  This adds the CPUFreq driver for mobile AMD Opteron/Athlon64 processors.

Unfortunately this patch will not actually enable ACPI_PROCESSOR in
the SMP=y ACPI=n case.  "select" doesn't work for targets that
have dependencies.

-Len

Re: [PATCH] powernow-k8: depend on acpi-processor for SMP systems

[Dave Jones]

On Thu, May 17, 2007 at 02:13:42PM -0400, Len Brown wrote:

 > > Index: linux/arch/i386/kernel/cpu/cpufreq/Kconfig
 > > ===================================================================
 > > --- linux.orig/arch/i386/kernel/cpu/cpufreq/Kconfig
 > > +++ linux/arch/i386/kernel/cpu/cpufreq/Kconfig
 > > @@ -81,6 +81,7 @@ config X86_POWERNOW_K7_ACPI
 > >  config X86_POWERNOW_K8
 > >  	tristate "AMD Opteron/Athlon64 PowerNow!"
 > >  	select CPU_FREQ_TABLE
 > > +	select ACPI_PROCESSOR if SMP
 > >  	depends on EXPERIMENTAL
 > >  	help
 > >  	  This adds the CPUFreq driver for mobile AMD Opteron/Athlon64 processors.
 > 
 > Unfortunately this patch will not actually enable ACPI_PROCESSOR in
 > the SMP=y ACPI=n case.  "select" doesn't work for targets that
 > have dependencies.

I don't think we can fix this perfectly tbh, but the above at
least gets us close for the majority of users.

Are there many x86-64 users that don't enable acpi ?

	Dave

-- 
http://www.codemonkey.org.uk

Re: [PATCH] powernow-k8: depend on acpi-processor for SMP systems

[Ed Sweetman]

Dave Jones wrote:
> On Thu, May 17, 2007 at 02:13:42PM -0400, Len Brown wrote:
>
>  > > Index: linux/arch/i386/kernel/cpu/cpufreq/Kconfig
>  > > ===================================================================
>  > > --- linux.orig/arch/i386/kernel/cpu/cpufreq/Kconfig
>  > > +++ linux/arch/i386/kernel/cpu/cpufreq/Kconfig
>  > > @@ -81,6 +81,7 @@ config X86_POWERNOW_K7_ACPI
>  > >  config X86_POWERNOW_K8
>  > >  	tristate "AMD Opteron/Athlon64 PowerNow!"
>  > >  	select CPU_FREQ_TABLE
>  > > +	select ACPI_PROCESSOR if SMP
>  > >  	depends on EXPERIMENTAL
>  > >  	help
>  > >  	  This adds the CPUFreq driver for mobile AMD Opteron/Athlon64 processors.
>  > 
>  > Unfortunately this patch will not actually enable ACPI_PROCESSOR in
>  > the SMP=y ACPI=n case.  "select" doesn't work for targets that
>  > have dependencies.
>
> I don't think we can fix this perfectly tbh, but the above at
> least gets us close for the majority of users.
>
> Are there many x86-64 users that don't enable acpi ?
>
> 	Dave
>
>   
I've just always compiled acpi_processor in, it's only logical that if 
you are using a power management feature, that you compile in the power 
management interface, and if your stuff deals directly with the cpu, you 
may want to compile the acpi_processor driver in.  The only reason I 
knew to do that though, was because i go through each option.  Someone 
else looking to just enable cpufreq, would skip the sub-drivers of ACPI, 
and never know better. We dont suggest anywhere in the cpufreq driver, 
we dont mention restrictions or limits of the driver without acpi, and 
we certainly dont select it or make it dependent (except silently and 
invisibly to the user). 

Every other cpufreq driver demands acpi.  In windows you have to have 
acpi, the p states are called acpi p states everywhere.  The problem 
here is that the author to the powernow_k8 driver found a way to get 
some cpufreq functionality without acpi. 

So to make everyone happy, maybe we should have the silently 
selected/deselected driver exposed to the user, as a sub-driver. 

->  Powernow K8 / athlon64  cpufreq driver     y/m/n
------->    ACPI support      y/m/n


we do this silently in the Kconfig, i dont see the problem with exposing 
it so we dont have to muck around forcing people to use acpi that dont 
need to and selecting drivers they may not really need. 

Re: [PATCH] powernow-k8: depend on acpi-processor for SMP systems

[Ed Sweetman]

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

Ed Sweetman wrote:
> Dave Jones wrote:
>> On Thu, May 17, 2007 at 02:13:42PM -0400, Len Brown wrote:
>>
>>  > > Index: linux/arch/i386/kernel/cpu/cpufreq/Kconfig
>>  > > ===================================================================
>>  > > --- linux.orig/arch/i386/kernel/cpu/cpufreq/Kconfig
>>  > > +++ linux/arch/i386/kernel/cpu/cpufreq/Kconfig
>>  > > @@ -81,6 +81,7 @@ config X86_POWERNOW_K7_ACPI
>>  > >  config X86_POWERNOW_K8
>>  > >      tristate "AMD Opteron/Athlon64 PowerNow!"
>>  > >      select CPU_FREQ_TABLE
>>  > > +    select ACPI_PROCESSOR if SMP
>>  > >      depends on EXPERIMENTAL
>>  > >      help
>>  > >        This adds the CPUFreq driver for mobile AMD 
>> Opteron/Athlon64 processors.
>>  >  > Unfortunately this patch will not actually enable 
>> ACPI_PROCESSOR in
>>  > the SMP=y ACPI=n case.  "select" doesn't work for targets that
>>  > have dependencies.
>>
>> I don't think we can fix this perfectly tbh, but the above at
>> least gets us close for the majority of users.
>>
>> Are there many x86-64 users that don't enable acpi ?
>>
>>     Dave
>>
>>   
> I've just always compiled acpi_processor in, it's only logical that if 
> you are using a power management feature, that you compile in the 
> power management interface, and if your stuff deals directly with the 
> cpu, you may want to compile the acpi_processor driver in.  The only 
> reason I knew to do that though, was because i go through each 
> option.  Someone else looking to just enable cpufreq, would skip the 
> sub-drivers of ACPI, and never know better. We dont suggest anywhere 
> in the cpufreq driver, we dont mention restrictions or limits of the 
> driver without acpi, and we certainly dont select it or make it 
> dependent (except silently and invisibly to the user).
> Every other cpufreq driver demands acpi.  In windows you have to have 
> acpi, the p states are called acpi p states everywhere.  The problem 
> here is that the author to the powernow_k8 driver found a way to get 
> some cpufreq functionality without acpi.
> So to make everyone happy, maybe we should have the silently 
> selected/deselected driver exposed to the user, as a sub-driver.
> ->  Powernow K8 / athlon64  cpufreq driver     y/m/n
> ------->    ACPI support      y/m/n
>

Here's a patch


[-- Attachment #2: powernow.patch --]
[-- Type: text/x-patch, Size: 682 bytes --]

--- ./linux-backup/arch/x86_64/kernel/cpufreq/Kconfig	2007-02-04 13:44:54.000000000 -0500
+++ ./linux-2.6.21-rc5-mm2/arch/x86_64/kernel/cpufreq/Kconfig	2007-05-17 17:37:24.000000000 -0400
@@ -10,7 +10,7 @@
 
 comment "CPUFreq processor drivers"
 
-config X86_POWERNOW_K8
+config  X86_POWERNOW_K8
 	tristate "AMD Opteron/Athlon64 PowerNow!"
 	select CPU_FREQ_TABLE
 	help
@@ -21,10 +21,10 @@
 	  If in doubt, say N.
 
 config X86_POWERNOW_K8_ACPI
-	bool
+	tristate "ACPI support"
 	depends on X86_POWERNOW_K8 && ACPI_PROCESSOR
 	depends on !(X86_POWERNOW_K8 = y && ACPI_PROCESSOR = m)
-	default y
+
 
 config X86_SPEEDSTEP_CENTRINO
 	tristate "Intel Enhanced SpeedStep (deprecated)"

Re: [PATCH] powernow-k8: depend on acpi-processor for SMP systems

[Dave Jones]

On Thu, May 17, 2007 at 05:29:04PM -0400, Ed Sweetman wrote:

 > Every other cpufreq driver demands acpi

This isn't true.

	Dave

-- 
http://www.codemonkey.org.uk

Re: [PATCH] powernow-k8: depend on acpi-processor for SMP systems

[Dave Jones]

On Thu, May 17, 2007 at 05:40:31PM -0400, Ed Sweetman wrote:

 > Here's a patch

(please inline patches so they can be quoted in replies).

having this as a tristate makes no sense, that code can't be modular.
Also, there's a gratuitous whitespace change, and the default should
probably stay.

If there's consensus we should make this a user-visible option,
we can do that, though I still don't think this is any more
perfect than the other option that Daniel posted.

	Dave

-- 
http://www.codemonkey.org.uk

Re: [PATCH] powernow-k8: depend on acpi-processor for SMP systems

[Ed Sweetman]

Dave Jones wrote:
> On Thu, May 17, 2007 at 05:40:31PM -0400, Ed Sweetman wrote:
>
>  > Here's a patch
>
> (please inline patches so they can be quoted in replies).
>
> having this as a tristate makes no sense, that code can't be modular.
> Also, there's a gratuitous whitespace change, and the default should
> probably stay.
>
> If there's consensus we should make this a user-visible option,
> we can do that, though I still don't think this is any more
> perfect than the other option that Daniel posted.
>
> 	Dave
>
>   
yes i know, i rushed it before dinner.  maybe this one?

--- ./linux-backup/arch/x86_64/kernel/cpufreq/Kconfig 2007-02-04 
13:44:54.000000000 -0500
+++ ./linux-2.6.21-rc5-mm2/arch/x86_64/kernel/cpufreq/Kconfig 2007-05-17 
18:13:07.000000000 -0400
@@ -10,20 +10,27 @@
 
comment "CPUFreq processor drivers"
 
-config X86_POWERNOW_K8
+config  X86_POWERNOW_K8
 tristate "AMD Opteron/Athlon64 PowerNow!"
 select CPU_FREQ_TABLE
 help
   This adds the CPUFreq driver for mobile AMD Opteron/Athlon64 processors.
+   An acpi interface is available if acpi support has been selected.
+   This is required for multi-socket and other systems but not 
necessarily required for UP single socket systems.
 
   For details, take a look at <file:Documentation/cpu-freq/>.
 
   If in doubt, say N.
 
config X86_POWERNOW_K8_ACPI
- bool
- depends on X86_POWERNOW_K8 && ACPI_PROCESSOR
- depends on !(X86_POWERNOW_K8 = y && ACPI_PROCESSOR = m)
+ bool "ACPI Support"
+ select ACPI_PROCESSOR
+ depends on X86_POWERNOW_K8
+ help
+     This provides access to the acpi tables for full p-state 
functionality. This driver is also required
+     for cpufreq to work with multi-socket and other smp systems.
+     
+     It is safe to say Y here.  
 default y
 
config X86_SPEEDSTEP_CENTRINO

Re: [PATCH] powernow-k8: depend on acpi-processor for SMP systems

[Pavel Machek]

On Thu 2007-05-17 06:24:22, Ed Sweetman wrote:
> Pavel Machek wrote:
> >Hi!
> >
> >  
> >>powernow-k8 uses PSB BIOS tables to read frequency info on UP systems, but
> >>on SMP it requires the acpi-processor driver. Kconfig should be updated
> >>accordingly to avoid the issues that users are running into.
> >>
> >>http://bugzilla.kernel.org/show_bug.cgi?id=8075
> >>https://bugs.gentoo.org/show_bug.cgi?id=178585
> >>
> >>Signed-off-by: Daniel Drake <dsd@gentoo.org>
> >>
> >>Index: linux/arch/i386/kernel/cpu/cpufreq/Kconfig
> >>===================================================================
> >>--- linux.orig/arch/i386/kernel/cpu/cpufreq/Kconfig
> >>+++ linux/arch/i386/kernel/cpu/cpufreq/Kconfig
> >>@@ -81,6 +81,7 @@ config X86_POWERNOW_K7_ACPI
> >> config X86_POWERNOW_K8
> >> 	tristate "AMD Opteron/Athlon64 PowerNow!"
> >> 	select CPU_FREQ_TABLE
> >>+	select ACPI_PROCESSOR if SMP
> >> 	depends on EXPERIMENTAL
> >> 	help
> >> 	  This adds the CPUFreq driver for mobile AMD Opteron/Athlon64 
> >> 	  processors.
> >>    
> >
> >It is still possible to run SMP kernel on UP machine -- so this sounds
> >wrong.
> 
> It's possible to run a kernel with drivers for hardware you dont have.  
> But not having the dependencies correct for those drivers for that 
> hardware you dont have just because it's possible you can run a kernel 
> with those drivers without that hardware isn't correct. Got it?

Breaking working configs is not fine, got it? We are having the thing
called "stable release" here. Feel free to fix code to decide during
runtime.

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html