| Message ID | 1452533760-13787-12-git-send-email-juri.lelli@arm.com |
|---|---|
| State | New |
| Headers | show |
On 11-01-16, 17:35, Juri Lelli wrote: > __cpufreq_governor works on policy, so policy->rwsem has to be held. > Add assertion for such condition. > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Cc: Viresh Kumar <viresh.kumar@linaro.org> > Signed-off-by: Juri Lelli <juri.lelli@arm.com> > --- > drivers/cpufreq/cpufreq.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index f1f9fbc..e7fc5c9 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1950,6 +1950,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, > /* Don't start any governor operations if we are entering suspend */ > if (cpufreq_suspended) > return 0; > + > + lockdep_assert_held(&policy->rwsem); > + We had an ABBA problem with the EXIT governor callback and so this rwsem is dropped just before that from set_policy().. commit 955ef4833574 ("cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT") -- viresh
On 01-02-16, 22:00, Rafael J. Wysocki wrote: > I'm not sure what you mean by "the sysfs lock" here? The policy rwsem > or something else? He perhaps referred to the s_active.lock that we see in traces. -- viresh
On 02-02-16, 13:37, Saravana Kannan wrote: > On 02/01/2016 10:34 PM, Viresh Kumar wrote: > >What will that solve? It will stay exactly same then as well, as we > >would be adding/removing these attributes from within the same > >policy->rwsem .. > > The problem isn't that you are holding the policy rwsem. The problem is that > we are trying to grab the same locks in different order. This is trying to > fix that. That's exactly what I was trying to say, sorry for not being very clear. Even if you would move the sysfs file creation thing into the cpufreq core, instead of governor, we will have locks this way: CPU0 CPU1 (sysfs read) (sysfs dir remove) s_active lock policy->rwsem policy->rwsem s_active lock (hang) And so I said, nothing will change. -- viresh
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index f1f9fbc..e7fc5c9 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1950,6 +1950,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, /* Don't start any governor operations if we are entering suspend */ if (cpufreq_suspended) return 0; + + lockdep_assert_held(&policy->rwsem); + /* * Governor might not be initiated here if ACPI _PPC changed * notification happened, so check it.
__cpufreq_governor works on policy, so policy->rwsem has to be held. Add assertion for such condition. Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Cc: Viresh Kumar <viresh.kumar@linaro.org> Signed-off-by: Juri Lelli <juri.lelli@arm.com> --- drivers/cpufreq/cpufreq.c | 3 +++ 1 file changed, 3 insertions(+) -- 2.2.2