2007-05-16 23:56:17

by Daniel Drake

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

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 <[email protected]>

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.


2007-05-17 00:04:06

by Dave Jones

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

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

2007-05-17 00:20:56

by Joshua Hoblitt

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

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 <[email protected]>
>
> 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.


Attachments:
(No filename) (1.07 kB)
(No filename) (189.00 B)
Download all attachments

2007-05-17 00:38:17

by Dave Jones

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

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

2007-05-17 00:55:38

by Daniel Drake

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

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

2007-05-17 01:09:17

by Ed Sweetman

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

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.


2007-05-17 09:07:29

by Pavel Machek

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

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 <[email protected]>
>
> 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

2007-05-17 10:29:43

by Ed Sweetman

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

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 <[email protected]>
>>
>> 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.


2007-05-17 18:15:26

by Len Brown

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

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 <[email protected]>
>
> 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

2007-05-17 18:25:18

by Dave Jones

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

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

2007-05-17 21:29:41

by Ed Sweetman

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

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.

2007-05-17 21:40:57

by Ed Sweetman

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

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


Attachments:
powernow.patch (682.00 B)

2007-05-17 21:44:57

by Dave Jones

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

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

2007-05-17 21:53:28

by Dave Jones

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

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

2007-05-17 22:16:32

by Ed Sweetman

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

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


2007-06-04 10:53:13

by Pavel Machek

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

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 <[email protected]>
> >>
> >>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