* Re: [Regression] 2.6.27-rc2-git4: suspend and power off fails on Asus M3A32-MVP
2008-08-12 16:03 ` Langsdorf, Mark
@ 2008-08-19 17:03 ` Rafael J. Wysocki
2008-08-19 17:02 ` Langsdorf, Mark
2008-08-19 19:37 ` [PATCH] Revert "[CPUFREQ][2/2] preregister support for powernow-k8" Rafael J. Wysocki
1 sibling, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2008-08-19 17:03 UTC (permalink / raw)
To: Langsdorf, Mark
Cc: Andrew Morton, Dave Jones, Linus Torvalds, LKML, kernel-testers,
Randy Dunlap, Arnold, Frank
On Tuesday, 12 of August 2008, Langsdorf, Mark wrote:
>
> > Unfortunately, your commit 34ae7f35a21694aa5cb8829dc5142c39d73d6ba0
> > ("[CPUFREQ][2/2] preregister support for powernow-k8") causes
> > a power off/
> > suspend/ hibernation regression on my test box with the Asus M3A32-MVP
> > mainboard (AMD 790 chipset) and the Phenom 9850 CPU.
> >
> > Apparently, the patch causes a crash in a CPU hotplug
> > notifier to happen, but I'm not really sure of that, since there are a
> lot of stack
> > traces resulting from it and I'm unable to capture them over a serial
> console.
> >
> > It is sufficient to revert commit
> > 34ae7f35a21694aa5cb8829dc5142c39d73d6ba0 to
> > make things work again.
>
> Thanks for the clear report. I'll look into it and try to fix it.
Any news?
Rafael
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] Revert "[CPUFREQ][2/2] preregister support for powernow-k8"
2008-08-12 16:03 ` Langsdorf, Mark
2008-08-19 17:03 ` Rafael J. Wysocki
@ 2008-08-19 19:37 ` Rafael J. Wysocki
2008-08-19 19:43 ` Dave Jones
1 sibling, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2008-08-19 19:37 UTC (permalink / raw)
To: Andrew Morton, Linus Torvalds
Cc: Langsdorf, Mark, Dave Jones, LKML, kernel-testers, Randy Dunlap,
Arnold, Frank, Torsten Kaiser
From: Rafael J. Wysocki <rjw@sisk.pl>
Revert "[CPUFREQ][2/2] preregister support for powernow-k8"
Revert commit 34ae7f35a21694aa5cb8829dc5142c39d73d6ba0
("[CPUFREQ][2/2] preregister support for powernow-k8") that causes
regressions tracked as:
http://bugzilla.kernel.org/show_bug.cgi?id=11296 ,
http://bugzilla.kernel.org/show_bug.cgi?id=11339 .
Namely, my test box with the Asus M3A32-MVP mainboard and the AMD
Phenom 9850 CPU crashes during hibernation, suspend to RAM and ACPI
power off, apparently due to a failing CPU hotplug notifier. In
turn, on the Torsten Kaiser's test system only one CPU is correctly
handled by cpufreq. These problems go away after reverting
34ae7f35a21694aa5cb8829dc5142c39d73d6ba0, but unfortunately they are
quite difficult to debug.
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
arch/x86/kernel/cpu/cpufreq/powernow-k8.c | 109 +++++++++---------------------
arch/x86/kernel/cpu/cpufreq/powernow-k8.h | 3
2 files changed, 37 insertions(+), 75 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
@@ -737,63 +737,44 @@ static int find_psb_table(struct powerno
#ifdef CONFIG_X86_POWERNOW_K8_ACPI
static void powernow_k8_acpi_pst_values(struct powernow_k8_data *data, unsigned int index)
{
- if (!data->acpi_data->state_count || (cpu_family == CPU_HW_PSTATE))
+ if (!data->acpi_data.state_count || (cpu_family == CPU_HW_PSTATE))
return;
- data->irt = (data->acpi_data->states[index].control >> IRT_SHIFT) & IRT_MASK;
- data->rvo = (data->acpi_data->states[index].control >> RVO_SHIFT) & RVO_MASK;
- data->exttype = (data->acpi_data->states[index].control >> EXT_TYPE_SHIFT) & EXT_TYPE_MASK;
- data->plllock = (data->acpi_data->states[index].control >> PLL_L_SHIFT) & PLL_L_MASK;
- data->vidmvs = 1 << ((data->acpi_data->states[index].control >> MVS_SHIFT) & MVS_MASK);
- data->vstable = (data->acpi_data->states[index].control >> VST_SHIFT) & VST_MASK;
-}
-
-
-static struct acpi_processor_performance *acpi_perf_data;
-static int preregister_valid;
-
-static int powernow_k8_cpu_preinit_acpi(void)
-{
- acpi_perf_data = alloc_percpu(struct acpi_processor_performance);
- if (!acpi_perf_data)
- return -ENODEV;
-
- if (acpi_processor_preregister_performance(acpi_perf_data))
- return -ENODEV;
- else
- preregister_valid = 1;
- return 0;
+ data->irt = (data->acpi_data.states[index].control >> IRT_SHIFT) & IRT_MASK;
+ data->rvo = (data->acpi_data.states[index].control >> RVO_SHIFT) & RVO_MASK;
+ data->exttype = (data->acpi_data.states[index].control >> EXT_TYPE_SHIFT) & EXT_TYPE_MASK;
+ data->plllock = (data->acpi_data.states[index].control >> PLL_L_SHIFT) & PLL_L_MASK;
+ data->vidmvs = 1 << ((data->acpi_data.states[index].control >> MVS_SHIFT) & MVS_MASK);
+ data->vstable = (data->acpi_data.states[index].control >> VST_SHIFT) & VST_MASK;
}
static int powernow_k8_cpu_init_acpi(struct powernow_k8_data *data)
{
struct cpufreq_frequency_table *powernow_table;
int ret_val;
- int cpu = 0;
- data->acpi_data = percpu_ptr(acpi_perf_data, cpu);
- if (acpi_processor_register_performance(data->acpi_data, data->cpu)) {
+ if (acpi_processor_register_performance(&data->acpi_data, data->cpu)) {
dprintk("register performance failed: bad ACPI data\n");
return -EIO;
}
/* verify the data contained in the ACPI structures */
- if (data->acpi_data->state_count <= 1) {
+ if (data->acpi_data.state_count <= 1) {
dprintk("No ACPI P-States\n");
goto err_out;
}
- if ((data->acpi_data->control_register.space_id != ACPI_ADR_SPACE_FIXED_HARDWARE) ||
- (data->acpi_data->status_register.space_id != ACPI_ADR_SPACE_FIXED_HARDWARE)) {
+ if ((data->acpi_data.control_register.space_id != ACPI_ADR_SPACE_FIXED_HARDWARE) ||
+ (data->acpi_data.status_register.space_id != ACPI_ADR_SPACE_FIXED_HARDWARE)) {
dprintk("Invalid control/status registers (%x - %x)\n",
- data->acpi_data->control_register.space_id,
- data->acpi_data->status_register.space_id);
+ data->acpi_data.control_register.space_id,
+ data->acpi_data.status_register.space_id);
goto err_out;
}
/* fill in data->powernow_table */
powernow_table = kmalloc((sizeof(struct cpufreq_frequency_table)
- * (data->acpi_data->state_count + 1)), GFP_KERNEL);
+ * (data->acpi_data.state_count + 1)), GFP_KERNEL);
if (!powernow_table) {
dprintk("powernow_table memory alloc failure\n");
goto err_out;
@@ -806,12 +787,12 @@ static int powernow_k8_cpu_init_acpi(str
if (ret_val)
goto err_out_mem;
- powernow_table[data->acpi_data->state_count].frequency = CPUFREQ_TABLE_END;
- powernow_table[data->acpi_data->state_count].index = 0;
+ powernow_table[data->acpi_data.state_count].frequency = CPUFREQ_TABLE_END;
+ powernow_table[data->acpi_data.state_count].index = 0;
data->powernow_table = powernow_table;
/* fill in data */
- data->numps = data->acpi_data->state_count;
+ data->numps = data->acpi_data.state_count;
if (first_cpu(per_cpu(cpu_core_map, data->cpu)) == data->cpu)
print_basics(data);
powernow_k8_acpi_pst_values(data, 0);
@@ -819,31 +800,16 @@ static int powernow_k8_cpu_init_acpi(str
/* notify BIOS that we exist */
acpi_processor_notify_smm(THIS_MODULE);
- /* determine affinity, from ACPI if available */
- if (preregister_valid) {
- if ((data->acpi_data->shared_type == CPUFREQ_SHARED_TYPE_ALL) ||
- (data->acpi_data->shared_type == CPUFREQ_SHARED_TYPE_ANY))
- data->starting_core_affinity = data->acpi_data->shared_cpu_map;
- else
- data->starting_core_affinity = cpumask_of_cpu(data->cpu);
- } else {
- /* best guess from family if not */
- if (cpu_family == CPU_HW_PSTATE)
- data->starting_core_affinity = cpumask_of_cpu(data->cpu);
- else
- data->starting_core_affinity = per_cpu(cpu_core_map, data->cpu);
- }
-
return 0;
err_out_mem:
kfree(powernow_table);
err_out:
- acpi_processor_unregister_performance(data->acpi_data, data->cpu);
+ acpi_processor_unregister_performance(&data->acpi_data, data->cpu);
/* data->acpi_data.state_count informs us at ->exit() whether ACPI was used */
- data->acpi_data->state_count = 0;
+ data->acpi_data.state_count = 0;
return -ENODEV;
}
@@ -855,10 +821,10 @@ static int fill_powernow_table_pstate(st
rdmsr(MSR_PSTATE_CUR_LIMIT, hi, lo);
data->max_hw_pstate = (hi & HW_PSTATE_MAX_MASK) >> HW_PSTATE_MAX_SHIFT;
- for (i = 0; i < data->acpi_data->state_count; i++) {
+ for (i = 0; i < data->acpi_data.state_count; i++) {
u32 index;
- index = data->acpi_data->states[i].control & HW_PSTATE_MASK;
+ index = data->acpi_data.states[i].control & HW_PSTATE_MASK;
if (index > data->max_hw_pstate) {
printk(KERN_ERR PFX "invalid pstate %d - bad value %d.\n", i, index);
printk(KERN_ERR PFX "Please report to BIOS manufacturer\n");
@@ -874,7 +840,7 @@ static int fill_powernow_table_pstate(st
powernow_table[i].index = index;
- powernow_table[i].frequency = data->acpi_data->states[i].core_frequency * 1000;
+ powernow_table[i].frequency = data->acpi_data.states[i].core_frequency * 1000;
}
return 0;
}
@@ -883,16 +849,16 @@ static int fill_powernow_table_fidvid(st
{
int i;
int cntlofreq = 0;
- for (i = 0; i < data->acpi_data->state_count; i++) {
+ for (i = 0; i < data->acpi_data.state_count; i++) {
u32 fid;
u32 vid;
if (data->exttype) {
- fid = data->acpi_data->states[i].status & EXT_FID_MASK;
- vid = (data->acpi_data->states[i].status >> VID_SHIFT) & EXT_VID_MASK;
+ fid = data->acpi_data.states[i].status & EXT_FID_MASK;
+ vid = (data->acpi_data.states[i].status >> VID_SHIFT) & EXT_VID_MASK;
} else {
- fid = data->acpi_data->states[i].control & FID_MASK;
- vid = (data->acpi_data->states[i].control >> VID_SHIFT) & VID_MASK;
+ fid = data->acpi_data.states[i].control & FID_MASK;
+ vid = (data->acpi_data.states[i].control >> VID_SHIFT) & VID_MASK;
}
dprintk(" %d : fid 0x%x, vid 0x%x\n", i, fid, vid);
@@ -933,10 +899,10 @@ static int fill_powernow_table_fidvid(st
cntlofreq = i;
}
- if (powernow_table[i].frequency != (data->acpi_data->states[i].core_frequency * 1000)) {
+ if (powernow_table[i].frequency != (data->acpi_data.states[i].core_frequency * 1000)) {
printk(KERN_INFO PFX "invalid freq entries %u kHz vs. %u kHz\n",
powernow_table[i].frequency,
- (unsigned int) (data->acpi_data->states[i].core_frequency * 1000));
+ (unsigned int) (data->acpi_data.states[i].core_frequency * 1000));
powernow_table[i].frequency = CPUFREQ_ENTRY_INVALID;
continue;
}
@@ -946,12 +912,11 @@ static int fill_powernow_table_fidvid(st
static void powernow_k8_cpu_exit_acpi(struct powernow_k8_data *data)
{
- if (data->acpi_data->state_count)
- acpi_processor_unregister_performance(data->acpi_data, data->cpu);
+ if (data->acpi_data.state_count)
+ acpi_processor_unregister_performance(&data->acpi_data, data->cpu);
}
#else
-static int powernow_k8_cpu_preinit_acpi(void) { return -ENODEV; }
static int powernow_k8_cpu_init_acpi(struct powernow_k8_data *data) { return -ENODEV; }
static void powernow_k8_cpu_exit_acpi(struct powernow_k8_data *data) { return; }
static void powernow_k8_acpi_pst_values(struct powernow_k8_data *data, unsigned int index) { return; }
@@ -1136,7 +1101,7 @@ static int powernowk8_verify(struct cpuf
static int __cpuinit powernowk8_cpu_init(struct cpufreq_policy *pol)
{
struct powernow_k8_data *data;
- cpumask_t oldmask = CPU_MASK_ALL;
+ cpumask_t oldmask;
int rc;
if (!cpu_online(pol->cpu))
@@ -1209,7 +1174,10 @@ static int __cpuinit powernowk8_cpu_init
/* run on any CPU again */
set_cpus_allowed_ptr(current, &oldmask);
- pol->cpus = data->starting_core_affinity;
+ if (cpu_family == CPU_HW_PSTATE)
+ pol->cpus = cpumask_of_cpu(pol->cpu);
+ else
+ pol->cpus = per_cpu(cpu_core_map, pol->cpu);
data->available_cores = &(pol->cpus);
/* Take a crude guess here.
@@ -1332,7 +1300,6 @@ static int __cpuinit powernowk8_init(voi
}
if (supported_cpus == num_online_cpus()) {
- powernow_k8_cpu_preinit_acpi();
printk(KERN_INFO PFX "Found %d %s "
"processors (%d cpu cores) (" VERSION ")\n",
num_online_nodes(),
@@ -1349,10 +1316,6 @@ static void __exit powernowk8_exit(void)
dprintk("exit\n");
cpufreq_unregister_driver(&cpufreq_amd64_driver);
-
-#ifdef CONFIG_X86_POWERNOW_K8_ACPI
- free_percpu(acpi_perf_data);
-#endif
}
MODULE_AUTHOR("Paul Devriendt <paul.devriendt@amd.com> and Mark Langsdorf <mark.langsdorf@amd.com>");
Index: linux-2.6/arch/x86/kernel/cpu/cpufreq/powernow-k8.h
===================================================================
--- linux-2.6.orig/arch/x86/kernel/cpu/cpufreq/powernow-k8.h
+++ linux-2.6/arch/x86/kernel/cpu/cpufreq/powernow-k8.h
@@ -33,13 +33,12 @@ struct powernow_k8_data {
#ifdef CONFIG_X86_POWERNOW_K8_ACPI
/* the acpi table needs to be kept. it's only available if ACPI was
* used to determine valid frequency/vid/fid states */
- struct acpi_processor_performance *acpi_data;
+ struct acpi_processor_performance acpi_data;
#endif
/* we need to keep track of associated cores, but let cpufreq
* handle hotplug events - so just point at cpufreq pol->cpus
* structure */
cpumask_t *available_cores;
- cpumask_t starting_core_affinity;
};
^ permalink raw reply [flat|nested] 6+ messages in thread