[PATCH v4] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.

[PATCH v4] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.

[Enric Balletbo i Serra]

When the devfreq driver and the governor driver are built as modules,
the call to devfreq_add_device() or governor_store() fails because the
governor driver is not loaded at the time the devfreq driver loads. The
devfreq driver has a build dependency on the governor but also should
have a runtime dependency. We need to make sure that the governor driver
is loaded before the devfreq driver.

This patch fixes this bug by adding a try_then_request_governor()
function. First tries to find the governor, and then, if it is not found,
it requests the module and tries again.

Fixes: 1b5c1be2c88e (PM / devfreq: map devfreq drivers to governor using name)
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

Changes in v4:
- Kept "locked" devfreq_list from the return of find_devfreq_governor() to
  the unlock of governor_store(). Requested by MyungJoo Ham.

Changes in v3:
- Remove unneded change in dev_err message.
- Fix err returned value in case to not find the governor.

Changes in v2:
- Add a new function to request the module and call that function from
  devfreq_add_device and governor_store.

 drivers/devfreq/devfreq.c | 53 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 47 insertions(+), 6 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 0b5b3abe054e..4ea6b19879a1 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -11,6 +11,7 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/kmod.h>
 #include <linux/sched.h>
 #include <linux/errno.h>
 #include <linux/err.h>
@@ -221,6 +222,46 @@ static struct devfreq_governor *find_devfreq_governor(const char *name)
 	return ERR_PTR(-ENODEV);
 }
 
+/**
+ * try_then_request_governor() - Try to find the governor and request the
+ *                               module if is not found.
+ * @name:	name of the governor
+ *
+ * Search the list of devfreq governors and request the module and try again
+ * if is not found. This can happen when both drivers (the governor driver
+ * and the driver that call devfreq_add_device) are built as modules.
+ * devfreq_list_lock should be held by the caller.
+ *
+ * Return: The matched governor's pointer.
+ */
+static struct devfreq_governor *try_then_request_governor(const char *name)
+{
+	struct devfreq_governor *governor;
+	int err = 0;
+
+	WARN(!mutex_is_locked(&devfreq_list_lock),
+	     "devfreq_list_lock must be locked.");
+
+	governor = find_devfreq_governor(name);
+	if (IS_ERR(governor)) {
+		mutex_unlock(&devfreq_list_lock);
+
+		if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND,
+			     DEVFREQ_NAME_LEN))
+			err = request_module("governor_%s", "simpleondemand");
+		else
+			err = request_module("governor_%s", name);
+		if (err)
+			return NULL;
+
+		mutex_lock(&devfreq_list_lock);
+
+		governor = find_devfreq_governor(name);
+	}
+
+	return governor;
+}
+
 static int devfreq_notify_transition(struct devfreq *devfreq,
 		struct devfreq_freqs *freqs, unsigned int state)
 {
@@ -643,11 +684,9 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	srcu_init_notifier_head(&devfreq->transition_notifier_list);
 
 	mutex_unlock(&devfreq->lock);
-
 	mutex_lock(&devfreq_list_lock);
-	list_add(&devfreq->node, &devfreq_list);
 
-	governor = find_devfreq_governor(devfreq->governor_name);
+	governor = try_then_request_governor(devfreq->governor_name);
 	if (IS_ERR(governor)) {
 		dev_err(dev, "%s: Unable to find governor for the device\n",
 			__func__);
@@ -663,14 +702,15 @@ struct devfreq *devfreq_add_device(struct device *dev,
 			__func__);
 		goto err_init;
 	}
+
+	list_add(&devfreq->node, &devfreq_list);
+
 	mutex_unlock(&devfreq_list_lock);
 
 	return devfreq;
 
 err_init:
-	list_del(&devfreq->node);
 	mutex_unlock(&devfreq_list_lock);
-
 	device_unregister(&devfreq->dev);
 err_dev:
 	if (devfreq)
@@ -989,7 +1029,8 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
 		return -EINVAL;
 
 	mutex_lock(&devfreq_list_lock);
-	governor = find_devfreq_governor(str_governor);
+
+	governor = try_then_request_governor(str_governor);
 	if (IS_ERR(governor)) {
 		ret = PTR_ERR(governor);
 		goto out;
-- 
2.18.0

RE: [PATCH v4] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.

[MyungJoo Ham]

>+		if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND,
>+			     DEVFREQ_NAME_LEN))
>+			err = request_module("governor_%s", "simpleondemand");
>+		else
>+			err = request_module("governor_%s", name);
>+		if (err)
>+			return NULL;

