From: Alex Chiang <achiang@redhat.com> Date: Wed, 23 Sep 2009 21:05:06 -0400 Subject: [cpufreq] avoid playing with cpus_allowed in powernow-k8 Message-id: <20090923210506.GF30562@algore> Patchwork-id: 20951 O-Subject: [RHEL 5.5 BZ523505 Patch 5/5] [CPUFREQ] cpumask: avoid playing with cpus_allowed in powernow-k8.c Bugzilla: 523505 RH-Acked-by: Dave Jones <davej@redhat.com> RH-Acked-by: Prarit Bhargava <prarit@redhat.com> Backport of upstream: commit 1ff6e97f1d993dff2f9b6f4a9173687370660232 Author: Rusty Russell <rusty@rustcorp.com.au> Date: Fri Jun 12 20:55:37 2009 +0930 [CPUFREQ] cpumask: avoid playing with cpus_allowed in powernow-k8.c cpumask: avoid playing with cpus_allowed in powernow-k8.c It's generally a very bad idea to mug some process's cpumask: it could legitimately and reasonably be changed by root, which could break us (if done before our code) or them (if we restore the wrong value). I did not replace powernowk8_target; it needs fixing, but it grabs a mutex (so no smp_call_function_single here) but Mark points out it can be called multiple times per second, so work_on_cpu is too heavy. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> To: cpufreq@vger.kernel.org Acked-by: Mark Langsdorf <mark.langsdorf@amd.com> Tested-by: Mark Langsdorf <mark.langsdorf@amd.com> Signed-off-by: Dave Jones <davej@redhat.com> Deviations from upstream include: RHEL5 has 41a2e503 (x86_64: opterons synchronize p-state using TSC) which doesn't seem to be upstream. The code introduced by 41a2e503 plays around with cpumasks too, but it's not clear to me how dangerous that is. I left that code alone. Upstream has 8691e5a (smp_call_function: get rid of the unused nonatomic/retry argument) which RHEL5 does not have. However, RHEL5 doesn't actually use the nonatomic/retry argument so I just filled in a dummy value of 1. diff --git a/arch/i386/kernel/cpu/cpufreq/powernow-k8.c b/arch/i386/kernel/cpu/cpufreq/powernow-k8.c index 9e9e30f..6f55c00 100644 --- a/arch/i386/kernel/cpu/cpufreq/powernow-k8.c +++ b/arch/i386/kernel/cpu/cpufreq/powernow-k8.c @@ -509,60 +509,49 @@ static int core_voltage_post_transition(struct powernow_k8_data *data, u32 reqvi return 0; } -static int check_supported_cpu(unsigned int cpu) +static void check_supported_cpu(void *_rc) { - cpumask_t oldmask = CPU_MASK_ALL; u32 eax, ebx, ecx, edx; - unsigned int rc = 0; - - oldmask = current->cpus_allowed; - set_cpus_allowed(current, cpumask_of_cpu(cpu)); + int *rc = _rc; - if (smp_processor_id() != cpu) { - printk(KERN_ERR PFX "limiting to cpu %u failed\n", cpu); - goto out; - } + *rc = -ENODEV; if (current_cpu_data.x86_vendor != X86_VENDOR_AMD) - goto out; + return; eax = cpuid_eax(CPUID_PROCESSOR_SIGNATURE); if (((eax & CPUID_XFAM) != CPUID_XFAM_K8) && ((eax & CPUID_XFAM) < CPUID_XFAM_10H)) - goto out; + return; if ((eax & CPUID_XFAM) == CPUID_XFAM_K8) { if (((eax & CPUID_USE_XFAM_XMOD) != CPUID_USE_XFAM_XMOD) || ((eax & CPUID_XMOD) > CPUID_XMOD_REV_G)) { printk(KERN_INFO PFX "Processor cpuid %x not supported\n", eax); - goto out; + return; } eax = cpuid_eax(CPUID_GET_MAX_CAPABILITIES); if (eax < CPUID_FREQ_VOLT_CAPABILITIES) { printk(KERN_INFO PFX "No frequency change capabilities detected\n"); - goto out; + return; } cpuid(CPUID_FREQ_VOLT_CAPABILITIES, &eax, &ebx, &ecx, &edx); if ((edx & P_STATE_TRANSITION_CAPABLE) != P_STATE_TRANSITION_CAPABLE) { printk(KERN_INFO PFX "Power state transitions not supported\n"); - goto out; + return; } } else { /* must be a HW Pstate capable processor */ cpuid(CPUID_FREQ_VOLT_CAPABILITIES, &eax, &ebx, &ecx, &edx); if ((edx & USE_HW_PSTATE) == USE_HW_PSTATE) cpu_family = CPU_HW_PSTATE; else - goto out; + return; } - rc = 1; - -out: - set_cpus_allowed(current, oldmask); - return rc; + *rc = 0; } static int check_pst_table(struct powernow_k8_data *data, struct pst_s *pst, u8 maxvid) @@ -1225,17 +1214,44 @@ static void sync_tables(int curcpu) } #endif +struct init_on_cpu { + struct powernow_k8_data *data; + int rc; +}; + +static void __cpuinit powernowk8_cpu_init_on_cpu(void *_init_on_cpu) +{ + struct init_on_cpu *init_on_cpu = _init_on_cpu; + + if (pending_bit_stuck()) { + printk(KERN_ERR PFX "failing init, change pending bit set\n"); + init_on_cpu->rc = -ENODEV; + return; + } + + if (query_current_values_with_pending_wait(init_on_cpu->data)) { + init_on_cpu->rc = -ENODEV; + return; + } + + if (cpu_family == CPU_OPTERON) + fidvid_msr_init(); + + init_on_cpu->rc = 0; +} + /* per CPU init entry point to the driver */ static int __cpuinit powernowk8_cpu_init(struct cpufreq_policy *pol) { struct powernow_k8_data *data; - cpumask_t oldmask = CPU_MASK_ALL; + struct init_on_cpu init_on_cpu; int rc; if (!cpu_online(pol->cpu)) return -ENODEV; - if (!check_supported_cpu(pol->cpu)) + smp_call_function_single(pol->cpu, check_supported_cpu, &rc, 1, 1); + if (rc) return -ENODEV; data = kzalloc(sizeof(struct powernow_k8_data), GFP_KERNEL); @@ -1269,27 +1285,12 @@ static int __cpuinit powernowk8_cpu_init(struct cpufreq_policy *pol) } /* only run on specific CPU from here on */ - oldmask = current->cpus_allowed; - set_cpus_allowed(current, cpumask_of_cpu(pol->cpu)); - - if (smp_processor_id() != pol->cpu) { - printk(KERN_ERR PFX "limiting to cpu %u failed\n", pol->cpu); - goto err_out; - } - - if (pending_bit_stuck()) { - printk(KERN_ERR PFX "failing init, change pending bit set\n"); - goto err_out; - } - - if (query_current_values_with_pending_wait(data)) - goto err_out; - - if (cpu_family == CPU_OPTERON) - fidvid_msr_init(); - - /* run on any CPU again */ - set_cpus_allowed(current, oldmask); + init_on_cpu.data = data; + smp_call_function_single(data->cpu, powernowk8_cpu_init_on_cpu, + &init_on_cpu, 1, 1); + rc = init_on_cpu.rc; + if (rc != 0) + goto err_out_exit_acpi; pol->governor = CPUFREQ_DEFAULT_GOVERNOR; pol->cpus = data->starting_core_affinity; @@ -1339,8 +1340,7 @@ static int __cpuinit powernowk8_cpu_init(struct cpufreq_policy *pol) #endif return 0; -err_out: - set_cpus_allowed(current, oldmask); +err_out_exit_acpi: powernow_k8_cpu_exit_acpi(data); kfree(data); @@ -1364,23 +1364,25 @@ static int __devexit powernowk8_cpu_exit (struct cpufreq_policy *pol) return 0; } +static void query_values_on_cpu(void *_err) +{ + int *err = _err; + struct powernow_k8_data *data = __get_cpu_var(powernow_data); + + *err = query_current_values_with_pending_wait(data); +} + static unsigned int powernowk8_get (unsigned int cpu) { struct powernow_k8_data *data = per_cpu(powernow_data, cpu); - cpumask_t oldmask = current->cpus_allowed; unsigned int khz = 0; + int err; if (!data) return -EINVAL; - set_cpus_allowed(current, cpumask_of_cpu(cpu)); - if (smp_processor_id() != cpu) { - printk(KERN_ERR PFX "limiting to CPU %d failed in powernowk8_get\n", cpu); - set_cpus_allowed(current, oldmask); - return 0; - } - - if (query_current_values_with_pending_wait(data)) + smp_call_function_single(cpu, query_values_on_cpu, &err, 1, true); + if (err) goto out; if (cpu_family == CPU_HW_PSTATE) @@ -1389,7 +1391,6 @@ static unsigned int powernowk8_get (unsigned int cpu) khz = find_khz_freq_from_fid(data->currfid); out: - set_cpus_allowed(current, oldmask); return khz; } @@ -1422,7 +1423,9 @@ static int __cpuinit powernowk8_init(void) #endif for_each_online_cpu(i) { - if (check_supported_cpu(i)) + int rc; + smp_call_function_single(i, check_supported_cpu, &rc, 1, 1); + if (rc == 0) supported_cpus++; }