[PATCH 1/2] cpufreq: unlock correct rwsem while updating policy->cpu

[PATCH 1/2] cpufreq: unlock correct rwsem while updating policy->cpu

[Viresh Kumar]

Current code looks like this:

        WARN_ON(lock_policy_rwsem_write(cpu));
        update_policy_cpu(policy, new_cpu);
        unlock_policy_rwsem_write(cpu);

{lock|unlock}_policy_rwsem_write(cpu) takes/releases policy->cpu's rwsem.
Because cpu is changing with the call to update_policy_cpu(), the
unlock_policy_rwsem_write() will release the incorrect lock.

The right solution would be to take rwsem lock/unlock for both old and new cpu.
This patch fixes this bug by taking both locks directly instead of calling
lock_policy_rwsem_write().

Reported-by: Jon Medhurst<tixy@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Hi Rafael,

Probably we can get this patch in for 3.12? The second one can go in 3.13.

These are compile tested only at my end. Tixy reported these and probably can
give his tested-by once he is done testing them.

 drivers/cpufreq/cpufreq.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

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));
+
 #ifdef CONFIG_CPU_FREQ_TABLE
 	cpufreq_frequency_table_update_policy_cpu(policy);
 #endif
@@ -1203,9 +1210,7 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
 
 		new_cpu = cpufreq_nominate_new_policy_cpu(policy, cpu, frozen);
 		if (new_cpu >= 0) {
-			WARN_ON(lock_policy_rwsem_write(cpu));
 			update_policy_cpu(policy, new_cpu);
-			unlock_policy_rwsem_write(cpu);
 
 			if (!frozen) {
 				pr_debug("%s: policy Kobject moved to cpu: %d "
-- 
1.7.12.rc2.18.g61b472e

[PATCH 2/2] cpufreq: make return type of lock_policy_rwsem_{read|write}() as void

[Viresh Kumar]

lock_policy_rwsem_{read|write}() currently has return type of int but it always
return zero and hence its return type must be void instead. This patch makes its
return type void and fixes all users of it as well.

Reported-by: Jon Medhurst<tixy@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 38 +++++++++++---------------------------
 1 file changed, 11 insertions(+), 27 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index c18bf7b..598af5c 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -67,13 +67,11 @@ static DEFINE_PER_CPU(char[CPUFREQ_NAME_LEN], cpufreq_cpu_governor);
 static DEFINE_PER_CPU(struct rw_semaphore, cpu_policy_rwsem);
 
 #define lock_policy_rwsem(mode, cpu)					\
-static int lock_policy_rwsem_##mode(int cpu)				\
+static void lock_policy_rwsem_##mode(int cpu)				\
 {									\
 	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);	\
 	BUG_ON(!policy);						\
 	down_##mode(&per_cpu(cpu_policy_rwsem, policy->cpu));		\
-									\
-	return 0;							\
 }
 
 lock_policy_rwsem(read, cpu);
@@ -653,13 +651,12 @@ static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)
 {
 	struct cpufreq_policy *policy = to_policy(kobj);
 	struct freq_attr *fattr = to_attr(attr);
-	ssize_t ret = -EINVAL;
+	ssize_t ret;
 
 	if (!down_read_trylock(&cpufreq_rwsem))
-		goto exit;
+		return -EINVAL;
 
-	if (lock_policy_rwsem_read(policy->cpu) < 0)
-		goto up_read;
+	lock_policy_rwsem_read(policy->cpu);
 
 	if (fattr->show)
 		ret = fattr->show(policy, buf);
@@ -667,10 +664,8 @@ static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)
 		ret = -EIO;
 
 	unlock_policy_rwsem_read(policy->cpu);
-
-up_read:
 	up_read(&cpufreq_rwsem);
-exit:
+
 	return ret;
 }
 
@@ -689,8 +684,7 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
 	if (!down_read_trylock(&cpufreq_rwsem))
 		goto unlock;
 
-	if (lock_policy_rwsem_write(policy->cpu) < 0)
-		goto up_read;
+	lock_policy_rwsem_write(policy->cpu);
 
 	if (fattr->store)
 		ret = fattr->store(policy, buf, count);
@@ -699,7 +693,6 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
 
 	unlock_policy_rwsem_write(policy->cpu);
 
