Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752935AbbGGIx5 (ORCPT ); Tue, 7 Jul 2015 04:53:57 -0400 Received: from mail-pd0-f171.google.com ([209.85.192.171]:35663 "EHLO mail-pd0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752711AbbGGIxu (ORCPT ); Tue, 7 Jul 2015 04:53:50 -0400 Date: Tue, 7 Jul 2015 14:23:42 +0530 From: Viresh Kumar To: Pan Xinhui Cc: "linux-kernel@vger.kernel.org" , linux-pm@vger.kernel.org, rjw@rjwysocki.net, "yanmin_zhang@linux.intel.com" , "mnipxh@163.com" Subject: Re: [PATCH] acpi-cpufreq.c: fix a memory leak in acpi_cpufreq_cpu_exit Message-ID: <20150707085342.GF14598@linux> References: <559A2073.1000301@intel.com> <20150707065449.GD14598@linux> <559B8537.5010309@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <559B8537.5010309@intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2259 Lines: 59 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. > 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? > 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. > 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() :) -- 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/