| Message ID | 2368277.VjYrsHUseA@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Mon, Jul 29, 2013 at 4:50 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > Having a deeper look at it, though, I see that in fact the whole > cpufreq_cpu_put() is needed if the CPU is not the last one for the given > policy and is not needed at all otherwise (as described in the changelog > of the patch below). > > Srivatsa, does this make sense to you? It makes atleast to me :) > This is not the only bad thing that happens there, however, because > kobject_put() should only be called for the policy kobject at this > point if the CPU is not the last one for that policy. > > Namely, if the given CPU is the last one for that policy, the > policy kobject's refcount should be 1 at this point, as set by > cpufreq_add_dev_interface(), and only needs to be dropped once for > the kobject to go away. This actually happens under the cpu == 1 > check, so it need not be done before by cpufreq_cpu_put(). But I see one more issue with this code. For the last cpu we are just calling kobject_put() and not cpufreq_cpu_put() and hence call to module_put() is skipped. I am not sure, but that will probably cause a problem when we try to rmmod the module? But which module then? As we can't compile cpufreq.c as module.. So, is this part of code junk? And so can be removed? -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/29/2013 01:21 PM, Viresh Kumar wrote: > On Mon, Jul 29, 2013 at 4:50 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: >> Having a deeper look at it, though, I see that in fact the whole >> cpufreq_cpu_put() is needed if the CPU is not the last one for the given >> policy and is not needed at all otherwise (as described in the changelog >> of the patch below). >> >> Srivatsa, does this make sense to you? > > It makes atleast to me :) > >> This is not the only bad thing that happens there, however, because >> kobject_put() should only be called for the policy kobject at this >> point if the CPU is not the last one for that policy. >> >> Namely, if the given CPU is the last one for that policy, the >> policy kobject's refcount should be 1 at this point, as set by >> cpufreq_add_dev_interface(), and only needs to be dropped once for >> the kobject to go away. This actually happens under the cpu == 1 >> check, so it need not be done before by cpufreq_cpu_put(). > > But I see one more issue with this code. For the last cpu we are just > calling kobject_put() and not cpufreq_cpu_put() and hence call to > module_put() is skipped. I am not sure, but that will probably cause > a problem when we try to rmmod the module? But which module then? > As we can't compile cpufreq.c as module.. So, is this part of code junk? > And so can be removed? > I tried to address this concern in my other mail to Rafael. (Sorry I forgot to CC you on that!). Let me know what you think of that solution. Regards, Srivatsa S. Bhat -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: linux-pm/drivers/cpufreq/cpufreq.c =================================================================== --- linux-pm.orig/drivers/cpufreq/cpufreq.c +++ linux-pm/drivers/cpufreq/cpufreq.c @@ -1181,7 +1181,6 @@ static int __cpufreq_remove_dev(struct d __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT); pr_debug("%s: removing link, cpu: %d\n", __func__, cpu); - cpufreq_cpu_put(data); /* If cpu is last user of policy, free policy */ if (cpus == 1) { @@ -1205,9 +1204,12 @@ static int __cpufreq_remove_dev(struct d free_cpumask_var(data->related_cpus); free_cpumask_var(data->cpus); kfree(data); - } else if (cpufreq_driver->target) { - __cpufreq_governor(data, CPUFREQ_GOV_START); - __cpufreq_governor(data, CPUFREQ_GOV_LIMITS); + } else { + cpufreq_cpu_put(data); + if (cpufreq_driver->target) { + __cpufreq_governor(data, CPUFREQ_GOV_START); + __cpufreq_governor(data, CPUFREQ_GOV_LIMITS); + } } per_cpu(cpufreq_policy_cpu, cpu) = -1;