On 26-06-20, 09:24, Quentin Perret wrote:
> On Friday 26 Jun 2020 at 09:21:42 (+0530), Viresh Kumar wrote:
> > The locking around governors handling isn't adequate currently. The list
> > of governors should never be traversed without locking in place. Also we
> > must make sure the governor isn't removed while it is still referenced
> > by code.
> >
> > Reported-by: Quentin Perret <qper...@google.com>
> > Signed-off-by: Viresh Kumar <viresh.ku...@linaro.org>
> > ---
> > drivers/cpufreq/cpufreq.c | 59 ++++++++++++++++++++++++---------------
> > 1 file changed, 36 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 0128de3603df..e798a1193bdf 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -621,6 +621,24 @@ static struct cpufreq_governor *find_governor(const
> > char *str_governor)
> > return NULL;
> > }
> >
> > +static struct cpufreq_governor *get_governor(const char *str_governor)
> > +{
> > + struct cpufreq_governor *t;
> > +
> > + mutex_lock(&cpufreq_governor_mutex);
> > + t = find_governor(str_governor);
> > + if (!t)
> > + goto unlock;
> > +
> > + if (!try_module_get(t->owner))
> > + t = NULL;
> > +
> > +unlock:
> > + mutex_unlock(&cpufreq_governor_mutex);
> > +
> > + return t;
> > +}
> > +
> > static unsigned int cpufreq_parse_policy(char *str_governor)
> > {
> > if (!strncasecmp(str_governor, "performance", CPUFREQ_NAME_LEN))
> > @@ -640,28 +658,14 @@ static struct cpufreq_governor
> > *cpufreq_parse_governor(char *str_governor)
> > {
> > struct cpufreq_governor *t;
> >
> > - mutex_lock(&cpufreq_governor_mutex);
> > -
> > - t = find_governor(str_governor);
> > - if (!t) {
> > - int ret;
> > -
> > - mutex_unlock(&cpufreq_governor_mutex);
> > -
> > - ret = request_module("cpufreq_%s", str_governor);
> > - if (ret)
> > - return NULL;
> > -
> > - mutex_lock(&cpufreq_governor_mutex);
> > + t = get_governor(str_governor);
> > + if (t)
> > + return t;
> >
> > - t = find_governor(str_governor);
> > - }
> > - if (t && !try_module_get(t->owner))
> > - t = NULL;
> > -
> > - mutex_unlock(&cpufreq_governor_mutex);
> > + if (request_module("cpufreq_%s", str_governor))
> > + return NULL;
> >
> > - return t;
> > + return get_governor(str_governor);
> > }
> >
> > /**
> > @@ -815,12 +819,14 @@ static ssize_t
> > show_scaling_available_governors(struct cpufreq_policy *policy,
> > goto out;
> > }
> >
> > + mutex_lock(&cpufreq_governor_mutex);
> > for_each_governor(t) {
> > if (i >= (ssize_t) ((PAGE_SIZE / sizeof(char))
> > - (CPUFREQ_NAME_LEN + 2)))
> > - goto out;
> > + break;
> > i += scnprintf(&buf[i], CPUFREQ_NAME_PLEN, "%s ", t->name);
> > }
> > + mutex_unlock(&cpufreq_governor_mutex);
> > out:
> > i += sprintf(&buf[i], "\n");
> > return i;
> > @@ -1058,11 +1064,14 @@ static int cpufreq_init_policy(struct
> > cpufreq_policy *policy)
> > struct cpufreq_governor *def_gov = cpufreq_default_governor();
> > struct cpufreq_governor *gov = NULL;
> > unsigned int pol = CPUFREQ_POLICY_UNKNOWN;
> > + bool put_governor = false;
> > + int ret;
> >
> > if (has_target()) {
> > /* Update policy governor to the one used before hotplug. */
> > - gov = find_governor(policy->last_governor);
> > + gov = get_governor(policy->last_governor);
> > if (gov) {
> > + put_governor = true;
> > pr_debug("Restoring governor %s for cpu %d\n",
> > policy->governor->name, policy->cpu);
> > } else if (def_gov) {
> > @@ -1089,7 +1098,11 @@ static int cpufreq_init_policy(struct cpufreq_policy
> > *policy)
> > return -ENODATA;
> > }
> >
> > - return cpufreq_set_policy(policy, gov, pol);
> > + ret = cpufreq_set_policy(policy, gov, pol);
> > + if (put_governor)
> > + module_put(gov->owner);
>
> Nit: I think you could safely do
>
> if (gov)
> module_put(gov->owner);
>
> and get rid of 'put_governor', given that try_module_get() and
> module_put() are nops if owner is NULL (which is guaranteed for
> the result of cpufreq_default_governor() as it is builtin).
I described why I chose to keep it that way in the other email, but I
am all for dropping the variable. And so what about this ?
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index e798a1193bdf..d9e9ae7051bb 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1064,18 +1064,17 @@ static int cpufreq_init_policy(struct cpufreq_policy
*policy)
struct cpufreq_governor *def_gov = cpufreq_default_governor();
struct cpufreq_governor *gov = NULL;
unsigned int pol = CPUFREQ_POLICY_UNKNOWN;
- bool put_governor = false;
int ret;
if (has_target()) {
/* Update policy governor to the one used before hotplug. */
gov = get_governor(policy->last_governor);
if (gov) {
- put_governor = true;
pr_debug("Restoring governor %s for cpu %d\n",
policy->governor->name, policy->cpu);
} else if (def_gov) {
gov = def_gov;
+ module_get(gov->owner);
} else {
return -ENODATA;
}
@@ -1099,7 +1098,7 @@ static int cpufreq_init_policy(struct cpufreq_policy
*policy)
}
ret = cpufreq_set_policy(policy, gov, pol);
- if (put_governor)
+ if (gov)
module_put(gov->owner);
return ret;
--
viresh