-up_read:
 	up_read(&cpufreq_rwsem);
 unlock:
 	put_online_cpus();
@@ -1143,7 +1136,7 @@ static int cpufreq_nominate_new_policy_cpu(struct cpufreq_policy *policy,
 	if (ret) {
 		pr_err("%s: Failed to move kobj: %d", __func__, ret);
 
-		WARN_ON(lock_policy_rwsem_write(old_cpu));
+		lock_policy_rwsem_write(old_cpu);
 		cpumask_set_cpu(old_cpu, policy->cpus);
 		unlock_policy_rwsem_write(old_cpu);
 
@@ -1196,7 +1189,7 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
 			policy->governor->name, CPUFREQ_NAME_LEN);
 #endif
 
-	WARN_ON(lock_policy_rwsem_write(cpu));
+	lock_policy_rwsem_write(cpu);
 	cpus = cpumask_weight(policy->cpus);
 
 	if (cpus > 1)
@@ -1459,14 +1452,11 @@ unsigned int cpufreq_get(unsigned int cpu)
 	if (!down_read_trylock(&cpufreq_rwsem))
 		return 0;
 
-	if (unlikely(lock_policy_rwsem_read(cpu)))
-		goto out_policy;
+	lock_policy_rwsem_read(cpu);
 
 	ret_freq = __cpufreq_get(cpu);
 
 	unlock_policy_rwsem_read(cpu);
-
-out_policy:
 	up_read(&cpufreq_rwsem);
 
 	return ret_freq;
@@ -1690,14 +1680,12 @@ int cpufreq_driver_target(struct cpufreq_policy *policy,
 {
 	int ret = -EINVAL;
 
-	if (unlikely(lock_policy_rwsem_write(policy->cpu)))
-		goto fail;
+	lock_policy_rwsem_write(policy->cpu);
 
 	ret = __cpufreq_driver_target(policy, target_freq, relation);
 
 	unlock_policy_rwsem_write(policy->cpu);
 
-fail:
 	return ret;
 }
 EXPORT_SYMBOL_GPL(cpufreq_driver_target);
@@ -1988,10 +1976,7 @@ int cpufreq_update_policy(unsigned int cpu)
 		goto no_policy;
 	}
 
-	if (unlikely(lock_policy_rwsem_write(cpu))) {
-		ret = -EINVAL;
-		goto fail;
-	}
+	lock_policy_rwsem_write(cpu);
 
 	pr_debug("updating policy for CPU %u\n", cpu);
 	memcpy(&new_policy, policy, sizeof(*policy));
@@ -2020,7 +2005,6 @@ int cpufreq_update_policy(unsigned int cpu)
 
 	unlock_policy_rwsem_write(cpu);
 
-fail:
 	cpufreq_cpu_put(policy);
 no_policy:
 	return ret;
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 1/2] cpufreq: unlock correct rwsem while updating policy->cpu

[Jon Medhurst (Tixy)]

On Mon, 2013-09-16 at 20:40 +0530, Viresh Kumar wrote:
> Current code looks like this:
> 
>         WARN_ON(lock_policy_rwsem_write(cpu));
>         update_policy_cpu(policy, new_cpu);
>         unlock_policy_rwsem_write(cpu);
> 
> {lock|unlock}_policy_rwsem_write(cpu) takes/releases policy->cpu's rwsem.
> Because cpu is changing with the call to update_policy_cpu(), the
> unlock_policy_rwsem_write() will release the incorrect lock.
> 
> The right solution would be to take rwsem lock/unlock for both old and new cpu.

I thought possibly something like that, then wondered if threads could
take the locks in different orders at the same time, leading to
deadlock? So as I wasn't familiar with cpufreq I thought I'd leave it to
the experts to worry about :-)

This patch contains a bug, see inline comment below. Even with that
fixed it still doesn't work for me. I get a lockdep warning (below). I
have verified the cpu and locks are different locks, and it's not a
harmless false positive because I later get the message 'cpufreq:
__cpufreq_remove_dev_prepare: Failed to stop governor'.

=============================================
[ INFO: possible recursive locking detected ]
3.11.0+ #4 Not tainted
---------------------------------------------
swapper/0/1 is trying to acquire lock:
  (&per_cpu(cpu_policy_rwsem, cpu)){+.+...}, at: [<c0293c03>] update_policy_cpu+0x53/0xa4

