[PATCH 1/5] cpufreq, do not return stale data to userspace

[PATCH 1/5] cpufreq, do not return stale data to userspace

[Prarit Bhargava]

Consider the following case.  Two threads are executing on a system.
Thread 1 is reading from /sys/.../cpufreq/<files> and thread 2 is
changing the cpufreq governor through /sys/.../cpufreq/scaling_governor.

Thread 2 acquires the mutexes in the write path, cpufreq_rwsem and
policy->rwsem, while thread 1 waits.

Thread 2 completes the changing of the scaling governor and releases
the the mutexes.

Thread 1 now acquires the mutexes and returns incorrect data as the
governor has changed.

The kernel cannot guarantee the governor from which the data came from, so
the kernel should fail with -EBUSY when the governor is being written.
Changing the down_read(&policy->rwsem) to a trylock fixes this stale data
issue.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Prarit Bhargava <prarit@redhat.com>
---
 drivers/cpufreq/cpufreq.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 644b54e..3f09ca9 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -765,7 +765,8 @@ static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)
 	if (!down_read_trylock(&cpufreq_rwsem))
 		return -EINVAL;
 
-	down_read(&policy->rwsem);
+	if (!down_read_trylock(&policy->rwsem))
+		return -EBUSY;
 
 	if (fattr->show)
 		ret = fattr->show(policy, buf);
-- 
1.7.9.3

[PATCH 2/5] cpufreq, fix locking around CPUFREQ_GOV_POLICY_EXIT calls

[Prarit Bhargava]

