Messages in this thread | | | Date | Mon, 16 Sep 2013 22:38:05 +0530 | Subject | Re: [PATCH 1/2] cpufreq: unlock correct rwsem while updating policy->cpu | From | Viresh Kumar <> |
| |
On 16 September 2013 21:57, Jon Medhurst (Tixy) <tixy@linaro.org> wrote: > If I take mainline code and just change the line above to:
You meant this line by above line?
unlock_policy_rwsem_write(cpu);
> up_write(&per_cpu(cpu_policy_rwsem, (per_cpu(cpufreq_cpu_data, > cpu))->last_cpu)); > then the big_little cpufreq driver works for me.
That would be same as: up_write(&per_cpu(cpu_policy_rwsem, policy->last_cpu));
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> index 43c24aa..c18bf7b 100644 >> --- a/drivers/cpufreq/cpufreq.c >> +++ b/drivers/cpufreq/cpufreq.c >> @@ -952,9 +952,16 @@ static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu) >> if (cpu == policy->cpu) >> return; >> >> + /* take direct locks as lock_policy_rwsem_write wouldn't work here */ >> + down_write(&per_cpu(cpu_policy_rwsem, policy->cpu)); >> + down_write(&per_cpu(cpu_policy_rwsem, cpu)); >> + >> policy->last_cpu = policy->cpu; >> policy->cpu = cpu; >> >> + up_write(&per_cpu(cpu_policy_rwsem, cpu)); >> + up_write(&per_cpu(cpu_policy_rwsem, policy->cpu)); > > You've just overwritten policy->cpu with cpu.
Stupid enough :)
> I tried using > policy->last_cpu to fix that, but it still doesn't work for me (giving > the lockdep warning I mentioned.) If I change the code to just lock the > original policy->cpu lock only, then all is fine.
Fixed my patch now.. find attached.. It mentions why lock for last cpu is enough here. Copied that here too..
+ /* + * Take direct locks as lock_policy_rwsem_write wouldn't work here. + * Also lock for last cpu is enough here as contention will happen only + * after policy->cpu is changed and after it is changed, other threads + * will try to acquire lock for new cpu. And policy is already updated + * by then. + */
> Also, this locking is now just happening around policy->cpu update, > whereas before this change, it was locked for the whole of > update_policy_cpu, i.e. cpufreq_frequency_table_update_policy_cpu and > the notifier callbacks. Is that change of lock coverage OK?
Yeah, the rwsem is only required for updating policy's fields and so that should be okay. [unhandled content-type:application/octet-stream] | |