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

[Viresh Kumar]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/