Impact: fix current->cpus_allowed overwriting.
on one AMD Fam10h SMP system:
when checking numactl output, when acpi=off
found output is not right.
# numactl --show
policy: default
preferred node: current
physcpubind: 5 7 10 11 12 13
cpubind: 1 2 3
nodebind: 1 2 3
membind: 0 1 2 3
it turns out in powernowk8_cpu_init there is path ( ACPI is off or _PSS is not there)
it will try to overwrite current->cpus_allowed with uninitialized oldmask.
caused by
| commit 2fdf66b491ac706657946442789ec644cc317e1a
| Author: Rusty Russell <[email protected]>
| Date: Wed Dec 31 18:08:47 2008 -0800
|
| cpumask: convert shared_cpu_map in acpi_processor* structs to cpumask_var_t
need to get oldmask early.
also remove some wrong warning when acpi is disabled.
and don't call exit_acpi if _PSS is not found.
with patch get numactl correct.
# numactl --show
policy: default
preferred node: current
physcpubind: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
cpubind: 0 1 2 3
nodebind: 0 1 2 3
membind: 0 1 2 3
Signed-off-by: Yinghai Lu <[email protected]>
---
arch/x86/kernel/cpu/cpufreq/powernow-k8.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
Index: linux-2.6/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
+++ linux-2.6/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
@@ -1124,6 +1124,7 @@ static int powernowk8_verify(struct cpuf
static int __cpuinit powernowk8_cpu_init(struct cpufreq_policy *pol)
{
struct powernow_k8_data *data;
+ int k8_cpu_acpi_inited = 0;
cpumask_t oldmask;
int rc;
@@ -1141,14 +1142,19 @@ static int __cpuinit powernowk8_cpu_init
data->cpu = pol->cpu;
data->currpstate = HW_PSTATE_INVALID;
+ oldmask = current->cpus_allowed;
rc = powernow_k8_cpu_init_acpi(data);
- if (rc) {
+ if (!rc) {
+ k8_cpu_acpi_inited = 1;
+ } else {
/*
* Use the PSB BIOS structure. This is only availabe on
* an UP version, and is deprecated by AMD.
*/
if (num_online_cpus() != 1) {
+ if (acpi_disabled)
+ goto err_out;
#ifndef CONFIG_ACPI_PROCESSOR
printk(KERN_ERR PFX "ACPI Processor support is required "
"for SMP systems but is absent. Please load the "
@@ -1164,6 +1170,8 @@ static int __cpuinit powernowk8_cpu_init
goto err_out;
}
if (pol->cpu != 0) {
+ if (acpi_disabled)
+ goto err_out;
printk(KERN_ERR FW_BUG PFX "No ACPI _PSS objects for "
"CPU other than CPU0. Complain to your BIOS "
"vendor.\n");
@@ -1176,7 +1184,6 @@ static int __cpuinit powernowk8_cpu_init
}
/* only run on specific CPU from here on */
- oldmask = current->cpus_allowed;
set_cpus_allowed_ptr(current, &cpumask_of_cpu(pol->cpu));
if (smp_processor_id() != pol->cpu) {
@@ -1218,7 +1225,8 @@ static int __cpuinit powernowk8_cpu_init
/* min/max the cpu is capable of */
if (cpufreq_frequency_table_cpuinfo(pol, data->powernow_table)) {
printk(KERN_ERR FW_BUG PFX "invalid powernow_table\n");
- powernow_k8_cpu_exit_acpi(data);
+ if (k8_cpu_acpi_inited)
+ powernow_k8_cpu_exit_acpi(data);
kfree(data->powernow_table);
kfree(data);
return -EINVAL;
@@ -1238,7 +1246,8 @@ static int __cpuinit powernowk8_cpu_init
err_out:
set_cpus_allowed_ptr(current, &oldmask);
- powernow_k8_cpu_exit_acpi(data);
+ if (k8_cpu_acpi_inited)
+ powernow_k8_cpu_exit_acpi(data);
kfree(data);
return -ENODEV;
On Thursday 29 January 2009 15:26:28 Yinghai Lu wrote:
>
> Impact: fix current->cpus_allowed overwriting.
...
> caused by
> | commit 2fdf66b491ac706657946442789ec644cc317e1a
> | Author: Rusty Russell <[email protected]>
> | Date: Wed Dec 31 18:08:47 2008 -0800
> |
> | cpumask: convert shared_cpu_map in acpi_processor* structs to cpumask_var_t
Hi Yinghai!
Indeed; Mike, that contained a bad conversion of
arch/x86/kernel/cpu/cpufreq/powernow-k8.c.
This patch reverts the bad change (this code should still be converted
to work_on_cpu of course, but that's a bigger change).
Yinghai, as far as I can tell your other fixes are independent (and
were a problem before this commit, yes?). Can you re-submit them on top
of this patch (which is for Ingo, but against latest Linus).
Subject: cpumask: fix powernow-k8: partial revert of 2fdf66b491ac706657946442789ec644cc317e1a
Impact: fix powernow-k8 when acpi=off (or other error).
There was a spurious change introduced into powernow-k8 in this patch:
the cause if that we try to "restore" the cpus_allowed we never saved.
See lkml "[PATCH] x86/powernow: fix cpus_allowed brokage when
acpi=off" from Yinghai for the bug report.
Cc: Mike Travis <[email protected]>
Cc: Yinghai Lu <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
diff --git a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
--- a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
+++ b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
@@ -1142,8 +1142,7 @@ static int __cpuinit powernowk8_cpu_init
data->cpu = pol->cpu;
data->currpstate = HW_PSTATE_INVALID;
- rc = powernow_k8_cpu_init_acpi(data);
- if (rc) {
+ if (powernow_k8_cpu_init_acpi(data)) {
/*
* Use the PSB BIOS structure. This is only availabe on
* an UP version, and is deprecated by AMD.
@@ -1161,17 +1160,20 @@ static int __cpuinit powernowk8_cpu_init
"ACPI maintainers and complain to your BIOS "
"vendor.\n");
#endif
- goto err_out;
+ kfree(data);
+ return -ENODEV;
}
if (pol->cpu != 0) {
printk(KERN_ERR FW_BUG PFX "No ACPI _PSS objects for "
"CPU other than CPU0. Complain to your BIOS "
"vendor.\n");
- goto err_out;
+ kfree(data);
+ return -ENODEV;
}
rc = find_psb_table(data);
if (rc) {
- goto err_out;
+ kfree(data);
+ return -ENODEV;
}
}
Rusty Russell wrote:
> On Thursday 29 January 2009 15:26:28 Yinghai Lu wrote:
>> Impact: fix current->cpus_allowed overwriting.
> ...
>> caused by
>> | commit 2fdf66b491ac706657946442789ec644cc317e1a
>> | Author: Rusty Russell <[email protected]>
>> | Date: Wed Dec 31 18:08:47 2008 -0800
>> |
>> | cpumask: convert shared_cpu_map in acpi_processor* structs to cpumask_var_t
>
> Hi Yinghai!
>
> Indeed; Mike, that contained a bad conversion of
> arch/x86/kernel/cpu/cpufreq/powernow-k8.c.
>
> This patch reverts the bad change (this code should still be converted
> to work_on_cpu of course, but that's a bigger change).
>
> Yinghai, as far as I can tell your other fixes are independent (and
> were a problem before this commit, yes?). Can you re-submit them on top
> of this patch (which is for Ingo, but against latest Linus).
no problem, after ingo pick your patch in tip/master, i will have another patch.
YH
>
> Subject: cpumask: fix powernow-k8: partial revert of 2fdf66b491ac706657946442789ec644cc317e1a
>
> Impact: fix powernow-k8 when acpi=off (or other error).
>
> There was a spurious change introduced into powernow-k8 in this patch:
> the cause if that we try to "restore" the cpus_allowed we never saved.
>
> See lkml "[PATCH] x86/powernow: fix cpus_allowed brokage when
> acpi=off" from Yinghai for the bug report.
>
* Rusty Russell <[email protected]> wrote:
> On Thursday 29 January 2009 15:26:28 Yinghai Lu wrote:
> >
> > Impact: fix current->cpus_allowed overwriting.
> ...
> > caused by
> > | commit 2fdf66b491ac706657946442789ec644cc317e1a
> > | Author: Rusty Russell <[email protected]>
> > | Date: Wed Dec 31 18:08:47 2008 -0800
> > |
> > | cpumask: convert shared_cpu_map in acpi_processor* structs to cpumask_var_t
>
> Hi Yinghai!
>
> Indeed; Mike, that contained a bad conversion of
> arch/x86/kernel/cpu/cpufreq/powernow-k8.c.
>
> This patch reverts the bad change (this code should still be converted
> to work_on_cpu of course, but that's a bigger change).
>
> Yinghai, as far as I can tell your other fixes are independent (and
> were a problem before this commit, yes?). Can you re-submit them on top
> of this patch (which is for Ingo, but against latest Linus).
>
> Subject: cpumask: fix powernow-k8: partial revert of 2fdf66b491ac706657946442789ec644cc317e1a
>
> Impact: fix powernow-k8 when acpi=off (or other error).
>
> There was a spurious change introduced into powernow-k8 in this patch:
> the cause if that we try to "restore" the cpus_allowed we never saved.
>
> See lkml "[PATCH] x86/powernow: fix cpus_allowed brokage when
> acpi=off" from Yinghai for the bug report.
>
> Cc: Mike Travis <[email protected]>
> Cc: Yinghai Lu <[email protected]>
> Signed-off-by: Rusty Russell <[email protected]>
applied to tip/x86/urgent, thanks guys!
Dave, is it fine if we send the fix to Linus via this route - or would you
like to do it yourself?
Ingo
On Fri, Jan 30, 2009 at 04:17:01PM +0100, Ingo Molnar wrote:
> > Yinghai, as far as I can tell your other fixes are independent (and
> > were a problem before this commit, yes?). Can you re-submit them on top
> > of this patch (which is for Ingo, but against latest Linus).
> >
> > Subject: cpumask: fix powernow-k8: partial revert of 2fdf66b491ac706657946442789ec644cc317e1a
> >
> > Impact: fix powernow-k8 when acpi=off (or other error).
> >
> > There was a spurious change introduced into powernow-k8 in this patch:
> > the cause if that we try to "restore" the cpus_allowed we never saved.
> >
> > See lkml "[PATCH] x86/powernow: fix cpus_allowed brokage when
> > acpi=off" from Yinghai for the bug report.
> >
> > Cc: Mike Travis <[email protected]>
> > Cc: Yinghai Lu <[email protected]>
> > Signed-off-by: Rusty Russell <[email protected]>
>
> applied to tip/x86/urgent, thanks guys!
>
> Dave, is it fine if we send the fix to Linus via this route - or would you
> like to do it yourself?
I thought this was only in your tree, so I was letting you do the work anyway :-)
I'm fine with it, as long as it goes in soon.
Dave
--
http://www.codemonkey.org.uk
Impact: cleanup
remove some wrong warning when acpi is disabled.
and don't call exit_acpi if _PSS is not found.
Signed-off-by: Yinghai Lu <[email protected]>
---
arch/x86/kernel/cpu/cpufreq/powernow-k8.c | 53 ++++++++++++++++--------------
1 file changed, 29 insertions(+), 24 deletions(-)
Index: linux-2.6/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
+++ linux-2.6/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
@@ -1124,6 +1124,7 @@ static int powernowk8_verify(struct cpuf
static int __cpuinit powernowk8_cpu_init(struct cpufreq_policy *pol)
{
struct powernow_k8_data *data;
+ int k8_cpu_acpi_inited = 0;
cpumask_t oldmask;
int rc;
@@ -1142,39 +1143,41 @@ static int __cpuinit powernowk8_cpu_init
data->cpu = pol->cpu;
data->currpstate = HW_PSTATE_INVALID;
- if (powernow_k8_cpu_init_acpi(data)) {
+ if (!powernow_k8_cpu_init_acpi(data)) {
+ k8_cpu_acpi_inited = 1;
+ } else {
/*
* Use the PSB BIOS structure. This is only availabe on
* an UP version, and is deprecated by AMD.
*/
if (num_online_cpus() != 1) {
+ if (!acpi_disabled) {
#ifndef CONFIG_ACPI_PROCESSOR
- printk(KERN_ERR PFX "ACPI Processor support is required "
- "for SMP systems but is absent. Please load the "
- "ACPI Processor module before starting this "
- "driver.\n");
+ printk(KERN_ERR PFX "ACPI Processor support is required "
+ "for SMP systems but is absent. Please load the "
+ "ACPI Processor module before starting this "
+ "driver.\n");
#else
- printk(KERN_ERR FW_BUG PFX "Your BIOS does not provide"
- " ACPI _PSS objects in a way that Linux "
- "understands. Please report this to the Linux "
- "ACPI maintainers and complain to your BIOS "
- "vendor.\n");
+ printk(KERN_ERR FW_BUG PFX "Your BIOS does not provide"
+ " ACPI _PSS objects in a way that Linux "
+ "understands. Please report this to the Linux "
+ "ACPI maintainers and complain to your BIOS "
+ "vendor.\n");
#endif
- kfree(data);
- return -ENODEV;
+ }
+ goto early_out;
}
if (pol->cpu != 0) {
- printk(KERN_ERR FW_BUG PFX "No ACPI _PSS objects for "
- "CPU other than CPU0. Complain to your BIOS "
- "vendor.\n");
- kfree(data);
- return -ENODEV;
+ if (!acpi_disabled) {
+ printk(KERN_ERR FW_BUG PFX "No ACPI _PSS objects for "
+ "CPU other than CPU0. Complain to your BIOS "
+ "vendor.\n");
+ }
+ goto early_out;
}
rc = find_psb_table(data);
- if (rc) {
- kfree(data);
- return -ENODEV;
- }
+ if (rc)
+ goto early_out;
}
/* only run on specific CPU from here on */
@@ -1220,7 +1223,8 @@ static int __cpuinit powernowk8_cpu_init
/* min/max the cpu is capable of */
if (cpufreq_frequency_table_cpuinfo(pol, data->powernow_table)) {
printk(KERN_ERR FW_BUG PFX "invalid powernow_table\n");
- powernow_k8_cpu_exit_acpi(data);
+ if (k8_cpu_acpi_inited)
+ powernow_k8_cpu_exit_acpi(data);
kfree(data->powernow_table);
kfree(data);
return -EINVAL;
@@ -1240,8 +1244,9 @@ static int __cpuinit powernowk8_cpu_init
err_out:
set_cpus_allowed_ptr(current, &oldmask);
- powernow_k8_cpu_exit_acpi(data);
-
+ if (k8_cpu_acpi_inited)
+ powernow_k8_cpu_exit_acpi(data);
+early_out:
kfree(data);
return -ENODEV;
}