[CPUFREQ] speedstep-centrino: fix SMP memory leak Venkatesh Pallipadi made me aware of a memory leak in speedstep-centrino: centrino_model is allocated for all CPUs. There were two possibilities: either make the centrino_model data per-CPU, or to share it across CPUs. I chose the former variant, as ACPI data may be broken and/or different for multiple CPUs. Additionally, I made centrino_cpu per-CPU, and removed a check whether setting allowed_cpus to policy->cpus resulted in smp_processor_id() == policy->cpu: if policy->cpus has more than one bit set, this check will fail very often. Signed-off-by: Dominik Brodowski --- arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c | 111 +++++++++++----------- 1 files changed, 57 insertions(+), 54 deletions(-) Index: 2.6.10-rc3/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c =================================================================== --- 2.6.10-rc3.orig/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c 2004-12-13 19:19:06.059549665 +0100 +++ 2.6.10-rc3/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c 2004-12-13 19:27:47.706540832 +0100 @@ -71,8 +71,8 @@ static int centrino_verify_cpu_id(const struct cpuinfo_x86 *c, const struct cpu_id *x); /* Operating points for current CPU */ -static struct cpu_model *centrino_model; -static const struct cpu_id *centrino_cpu; +static struct cpu_model *centrino_model[NR_CPUS]; +static const struct cpu_id *centrino_cpu[NR_CPUS]; #ifdef CONFIG_X86_SPEEDSTEP_CENTRINO_TABLE @@ -255,7 +255,7 @@ return -ENOENT; } - centrino_model = model; + centrino_model[policy->cpu] = model; dprintk("found \"%s\": max frequency: %dkHz\n", model->model_name, model->max_freq); @@ -277,7 +277,7 @@ } /* To be called only after centrino_model is initialized */ -static unsigned extract_clock(unsigned msr) +static unsigned extract_clock(unsigned msr, unsigned int cpu) { int i; @@ -286,20 +286,20 @@ * for centrino, as some DSDTs are buggy. * Ideally, this can be done using the acpi_data structure. */ - if ((centrino_cpu == &cpu_ids[CPU_BANIAS]) || - (centrino_cpu == &cpu_ids[CPU_DOTHAN_A1]) || - (centrino_cpu == &cpu_ids[CPU_DOTHAN_B0])) { + if ((centrino_cpu[cpu] == &cpu_ids[CPU_BANIAS]) || + (centrino_cpu[cpu] == &cpu_ids[CPU_DOTHAN_A1]) || + (centrino_cpu[cpu] == &cpu_ids[CPU_DOTHAN_B0])) { msr = (msr >> 8) & 0xff; return msr * 100000; } - if ((!centrino_model) || (!centrino_model->op_points)) + if ((!centrino_model[cpu]) || (!centrino_model[cpu]->op_points)) return 0; msr &= 0xffff; - for (i=0;centrino_model->op_points[i].frequency != CPUFREQ_TABLE_END; i++) { - if (msr == centrino_model->op_points[i].index) - return centrino_model->op_points[i].frequency; + for (i=0;centrino_model[cpu]->op_points[i].frequency != CPUFREQ_TABLE_END; i++) { + if (msr == centrino_model[cpu]->op_points[i].index) + return centrino_model[cpu]->op_points[i].frequency; } return 0; } @@ -317,7 +317,7 @@ rdmsr(MSR_IA32_PERF_STATUS, l, h); set_cpus_allowed(current, saved_mask); - return extract_clock(l); + return extract_clock(l, cpu); } @@ -339,6 +339,7 @@ struct acpi_object_list arg_list = {1, &arg0}; unsigned long cur_freq; int result = 0, i; + unsigned int cpu = policy->cpu; /* _PDC settings */ arg0.buffer.length = 12; @@ -350,7 +351,7 @@ p.pdc = &arg_list; /* register with ACPI core */ - if (acpi_processor_register_performance(&p, policy->cpu)) { + if (acpi_processor_register_performance(&p, cpu)) { printk(KERN_INFO PFX "obtaining ACPI data failed\n"); return -EIO; } @@ -392,49 +393,49 @@ } } - centrino_model = kmalloc(sizeof(struct cpu_model), GFP_KERNEL); - if (!centrino_model) { + centrino_model[cpu] = kmalloc(sizeof(struct cpu_model), GFP_KERNEL); + if (!centrino_model[cpu]) { result = -ENOMEM; goto err_unreg; } - memset(centrino_model, 0, sizeof(struct cpu_model)); + memset(centrino_model[cpu], 0, sizeof(struct cpu_model)); - centrino_model->model_name=NULL; - centrino_model->max_freq = p.states[0].core_frequency * 1000; - centrino_model->op_points = kmalloc(sizeof(struct cpufreq_frequency_table) * + centrino_model[cpu]->model_name=NULL; + centrino_model[cpu]->max_freq = p.states[0].core_frequency * 1000; + centrino_model[cpu]->op_points = kmalloc(sizeof(struct cpufreq_frequency_table) * (p.state_count + 1), GFP_KERNEL); - if (!centrino_model->op_points) { + if (!centrino_model[cpu]->op_points) { result = -ENOMEM; goto err_kfree; } for (i=0; iop_points[i].index = p.states[i].control; - centrino_model->op_points[i].frequency = p.states[i].core_frequency * 1000; + centrino_model[cpu]->op_points[i].index = p.states[i].control; + centrino_model[cpu]->op_points[i].frequency = p.states[i].core_frequency * 1000; dprintk("adding state %i with frequency %u and control value %04x\n", - i, centrino_model->op_points[i].frequency, centrino_model->op_points[i].index); + i, centrino_model[cpu]->op_points[i].frequency, centrino_model[cpu]->op_points[i].index); } - centrino_model->op_points[p.state_count].frequency = CPUFREQ_TABLE_END; + centrino_model[cpu]->op_points[p.state_count].frequency = CPUFREQ_TABLE_END; - cur_freq = get_cur_freq(policy->cpu); + cur_freq = get_cur_freq(cpu); for (i=0; iop_points[i].frequency = CPUFREQ_ENTRY_INVALID; + centrino_model[cpu]->op_points[i].frequency = CPUFREQ_ENTRY_INVALID; continue; } - if (extract_clock(centrino_model->op_points[i].index) != - (centrino_model->op_points[i].frequency)) { + if (extract_clock(centrino_model[cpu]->op_points[i].index, cpu) != + (centrino_model[cpu]->op_points[i].frequency)) { dprintk("Invalid encoded frequency (%u vs. %u)\n", - extract_clock(centrino_model->op_points[i].index), - centrino_model->op_points[i].frequency); + extract_clock(centrino_model[cpu]->op_points[i].index, cpu), + centrino_model[cpu]->op_points[i].frequency); result = -EINVAL; goto err_kfree_all; } - if (cur_freq == centrino_model->op_points[i].frequency) + if (cur_freq == centrino_model[cpu]->op_points[i].frequency) p.state = i; } @@ -444,11 +445,11 @@ return 0; err_kfree_all: - kfree(centrino_model->op_points); + kfree(centrino_model[cpu]->op_points); err_kfree: - kfree(centrino_model); + kfree(centrino_model[cpu]); err_unreg: - acpi_processor_unregister_performance(&p, policy->cpu); + acpi_processor_unregister_performance(&p, cpu); printk(KERN_INFO PFX "invalid ACPI data\n"); return (result); } @@ -473,13 +474,13 @@ break; if (i != N_IDS) - centrino_cpu = &cpu_ids[i]; + centrino_cpu[policy->cpu] = &cpu_ids[i]; if (centrino_cpu_init_acpi(policy)) { if (policy->cpu != 0) return -ENODEV; - if (!centrino_cpu) { + if (!centrino_cpu[policy->cpu]) { printk(KERN_INFO PFX "found unsupported CPU with " "Enhanced SpeedStep: send /proc/cpuinfo to " MAINTAINER "\n"); @@ -516,32 +517,34 @@ dprintk("centrino_cpu_init: cur=%dkHz\n", policy->cur); - ret = cpufreq_frequency_table_cpuinfo(policy, centrino_model->op_points); + ret = cpufreq_frequency_table_cpuinfo(policy, centrino_model[policy->cpu]->op_points); if (ret) return (ret); - cpufreq_frequency_table_get_attr(centrino_model->op_points, policy->cpu); + cpufreq_frequency_table_get_attr(centrino_model[policy->cpu]->op_points, policy->cpu); return 0; } static int centrino_cpu_exit(struct cpufreq_policy *policy) { - if (!centrino_model) + unsigned int cpu = policy->cpu; + + if (!centrino_model[cpu]) return -ENODEV; - cpufreq_frequency_table_put_attr(policy->cpu); + cpufreq_frequency_table_put_attr(cpu); #ifdef CONFIG_X86_SPEEDSTEP_CENTRINO_ACPI - if (!centrino_model->model_name) { + if (!centrino_model[cpu]->model_name) { dprintk("unregistering and freeing ACPI data\n"); - acpi_processor_unregister_performance(&p, policy->cpu); - kfree(centrino_model->op_points); - kfree(centrino_model); + acpi_processor_unregister_performance(&p, cpu); + kfree(centrino_model[cpu]->op_points); + kfree(centrino_model[cpu]); } #endif - centrino_model = NULL; + centrino_model[cpu] = NULL; return 0; } @@ -555,7 +558,7 @@ */ static int centrino_verify (struct cpufreq_policy *policy) { - return cpufreq_frequency_table_verify(policy, centrino_model->op_points); + return cpufreq_frequency_table_verify(policy, centrino_model[policy->cpu]->op_points); } /** @@ -571,12 +574,12 @@ unsigned int relation) { unsigned int newstate = 0; - unsigned int msr, oldmsr, h; + unsigned int msr, oldmsr, h, cpu = policy->cpu; struct cpufreq_freqs freqs; cpumask_t saved_mask; int retval; - if (centrino_model == NULL) + if (centrino_model[cpu] == NULL) return -ENODEV; /* @@ -585,18 +588,18 @@ */ saved_mask = current->cpus_allowed; set_cpus_allowed(current, policy->cpus); - if (smp_processor_id() != policy->cpu) { + if (!cpu_isset(smp_processor_id(), policy->cpus)) { dprintk("couldn't limit to CPUs in this domain\n"); return(-EAGAIN); } - if (cpufreq_frequency_table_target(policy, centrino_model->op_points, target_freq, + if (cpufreq_frequency_table_target(policy, centrino_model[cpu]->op_points, target_freq, relation, &newstate)) { retval = -EINVAL; goto migrate_end; } - msr = centrino_model->op_points[newstate].index; + msr = centrino_model[cpu]->op_points[newstate].index; rdmsr(MSR_IA32_PERF_CTL, oldmsr, h); if (msr == (oldmsr & 0xffff)) { @@ -605,9 +608,9 @@ goto migrate_end; } - freqs.cpu = policy->cpu; - freqs.old = extract_clock(oldmsr); - freqs.new = extract_clock(msr); + freqs.cpu = cpu; + freqs.old = extract_clock(oldmsr, cpu); + freqs.new = extract_clock(msr, cpu); dprintk("target=%dkHz old=%d new=%d msr=%04x\n", target_freq, freqs.old, freqs.new, msr);