You are returning without the lock acquired..

Re: [PATCH v4] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.

[Chanwoo Choi]

Hi Enric,

On 2018년 07월 03일 22:29, Enric Balletbo i Serra wrote:
> When the devfreq driver and the governor driver are built as modules,
> the call to devfreq_add_device() or governor_store() fails because the
> governor driver is not loaded at the time the devfreq driver loads. The
> devfreq driver has a build dependency on the governor but also should
> have a runtime dependency. We need to make sure that the governor driver
> is loaded before the devfreq driver.
> 
> This patch fixes this bug by adding a try_then_request_governor()
> function. First tries to find the governor, and then, if it is not found,
> it requests the module and tries again.
> 
> Fixes: 1b5c1be2c88e (PM / devfreq: map devfreq drivers to governor using name)
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 
> Changes in v4:
> - Kept "locked" devfreq_list from the return of find_devfreq_governor() to
>   the unlock of governor_store(). Requested by MyungJoo Ham.
> 
> Changes in v3:
> - Remove unneded change in dev_err message.
> - Fix err returned value in case to not find the governor.
> 
> Changes in v2:
> - Add a new function to request the module and call that function from
>   devfreq_add_device and governor_store.
> 
>  drivers/devfreq/devfreq.c | 53 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 47 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 0b5b3abe054e..4ea6b19879a1 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -11,6 +11,7 @@
>   */
>  
>  #include <linux/kernel.h>
> +#include <linux/kmod.h>
>  #include <linux/sched.h>
>  #include <linux/errno.h>
>  #include <linux/err.h>
> @@ -221,6 +222,46 @@ static struct devfreq_governor *find_devfreq_governor(const char *name)
>  	return ERR_PTR(-ENODEV);
>  }
>  
> +/**
> + * try_then_request_governor() - Try to find the governor and request the
> + *                               module if is not found.
> + * @name:	name of the governor

Usually, devfreq used 'governor_name' indicating the name of governor.
you better to use 'governor_name' instead of 'name' for more readability.

> + *
> + * Search the list of devfreq governors and request the module and try again
> + * if is not found. This can happen when both drivers (the governor driver
> + * and the driver that call devfreq_add_device) are built as modules.
> + * devfreq_list_lock should be held by the caller.
> + *
> + * Return: The matched governor's pointer.

Usually, devfreq.c didn;t use the 'Return: ...'. So, you better to explain
what is returned from this function with function description.

> + */
> +static struct devfreq_governor *try_then_request_governor(const char *name)

ditto. (name -> governor_name)

> +{
> +	struct devfreq_governor *governor;
> +	int err = 0;

You have to check whether governor name is NULL or not.

	if (IS_ERR_OR_NULL(name)) {                                             
		pr_err("DEVFREQ: %s: Invalid parameters\n", __func__);          
		return ERR_PTR(-EINVAL);
	}

> +
> +	WARN(!mutex_is_locked(&devfreq_list_lock),
> +	     "devfreq_list_lock must be locked.");
> +
> +	governor = find_devfreq_governor(name);
> +	if (IS_ERR(governor)) {
> +		mutex_unlock(&devfreq_list_lock);
> +
> +		if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND,
> +			     DEVFREQ_NAME_LEN))
> +			err = request_module("governor_%s", "simpleondemand");
> +		else
> +			err = request_module("governor_%s", name);
> +		if (err)
> +			return NULL;