but task is already holding lock:
  (&per_cpu(cpu_policy_rwsem, cpu)){+.+...}, at: [<c0293bef>] update_policy_cpu+0x3f/0xa4
other info that might help us debug this:
  Possible unsafe locking scenario:
        CPU0
        ----
   lock(&per_cpu(cpu_policy_rwsem, cpu));
   lock(&per_cpu(cpu_policy_rwsem, cpu));

*** DEADLOCK ***

  May be due to missing lock nesting notation
4 locks held by swapper/0/1:
 #0:  (bL_switcher_activation_lock){+.+.+.}, at: [<c0019b03>] bL_switcher_enable+0x17/0x2d8
 #1:  ((bL_activation_notifier).rwsem){.+.+.+}, at: [<c00370bd>] __blocking_notifier_call_chain+0x1d/0x40
 #2:  (subsys mutex#6){+.+.+.}, at: [<c023279d>] subsys_interface_unregister+0x1d/0x68
 #3:  (&per_cpu(cpu_policy_rwsem, cpu)){+.+...}, at: [<c0293bef>] update_policy_cpu+0x3f/0xa4

> This patch fixes this bug by taking both locks directly instead of calling
> lock_policy_rwsem_write().
> 
> Reported-by: Jon Medhurst<tixy@linaro.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> Hi Rafael,
> 
> Probably we can get this patch in for 3.12? The second one can go in 3.13.
> 
> These are compile tested only at my end. Tixy reported these and probably can
> give his tested-by once he is done testing them.
> 
>  drivers/cpufreq/cpufreq.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
If I take mainline code and just change the line above to:
   up_write(&per_cpu(cpu_policy_rwsem, (per_cpu(cpufreq_cpu_data,
cpu))->last_cpu));
then the big_little cpufreq driver works for me.

> 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. 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.

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?

