Message-ID: <559B9C88.4000909@intel.com>
Date: Tue, 07 Jul 2015 17:31:52 +0800
From: Pan Xinhui <xinhuix.pan@intel.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0
MIME-Version: 1.0
To: Viresh Kumar <viresh.kumar@linaro.org>
CC: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        linux-pm@vger.kernel.org, rjw@rjwysocki.net,
        "yanmin_zhang@linux.intel.com" <yanmin_zhang@linux.intel.com>,
        "mnipxh@163.com" <mnipxh@163.com>
Subject: Re: [PATCH] acpi-cpufreq.c: fix a memory leak in acpi_cpufreq_cpu_exit
References: <559A2073.1000301@intel.com> <20150707065449.GD14598@linux> <559B8537.5010309@intel.com> <20150707085342.GF14598@linux>
In-Reply-To: <20150707085342.GF14598@linux>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2982
Lines: 81

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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/