Messages in this thread | | | Date | Tue, 07 Jul 2015 17:31:52 +0800 | From | Pan Xinhui <> | Subject | Re: [PATCH] acpi-cpufreq.c: fix a memory leak in acpi_cpufreq_cpu_exit |
| |
hi, Viresh thanks for your quick reply :)
On 2015年07月07日 16:53, Viresh Kumar wrote: > On 07-07-15, 15:52, Pan Xinhui wrote: >> I have latest codes. >> codes in cpufreq.c are below. >> 1436 down_write(&policy->rwsem); >> 1437 cpumask_clear_cpu(cpu, policy->cpus); >> 1438 >> 1439 if (policy_is_inactive(policy)) { >> 1440 if (has_target()) >> 1441 strncpy(policy->last_governor, policy->governor->name, >> 1442 CPUFREQ_NAME_LEN); >> 1443 } else if (cpu == policy->cpu) { >> 1444 /* Nominate new CPU */ >> 1445 policy->cpu = cpumask_any(policy->cpus); >> 1446 } >> 1447 up_write(&policy->rwsem); > > Sigh. Too bad. So what has changed is that the sysfs directory is > allocated to policy->cpu during init and never changed. But > policy->cpu can surely change. > > Sorry for that. >
That's OKay. You are very busy reviewing codes written by still fresh guys like me.
>> back to my previous patch, you suggest me to use policy->driver_data to *store* data and don't need use per_cpu anymore. >> codes in acpi-cpufreq.c are below. >> 365 static unsigned int get_cur_freq_on_cpu(unsigned int cpu) >> 366 { >> 367 struct acpi_cpufreq_data *data = per_cpu(acfreq_data, cpu); >> 368 unsigned int freq; >> 369 unsigned int cached_freq; >> >> we get *data* through per_cpu for now, as the parameter is cpu only. >> If we store *data* in policy->driver_data, we need call >> struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu) to get policy. >> We do a full codes review, and find there should be deadlock if we doing so. > > Why? > sorry, after double check. it's not caused by cpufreq_cpu_get. I am working on several branches, these codes are little different, it's OKay here. Sorry for mistakes.
>> But as cpufreq code offers >> 238 /* Only for cpufreq core internal use */ >> 239 struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu) >> >> I have a small question,if we can use *cpufreq_cpu_get_raw* in ->get callback, which is already lock hold, >> But the comment(line 238) is... hmm. > > That is more internal to the core. Better don't use it. >
yes, *cpufreq_cpu_get* is OKay. thanks.
>> thanks for your kind reply. any advices or comments are welcome. > > Anyway, your patch is far from complete. You have just fixed a single > place where per-cpu data is accessed with policy->cpu. What about rest > of the code? Like target() :) > I have generated one patch which replacing all per_cpu with *driver_data*, it works well in our Intel's branch for at least 2 days. Let me do more codes review and tests before sending to LKML. :) thanks for your advices :) it's really good.
thanks xinhui
> -- > viresh >
| |