| Message ID | 466c5246b6ad83b960eeb63ab34a04379448980d.1424345053.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 2 April 2015 at 10:08, Saravana Kannan <skannan@codeaurora.org> wrote: > What's wrong with this behavior? If you cat scaling_min/max_freq, it's going > to show the last minimum and maximum freqs too. I don't think this needs to > be set to NULL until the governor is unloaded. Which you are already taking > care of in the previous patch. > > It's handy to be able to tell what governor will be restored when a cluster > if offline. > > Nacked because I think this patch is not helping. Oh, I really believe this patch makes sense. :) Don't confuse it with the 'last-governor' thing or scaling_min/max stuff.. Its different. Consider a scenario: - Cluster was using ONDEMAND governor. - Cluster hotplugged out - Governor removed, but the pointer policy->governor isn't updated. -> At this point accessing 'scaling_governor' or 'scaling_setspeed' can get called and that will result in a crash. - We go ahead and insert the governor again -> At this point also the same problem will happen as governor will get allocated again. Now, what we are talking about here is a pointer to the governor, not its name which is stored in 'last_governor' in the previous patch. We store that name as it is used for internal working of the core, not for userspace. What we expose to userspace must be consistent, and so if the governor is removed, we will not be able to provide 'scaling_setspeed' or 'scaling_governor' to userspace. Also, it is scaling-governor, not what the next governor is going to be. Plus, what should we print on scaling_setspeed of a cluster not running anymore ? And so why keep something that we can't provide to userspace all the time. That may break userspace if it relies on it.. -- 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
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index b7cd1bf97044..cab4cfdd3ebb 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1094,7 +1094,6 @@ static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu) if (likely(policy)) { /* Policy should be inactive here */ WARN_ON(!policy_is_inactive(policy)); - policy->governor = NULL; } return policy; @@ -1497,6 +1496,8 @@ static int __cpufreq_remove_dev_finish(struct device *dev, if (!cpufreq_suspended) cpufreq_policy_free(policy); + else + policy->governor = NULL; } else if (has_target()) { ret = __cpufreq_governor(policy, CPUFREQ_GOV_START); if (!ret)
Later commits would change the way policies are managed today. Policies wouldn't be freed on cpu hotplug (currently they aren't freed on suspend), and while the CPU is offline, the sysfs cpufreq files would still be present. Because we don't mark policy->governor as NULL, it still contains pointer of the last governor it used. And when we read the 'scaling_governor' file, it shows the old value. To prevent from this, mark policy->governor as NULL after we have issued CPUFREQ_GOV_POLICY_EXIT event for its governor. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/cpufreq/cpufreq.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)