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