On 8 August 2013 13:09, Jon Medhurst (Tixy) tixy@linaro.org wrote:
On Wed, 2013-08-07 at 18:52 +0530, Viresh Kumar wrote:
While getting support for In-Kernel-Switcher in big LITTLE cpufreq driver we introduced cpu_last_req_freq per-cpu variable that stores the last frequency requested for a cpu. It was important for IKS as CPUs in the same cluster can have separate struct cpufreq_policy associated with them and so cpufreq core may try to set different frequencies on both CPUs of same cluster.
But for non-IKS solution or MP we don't need to cache last requested frequency. Currently there is a bug in code where if cpufreq_driver->get() is called for any cpu other than policy->cpu, we are returning 0, because we set cpu_last_req_freq only for policy->cpu and not for others.
Do you know what the observable symptoms of this are?
Ok.. I skipped the history behind this commit earlier.. Lets have it now:
I was playing around with cpufreq stuff some time back, on tc2 using ondemand governor. I got stuck on a bug for sometime where taking cpu4 down and bringing it back crashed my system..
Then I spent some time trying to understand what's going wrong and found above to be the culprit.
Sequence of events is like this: - offline cpu4 (2nd A15, 1st is still there) and online it back. -> cpufreq.c: cpufreq_cpu_callback(CPU_ONLINE) -> cpufreq_update_policy(4) -> cpufreq_driver->get(4) and zero is returned due to mentioned issue -> cpufreq_out_of_sync() is called and it sets policy->cur = 0..
-> cpufreq_governor.c: cpufreq_governor_dbs(CPUFREQ_GOV_START) -> It checks below and returns error if (!policy->cur) return -EINVAL;
-> Caller of cpufreq_governor_dbs(CPUFREQ_GOV_START) wasn't checking return values of this function (Though I fixed it now in a separate thread).. and called cpufreq_governor_dbs(CPUFREQ_GOV_LIMITS) -> cpufreq_governor_dbs(CPUFREQ_GOV_LIMITS) tried to access: cpu_cdbs->cur_policy->cur and cpu_cdbs->cur_policy was set to NULL earlier when CPUFREQ_GOV_STOP was called while removing cpu.
And so the crash.
As this is a fix which needs to get into LSK I've added the linaro-kernel list to the cc so it has more public visibility.
Thanks. Adding Broonie directly as well so that he doesn't miss it :)