When error happen, you unlock the mutex. If failed to request module,
you should restore the previous state. Please mutex_lock(&devfreq_list_lock)
before return.

> +
> +		mutex_lock(&devfreq_list_lock);
> +
> +		governor = find_devfreq_governor(name);
> +	}
> +
> +	return governor;
> +}
> +
>  static int devfreq_notify_transition(struct devfreq *devfreq,
>  		struct devfreq_freqs *freqs, unsigned int state)
>  {
> @@ -643,11 +684,9 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	srcu_init_notifier_head(&devfreq->transition_notifier_list);
>  
>  	mutex_unlock(&devfreq->lock);
> -

This change is not related to this patch.

>  	mutex_lock(&devfreq_list_lock);
> -	list_add(&devfreq->node, &devfreq_list);
>  
> -	governor = find_devfreq_governor(devfreq->governor_name);
> +	governor = try_then_request_governor(devfreq->governor_name);
>  	if (IS_ERR(governor)) {
>  		dev_err(dev, "%s: Unable to find governor for the device\n",
>  			__func__);
> @@ -663,14 +702,15 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  			__func__);
>  		goto err_init;
>  	}
> +
> +	list_add(&devfreq->node, &devfreq_list);
> +
>  	mutex_unlock(&devfreq_list_lock);
>  
>  	return devfreq;
>  
>  err_init:
> -	list_del(&devfreq->node);
>  	mutex_unlock(&devfreq_list_lock);
> -

This change is not related to this patch.

>  	device_unregister(&devfreq->dev);
>  err_dev:
>  	if (devfreq)
> @@ -989,7 +1029,8 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>  		return -EINVAL;
>  
>  	mutex_lock(&devfreq_list_lock);
> -	governor = find_devfreq_governor(str_governor);
> +

Don't need to add the blank line. It is enough to change the function
from find_devfreq_governor to try_then_request_governor.

> +	governor = try_then_request_governor(str_governor);
>  	if (IS_ERR(governor)) {
>  		ret = PTR_ERR(governor);
>  		goto out;
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v4] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.

[Enric Balletbo i Serra]

Hi Chanwoo,

On 04/07/18 03:06, Chanwoo Choi wrote:
> Hi Enric,
> 
> On 2018년 07월 03일 22:29, Enric Balletbo i Serra wrote:
>> When the devfreq driver and the governor driver are built as modules,
>> the call to devfreq_add_device() or governor_store() fails because the
>> governor driver is not loaded at the time the devfreq driver loads. The
>> devfreq driver has a build dependency on the governor but also should
>> have a runtime dependency. We need to make sure that the governor driver
>> is loaded before the devfreq driver.
>>
>> This patch fixes this bug by adding a try_then_request_governor()
>> function. First tries to find the governor, and then, if it is not found,
>> it requests the module and tries again.
>>
>> Fixes: 1b5c1be2c88e (PM / devfreq: map devfreq drivers to governor using name)
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>>
>> Changes in v4:
>> - Kept "locked" devfreq_list from the return of find_devfreq_governor() to
>>   the unlock of governor_store(). Requested by MyungJoo Ham.
>>
>> Changes in v3:
>> - Remove unneded change in dev_err message.
>> - Fix err returned value in case to not find the governor.
>>
>> Changes in v2:
>> - Add a new function to request the module and call that function from
>>   devfreq_add_device and governor_store.
>>
>>  drivers/devfreq/devfreq.c | 53 ++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 47 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 0b5b3abe054e..4ea6b19879a1 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -11,6 +11,7 @@
>>   */
>>  
>>  #include <linux/kernel.h>
>> +#include <linux/kmod.h>
>>  #include <linux/sched.h>
>>  #include <linux/errno.h>
>>  #include <linux/err.h>
>> @@ -221,6 +222,46 @@ static struct devfreq_governor *find_devfreq_governor(const char *name)
>>  	return ERR_PTR(-ENODEV);
>>  }
>>  
>> +/**
>> + * try_then_request_governor() - Try to find the governor and request the
>> + *                               module if is not found.
>> + * @name:	name of the governor
> 
> Usually, devfreq used 'governor_name' indicating the name of governor.
> you better to use 'governor_name' instead of 'name' for more readability.
> 

I don't mind to change if you want. But let me try to convince you first. I used
name for two reasons:
1. I saw that you are using governor_name sometimes, but find_devfreq_governor
uses name not governor_name. IMHO the function name in these two specific cases
'try_then_request_governor(name)' is enough readable.
2. If we want to use governor_name and then do not have the line exceeding the
80 characters I need to split the function in two lines. For me the readability
is better when you have all in one line.

If I did not convince you, just let me now and I'll change for governor_name :)

>> + *
>> + * Search the list of devfreq governors and request the module and try again
>> + * if is not found. This can happen when both drivers (the governor driver
>> + * and the driver that call devfreq_add_device) are built as modules.
>> + * devfreq_list_lock should be held by the caller.
>> + *
>> + * Return: The matched governor's pointer.
> 
> Usually, devfreq.c didn;t use the 'Return: ...'. So, you better to explain
> what is returned from this function with function description.
> 

OK.

>> + */
>> +static struct devfreq_governor *try_then_request_governor(const char *name)
> 
> ditto. (name -> governor_name)
> 

