| Message ID | 1415199239-19019-5-git-send-email-prarit@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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, >
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 :) -- 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
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,
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(-)