On 2 April 2015 at 10:04, Saravana Kannan skannan@codeaurora.org wrote:
This governor restore code actually has bug -- at least in 3.12 (I think) but probably has a bug in this kernel too. So, this patch actually also fixes a bug.
The bug has to do with restoring the wrong governor (as in, not the governor that was last picked by userspace) for a particular hot plug sequence.
Assume one cluster has CPUs 2, 3. Assume default governor is performance. CPU2 is first brought online. Governor is changed to ondemand. CPU2 is taken offline. CPU3 is brought online. Governor gets set to performance. So, governor for the cluster is now not what was last picked by userspace.
Wasn't aware of that, you are right. Updated my logs for this.
for_each_present_cpu(cpu) {if (cpu_online(cpu))continue;if (!strcmp(per_cpu(cpufreq_cpu_governor, cpu),governor->name))
strcpy(per_cpu(cpufreq_cpu_governor, cpu), "\0");
/* clear last_governor for all inactive policies */read_lock_irqsave(&cpufreq_driver_lock, flags);You are writing to a variable that's protected. This should be a write lock.
cpufreq_driver_lock isn't responsible for protecting policy fields, but the list of policies. And so was a read lock here.
for_each_inactive_policy(policy, tpolicy) {if (!strcmp(policy->last_governor, governor->name))strcpy(policy->last_governor, "\0"); }read_unlock_irqrestore(&cpufreq_driver_lock, flags);
And we don't need to take writelock to policy->rwsem here as the policy is already inactive.
Nack due to lock.
Just to clear the concepts, and not fighting with you :)
I don't think you used Nack correctly here. You have raised some mistakes in the patch here, and its not that the patch and the idea behind it is not acceptable.
So, probably Nack was a bit harsh :)