I convinced you? ;)

>> +{
>> +	struct devfreq_governor *governor;
>> +	int err = 0;
> 
> You have to check whether governor name is NULL or not.
> 
> 	if (IS_ERR_OR_NULL(name)) {                                             
> 		pr_err("DEVFREQ: %s: Invalid parameters\n", __func__);          
> 		return ERR_PTR(-EINVAL);
> 	}
> 

Right.

>> +
>> +	WARN(!mutex_is_locked(&devfreq_list_lock),
>> +	     "devfreq_list_lock must be locked.");
>> +
>> +	governor = find_devfreq_governor(name);
>> +	if (IS_ERR(governor)) {
>> +		mutex_unlock(&devfreq_list_lock);
>> +
>> +		if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND,
>> +			     DEVFREQ_NAME_LEN))
>> +			err = request_module("governor_%s", "simpleondemand");
>> +		else
>> +			err = request_module("governor_%s", name);
>> +		if (err)
>> +			return NULL;
> 
> When error happen, you unlock the mutex. If failed to request module,
> you should restore the previous state. Please mutex_lock(&devfreq_list_lock)
> before return.
> 

Oh right, my bad.

>> +
>> +		mutex_lock(&devfreq_list_lock);
>> +
>> +		governor = find_devfreq_governor(name);
>> +	}
>> +
>> +	return governor;
>> +}
>> +
>>  static int devfreq_notify_transition(struct devfreq *devfreq,
>>  		struct devfreq_freqs *freqs, unsigned int state)
>>  {
>> @@ -643,11 +684,9 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>  	srcu_init_notifier_head(&devfreq->transition_notifier_list);
>>  
>>  	mutex_unlock(&devfreq->lock);
>> -
> 
> This change is not related to this patch.
> 
Ack.
>>  	mutex_lock(&devfreq_list_lock);
>> -	list_add(&devfreq->node, &devfreq_list);
>>  
>> -	governor = find_devfreq_governor(devfreq->governor_name);
>> +	governor = try_then_request_governor(devfreq->governor_name);
>>  	if (IS_ERR(governor)) {
>>  		dev_err(dev, "%s: Unable to find governor for the device\n",
>>  			__func__);
>> @@ -663,14 +702,15 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>  			__func__);
>>  		goto err_init;
>>  	}
>> +
>> +	list_add(&devfreq->node, &devfreq_list);
>> +
>>  	mutex_unlock(&devfreq_list_lock);
>>  
>>  	return devfreq;
>>  
>>  err_init:
>> -	list_del(&devfreq->node);
>>  	mutex_unlock(&devfreq_list_lock);
>> -
> 
> This change is not related to this patch.
> 

