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

[Chanwoo Choi <cw00.choi@samsung.com>]

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)
>