commit 955ef4833574636819cd269cfbae12f79cbde63a (" cpufreq: Drop rwsem
lock around CPUFREQ_GOV_POLICY_EXIT") opens up a hole in the locking
scheme for cpufreq.

Simple tests such as rapidly switching the governor between ondemand and
performance or attempting to read policy values while a governor switch occurs
now fail with very NULL pointer warnings, sysfs namespace collisions, and
system hangs.  In short, the locking that policy->rwsem is supposed to provide
is currently broken.

The identified commit attempts to resolve a lockdep warning by removing
a lock around a section of code which does a shutdown of the
existing policy.  The problem is that this is part of the _critical_ section of
code that switches the governors and must be protected by the lock; without
locking readers may access now NULL or stale data, and writes may collide with
each other.

With the previous patch, which now returns -EBUSY during times of
contention the deadlock reported in
955ef4833574636819cd269cfbae12f79cbde63a (" cpufreq: Drop rwsem lock
around CPUFREQ_GOV_POLICY_EXIT") cannot occur, so adding the locks back
into this section of code is possible.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Prarit Bhargava <prarit@redhat.com>
---
 drivers/cpufreq/cpufreq.c |    4 ----
 include/linux/cpufreq.h   |    4 ----
 2 files changed, 8 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 3f09ca9..e33cb15 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2222,9 +2222,7 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
 	/* end old governor */
 	if (old_gov) {
 		__cpufreq_governor(policy, CPUFREQ_GOV_STOP);
-		up_write(&policy->rwsem);
 		__cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
-		down_write(&policy->rwsem);
 	}
 
 	/* start new governor */
@@ -2233,9 +2231,7 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
 		if (!__cpufreq_governor(policy, CPUFREQ_GOV_START))
 			goto out;
 
-		up_write(&policy->rwsem);
 		__cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
-		down_write(&policy->rwsem);
 	}
 
 	/* new governor failed, so re-start old one */
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 503b085..43909ad 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -100,10 +100,6 @@ struct cpufreq_policy {
 	 * - Any routine that will write to the policy structure and/or may take away
 	 *   the policy altogether (eg. CPU hotplug), will hold this lock in write
 	 *   mode before doing so.
-	 *
-	 * Additional rules:
-	 * - Lock should not be held across
-	 *     __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
 	 */
 	struct rw_semaphore	rwsem;
 
-- 
1.7.9.3

Re: [PATCH 2/5] cpufreq, fix locking around CPUFREQ_GOV_POLICY_EXIT calls

[Viresh Kumar]

On 5 November 2014 20:23, Prarit Bhargava <prarit@redhat.com> wrote:
> commit 955ef4833574636819cd269cfbae12f79cbde63a (" cpufreq: Drop rwsem
> lock around CPUFREQ_GOV_POLICY_EXIT") opens up a hole in the locking
> scheme for cpufreq.
>
> Simple tests such as rapidly switching the governor between ondemand and
> performance or attempting to read policy values while a governor switch occurs
> now fail with very NULL pointer warnings, sysfs namespace collisions, and
> system hangs.  In short, the locking that policy->rwsem is supposed to provide
> is currently broken.
>
> The identified commit attempts to resolve a lockdep warning by removing
> a lock around a section of code which does a shutdown of the
> existing policy.  The problem is that this is part of the _critical_ section of
> code that switches the governors and must be protected by the lock; without
> locking readers may access now NULL or stale data, and writes may collide with
> each other.
>
> With the previous patch, which now returns -EBUSY during times of
> contention the deadlock reported in
> 955ef4833574636819cd269cfbae12f79cbde63a (" cpufreq: Drop rwsem lock
> around CPUFREQ_GOV_POLICY_EXIT") cannot occur, so adding the locks back
> into this section of code is possible.

I still fail to understand why ? What will the _trylock() change ?

Re: [PATCH 2/5] cpufreq, fix locking around CPUFREQ_GOV_POLICY_EXIT calls

[Prarit Bhargava]

On 11/10/2014 05:44 AM, Viresh Kumar wrote:
> On 5 November 2014 20:23, Prarit Bhargava <prarit@redhat.com> wrote:
>> commit 955ef4833574636819cd269cfbae12f79cbde63a (" cpufreq: Drop rwsem
>> lock around CPUFREQ_GOV_POLICY_EXIT") opens up a hole in the locking
>> scheme for cpufreq.
>>
>> Simple tests such as rapidly switching the governor between ondemand and
>> performance or attempting to read policy values while a governor switch occurs
>> now fail with very NULL pointer warnings, sysfs namespace collisions, and
>> system hangs.  In short, the locking that policy->rwsem is supposed to provide
>> is currently broken.
>>
>> The identified commit attempts to resolve a lockdep warning by removing
>> a lock around a section of code which does a shutdown of the
>> existing policy.  The problem is that this is part of the _critical_ section of
>> code that switches the governors and must be protected by the lock; without
>> locking readers may access now NULL or stale data, and writes may collide with
>> each other.
>>
>> With the previous patch, which now returns -EBUSY during times of
>> contention the deadlock reported in
>> 955ef4833574636819cd269cfbae12f79cbde63a (" cpufreq: Drop rwsem lock
>> around CPUFREQ_GOV_POLICY_EXIT") cannot occur, so adding the locks back
>> into this section of code is possible.
> 
> I still fail to understand why ? What will the _trylock() change ?

viresh, afaict read_trylock will return 0 when busy with write:

static inline int queue_read_trylock(struct qrwlock *lock)
{
        u32 cnts;

        cnts = atomic_read(&lock->cnts);
        if (likely(!(cnts & _QW_WMASK))) {

so the deadlock will not occur.  the show() is opened, write lock is taken, and
if the thread is rescheduled and takes read lock the trylock will return 0, and
the thread will return -EBUSY to userspace avoiding the deadlock.

P.
> 

Re: [PATCH 2/5] cpufreq, fix locking around CPUFREQ_GOV_POLICY_EXIT calls

[Viresh Kumar]

On 10 November 2014 17:56, Prarit Bhargava <prarit@redhat.com> wrote:

>> I still fail to understand why ? What will the _trylock() change ?
>
> viresh, afaict read_trylock will return 0 when busy with write:

Yes..

> static inline int queue_read_trylock(struct qrwlock *lock)
> {
>         u32 cnts;
>
>         cnts = atomic_read(&lock->cnts);
>         if (likely(!(cnts & _QW_WMASK))) {
>
> so the deadlock will not occur.  the show() is opened, write lock is taken, and
> if the thread is rescheduled and takes read lock the trylock will return 0, and
> the thread will return -EBUSY to userspace avoiding the deadlock.

Which deadlock? And also your changelog talks about accessing invalid pointers
without the trylock change, how can that be possible? After the read
lock is taken,
all the pointers should be valid.

Re: [PATCH 2/5] cpufreq, fix locking around CPUFREQ_GOV_POLICY_EXIT calls

[Prarit Bhargava]

On 11/10/2014 10:37 PM, Viresh Kumar wrote:
> On 10 November 2014 17:56, Prarit Bhargava <prarit@redhat.com> wrote:
> 
>>> I still fail to understand why ? What will the _trylock() change ?
>>
>> viresh, afaict read_trylock will return 0 when busy with write:
> 
> Yes..
> 
>> static inline int queue_read_trylock(struct qrwlock *lock)
>> {
>>         u32 cnts;
>>
>>         cnts = atomic_read(&lock->cnts);
>>         if (likely(!(cnts & _QW_WMASK))) {
>>
>> so the deadlock will not occur.  the show() is opened, write lock is taken, and
>> if the thread is rescheduled and takes read lock the trylock will return 0, and
>> the thread will return -EBUSY to userspace avoiding the deadlock.
> 
> Which deadlock? 

the deadlock in commit 955ef4833574636819cd269cfbae12f79cbde63a

[   75.471265]        CPU0                    CPU1
[   75.476327]        ----                    ----
[   75.481385]   lock(&policy->rwsem);
[   75.485307]                                lock(s_active#219);
[   75.491857]                                lock(&policy->rwsem);
[   75.498592]   lock(s_active#219);
[   75.502331]
[   75.502331]  *** DEADLOCK ***

And also your changelog talks about accessing invalid pointers
> without the trylock change, how can that be possible? After the read
> lock is taken,
> all the pointers should be valid.

consider the following very simple case:

the governor is ondemand.  cpu 0 reads cpuinfo_cur_freq. cpu0 expects to get the
current cpu freq for the ondemand governor.

simultaneously, cpu1 changes the governor from ondemand to userspace.

the two threads will race for the policy->mutex

suppose cpu0 gets it first.  then there is no problem.  the userspace program
for cpu0 gets exactly the data it is expecting.

Now suppose cpu1 gets the lock and starts to write ... cpu0 is blocked.

cpu1 completes the governor change, and cpu0 gets the mutex ... and returns
bogus data at this point.

P.
> 

Re: [PATCH 2/5] cpufreq, fix locking around CPUFREQ_GOV_POLICY_EXIT calls

[Viresh Kumar]

On 11 November 2014 17:45, Prarit Bhargava <prarit@redhat.com> wrote:
> the deadlock in commit 955ef4833574636819cd269cfbae12f79cbde63a
>
> [   75.471265]        CPU0                    CPU1
> [   75.476327]        ----                    ----
> [   75.481385]   lock(&policy->rwsem);
> [   75.485307]                                lock(s_active#219);
> [   75.491857]                                lock(&policy->rwsem);
> [   75.498592]   lock(s_active#219);
> [   75.502331]
> [   75.502331]  *** DEADLOCK ***

I wanted to understand how this deadlock is prevented by a simple change
to trylock..

>> And also your changelog talks about accessing invalid pointers
>> without the trylock change, how can that be possible? After the read
>> lock is taken,
>> all the pointers should be valid.
>
> consider the following very simple case:
>
> the governor is ondemand.  cpu 0 reads cpuinfo_cur_freq. cpu0 expects to get the
> current cpu freq for the ondemand governor.

Name it A.

>
> simultaneously, cpu1 changes the governor from ondemand to userspace.

Name it B.

>
> the two threads will race for the policy->mutex
>
> suppose cpu0 gets it first.  then there is no problem.  the userspace program
> for cpu0 gets exactly the data it is expecting.
>
> Now suppose cpu1 gets the lock and starts to write ... cpu0 is blocked.
>
> cpu1 completes the governor change, and cpu0 gets the mutex ... and returns
> bogus data at this point.

What do you mean by bogus here? That userspace wouldn't be able to know if
the value is for which governor?

If that's the case than it can still happen. Issue both above commands at almost
the same time. You will never be able to differentiate if the sequence is:

- A followed by B
- B followed by A
- A waited for B and so returned -EBUSY (Only this will be clear)

And the value read can still be bogus. So, we haven't solved the problem at all.

Re: [PATCH 2/5] cpufreq, fix locking around CPUFREQ_GOV_POLICY_EXIT calls

[Saravana Kannan]

On 11/11/2014 05:07 AM, Viresh Kumar wrote:
> On 11 November 2014 17:45, Prarit Bhargava <prarit@redhat.com> wrote:
>> the deadlock in commit 955ef4833574636819cd269cfbae12f79cbde63a
>>
>> [   75.471265]        CPU0                    CPU1
>> [   75.476327]        ----                    ----
>> [   75.481385]   lock(&policy->rwsem);
>> [   75.485307]                                lock(s_active#219);
>> [   75.491857]                                lock(&policy->rwsem);
>> [   75.498592]   lock(s_active#219);
>> [   75.502331]
>> [   75.502331]  *** DEADLOCK ***
>
> I wanted to understand how this deadlock is prevented by a simple change
> to trylock..
>
>>> And also your changelog talks about accessing invalid pointers
>>> without the trylock change, how can that be possible? After the read
>>> lock is taken,
>>> all the pointers should be valid.
>>
>> consider the following very simple case:
>>
>> the governor is ondemand.  cpu 0 reads cpuinfo_cur_freq. cpu0 expects to get the
>> current cpu freq for the ondemand governor.
>
> Name it A.
>
>>
>> simultaneously, cpu1 changes the governor from ondemand to userspace.
>
> Name it B.
>
>>
>> the two threads will race for the policy->mutex
>>
>> suppose cpu0 gets it first.  then there is no problem.  the userspace program
>> for cpu0 gets exactly the data it is expecting.
>>
>> Now suppose cpu1 gets the lock and starts to write ... cpu0 is blocked.
>>
>> cpu1 completes the governor change, and cpu0 gets the mutex ... and returns
>> bogus data at this point.
>
> What do you mean by bogus here? That userspace wouldn't be able to know if
> the value is for which governor?
>
> If that's the case than it can still happen. Issue both above commands at almost
> the same time. You will never be able to differentiate if the sequence is:
>
> - A followed by B
> - B followed by A
> - A waited for B and so returned -EBUSY (Only this will be clear)
>
> And the value read can still be bogus. So, we haven't solved the problem at all.

Ah, we are on this topic again I see. I didn't read the patch/thread 
fully, but I can guess where this is going by reading the partial set of 
patches.

Prarit,

You can't just try lock to avoid the deadlock. If you do, then the 
userspace API becomes a mess. Writes to scaling_governor (or anything 
else) will no longer by guaranteed to work. Userspace will have to read 
back, check and retry. That would break a ton of existing userpace scripts.

Viresh,

The deadlock scenario is read. That's why the code is what it is today.

All,

IMO, the right way to fix this is to have the governor have over it's 
list of attributes it want to expose thru sysfs to the cpufreq 
framework. Then the framework can add/remove this in the right order 
when the governors are changed. The framework can do this outside of the 
policy lock being held when the governors are switched. This would allow 
avoid the original deadlock between sysfs locks and the policy lock 
without just ever having to fail userspace writes to scaling_governor.

-Saravana


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 3/5] cpufreq, dbs_data->usage count must be atomic

[Prarit Bhargava]

The usage_count value can be modified from several cpus concurrently if
!CPUFREQ_HAVE_GOVERNOR_PER_POLICY.  This leads to a variety of panics in
which the usage_count is negative or exceeds the number of cpus in the
system.  It must be switched to atomic_t and protected with a mutex to make sure
that future read/writes obtain the correct data.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Prarit Bhargava <prarit@redhat.com>
---
 drivers/cpufreq/cpufreq_governor.c |   16 ++++++++++++----
 drivers/cpufreq/cpufreq_governor.h |    3 ++-
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 1b44496..32ad9db 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -265,7 +265,9 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 		if (have_governor_per_policy()) {
 			WARN_ON(dbs_data);
 		} else if (dbs_data) {
-			dbs_data->usage_count++;
+			mutex_lock(&dbs_data->usage_count_mutex);
+			atomic_inc(&dbs_data->usage_count);
+			mutex_unlock(&dbs_data->usage_count_mutex);
 			policy->governor_data = dbs_data;
 			return 0;
 		}
@@ -277,7 +279,10 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 		}
 
 		dbs_data->cdata = cdata;
-		dbs_data->usage_count = 1;
+		mutex_init(&dbs_data->usage_count_mutex);
+		mutex_lock(&dbs_data->usage_count_mutex);
+		atomic_set(&dbs_data->usage_count, 1);
+		mutex_unlock(&dbs_data->usage_count_mutex);
 		rc = cdata->init(dbs_data);
 		if (rc) {
 			pr_err("%s: POLICY_INIT: init() failed\n", __func__);
@@ -322,7 +327,8 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 
 		return 0;
 	case CPUFREQ_GOV_POLICY_EXIT:
-		if (!--dbs_data->usage_count) {
+		mutex_lock(&dbs_data->usage_count_mutex);
+		if (atomic_dec_and_test(&dbs_data->usage_count)) {
 			sysfs_remove_group(get_governor_parent_kobj(policy),
 					get_sysfs_attr(dbs_data));
 
@@ -338,9 +344,11 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 			}
 
 			cdata->exit(dbs_data);
+			mutex_unlock(&dbs_data->usage_count_mutex);
 			kfree(dbs_data);
 			cdata->gdbs_data = NULL;
-		}
+		} else
+			mutex_unlock(&dbs_data->usage_count_mutex);
 
 		policy->governor_data = NULL;
 		return 0;
diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
index cc401d1..53ca449 100644
--- a/drivers/cpufreq/cpufreq_governor.h
+++ b/drivers/cpufreq/cpufreq_governor.h
@@ -219,7 +219,8 @@ struct common_dbs_data {
 struct dbs_data {
 	struct common_dbs_data *cdata;
 	unsigned int min_sampling_rate;
-	int usage_count;
+	struct mutex usage_count_mutex;
+	atomic_t usage_count;
 	void *tuners;
 
 	/* dbs_mutex protects dbs_enable in governor start/stop */
-- 
1.7.9.3

Re: [PATCH 3/5] cpufreq, dbs_data->usage count must be atomic

[Rafael J. Wysocki]

On Wednesday, November 05, 2014 09:53:57 AM Prarit Bhargava wrote:
> The usage_count value can be modified from several cpus concurrently if
> !CPUFREQ_HAVE_GOVERNOR_PER_POLICY.  This leads to a variety of panics in
> which the usage_count is negative or exceeds the number of cpus in the
> system.  It must be switched to atomic_t and protected with a mutex to make sure
> that future read/writes obtain the correct data.

Why do you need to use a new mutex and atomic_t at the same time?  Most likely
one of them is redundant.

> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> ---
>  drivers/cpufreq/cpufreq_governor.c |   16 ++++++++++++----
>  drivers/cpufreq/cpufreq_governor.h |    3 ++-
>  2 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> index 1b44496..32ad9db 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -265,7 +265,9 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>  		if (have_governor_per_policy()) {
>  			WARN_ON(dbs_data);
>  		} else if (dbs_data) {
> -			dbs_data->usage_count++;
> +			mutex_lock(&dbs_data->usage_count_mutex);
> +			atomic_inc(&dbs_data->usage_count);
> +			mutex_unlock(&dbs_data->usage_count_mutex);
>  			policy->governor_data = dbs_data;
>  			return 0;
>  		}
> @@ -277,7 +279,10 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>  		}
>  
>  		dbs_data->cdata = cdata;
> -		dbs_data->usage_count = 1;
> +		mutex_init(&dbs_data->usage_count_mutex);
> +		mutex_lock(&dbs_data->usage_count_mutex);
> +		atomic_set(&dbs_data->usage_count, 1);
> +		mutex_unlock(&dbs_data->usage_count_mutex);

The locking here isn't necessary (if it was, someone could be using an
uninitialized mutex).

>  		rc = cdata->init(dbs_data);
>  		if (rc) {
>  			pr_err("%s: POLICY_INIT: init() failed\n", __func__);
> @@ -322,7 +327,8 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>  
>  		return 0;
>  	case CPUFREQ_GOV_POLICY_EXIT:
> -		if (!--dbs_data->usage_count) {
> +		mutex_lock(&dbs_data->usage_count_mutex);
> +		if (atomic_dec_and_test(&dbs_data->usage_count)) {
>  			sysfs_remove_group(get_governor_parent_kobj(policy),
>  					get_sysfs_attr(dbs_data));
>  
> @@ -338,9 +344,11 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>  			}
>  
>  			cdata->exit(dbs_data);
> +			mutex_unlock(&dbs_data->usage_count_mutex);
>  			kfree(dbs_data);
>  			cdata->gdbs_data = NULL;
> -		}
> +		} else
> +			mutex_unlock(&dbs_data->usage_count_mutex);
>  
>  		policy->governor_data = NULL;
>  		return 0;
> diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
> index cc401d1..53ca449 100644
> --- a/drivers/cpufreq/cpufreq_governor.h
> +++ b/drivers/cpufreq/cpufreq_governor.h
> @@ -219,7 +219,8 @@ struct common_dbs_data {
>  struct dbs_data {
>  	struct common_dbs_data *cdata;
>  	unsigned int min_sampling_rate;
> -	int usage_count;
> +	struct mutex usage_count_mutex;
> +	atomic_t usage_count;
>  	void *tuners;
>  
>  	/* dbs_mutex protects dbs_enable in governor start/stop */
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 3/5] cpufreq, dbs_data->usage count must be atomic

[Viresh Kumar]

On 5 November 2014 20:23, Prarit Bhargava <prarit@redhat.com> wrote:
> The usage_count value can be modified from several cpus concurrently if
> !CPUFREQ_HAVE_GOVERNOR_PER_POLICY.  This leads to a variety of panics in
> which the usage_count is negative or exceeds the number of cpus in the
> system.  It must be switched to atomic_t and protected with a mutex to make sure
> that future read/writes obtain the correct data.

I think it would be best to serialize calls to ->governor() with some
locking in place
instead of adding per-variable locks. There can be other points of
contention as well.

I have done something similar in the solution I tried. Can pick some
of those patches.

[PATCH 4/5] cpufreq, policy->initialized count must be atomic

[Prarit Bhargava]

The policy->initialized value can be modified from several cpus concurrently if
!CPUFREQ_HAVE_GOVERNOR_PER_POLICY.  This leads to a situation where a
governor maybe switched out even though the governor->initialized is greater
than one.  It must be switched to atomic_t and protected with a mutex to
make sure that future read/writes obtain the correct data.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Prarit Bhargava <prarit@redhat.com>
---
 drivers/cpufreq/cpufreq.c          |   12 +++++++++---
 drivers/cpufreq/cpufreq_governor.c |    4 ++--
 include/linux/cpufreq.h            |    3 ++-
 3 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index e33cb15..cf11de6 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2059,10 +2059,12 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
 	ret = policy->governor->governor(policy, event);
 
 	if (!ret) {
+		mutex_lock(&policy->governor->initialized_mutex);
 		if (event == CPUFREQ_GOV_POLICY_INIT)
-			policy->governor->initialized++;
+			atomic_inc(&policy->governor->initialized);
 		else if (event == CPUFREQ_GOV_POLICY_EXIT)
-			policy->governor->initialized--;
+			atomic_dec(&policy->governor->initialized);
+		mutex_unlock(&policy->governor->initialized_mutex);
 	} else {
 		/* Restore original values */
 		mutex_lock(&cpufreq_governor_lock);
@@ -2092,7 +2094,11 @@ int cpufreq_register_governor(struct cpufreq_governor *governor)
 
 	mutex_lock(&cpufreq_governor_mutex);
 
-	governor->initialized = 0;
+	mutex_init(&governor->initialized_mutex);
+	mutex_lock(&governor->initialized_mutex);
+	atomic_set(&governor->initialized, 0);
+	mutex_unlock(&governor->initialized_mutex);
+
 	err = -EBUSY;
 	if (__find_governor(governor->name) == NULL) {
 		err = 0;
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 32ad9db..b1ee597 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -315,7 +315,7 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 					latency * LATENCY_MULTIPLIER));
 
 		if ((cdata->governor == GOV_CONSERVATIVE) &&
-				(!policy->governor->initialized)) {
+		    (!atomic_read(&policy->governor->initialized))) {
 			struct cs_ops *cs_ops = dbs_data->cdata->gov_ops;
 
 			cpufreq_register_notifier(cs_ops->notifier_block,
@@ -336,7 +336,7 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 				cpufreq_put_global_kobject();
 
 			if ((dbs_data->cdata->governor == GOV_CONSERVATIVE) &&
-				(policy->governor->initialized == 1)) {
+			    atomic_read(&policy->governor->initialized) == 1) {
 				struct cs_ops *cs_ops = dbs_data->cdata->gov_ops;
 
 				cpufreq_unregister_notifier(cs_ops->notifier_block,
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 43909ad..73cec45 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -431,7 +431,8 @@ static inline unsigned long cpufreq_scale(unsigned long old, u_int div,
 
 struct cpufreq_governor {
 	char	name[CPUFREQ_NAME_LEN];
-	int	initialized;
+	struct mutex initialized_mutex;
+	atomic_t initialized;
 	int	(*governor)	(struct cpufreq_policy *policy,
 				 unsigned int event);
 	ssize_t	(*show_setspeed)	(struct cpufreq_policy *policy,
-- 
1.7.9.3

Re: [PATCH 4/5] cpufreq, policy->initialized count must be atomic

[Rafael J. Wysocki]

On Wednesday, November 05, 2014 09:53:58 AM Prarit Bhargava wrote:
> The policy->initialized value can be modified from several cpus concurrently if
> !CPUFREQ_HAVE_GOVERNOR_PER_POLICY.  This leads to a situation where a
> governor maybe switched out even though the governor->initialized is greater
> than one.  It must be switched to atomic_t and protected with a mutex to
> make sure that future read/writes obtain the correct data.

Same comments as for [3/5].  If you add a new mutex, you can avoid using
atomic_t.

> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> ---
>  drivers/cpufreq/cpufreq.c          |   12 +++++++++---
>  drivers/cpufreq/cpufreq_governor.c |    4 ++--
>  include/linux/cpufreq.h            |    3 ++-
>  3 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index e33cb15..cf11de6 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2059,10 +2059,12 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
>  	ret = policy->governor->governor(policy, event);
>  
>  	if (!ret) {
> +		mutex_lock(&policy->governor->initialized_mutex);
>  		if (event == CPUFREQ_GOV_POLICY_INIT)
> -			policy->governor->initialized++;
> +			atomic_inc(&policy->governor->initialized);
>  		else if (event == CPUFREQ_GOV_POLICY_EXIT)
> -			policy->governor->initialized--;
> +			atomic_dec(&policy->governor->initialized);
> +		mutex_unlock(&policy->governor->initialized_mutex);
>  	} else {
>  		/* Restore original values */
>  		mutex_lock(&cpufreq_governor_lock);
> @@ -2092,7 +2094,11 @@ int cpufreq_register_governor(struct cpufreq_governor *governor)
>  
>  	mutex_lock(&cpufreq_governor_mutex);
>  
> -	governor->initialized = 0;
> +	mutex_init(&governor->initialized_mutex);
> +	mutex_lock(&governor->initialized_mutex);
> +	atomic_set(&governor->initialized, 0);
> +	mutex_unlock(&governor->initialized_mutex);
> +
>  	err = -EBUSY;
>  	if (__find_governor(governor->name) == NULL) {
>  		err = 0;
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> index 32ad9db..b1ee597 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -315,7 +315,7 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>  					latency * LATENCY_MULTIPLIER));
>  
>  		if ((cdata->governor == GOV_CONSERVATIVE) &&
> -				(!policy->governor->initialized)) {
> +		    (!atomic_read(&policy->governor->initialized))) {
>  			struct cs_ops *cs_ops = dbs_data->cdata->gov_ops;
>  
>  			cpufreq_register_notifier(cs_ops->notifier_block,
> @@ -336,7 +336,7 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>  				cpufreq_put_global_kobject();
>  
>  			if ((dbs_data->cdata->governor == GOV_CONSERVATIVE) &&
> -				(policy->governor->initialized == 1)) {
> +			    atomic_read(&policy->governor->initialized) == 1) {
>  				struct cs_ops *cs_ops = dbs_data->cdata->gov_ops;
>  
>  				cpufreq_unregister_notifier(cs_ops->notifier_block,
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 43909ad..73cec45 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -431,7 +431,8 @@ static inline unsigned long cpufreq_scale(unsigned long old, u_int div,
>  
>  struct cpufreq_governor {
>  	char	name[CPUFREQ_NAME_LEN];
> -	int	initialized;
> +	struct mutex initialized_mutex;
> +	atomic_t initialized;
>  	int	(*governor)	(struct cpufreq_policy *policy,
>  				 unsigned int event);
>  	ssize_t	(*show_setspeed)	(struct cpufreq_policy *policy,
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 4/5] cpufreq, policy->initialized count must be atomic

[Viresh Kumar]

On 5 November 2014 20:23, Prarit Bhargava <prarit@redhat.com> wrote:
> The policy->initialized value can be modified from several cpus concurrently if
> !CPUFREQ_HAVE_GOVERNOR_PER_POLICY.  This leads to a situation where a
> governor maybe switched out even though the governor->initialized is greater
> than one.  It must be switched to atomic_t and protected with a mutex to
> make sure that future read/writes obtain the correct data.

Can you show a sequence of events to demonstrate the race you are talking
about? As far as I can see, there are no races :)

[PATCH 5/5] cpufreq, add BUG() messages in critical paths to aid debugging failures

[Prarit Bhargava]

Add some additional debug to capture failures in the locking scheme for
cpufreq.  Instead of just a NULL pointer, these warnings will capture failure
points if the locking scheme for cpufreq is broken.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Prarit Bhargava <prarit@redhat.com>
---
 drivers/cpufreq/cpufreq_governor.c |   32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index b1ee597..f158882 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -161,9 +161,18 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
 EXPORT_SYMBOL_GPL(dbs_check_cpu);
 
 static inline void __gov_queue_work(int cpu, struct dbs_data *dbs_data,
-		unsigned int delay)
+				    unsigned int delay,
+				    struct cpufreq_policy *policy)
 {
-	struct cpu_dbs_common_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
+	struct cpu_dbs_common_info *cdbs;
+
+	if (!dbs_data->cdata) {
+		pr_emerg("common_dbs_data is NULL for %s but initialized = %d",
+			 policy->governor->name,
+			 atomic_read(&policy->governor->initialized));
+		BUG();
+	}
+	cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
 
 	mod_delayed_work_on(cpu, system_wq, &cdbs->work, delay);
 }
@@ -185,10 +194,11 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
 		 * those works are canceled during CPU_DOWN_PREPARE so they
 		 * can't possibly run on any other CPU.
 		 */
-		__gov_queue_work(raw_smp_processor_id(), dbs_data, delay);
+		__gov_queue_work(raw_smp_processor_id(), dbs_data, delay,
+				 policy);
 	} else {
 		for_each_cpu(i, policy->cpus)
-			__gov_queue_work(i, dbs_data, delay);
+			__gov_queue_work(i, dbs_data, delay, policy);
 	}
 
 out_unlock:
@@ -258,7 +268,13 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 	else
 		dbs_data = cdata->gdbs_data;
 
-	WARN_ON(!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT));
+	if (!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT)) {
+		pr_emerg("governor_data is NULL but governor %s is initialized = %d [governor_enabled = %d event = %u]\n",
+			 policy->governor->name,
+			 atomic_read(&policy->governor->initialized),
+			 policy->governor_enabled, event);
+		BUG();
+	}
 
 	switch (event) {
 	case CPUFREQ_GOV_POLICY_INIT:
@@ -329,6 +345,12 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 	case CPUFREQ_GOV_POLICY_EXIT:
 		mutex_lock(&dbs_data->usage_count_mutex);
 		if (atomic_dec_and_test(&dbs_data->usage_count)) {
+			if (atomic_read(&policy->governor->initialized) > 1) {
+				pr_emerg("Removing governor %s but initialized = %d, dbs_data->usage_count = 0\n",
+					 policy->governor->name,
+				   atomic_read(&policy->governor->initialized));
+				BUG();
+			}
 			sysfs_remove_group(get_governor_parent_kobj(policy),
 					get_sysfs_attr(dbs_data));
 
-- 
1.7.9.3

Re: [PATCH 5/5] cpufreq, add BUG() messages in critical paths to aid debugging failures

[Rafael J. Wysocki]

On Wednesday, November 05, 2014 09:53:59 AM Prarit Bhargava wrote:
> Add some additional debug to capture failures in the locking scheme for
> cpufreq.  Instead of just a NULL pointer, these warnings will capture failure
> points if the locking scheme for cpufreq is broken.
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> ---
>  drivers/cpufreq/cpufreq_governor.c |   32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> index b1ee597..f158882 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -161,9 +161,18 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
>  EXPORT_SYMBOL_GPL(dbs_check_cpu);
>  
>  static inline void __gov_queue_work(int cpu, struct dbs_data *dbs_data,
> -		unsigned int delay)
> +				    unsigned int delay,
> +				    struct cpufreq_policy *policy)
>  {
> -	struct cpu_dbs_common_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
> +	struct cpu_dbs_common_info *cdbs;
> +
> +	if (!dbs_data->cdata) {
> +		pr_emerg("common_dbs_data is NULL for %s but initialized = %d",
> +			 policy->governor->name,
> +			 atomic_read(&policy->governor->initialized));
> +		BUG();

Is it necessary to crash the kernel here?

> +	}
> +	cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
>  
>  	mod_delayed_work_on(cpu, system_wq, &cdbs->work, delay);
>  }
> @@ -185,10 +194,11 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
>  		 * those works are canceled during CPU_DOWN_PREPARE so they
>  		 * can't possibly run on any other CPU.
>  		 */
> -		__gov_queue_work(raw_smp_processor_id(), dbs_data, delay);
> +		__gov_queue_work(raw_smp_processor_id(), dbs_data, delay,
> +				 policy);
>  	} else {
>  		for_each_cpu(i, policy->cpus)
> -			__gov_queue_work(i, dbs_data, delay);
> +			__gov_queue_work(i, dbs_data, delay, policy);
>  	}
>  
>  out_unlock:
> @@ -258,7 +268,13 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>  	else
>  		dbs_data = cdata->gdbs_data;
>  
> -	WARN_ON(!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT));
> +	if (!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT)) {
> +		pr_emerg("governor_data is NULL but governor %s is initialized = %d [governor_enabled = %d event = %u]\n",
> +			 policy->governor->name,
> +			 atomic_read(&policy->governor->initialized),
> +			 policy->governor_enabled, event);
> +		BUG();

And here?

> +	}
>  
>  	switch (event) {
>  	case CPUFREQ_GOV_POLICY_INIT:
> @@ -329,6 +345,12 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>  	case CPUFREQ_GOV_POLICY_EXIT:
>  		mutex_lock(&dbs_data->usage_count_mutex);
>  		if (atomic_dec_and_test(&dbs_data->usage_count)) {
> +			if (atomic_read(&policy->governor->initialized) > 1) {
> +				pr_emerg("Removing governor %s but initialized = %d, dbs_data->usage_count = 0\n",
> +					 policy->governor->name,
> +				   atomic_read(&policy->governor->initialized));
> +				BUG();
> +			}
>  			sysfs_remove_group(get_governor_parent_kobj(policy),
>  					get_sysfs_attr(dbs_data));
>  
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 5/5] cpufreq, add BUG() messages in critical paths to aid debugging failures

[Prarit Bhargava]

On 11/07/2014 09:00 PM, Rafael J. Wysocki wrote:
> On Wednesday, November 05, 2014 09:53:59 AM Prarit Bhargava wrote:
>> Add some additional debug to capture failures in the locking scheme for
>> cpufreq.  Instead of just a NULL pointer, these warnings will capture failure
>> points if the locking scheme for cpufreq is broken.
>>
>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>> Cc: Viresh Kumar <viresh.kumar@linaro.org>
>> Cc: linux-pm@vger.kernel.org
>> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
>> ---
>>  drivers/cpufreq/cpufreq_governor.c |   32 +++++++++++++++++++++++++++-----
>>  1 file changed, 27 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
>> index b1ee597..f158882 100644
>> --- a/drivers/cpufreq/cpufreq_governor.c
>> +++ b/drivers/cpufreq/cpufreq_governor.c
>> @@ -161,9 +161,18 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
>>  EXPORT_SYMBOL_GPL(dbs_check_cpu);
>>  
>>  static inline void __gov_queue_work(int cpu, struct dbs_data *dbs_data,
>> -		unsigned int delay)
>> +				    unsigned int delay,
>> +				    struct cpufreq_policy *policy)
>>  {
>> -	struct cpu_dbs_common_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
>> +	struct cpu_dbs_common_info *cdbs;
>> +
>> +	if (!dbs_data->cdata) {
>> +		pr_emerg("common_dbs_data is NULL for %s but initialized = %d",
>> +			 policy->governor->name,
>> +			 atomic_read(&policy->governor->initialized));
>> +		BUG();
> 
> Is it necessary to crash the kernel here?

Yes.  dbs_data->cdata is referenced right below.

> 
>> +	}
>> +	cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);

and we'll NULL pointer panic right here without any of the debug info above :(

>>  
>>  	mod_delayed_work_on(cpu, system_wq, &cdbs->work, delay);
>>  }
>> @@ -185,10 +194,11 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
>>  		 * those works are canceled during CPU_DOWN_PREPARE so they
>>  		 * can't possibly run on any other CPU.
>>  		 */
>> -		__gov_queue_work(raw_smp_processor_id(), dbs_data, delay);
>> +		__gov_queue_work(raw_smp_processor_id(), dbs_data, delay,
>> +				 policy);
>>  	} else {
>>  		for_each_cpu(i, policy->cpus)
>> -			__gov_queue_work(i, dbs_data, delay);
>> +			__gov_queue_work(i, dbs_data, delay, policy);
>>  	}
>>  
>>  out_unlock:
>> @@ -258,7 +268,13 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>>  	else
>>  		dbs_data = cdata->gdbs_data;
>>  
>> -	WARN_ON(!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT));
>> +	if (!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT)) {
>> +		pr_emerg("governor_data is NULL but governor %s is initialized = %d [governor_enabled = %d event = %u]\n",
>> +			 policy->governor->name,
>> +			 atomic_read(&policy->governor->initialized),
>> +			 policy->governor_enabled, event);
>> +		BUG();
> 
> And here?
> 

Ditto -- dbs_data is dereferenced in the call path and will NULL pointer panic.

P.

>> +	}
>>  
>>  	switch (event) {
>>  	case CPUFREQ_GOV_POLICY_INIT:
>> @@ -329,6 +345,12 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>>  	case CPUFREQ_GOV_POLICY_EXIT:
>>  		mutex_lock(&dbs_data->usage_count_mutex);
>>  		if (atomic_dec_and_test(&dbs_data->usage_count)) {
>> +			if (atomic_read(&policy->governor->initialized) > 1) {
>> +				pr_emerg("Removing governor %s but initialized = %d, dbs_data->usage_count = 0\n",
>> +					 policy->governor->name,
>> +				   atomic_read(&policy->governor->initialized));
>> +				BUG();
>> +			}
>>  			sysfs_remove_group(get_governor_parent_kobj(policy),
>>  					get_sysfs_attr(dbs_data));
>>  
>>
> 

Re: [PATCH 5/5] cpufreq, add BUG() messages in critical paths to aid debugging failures

[Rafael J. Wysocki]

On Saturday, November 08, 2014 08:33:35 AM Prarit Bhargava wrote:
> 
> On 11/07/2014 09:00 PM, Rafael J. Wysocki wrote:
> > On Wednesday, November 05, 2014 09:53:59 AM Prarit Bhargava wrote:
> >> Add some additional debug to capture failures in the locking scheme for
> >> cpufreq.  Instead of just a NULL pointer, these warnings will capture failure
> >> points if the locking scheme for cpufreq is broken.
> >>
> >> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> >> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> >> Cc: linux-pm@vger.kernel.org
> >> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> >> ---
> >>  drivers/cpufreq/cpufreq_governor.c |   32 +++++++++++++++++++++++++++-----
> >>  1 file changed, 27 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> >> index b1ee597..f158882 100644
> >> --- a/drivers/cpufreq/cpufreq_governor.c
> >> +++ b/drivers/cpufreq/cpufreq_governor.c
> >> @@ -161,9 +161,18 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
> >>  EXPORT_SYMBOL_GPL(dbs_check_cpu);
> >>  
> >>  static inline void __gov_queue_work(int cpu, struct dbs_data *dbs_data,
> >> -		unsigned int delay)
> >> +				    unsigned int delay,
> >> +				    struct cpufreq_policy *policy)
> >>  {
> >> -	struct cpu_dbs_common_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
> >> +	struct cpu_dbs_common_info *cdbs;
> >> +
> >> +	if (!dbs_data->cdata) {
> >> +		pr_emerg("common_dbs_data is NULL for %s but initialized = %d",
> >> +			 policy->governor->name,
> >> +			 atomic_read(&policy->governor->initialized));
> >> +		BUG();
> > 
> > Is it necessary to crash the kernel here?
> 
> Yes.  dbs_data->cdata is referenced right below.
> 
> > 
> >> +	}
> >> +	cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
> 
> and we'll NULL pointer panic right here without any of the debug info above :(

Can we possibly avoid the panic?

Adding BUG() instead of a NULL pointer deref is not much improvement.

> 
> >>  
> >>  	mod_delayed_work_on(cpu, system_wq, &cdbs->work, delay);
> >>  }
> >> @@ -185,10 +194,11 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
> >>  		 * those works are canceled during CPU_DOWN_PREPARE so they
> >>  		 * can't possibly run on any other CPU.
> >>  		 */
> >> -		__gov_queue_work(raw_smp_processor_id(), dbs_data, delay);
> >> +		__gov_queue_work(raw_smp_processor_id(), dbs_data, delay,
> >> +				 policy);
> >>  	} else {
> >>  		for_each_cpu(i, policy->cpus)
> >> -			__gov_queue_work(i, dbs_data, delay);
> >> +			__gov_queue_work(i, dbs_data, delay, policy);
> >>  	}
> >>  
> >>  out_unlock:
> >> @@ -258,7 +268,13 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
> >>  	else
> >>  		dbs_data = cdata->gdbs_data;
> >>  
> >> -	WARN_ON(!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT));
> >> +	if (!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT)) {
> >> +		pr_emerg("governor_data is NULL but governor %s is initialized = %d [governor_enabled = %d event = %u]\n",
> >> +			 policy->governor->name,
> >> +			 atomic_read(&policy->governor->initialized),
> >> +			 policy->governor_enabled, event);
> >> +		BUG();
> > 
> > And here?
> > 
> 
> Ditto -- dbs_data is dereferenced in the call path and will NULL pointer panic.
> 
> P.
> 
> >> +	}
> >>  
> >>  	switch (event) {
> >>  	case CPUFREQ_GOV_POLICY_INIT:
> >> @@ -329,6 +345,12 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
> >>  	case CPUFREQ_GOV_POLICY_EXIT:
> >>  		mutex_lock(&dbs_data->usage_count_mutex);
> >>  		if (atomic_dec_and_test(&dbs_data->usage_count)) {
> >> +			if (atomic_read(&policy->governor->initialized) > 1) {
> >> +				pr_emerg("Removing governor %s but initialized = %d, dbs_data->usage_count = 0\n",
> >> +					 policy->governor->name,
> >> +				   atomic_read(&policy->governor->initialized));
> >> +				BUG();
> >> +			}
> >>  			sysfs_remove_group(get_governor_parent_kobj(policy),
> >>  					get_sysfs_attr(dbs_data));
> >>  
> >>
> > 
> --
> 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

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 5/5] cpufreq, add BUG() messages in critical paths to aid debugging failures

[Prarit Bhargava]

On 11/08/2014 04:46 PM, Rafael J. Wysocki wrote:
> On Saturday, November 08, 2014 08:33:35 AM Prarit Bhargava wrote:
>>
>> On 11/07/2014 09:00 PM, Rafael J. Wysocki wrote:
>>> On Wednesday, November 05, 2014 09:53:59 AM Prarit Bhargava wrote:
>>>> Add some additional debug to capture failures in the locking scheme for
>>>> cpufreq.  Instead of just a NULL pointer, these warnings will capture failure
>>>> points if the locking scheme for cpufreq is broken.
>>>>
>>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>>>> Cc: Viresh Kumar <viresh.kumar@linaro.org>
>>>> Cc: linux-pm@vger.kernel.org
>>>> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
>>>> ---
>>>>  drivers/cpufreq/cpufreq_governor.c |   32 +++++++++++++++++++++++++++-----
>>>>  1 file changed, 27 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
>>>> index b1ee597..f158882 100644
>>>> --- a/drivers/cpufreq/cpufreq_governor.c
>>>> +++ b/drivers/cpufreq/cpufreq_governor.c
>>>> @@ -161,9 +161,18 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
>>>>  EXPORT_SYMBOL_GPL(dbs_check_cpu);
>>>>  
>>>>  static inline void __gov_queue_work(int cpu, struct dbs_data *dbs_data,
>>>> -		unsigned int delay)
>>>> +				    unsigned int delay,
>>>> +				    struct cpufreq_policy *policy)
>>>>  {
>>>> -	struct cpu_dbs_common_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
>>>> +	struct cpu_dbs_common_info *cdbs;
>>>> +
>>>> +	if (!dbs_data->cdata) {
>>>> +		pr_emerg("common_dbs_data is NULL for %s but initialized = %d",
>>>> +			 policy->governor->name,
>>>> +			 atomic_read(&policy->governor->initialized));
>>>> +		BUG();
>>>
>>> Is it necessary to crash the kernel here?
>>
>> Yes.  dbs_data->cdata is referenced right below.
>>
>>>
>>>> +	}
>>>> +	cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
>>
>> and we'll NULL pointer panic right here without any of the debug info above :(
> 
> Can we possibly avoid the panic?
> 
> Adding BUG() instead of a NULL pointer deref is not much improvement.

(sorry  for the lowercase typing.  i fractured my elbow and have resorted to
single hand typing ....)

rafael, i understand your concern and my description is clearly lacking for this
patch.  this patch is not meant to be a fix but is meant to capture debug info
for future issues in this code.  i thought about only doing the pr_emerg() but
that results in situations where other threads may continue processing and i
lose state in crashdump :(.   bug() is a good idea here imo.

P.
> 
>>
>>>>  
>>>>  	mod_delayed_work_on(cpu, system_wq, &cdbs->work, delay);
>>>>  }
>>>> @@ -185,10 +194,11 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
>>>>  		 * those works are canceled during CPU_DOWN_PREPARE so they
>>>>  		 * can't possibly run on any other CPU.
>>>>  		 */
>>>> -		__gov_queue_work(raw_smp_processor_id(), dbs_data, delay);
>>>> +		__gov_queue_work(raw_smp_processor_id(), dbs_data, delay,
>>>> +				 policy);
>>>>  	} else {
>>>>  		for_each_cpu(i, policy->cpus)
>>>> -			__gov_queue_work(i, dbs_data, delay);
>>>> +			__gov_queue_work(i, dbs_data, delay, policy);
>>>>  	}
>>>>  
>>>>  out_unlock:
>>>> @@ -258,7 +268,13 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>>>>  	else
>>>>  		dbs_data = cdata->gdbs_data;
>>>>  
>>>> -	WARN_ON(!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT));
>>>> +	if (!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT)) {
>>>> +		pr_emerg("governor_data is NULL but governor %s is initialized = %d [governor_enabled = %d event = %u]\n",
>>>> +			 policy->governor->name,
>>>> +			 atomic_read(&policy->governor->initialized),
>>>> +			 policy->governor_enabled, event);
>>>> +		BUG();
>>>
>>> And here?
>>>
>>
>> Ditto -- dbs_data is dereferenced in the call path and will NULL pointer panic.
>>
>> P.
>>
>>>> +	}
>>>>  
>>>>  	switch (event) {
>>>>  	case CPUFREQ_GOV_POLICY_INIT:
>>>> @@ -329,6 +345,12 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>>>>  	case CPUFREQ_GOV_POLICY_EXIT:
>>>>  		mutex_lock(&dbs_data->usage_count_mutex);
>>>>  		if (atomic_dec_and_test(&dbs_data->usage_count)) {
>>>> +			if (atomic_read(&policy->governor->initialized) > 1) {
>>>> +				pr_emerg("Removing governor %s but initialized = %d, dbs_data->usage_count = 0\n",
>>>> +					 policy->governor->name,
>>>> +				   atomic_read(&policy->governor->initialized));
>>>> +				BUG();
>>>> +			}
>>>>  			sysfs_remove_group(get_governor_parent_kobj(policy),
>>>>  					get_sysfs_attr(dbs_data));
>>>>  
>>>>
>>>
>> --
>> 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
> 

Re: [PATCH 5/5] cpufreq, add BUG() messages in critical paths to aid debugging failures

[Viresh Kumar]

On 5 November 2014 20:23, Prarit Bhargava <prarit@redhat.com> wrote:
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> index b1ee597..f158882 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -161,9 +161,18 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
>  EXPORT_SYMBOL_GPL(dbs_check_cpu);
>
>  static inline void __gov_queue_work(int cpu, struct dbs_data *dbs_data,
> -               unsigned int delay)
> +                                   unsigned int delay,
> +                                   struct cpufreq_policy *policy)
>  {
> -       struct cpu_dbs_common_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);

I will let it crash right here instead of additional code :)

> @@ -258,7 +268,13 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
> -       WARN_ON(!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT));
> +       if (!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT)) {
> +               pr_emerg("governor_data is NULL but governor %s is initialized = %d [governor_enabled = %d event = %u]\n",
> +                        policy->governor->name,
> +                        atomic_read(&policy->governor->initialized),
> +                        policy->governor_enabled, event);
> +               BUG();

How is the BUG better than the WARN here ?

>         switch (event) {
>         case CPUFREQ_GOV_POLICY_INIT:
> @@ -329,6 +345,12 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>         case CPUFREQ_GOV_POLICY_EXIT:
>                 mutex_lock(&dbs_data->usage_count_mutex);
>                 if (atomic_dec_and_test(&dbs_data->usage_count)) {
> +                       if (atomic_read(&policy->governor->initialized) > 1) {

Isn't this wrong? Consider 4 CPUs with separate clock line and have set
governor-per-policy to true. EXIT will be called for every CPU hotplug and
initialized will be 4 initially..

Or I am still vacation lag'd ? :)

> +                               pr_emerg("Removing governor %s but initialized = %d, dbs_data->usage_count = 0\n",
> +                                        policy->governor->name,
> +                                  atomic_read(&policy->governor->initialized));
> +                               BUG();
> +                       }
>                         sysfs_remove_group(get_governor_parent_kobj(policy),
>                                         get_sysfs_attr(dbs_data));
>
> --
> 1.7.9.3
>

Re: [PATCH 5/5] cpufreq, add BUG() messages in critical paths to aid debugging failures

[Prarit Bhargava]

On 11/10/2014 11:23 PM, Viresh Kumar wrote:
> On 5 November 2014 20:23, Prarit Bhargava <prarit@redhat.com> wrote:
>> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
>> index b1ee597..f158882 100644
>> --- a/drivers/cpufreq/cpufreq_governor.c
>> +++ b/drivers/cpufreq/cpufreq_governor.c
>> @@ -161,9 +161,18 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
>>  EXPORT_SYMBOL_GPL(dbs_check_cpu);
>>
>>  static inline void __gov_queue_work(int cpu, struct dbs_data *dbs_data,
>> -               unsigned int delay)
>> +                                   unsigned int delay,
>> +                                   struct cpufreq_policy *policy)
>>  {
>> -       struct cpu_dbs_common_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
> 
> I will let it crash right here instead of additional code :)

the problem is tht the userful information is the values of initialized,
enabled, and what the  event was :(

in every case i ended up needing the values.

> 
>> @@ -258,7 +268,13 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>> -       WARN_ON(!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT));
>> +       if (!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT)) {
>> +               pr_emerg("governor_data is NULL but governor %s is initialized = %d [governor_enabled = %d event = %u]\n",
>> +                        policy->governor->name,
>> +                        atomic_read(&policy->governor->initialized),
>> +                        policy->governor_enabled, event);
>> +               BUG();
> 
> How is the BUG better than the WARN here ?
> 

we null pointer panic later on, and again the useful values are the ones displayed.

>>         switch (event) {
>>         case CPUFREQ_GOV_POLICY_INIT:
>> @@ -329,6 +345,12 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>>         case CPUFREQ_GOV_POLICY_EXIT:
>>                 mutex_lock(&dbs_data->usage_count_mutex);
>>                 if (atomic_dec_and_test(&dbs_data->usage_count)) {
>> +                       if (atomic_read(&policy->governor->initialized) > 1) {
> 
> Isn't this wrong? Consider 4 CPUs with separate clock line and have set
> governor-per-policy to true. EXIT will be called for every CPU hotplug and
> initialized will be 4 initially..
> 
> Or I am still vacation lag'd ? :)

oh, is that right?  i'll look into that.

P.
> 
>> +                               pr_emerg("Removing governor %s but initialized = %d, dbs_data->usage_count = 0\n",
>> +                                        policy->governor->name,
>> +                                  atomic_read(&policy->governor->initialized));
>> +                               BUG();
>> +                       }
>>                         sysfs_remove_group(get_governor_parent_kobj(policy),
>>                                         get_sysfs_attr(dbs_data));
>>
>> --
>> 1.7.9.3
>>

Re: [PATCH 5/5] cpufreq, add BUG() messages in critical paths to aid debugging failures

[Viresh Kumar]

On 11 November 2014 17:48, Prarit Bhargava <prarit@redhat.com> wrote:
> the problem is tht the userful information is the values of initialized,
> enabled, and what the  event was :(
>
> in every case i ended up needing the values.

So, just add a pr_debug instead and let every thing crash as it does today.

>>> @@ -258,7 +268,13 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>>> -       WARN_ON(!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT));
>>> +       if (!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT)) {
>>> +               pr_emerg("governor_data is NULL but governor %s is initialized = %d [governor_enabled = %d event = %u]\n",
>>> +                        policy->governor->name,
>>> +                        atomic_read(&policy->governor->initialized),
>>> +                        policy->governor_enabled, event);
>>> +               BUG();
>>
>> How is the BUG better than the WARN here ?
>>
>
> we null pointer panic later on, and again the useful values are the ones displayed.

For the values, I would add a pr_debug() for all cases. And maybe just
do s/WARN_ON/BUG_ON

>>>         switch (event) {
>>>         case CPUFREQ_GOV_POLICY_INIT:
>>> @@ -329,6 +345,12 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>>>         case CPUFREQ_GOV_POLICY_EXIT:
>>>                 mutex_lock(&dbs_data->usage_count_mutex);
>>>                 if (atomic_dec_and_test(&dbs_data->usage_count)) {
>>> +                       if (atomic_read(&policy->governor->initialized) > 1) {
>>
>> Isn't this wrong? Consider 4 CPUs with separate clock line and have set
>> governor-per-policy to true. EXIT will be called for every CPU hotplug and
>> initialized will be 4 initially..
>>
>> Or I am still vacation lag'd ? :)
>
> oh, is that right?  i'll look into that.

What is right? sorry couldn't understand :(