| Message ID | 20190730100819.8056-1-zbestahu@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | PM / devfreq: Drop the name check to request module in try_then_request_governor() | expand |
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. 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. > > 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" >
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. > > > 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. Maybe we can rename the governor file name to governor_simpleondemand.c, just not compatible to module name compared with this change. 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" > > > >
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); 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'. > > 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" >>> >> >> > > >
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? 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" > >>> > >> > >> > > > > > > > >
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" >>>>> >>>> >>>> >>> >>> >>> >> >> > > >
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"