All of lore.kernel.org
 help / color / mirror / Atom feed
From: Prarit Bhargava <prarit@redhat.com>
To: linux-kernel@vger.kernel.org
Cc: robert.schoene@tu-dresden.de, sboyd@codeaurora.org,
	Prarit Bhargava <prarit@redhat.com>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Viresh Kumar <viresh.kumar@linaro.org>,
	linux-pm@vger.kernel.org
Subject: [PATCH 2/5] cpufreq, fix locking around CPUFREQ_GOV_POLICY_EXIT calls
Date: Wed,  5 Nov 2014 09:53:56 -0500	[thread overview]
Message-ID: <1415199239-19019-3-git-send-email-prarit@redhat.com> (raw)
In-Reply-To: <1415199239-19019-1-git-send-email-prarit@redhat.com>

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

  parent reply	other threads:[~2014-11-05 14:53 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-11-05 14:53 [PATCH 0/5] cpufreq, fix locking and data issues Prarit Bhargava
2014-11-05 14:53 ` [PATCH 1/5] cpufreq, do not return stale data to userspace Prarit Bhargava
2014-11-05 14:53 ` Prarit Bhargava [this message]
2014-11-10 10:44   ` [PATCH 2/5] cpufreq, fix locking around CPUFREQ_GOV_POLICY_EXIT calls Viresh Kumar
2014-11-10 12:26     ` Prarit Bhargava
2014-11-11  3:37       ` Viresh Kumar
2014-11-11 12:15         ` Prarit Bhargava
2014-11-11 13:07           ` Viresh Kumar
2014-11-13 21:58             ` Saravana Kannan
2014-11-05 14:53 ` [PATCH 3/5] cpufreq, dbs_data->usage count must be atomic Prarit Bhargava
2014-11-08  1:57   ` Rafael J. Wysocki
2014-11-11  3:40   ` Viresh Kumar
2014-11-05 14:53 ` [PATCH 4/5] cpufreq, policy->initialized " Prarit Bhargava
2014-11-08  1:59   ` Rafael J. Wysocki
2014-11-11  3:55   ` Viresh Kumar
2014-11-05 14:53 ` [PATCH 5/5] cpufreq, add BUG() messages in critical paths to aid debugging failures Prarit Bhargava
2014-11-08  2:00   ` Rafael J. Wysocki
2014-11-08 13:33     ` Prarit Bhargava
2014-11-08 21:46       ` Rafael J. Wysocki
2014-11-09 14:12         ` Prarit Bhargava
2014-11-11  4:23   ` Viresh Kumar
2014-11-11 12:18     ` Prarit Bhargava
2014-11-11 13:11       ` Viresh Kumar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1415199239-19019-3-git-send-email-prarit@redhat.com \
    --to=prarit@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=rjw@rjwysocki.net \
    --cc=robert.schoene@tu-dresden.de \
    --cc=sboyd@codeaurora.org \
    --cc=viresh.kumar@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.