> +
>  #ifdef CONFIG_CPU_FREQ_TABLE
>  	cpufreq_frequency_table_update_policy_cpu(policy);
>  #endif
> @@ -1203,9 +1210,7 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
>  
>  		new_cpu = cpufreq_nominate_new_policy_cpu(policy, cpu, frozen);
>  		if (new_cpu >= 0) {
> -			WARN_ON(lock_policy_rwsem_write(cpu));
>  			update_policy_cpu(policy, new_cpu);
> -			unlock_policy_rwsem_write(cpu);
>  
>  			if (!frozen) {
>  				pr_debug("%s: policy Kobject moved to cpu: %d "

-- 
Tixy

Re: [PATCH 1/2] cpufreq: unlock correct rwsem while updating policy->cpu

[Viresh Kumar]

[-- Attachment #1: Type: text/plain, Size: 2217 bytes --]

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.

[-- Attachment #2: 0001-cpufreq-unlock-correct-rwsem-while-updating-policy-c.patch --]
[-- Type: application/octet-stream, Size: 2344 bytes --]

From 51e3458f7853cca99cfbdfa5ac419dfde471c5fa Mon Sep 17 00:00:00 2001
Message-Id: <51e3458f7853cca99cfbdfa5ac419dfde471c5fa.1379351158.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Mon, 16 Sep 2013 20:17:08 +0530
Subject: [PATCH] cpufreq: unlock correct rwsem while updating policy->cpu

Current code looks like this:

        WARN_ON(lock_policy_rwsem_write(cpu));
        update_policy_cpu(policy, new_cpu);
        unlock_policy_rwsem_write(cpu);

{lock|unlock}_policy_rwsem_write(cpu) takes/releases policy->cpu's rwsem.
Because cpu is changing with the call to update_policy_cpu(), the
unlock_policy_rwsem_write() will release the incorrect lock.

The right solution would be to take rwsem lock/unlock for both old and new cpu.
This patch fixes this bug by taking both locks directly instead of calling
lock_policy_rwsem_write().

Reported-by: Jon Medhurst<tixy@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 43c24aa..1479522 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -952,9 +952,20 @@ 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.
+	 * 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.
+	 */
+	down_write(&per_cpu(cpu_policy_rwsem, policy->cpu));
+
 	policy->last_cpu = policy->cpu;
 	policy->cpu = cpu;
 
+	up_write(&per_cpu(cpu_policy_rwsem, policy->last_cpu));
+
 #ifdef CONFIG_CPU_FREQ_TABLE
 	cpufreq_frequency_table_update_policy_cpu(policy);
 #endif
@@ -1203,9 +1214,7 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
 
 		new_cpu = cpufreq_nominate_new_policy_cpu(policy, cpu, frozen);
 		if (new_cpu >= 0) {
-			WARN_ON(lock_policy_rwsem_write(cpu));
 			update_policy_cpu(policy, new_cpu);
-			unlock_policy_rwsem_write(cpu);
 
 			if (!frozen) {
 				pr_debug("%s: policy Kobject moved to cpu: %d "
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 1/2] cpufreq: unlock correct rwsem while updating policy->cpu

[Rafael J. Wysocki]

On Monday, September 16, 2013 10:38:05 PM Viresh Kumar wrote:
> 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..

Care to resend with a subject indicating that that's a patch update?

Like [PATCH v2] etc. or similar?

Rafael

Re: [PATCH 1/2] cpufreq: unlock correct rwsem while updating policy->cpu

[Jon Medhurst (Tixy)]

On Mon, 2013-09-16 at 22:38 +0530, Viresh Kumar wrote:
> 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);

Yes.

> >    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));

Yes. (I had cut'n'pasted from the unlock_policy_rwsem_ macro)

> >> 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..

The commit log to that patch still mentions taking both locks.
The code itself fixes the lockdep errors I had, so

Tested-by: Jon Medhurst <tixy@linaro.org>

however, I still have concerns about the locking (below)...

>  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.

But what about reads, like in cpufreq_frequency_table_update_policy_cpu
which is called immediately after the unlocking writes? Should that not
be covered by a reader lock?

And after that call, policy is passed into blocking_notifier_call_chain,
do those callbacks not want to look at policy fields? Or are they going
to do there own locking?

Or is update_policy_cpu itself meant to be called with a read lock held?
(It doesn't appear to be as the semaphore 'activiy' field of the lock is
zero).

Or is there some other non-obvious synchronisation method which means
the policy object can't change?

This is the first time I've looked at this code, so feel free just to
say 'it's complicated, just trust me, I'm the expert, I know what I'm
doing'...

-- 
Tixy

Re: [PATCH 1/2] cpufreq: unlock correct rwsem while updating policy->cpu

[Viresh Kumar]

On 17 September 2013 00:04, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> Care to resend with a subject indicating that that's a patch update?
>
> Like [PATCH v2] etc. or similar?

Yeah.. I was waiting for Tixy to test it once..

Re: [PATCH 1/2] cpufreq: unlock correct rwsem while updating policy->cpu

[Viresh Kumar]

On 17 September 2013 00:12, Jon Medhurst (Tixy) <tixy@linaro.org> wrote:
> The commit log to that patch still mentions taking both locks.

Yeah, it was sent in hurry to just give you a working solution and I forgot
to see the log if it is still valid.

> The code itself fixes the lockdep errors I had, so
>
> Tested-by: Jon Medhurst <tixy@linaro.org>

Great!!

> however, I still have concerns about the locking (below)...

:(

> But what about reads, like in cpufreq_frequency_table_update_policy_cpu
> which is called immediately after the unlocking writes? Should that not
> be covered by a reader lock?
>
> And after that call, policy is passed into blocking_notifier_call_chain,
> do those callbacks not want to look at policy fields? Or are they going
> to do there own locking?

policy->cpu is used at multiple places outside of cpufreq.c and cpufreq
core can't really put read locks for those accesses. Things will turn bad
only if cpufreq core has got these races where we are trying to access
a struct or pointer that belonged to the last policy->cpu, which is updated
now.. For example the case of lock you reported.. And so lock is required
only for those places..

> Or is update_policy_cpu itself meant to be called with a read lock held?

No.

> This is the first time I've looked at this code, so feel free just to
> say 'it's complicated, just trust me, I'm the expert, I know what I'm
> doing'...

We aren't that rude :)

Now, that you have tested this patch let me resent it...

Re: [PATCH 2/2] cpufreq: make return type of lock_policy_rwsem_{read|write}() as void

[Jon Medhurst (Tixy)]

On Mon, 2013-09-16 at 20:40 +0530, Viresh Kumar wrote:
> lock_policy_rwsem_{read|write}() currently has return type of int but it always
> return zero and hence its return type must be void instead. This patch makes its
> return type void and fixes all users of it as well.
> 
> Reported-by: Jon Medhurst<tixy@linaro.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Tested-by: Jon Medhurst <tixy@linaro.org>

> ---
>  drivers/cpufreq/cpufreq.c | 38 +++++++++++---------------------------
>  1 file changed, 11 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index c18bf7b..598af5c 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -67,13 +67,11 @@ static DEFINE_PER_CPU(char[CPUFREQ_NAME_LEN], cpufreq_cpu_governor);
>  static DEFINE_PER_CPU(struct rw_semaphore, cpu_policy_rwsem);
>  
>  #define lock_policy_rwsem(mode, cpu)					\
> -static int lock_policy_rwsem_##mode(int cpu)				\
> +static void lock_policy_rwsem_##mode(int cpu)				\
>  {									\
>  	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);	\
>  	BUG_ON(!policy);						\
>  	down_##mode(&per_cpu(cpu_policy_rwsem, policy->cpu));		\
> -									\
> -	return 0;							\
>  }
>  
>  lock_policy_rwsem(read, cpu);
> @@ -653,13 +651,12 @@ static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)
>  {
>  	struct cpufreq_policy *policy = to_policy(kobj);
>  	struct freq_attr *fattr = to_attr(attr);
> -	ssize_t ret = -EINVAL;
> +	ssize_t ret;
>  
>  	if (!down_read_trylock(&cpufreq_rwsem))
> -		goto exit;
> +		return -EINVAL;
>  
> -	if (lock_policy_rwsem_read(policy->cpu) < 0)
> -		goto up_read;
> +	lock_policy_rwsem_read(policy->cpu);
>  
>  	if (fattr->show)
>  		ret = fattr->show(policy, buf);
> @@ -667,10 +664,8 @@ static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)
>  		ret = -EIO;
>  
>  	unlock_policy_rwsem_read(policy->cpu);
> -
> -up_read:
>  	up_read(&cpufreq_rwsem);
> -exit:
> +
>  	return ret;
>  }
>  
> @@ -689,8 +684,7 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
>  	if (!down_read_trylock(&cpufreq_rwsem))
>  		goto unlock;
>  
> -	if (lock_policy_rwsem_write(policy->cpu) < 0)
> -		goto up_read;
> +	lock_policy_rwsem_write(policy->cpu);
>  
>  	if (fattr->store)
>  		ret = fattr->store(policy, buf, count);
> @@ -699,7 +693,6 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
>  
>  	unlock_policy_rwsem_write(policy->cpu);
>  
> -up_read:
>  	up_read(&cpufreq_rwsem);
>  unlock:
>  	put_online_cpus();
> @@ -1143,7 +1136,7 @@ static int cpufreq_nominate_new_policy_cpu(struct cpufreq_policy *policy,
>  	if (ret) {
>  		pr_err("%s: Failed to move kobj: %d", __func__, ret);
>  
> -		WARN_ON(lock_policy_rwsem_write(old_cpu));
> +		lock_policy_rwsem_write(old_cpu);
>  		cpumask_set_cpu(old_cpu, policy->cpus);
>  		unlock_policy_rwsem_write(old_cpu);
>  
> @@ -1196,7 +1189,7 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
>  			policy->governor->name, CPUFREQ_NAME_LEN);
>  #endif
>  
> -	WARN_ON(lock_policy_rwsem_write(cpu));
> +	lock_policy_rwsem_write(cpu);
>  	cpus = cpumask_weight(policy->cpus);
>  
>  	if (cpus > 1)
> @@ -1459,14 +1452,11 @@ unsigned int cpufreq_get(unsigned int cpu)
>  	if (!down_read_trylock(&cpufreq_rwsem))
>  		return 0;
>  
> -	if (unlikely(lock_policy_rwsem_read(cpu)))
> -		goto out_policy;
> +	lock_policy_rwsem_read(cpu);
>  
>  	ret_freq = __cpufreq_get(cpu);
>  
>  	unlock_policy_rwsem_read(cpu);
> -
> -out_policy:
>  	up_read(&cpufreq_rwsem);
>  
>  	return ret_freq;
> @@ -1690,14 +1680,12 @@ int cpufreq_driver_target(struct cpufreq_policy *policy,
>  {
>  	int ret = -EINVAL;
>  
> -	if (unlikely(lock_policy_rwsem_write(policy->cpu)))
> -		goto fail;
> +	lock_policy_rwsem_write(policy->cpu);
>  
>  	ret = __cpufreq_driver_target(policy, target_freq, relation);
>  
>  	unlock_policy_rwsem_write(policy->cpu);
>  
> -fail:
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(cpufreq_driver_target);
> @@ -1988,10 +1976,7 @@ int cpufreq_update_policy(unsigned int cpu)
>  		goto no_policy;
>  	}
>  
> -	if (unlikely(lock_policy_rwsem_write(cpu))) {
> -		ret = -EINVAL;
> -		goto fail;
> -	}
> +	lock_policy_rwsem_write(cpu);
>  
>  	pr_debug("updating policy for CPU %u\n", cpu);
>  	memcpy(&new_policy, policy, sizeof(*policy));
> @@ -2020,7 +2005,6 @@ int cpufreq_update_policy(unsigned int cpu)
>  
>  	unlock_policy_rwsem_write(cpu);
>  
> -fail:
>  	cpufreq_cpu_put(policy);
>  no_policy:
>  	return ret;

Re: [PATCH 2/2] cpufreq: make return type of lock_policy_rwsem_{read|write}() as void

[Rafael J. Wysocki]

On Monday, September 16, 2013 08:40:17 PM Viresh Kumar wrote:
> lock_policy_rwsem_{read|write}() currently has return type of int but it always
> return zero and hence its return type must be void instead. This patch makes its
> return type void and fixes all users of it as well.
> 
> Reported-by: Jon Medhurst<tixy@linaro.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

This doesn't apply to bleeding-edge for me any more after I have applied your
series of 44 patches to it.  Care to resend?

Rafael

Re: [PATCH 2/2] cpufreq: make return type of lock_policy_rwsem_{read|write}() as void

[Viresh Kumar]

On 30 September 2013 23:58, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Monday, September 16, 2013 08:40:17 PM Viresh Kumar wrote:
>> lock_policy_rwsem_{read|write}() currently has return type of int but it always
>> return zero and hence its return type must be void instead. This patch makes its
>> return type void and fixes all users of it as well.
>>
>> Reported-by: Jon Medhurst<tixy@linaro.org>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>
> This doesn't apply to bleeding-edge for me any more after I have applied your
> series of 44 patches to it.  Care to resend?

Done..

Re: [PATCH 2/2] cpufreq: make return type of lock_policy_rwsem_{read|write}() as void

[Rafael J. Wysocki]

On Wednesday, October 02, 2013 02:13:36 PM Viresh Kumar wrote:
> On 30 September 2013 23:58, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Monday, September 16, 2013 08:40:17 PM Viresh Kumar wrote:
> >> lock_policy_rwsem_{read|write}() currently has return type of int but it always
> >> return zero and hence its return type must be void instead. This patch makes its
> >> return type void and fixes all users of it as well.
> >>
> >> Reported-by: Jon Medhurst<tixy@linaro.org>
> >> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> >
> > This doesn't apply to bleeding-edge for me any more after I have applied your
> > series of 44 patches to it.  Care to resend?
> 
> Done..

Which one is it?

Re: [PATCH 2/2] cpufreq: make return type of lock_policy_rwsem_{read|write}() as void

[Viresh Kumar]

On 2 October 2013 22:08, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Wednesday, October 02, 2013 02:13:36 PM Viresh Kumar wrote:
>> On 30 September 2013 23:58, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > On Monday, September 16, 2013 08:40:17 PM Viresh Kumar wrote:
>> >> lock_policy_rwsem_{read|write}() currently has return type of int but it always
>> >> return zero and hence its return type must be void instead. This patch makes its
>> >> return type void and fixes all users of it as well.
>> >>
>> >> Reported-by: Jon Medhurst<tixy@linaro.org>
>> >> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>> >
>> > This doesn't apply to bleeding-edge for me any more after I have applied your
>> > series of 44 patches to it.  Care to resend?
>>
>> Done..
>
> Which one is it?
>

 [PATCH RESEND 01/11] cpufreq: make return type of
lock_policy_rwsem_{read|write}() as void