Ack.

>>  	device_unregister(&devfreq->dev);
>>  err_dev:
>>  	if (devfreq)
>> @@ -989,7 +1029,8 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>  		return -EINVAL;
>>  
>>  	mutex_lock(&devfreq_list_lock);
>> -	governor = find_devfreq_governor(str_governor);
>> +
> 
> Don't need to add the blank line. It is enough to change the function
> from find_devfreq_governor to try_then_request_governor.
> 
>> +	governor = try_then_request_governor(str_governor);
>>  	if (IS_ERR(governor)) {
>>  		ret = PTR_ERR(governor);
>>  		goto out;
>>
> 
>

Preparing next version, many thanks for the review.

 Enric

Re: [PATCH v4] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.

[Chanwoo Choi]

Hi Enric,

On 2018년 07월 04일 17:16, Enric Balletbo i Serra wrote:
> Hi Chanwoo,
> 
> On 04/07/18 03:06, Chanwoo Choi wrote:
>> Hi Enric,
>>
>> On 2018년 07월 03일 22:29, Enric Balletbo i Serra wrote:
>>> When the devfreq driver and the governor driver are built as modules,
>>> the call to devfreq_add_device() or governor_store() fails because the
>>> governor driver is not loaded at the time the devfreq driver loads. The
>>> devfreq driver has a build dependency on the governor but also should
>>> have a runtime dependency. We need to make sure that the governor driver
>>> is loaded before the devfreq driver.
>>>
>>> This patch fixes this bug by adding a try_then_request_governor()
>>> function. First tries to find the governor, and then, if it is not found,
>>> it requests the module and tries again.
>>>
>>> Fixes: 1b5c1be2c88e (PM / devfreq: map devfreq drivers to governor using name)
>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>> ---
>>>
>>> Changes in v4:
>>> - Kept "locked" devfreq_list from the return of find_devfreq_governor() to
>>>   the unlock of governor_store(). Requested by MyungJoo Ham.
>>>
>>> Changes in v3:
>>> - Remove unneded change in dev_err message.
>>> - Fix err returned value in case to not find the governor.
>>>
>>> Changes in v2:
>>> - Add a new function to request the module and call that function from
>>>   devfreq_add_device and governor_store.
>>>
>>>  drivers/devfreq/devfreq.c | 53 ++++++++++++++++++++++++++++++++++-----
>>>  1 file changed, 47 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index 0b5b3abe054e..4ea6b19879a1 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -11,6 +11,7 @@
>>>   */
>>>  
>>>  #include <linux/kernel.h>
>>> +#include <linux/kmod.h>
>>>  #include <linux/sched.h>
>>>  #include <linux/errno.h>
>>>  #include <linux/err.h>
>>> @@ -221,6 +222,46 @@ static struct devfreq_governor *find_devfreq_governor(const char *name)
>>>  	return ERR_PTR(-ENODEV);
>>>  }
>>>  
>>> +/**
>>> + * try_then_request_governor() - Try to find the governor and request the
>>> + *                               module if is not found.
>>> + * @name:	name of the governor
>>
>> Usually, devfreq used 'governor_name' indicating the name of governor.
>> you better to use 'governor_name' instead of 'name' for more readability.
>>
> 
> I don't mind to change if you want. But let me try to convince you first. I used
> name for two reasons:
> 1. I saw that you are using governor_name sometimes, but find_devfreq_governor
> uses name not governor_name. IMHO the function name in these two specific cases
> 'try_then_request_governor(name)' is enough readable.

OK. skip the my comment of changing the variable name. Thanks.

> 2. If we want to use governor_name and then do not have the line exceeding the
> 80 characters I need to split the function in two lines. For me the readability
> is better when you have all in one line.
> 
> If I did not convince you, just let me now and I'll change for governor_name :)
> 

(snip)

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v4] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.

[Chanwoo Choi]

Hi Enric,

Please send this patch to stable-kernel mailing list.

Regards,
Chanwoo Choi

On 2018년 07월 04일 17:26, Chanwoo Choi wrote:
> Hi Enric,
> 
> On 2018년 07월 04일 17:16, Enric Balletbo i Serra wrote:
>> Hi Chanwoo,
>>
>> On 04/07/18 03:06, Chanwoo Choi wrote:
>>> Hi Enric,
>>>
>>> On 2018년 07월 03일 22:29, Enric Balletbo i Serra wrote:
>>>> When the devfreq driver and the governor driver are built as modules,
>>>> the call to devfreq_add_device() or governor_store() fails because the
>>>> governor driver is not loaded at the time the devfreq driver loads. The
>>>> devfreq driver has a build dependency on the governor but also should
>>>> have a runtime dependency. We need to make sure that the governor driver
>>>> is loaded before the devfreq driver.
>>>>
>>>> This patch fixes this bug by adding a try_then_request_governor()
>>>> function. First tries to find the governor, and then, if it is not found,
>>>> it requests the module and tries again.
>>>>
>>>> Fixes: 1b5c1be2c88e (PM / devfreq: map devfreq drivers to governor using name)
>>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>>> ---
>>>>
>>>> Changes in v4:
>>>> - Kept "locked" devfreq_list from the return of find_devfreq_governor() to
>>>>   the unlock of governor_store(). Requested by MyungJoo Ham.
>>>>
>>>> Changes in v3:
>>>> - Remove unneded change in dev_err message.
>>>> - Fix err returned value in case to not find the governor.
>>>>
>>>> Changes in v2:
>>>> - Add a new function to request the module and call that function from
>>>>   devfreq_add_device and governor_store.
>>>>
>>>>  drivers/devfreq/devfreq.c | 53 ++++++++++++++++++++++++++++++++++-----
>>>>  1 file changed, 47 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>> index 0b5b3abe054e..4ea6b19879a1 100644
>>>> --- a/drivers/devfreq/devfreq.c
>>>> +++ b/drivers/devfreq/devfreq.c
>>>> @@ -11,6 +11,7 @@
>>>>   */
>>>>  
>>>>  #include <linux/kernel.h>
>>>> +#include <linux/kmod.h>
>>>>  #include <linux/sched.h>
>>>>  #include <linux/errno.h>
>>>>  #include <linux/err.h>
>>>> @@ -221,6 +222,46 @@ static struct devfreq_governor *find_devfreq_governor(const char *name)
>>>>  	return ERR_PTR(-ENODEV);
>>>>  }
>>>>  
>>>> +/**
>>>> + * try_then_request_governor() - Try to find the governor and request the
>>>> + *                               module if is not found.
>>>> + * @name:	name of the governor
>>>
>>> Usually, devfreq used 'governor_name' indicating the name of governor.
>>> you better to use 'governor_name' instead of 'name' for more readability.
>>>
>>
>> I don't mind to change if you want. But let me try to convince you first. I used
>> name for two reasons:
>> 1. I saw that you are using governor_name sometimes, but find_devfreq_governor
>> uses name not governor_name. IMHO the function name in these two specific cases
>> 'try_then_request_governor(name)' is enough readable.
> 
> OK. skip the my comment of changing the variable name. Thanks.
> 
>> 2. If we want to use governor_name and then do not have the line exceeding the
>> 80 characters I need to split the function in two lines. For me the readability
>> is better when you have all in one line.
>>
>> If I did not convince you, just let me now and I'll change for governor_name :)
>>
> 
> (snip)
>