Re: [PATCH] acpi-cpufreq.c: fix a memory leak in acpi_cpufreq_cpu_exit

[Pan Xinhui <xinhuix.pan@intel.com>]

hi, Viresh
	thanks for your reply.

On 2015年07月07日 14:54, Viresh Kumar wrote:
> On 06-07-15, 14:30, Pan Xinhui wrote:
>>
>> policy->cpu in acpi_cpufreq_cpu_init/exit is the same cpu in most cases.
>> However during cpu hotplug,
>> cpufreq core might nominate a new cpu for policy->cpu.
> 
> Why aren't above lines well aligned? A simple trick to share for vim
> users:
> 
> - Select lines you want to auto-align with shift+v and up-down keys
> - press gq
> - That's it and vim will do it for you. You need to set vim's
>   'textwidth' to 72 or 80, based on what you are editing, so that vim
>   knows where you need to break the line. I have this in vimrc
> 
>         set textwidth=80
>         au FileType gitcommit set textwidth=72
> 

thanks, that will save me time.

> 
> 
> Back to the real stuff. Few core changes have gone into v4.2-rc1 and
> policy->cpu doesn't change any longer on hotplug (unless its a
> physical hotplug). So you shouldn't see any issues.
> 
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);

line 1445 will change the policy->cpu.
for example, cpu2,3 has same policy, and policy->cpu is 2 at beginning.
If we disable cpu2, policy->cpu is 3.
yes, at most time, cpu0,1,2,3,,etc share the same policy, and policy->cpu is 0 which can't be offline.
So no memory leak. it is just lucky. :)

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.
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.

thanks for your kind reply. any advices or comments are welcome.

thanks
xinhui