Re: [PATCH] PM / devfreq: Drop the name check to request module in try_then_request_governor()

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

On 19. 7. 31. 오후 2:57, Yue Hu wrote:
> On Wed, 31 Jul 2019 14:55:39 +0900
> Chanwoo Choi <cw00.choi@samsung.com> wrote:
> 
>> On 19. 7. 31. 오후 2:38, Yue Hu wrote:
>>> On Wed, 31 Jul 2019 09:33:06 +0900
>>> Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>>   
>>>> On 19. 7. 30. 오후 7:08, Yue Hu wrote:  
>>>>> From: Yue Hu <huyue2@yulong.com>
>>>>>
>>>>> No need to check specific governor name of `simple_ondemand` to request
>>>>> module, let's change the name string to `simpleondemand` to keep the
>>>>> consistency on loading module if needed.    
>>>>
>>>> NACK.
>>>>
>>>> hmm.... It is impossible to change the devfreq governor name
>>>> because there are many reason.
>>>>
>>>> The devfreq governor could be changed through the sysfs interface
>>>> on runtime. For a long time, many users or platforms change
>>>> the devfreq governor with the defined governor name through sysfs.
>>>> If it is just changed, it breaks ABI interface and cannot support
>>>> the compatibility. It is very critical problem. Please drop it.  
>>>
>>> Yes, needs update also if using sysfs. it's problem indeed.  
>>
>> No, It is impossible to update it. You have to change all kind of
>> platform in the world. We never know the all use-case in the world.
>> As I said, it break the ABI interface. 
>>
>>>   
>>>>
>>>>
>>>> Maybe, you didn't check the usage of devfreq device driver
>>>> in the mainline kernel. Almost devfreq device using simple_ondemand
>>>> governor have to add the governor name with devfreq_add_device().
>>>> If changed the governor name, it cause the fault of device driver
>>>> using the devfreq framework with simple_ondemand.  
>>>
>>> Currently, seems no devfreq users use the simple_ondemand directly in
>>> mainline kernel.  
>>
>> You can find them in the mainline kernel as following:
>>
>> drivers/gpu/drm/panfrost/panfrost_devfreq.c:160:&panfrost_devfreq_profile, "simple_ondemand", NULL);
>> drivers/gpu/drm/msm/msm_gpu.c:98:		&msm_devfreq_profile, "simple_ondemand", NULL);
> 
> drm related code is already updated as below link:
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=67fe62dcf713c36f4766c0218cc14796ee9536e1
> 
>>
>> drivers/scsi/ufs/ufshcd.c:1333:			DEVFREQ_GOV_SIMPLE_ONDEMAND,
>> drivers/devfreq/tegra20-devfreq.c:176:		DEVFREQ_GOV_SIMPLE_ONDEMAND, NULL);
>> drivers/devfreq/rk3399_dmc.c:452:		DEVFREQ_GOV_SIMPLE_ONDEMAND,
>> drivers/devfreq/exynos-bus.c:437:		DEVFREQ_GOV_SIMPLE_ONDEMAND,
>>
>>>
>>> Maybe we can rename the governor file name to governor_simpleondemand.c,
>>> just not compatible to module name compared with this change.  
>>
>> The file name was already 'drivers/devfreq/governor_simpleondemand.c'.
> 
> Sorry for the typo error. I mean governor_simple_ondemand.c?

Actually, it is not necessary because there are no benefit.

> 
> Thanks.
> 
>>
>>
>>>
>>> Thanks.
>>>   
>>>>
>>>>  
>>>>>
>>>>> Signed-off-by: Yue Hu <huyue2@yulong.com>
>>>>> ---
>>>>>  drivers/devfreq/devfreq.c | 6 +-----
>>>>>  include/linux/devfreq.h   | 2 +-
>>>>>  2 files changed, 2 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>>> index 784c08e..baff682 100644
>>>>> --- a/drivers/devfreq/devfreq.c
>>>>> +++ b/drivers/devfreq/devfreq.c
>>>>> @@ -246,11 +246,7 @@ static struct devfreq_governor *try_then_request_governor(const char *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);
>>>>> +		err = request_module("governor_%s", name);
>>>>>  		/* Restore previous state before return */
>>>>>  		mutex_lock(&devfreq_list_lock);
>>>>>  		if (err)
>>>>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>>>>> index 2bae9ed..41e8792 100644
>>>>> --- a/include/linux/devfreq.h
>>>>> +++ b/include/linux/devfreq.h
>>>>> @@ -17,7 +17,7 @@
>>>>>  #define DEVFREQ_NAME_LEN 16
>>>>>  
>>>>>  /* DEVFREQ governor name */
>>>>> -#define DEVFREQ_GOV_SIMPLE_ONDEMAND	"simple_ondemand"
>>>>> +#define DEVFREQ_GOV_SIMPLE_ONDEMAND	"simpleondemand"
>>>>>  #define DEVFREQ_GOV_PERFORMANCE		"performance"
>>>>>  #define DEVFREQ_GOV_POWERSAVE		"powersave"
>>>>>  #define DEVFREQ_GOV_USERSPACE		"userspace"
>>>>>     
>>>>
>>>>  
>>>
>>>
>>>   
>>
>>
> 
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics