diff mbox series

[V3,1/3] cpufreq: Fix locking issues with governors

Message ID f366ed3dd3dd111e42173b366fe4dd5a0e4647fd.1593143118.git.viresh.kumar@linaro.org
State Superseded
Headers show
Series cpufreq: Allow default governor on cmdline and fix locking issues | expand

Commit Message

Viresh Kumar June 26, 2020, 3:51 a.m. UTC 
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 <qperret@google.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

---
 drivers/cpufreq/cpufreq.c | 59 ++++++++++++++++++++++++---------------
 1 file changed, 36 insertions(+), 23 deletions(-)

-- 
2.25.0.rc1.19.g042ed3e048af

Comments

Quentin Perret June 26, 2020, 8:24 a.m. UTC | #1 
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 <qperret@google.com>

> Signed-off-by: Viresh Kumar <viresh.kumar@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).

But other than that:

Reviewed-by: Quentin Perret <qperret@google.com>


Thanks!
Viresh Kumar June 29, 2020, 2:13 a.m. UTC | #2 
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 <qperret@google.com>

> > Signed-off-by: Viresh Kumar <viresh.kumar@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
Quentin Perret June 29, 2020, 8:05 a.m. UTC | #3 
On Monday 29 Jun 2020 at 07:43:09 (+0530), Viresh Kumar wrote:
> 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;


Right, I guess this is a good trade-off, so that works for me.

Thanks,
Quentin
Rafael J. Wysocki June 29, 2020, 1:13 p.m. UTC | #4 
On Mon, Jun 29, 2020 at 4:13 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>

> 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 <qperret@google.com>

> > > Signed-off-by: Viresh Kumar <viresh.kumar@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 ?


Works for me.

> 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
diff mbox series

Patch

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);
+
+	return ret;
 }
 
 static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)