Re: [PATCH 1/3] PM / devfreq: Fix available_governor sysfs
On 2017년 01월 24일 12:51, MyungJoo Ham wrote:
>> The devfreq using passive governor is not able to change the governor.
>> So, the user can not change the governor through 'available_governor' sysfs
>> entry. Also, the devfreq which don't use the passive governor is not able to
>> change to 'passive' governor on the fly.
>
> Another thoughts on the characteristics of 'passive' governor:
>
> 1. Should we prohibit moving from "others" to "passive"?
The relation between parent devfreq and passive devfreq
is fixed by h/w because they share the one power line.
But,
if you want to permit that some devfreq change
their governor to passive governor. The current design
of devfreq does not support it. We must need to
rework the devfreq for the moving from 'others' to 'passive'.
The devfreq should consider the multiple dependency on hierachry
as CCF (Common Clock Framework).
devfreq2 (passive)
devfreq5 (passive)
devfreq6 (passive)
devfreq3 (passive)
devfreq4 (passive)
devfreq7 (passive)
devfreq8 (passive)
I add some examples as following:
Example1,
There is one parent devfreq which includes
the four passive devfreqs as following:
parent-dev1 (ondemand)
passive-dev1 (passive)
passive-dev2 (passive)
passive-dev3 (passive)
passive-dev4 (passive)
new-parent-dev2 (ondemand)
If changing the governor of 'parent-dev1' from ondemand to passive,
the user have to inform the information of new parent devfreq(new-parent-dev2).
Maybe, following command should be executed.
echo [new-parent-dev2] > /sys/class/devfreq/[parent-dev1]/parent
new-parent-dev2
echo passive > /sys/class/devfreq/[parent-dev1]/governor
After that, the final hierarchy will be following:
new-parent-dev2 (ondemand)
parent-dev1 (passive)
passive-dev1 (passive)
passive-dev2 (passive)
passive-dev3 (passive)
passive-dev4 (passive)
Example2,
Before,
parent-dev1 (ondemand)
passive-dev1 (passive)
passive-dev2 (passive)
passive-dev3 (passive)
passive-dev4 (passive)
new-parent-dev2 (ondemand)
After that, if new-parent-dev2 use the passive governor with parent-dev1 device.
parent-dev1 (ondemand) - control voltage and freq
passive-dev1 (passive) - control freq
passive-dev2 (passive) - control freq
passive-dev3 (passive) - control freq
passive-dev4 (passive) - control freq
new-parent-dev2 (passive) - control voltage and freq
Example3,
There is one parent devfreq which includes
the four passive devfreqs as following:
parent-dev2 (ondemand)
new-parent-dev3 (passive)
parent-dev1 (ondemand)
passive-dev1 (passive)
passive-dev2 (passive)
passive-dev3 (passive)
passive-dev4 (passive)
After that, if parent-dev1 use the passive governor with new-parent-dev3 device.
parent-dev2 (ondemand)
new-parent-dev3 (passive)
parent-dev1 (passive)
passive-dev1 (passive)
passive-dev2 (passive)
passive-dev3 (passive)
passive-dev4 (passive)
> 2. Should we show "passive" in the available list if it's not passive now?
Yes. I added the test result on cover letter about this.
Even if the parent devfreq device doesn't support the passive governor,
their 'available_governor' shows the 'passive' governor.
> 3. Why don't we show anyway and reject it when actually tries to change?
I think that the sysfs entry have to provide the correct information
to user-space. If available_governor shows the name of unsupported
governor, it is not reasonable and appropriate.
- cat /sys/class/devfreq/[devfreq name]/available_governor
So, the 'available_governor' should only show the supported governors.
> 4. Or should we add a value in devfreq struct that is confired at devfreq
> device add, which prohibits changing governors? (and passive will
> return error if that flag is not set or it will set the value automatically)
If we add some flags to devfreq for passive govenror,
devfreq will prohibits the changing governors.
And, avaiable_governor function will use the new flags
to show the only supported governors.
I tried to use the existing fields of struct devfreq
without new field. But if you want to add new field,
I'll do.
>
> Cheers,
> MyungJoo
>
>>
>> Fixes: 996133119f57 ("PM / devfreq: Add new passive governor")
>> Cc: sta...@vger.kernel.org
>> Signed-off-by: Chanwoo Choi
>> ---
>> drivers/devfreq/devfreq.c | 34 +-
>> 1 file changed, 33 insertions(+), 1 deletion(-)
--
Best Regards,
Chanwoo Choi
Samsung Electronics
Re: [PATCH 1/3] PM / devfreq: Fix available_governor sysfs
On 2017년 01월 24일 12:51, MyungJoo Ham wrote:
>> The devfreq using passive governor is not able to change the governor.
>> So, the user can not change the governor through 'available_governor' sysfs
>> entry. Also, the devfreq which don't use the passive governor is not able to
>> change to 'passive' governor on the fly.
>
> Another thoughts on the characteristics of 'passive' governor:
>
> 1. Should we prohibit moving from "others" to "passive"?
The relation between parent devfreq and passive devfreq
is fixed by h/w because they share the one power line.
But,
if you want to permit that some devfreq change
their governor to passive governor. The current design
of devfreq does not support it. We must need to
rework the devfreq for the moving from 'others' to 'passive'.
The devfreq should consider the multiple dependency on hierachry
as CCF (Common Clock Framework).
devfreq2 (passive)
devfreq5 (passive)
devfreq6 (passive)
devfreq3 (passive)
devfreq4 (passive)
devfreq7 (passive)
devfreq8 (passive)
I add some examples as following:
Example1,
There is one parent devfreq which includes
the four passive devfreqs as following:
parent-dev1 (ondemand)
passive-dev1 (passive)
passive-dev2 (passive)
passive-dev3 (passive)
passive-dev4 (passive)
new-parent-dev2 (ondemand)
If changing the governor of 'parent-dev1' from ondemand to passive,
the user have to inform the information of new parent devfreq(new-parent-dev2).
Maybe, following command should be executed.
echo [new-parent-dev2] > /sys/class/devfreq/[parent-dev1]/parent
new-parent-dev2
echo passive > /sys/class/devfreq/[parent-dev1]/governor
After that, the final hierarchy will be following:
new-parent-dev2 (ondemand)
parent-dev1 (passive)
passive-dev1 (passive)
passive-dev2 (passive)
passive-dev3 (passive)
passive-dev4 (passive)
Example2,
Before,
parent-dev1 (ondemand)
passive-dev1 (passive)
passive-dev2 (passive)
passive-dev3 (passive)
passive-dev4 (passive)
new-parent-dev2 (ondemand)
After that, if new-parent-dev2 use the passive governor with parent-dev1 device.
parent-dev1 (ondemand) - control voltage and freq
passive-dev1 (passive) - control freq
passive-dev2 (passive) - control freq
passive-dev3 (passive) - control freq
passive-dev4 (passive) - control freq
new-parent-dev2 (passive) - control voltage and freq
Example3,
There is one parent devfreq which includes
the four passive devfreqs as following:
parent-dev2 (ondemand)
new-parent-dev3 (passive)
parent-dev1 (ondemand)
passive-dev1 (passive)
passive-dev2 (passive)
passive-dev3 (passive)
passive-dev4 (passive)
After that, if parent-dev1 use the passive governor with new-parent-dev3 device.
parent-dev2 (ondemand)
new-parent-dev3 (passive)
parent-dev1 (passive)
passive-dev1 (passive)
passive-dev2 (passive)
passive-dev3 (passive)
passive-dev4 (passive)
> 2. Should we show "passive" in the available list if it's not passive now?
Yes. I added the test result on cover letter about this.
Even if the parent devfreq device doesn't support the passive governor,
their 'available_governor' shows the 'passive' governor.
> 3. Why don't we show anyway and reject it when actually tries to change?
I think that the sysfs entry have to provide the correct information
to user-space. If available_governor shows the name of unsupported
governor, it is not reasonable and appropriate.
- cat /sys/class/devfreq/[devfreq name]/available_governor
So, the 'available_governor' should only show the supported governors.
> 4. Or should we add a value in devfreq struct that is confired at devfreq
> device add, which prohibits changing governors? (and passive will
> return error if that flag is not set or it will set the value automatically)
If we add some flags to devfreq for passive govenror,
devfreq will prohibits the changing governors.
And, avaiable_governor function will use the new flags
to show the only supported governors.
I tried to use the existing fields of struct devfreq
without new field. But if you want to add new field,
I'll do.
>
> Cheers,
> MyungJoo
>
>>
>> Fixes: 996133119f57 ("PM / devfreq: Add new passive governor")
>> Cc: sta...@vger.kernel.org
>> Signed-off-by: Chanwoo Choi
>> ---
>> drivers/devfreq/devfreq.c | 34 +-
>> 1 file changed, 33 insertions(+), 1 deletion(-)
--
Best Regards,
Chanwoo Choi
Samsung Electronics
RE: [PATCH 1/3] PM / devfreq: Fix available_governor sysfs
> The devfreq using passive governor is not able to change the governor.
> So, the user can not change the governor through 'available_governor' sysfs
> entry. Also, the devfreq which don't use the passive governor is not able to
> change to 'passive' governor on the fly.
Another thoughts on the characteristics of 'passive' governor:
1. Should we prohibit moving from "others" to "passive"?
2. Should we show "passive" in the available list if it's not passive now?
3. Why don't we show anyway and reject it when actually tries to change?
4. Or should we add a value in devfreq struct that is confired at devfreq
device add, which prohibits changing governors? (and passive will
return error if that flag is not set or it will set the value automatically)
Cheers,
MyungJoo
>
> Fixes: 996133119f57 ("PM / devfreq: Add new passive governor")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Chanwoo Choi
> ---
> drivers/devfreq/devfreq.c | 34 +-
> 1 file changed, 33 insertions(+), 1 deletion(-)
RE: [PATCH 1/3] PM / devfreq: Fix available_governor sysfs
> The devfreq using passive governor is not able to change the governor.
> So, the user can not change the governor through 'available_governor' sysfs
> entry. Also, the devfreq which don't use the passive governor is not able to
> change to 'passive' governor on the fly.
Another thoughts on the characteristics of 'passive' governor:
1. Should we prohibit moving from "others" to "passive"?
2. Should we show "passive" in the available list if it's not passive now?
3. Why don't we show anyway and reject it when actually tries to change?
4. Or should we add a value in devfreq struct that is confired at devfreq
device add, which prohibits changing governors? (and passive will
return error if that flag is not set or it will set the value automatically)
Cheers,
MyungJoo
>
> Fixes: 996133119f57 ("PM / devfreq: Add new passive governor")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Chanwoo Choi
> ---
> drivers/devfreq/devfreq.c | 34 +-
> 1 file changed, 33 insertions(+), 1 deletion(-)
RE: [PATCH 1/3] PM / devfreq: Fix available_governor sysfs
> The devfreq using passive governor is not able to change the governor.
> So, the user can not change the governor through 'available_governor' sysfs
> entry. Also, the devfreq which don't use the passive governor is not able to
> change to 'passive' governor on the fly.
>
> Fixes: 996133119f57 ("PM / devfreq: Add new passive governor")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Chanwoo Choi
> ---
> drivers/devfreq/devfreq.c | 34 +-
> 1 file changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 4bd7a8f71b07..a2c575a5a9ab 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -43,6 +43,11 @@
> static LIST_HEAD(devfreq_list);
> static DEFINE_MUTEX(devfreq_list_lock);
>
> +static int is_passive_gov(const char *governor_name)
> +{
> + return (!strncmp(governor_name, "passive", 7)) ? 1 : 0;
> +}
> +
Having a special function for a governor in devfreq.c isn't looking good.
Could you create it more general?
(e.g., denying being replaced from passive governor)
I'd suggest to "define" data value of event_handler to include the reason
of STOP event for DEVFREQ_GOV_STOP. Then, a governor may "reject" it
depending on the reason. (the reason is to be defined in devfreq.h as well)
Then, the modification can be minimal and general for all others.
The modification in this commit looks too hacky.
Cheers,
MyungJoo
RE: [PATCH 1/3] PM / devfreq: Fix available_governor sysfs
> The devfreq using passive governor is not able to change the governor.
> So, the user can not change the governor through 'available_governor' sysfs
> entry. Also, the devfreq which don't use the passive governor is not able to
> change to 'passive' governor on the fly.
>
> Fixes: 996133119f57 ("PM / devfreq: Add new passive governor")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Chanwoo Choi
> ---
> drivers/devfreq/devfreq.c | 34 +-
> 1 file changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 4bd7a8f71b07..a2c575a5a9ab 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -43,6 +43,11 @@
> static LIST_HEAD(devfreq_list);
> static DEFINE_MUTEX(devfreq_list_lock);
>
> +static int is_passive_gov(const char *governor_name)
> +{
> + return (!strncmp(governor_name, "passive", 7)) ? 1 : 0;
> +}
> +
Having a special function for a governor in devfreq.c isn't looking good.
Could you create it more general?
(e.g., denying being replaced from passive governor)
I'd suggest to "define" data value of event_handler to include the reason
of STOP event for DEVFREQ_GOV_STOP. Then, a governor may "reject" it
depending on the reason. (the reason is to be defined in devfreq.h as well)
Then, the modification can be minimal and general for all others.
The modification in this commit looks too hacky.
Cheers,
MyungJoo
[PATCH 1/3] PM / devfreq: Fix available_governor sysfs
The devfreq using passive governor is not able to change the governor.
So, the user can not change the governor through 'available_governor' sysfs
entry. Also, the devfreq which don't use the passive governor is not able to
change to 'passive' governor on the fly.
Fixes: 996133119f57 ("PM / devfreq: Add new passive governor")
Cc: sta...@vger.kernel.org
Signed-off-by: Chanwoo Choi
---
drivers/devfreq/devfreq.c | 34 +-
1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 4bd7a8f71b07..a2c575a5a9ab 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -43,6 +43,11 @@
static LIST_HEAD(devfreq_list);
static DEFINE_MUTEX(devfreq_list_lock);
+static int is_passive_gov(const char *governor_name)
+{
+ return (!strncmp(governor_name, "passive", 7)) ? 1 : 0;
+}
+
/**
* find_device_devfreq() - find devfreq struct using device pointer
* @dev: device pointer used to lookup device devfreq.
@@ -933,6 +938,10 @@ static ssize_t governor_store(struct device *dev, struct
device_attribute *attr,
if (ret != 1)
return -EINVAL;
+ /* The passive devfreq is not able to change the governor. */
+ if (is_passive_gov(df->governor_name))
+ return 0;
+
mutex_lock(_list_lock);
governor = find_devfreq_governor(str_governor);
if (IS_ERR(governor)) {
@@ -972,12 +981,35 @@ static ssize_t available_governors_show(struct device *d,
char *buf)
{
struct devfreq_governor *tmp_governor;
+ struct devfreq *df = to_devfreq(d);
ssize_t count = 0;
mutex_lock(_list_lock);
- list_for_each_entry(tmp_governor, _governor_list, node)
+
+ /*
+* The passive devfreq shows only passive governor.
+* The governor except for passive are not available
+* for passive devfreq device.
+*/
+ if (is_passive_gov(df->governor_name)) {
+ count += scnprintf([count], (PAGE_SIZE - count - 2),
+ "%s ", df->governor_name);
+ goto out;
+ }
+
+ /*
+* The devfreq device show the registered governor except for
+* 'passive' governor.
+*/
+ list_for_each_entry(tmp_governor, _governor_list, node) {
+ if (is_passive_gov(tmp_governor->name))
+ continue;
+
count += scnprintf([count], (PAGE_SIZE - count - 2),
"%s ", tmp_governor->name);
+ }
+
+out:
mutex_unlock(_list_lock);
/* Truncate the trailing space */
--
1.9.1
[PATCH 1/3] PM / devfreq: Fix available_governor sysfs
The devfreq using passive governor is not able to change the governor.
So, the user can not change the governor through 'available_governor' sysfs
entry. Also, the devfreq which don't use the passive governor is not able to
change to 'passive' governor on the fly.
Fixes: 996133119f57 ("PM / devfreq: Add new passive governor")
Cc: sta...@vger.kernel.org
Signed-off-by: Chanwoo Choi
---
drivers/devfreq/devfreq.c | 34 +-
1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 4bd7a8f71b07..a2c575a5a9ab 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -43,6 +43,11 @@
static LIST_HEAD(devfreq_list);
static DEFINE_MUTEX(devfreq_list_lock);
+static int is_passive_gov(const char *governor_name)
+{
+ return (!strncmp(governor_name, "passive", 7)) ? 1 : 0;
+}
+
/**
* find_device_devfreq() - find devfreq struct using device pointer
* @dev: device pointer used to lookup device devfreq.
@@ -933,6 +938,10 @@ static ssize_t governor_store(struct device *dev, struct
device_attribute *attr,
if (ret != 1)
return -EINVAL;
+ /* The passive devfreq is not able to change the governor. */
+ if (is_passive_gov(df->governor_name))
+ return 0;
+
mutex_lock(_list_lock);
governor = find_devfreq_governor(str_governor);
if (IS_ERR(governor)) {
@@ -972,12 +981,35 @@ static ssize_t available_governors_show(struct device *d,
char *buf)
{
struct devfreq_governor *tmp_governor;
+ struct devfreq *df = to_devfreq(d);
ssize_t count = 0;
mutex_lock(_list_lock);
- list_for_each_entry(tmp_governor, _governor_list, node)
+
+ /*
+* The passive devfreq shows only passive governor.
+* The governor except for passive are not available
+* for passive devfreq device.
+*/
+ if (is_passive_gov(df->governor_name)) {
+ count += scnprintf([count], (PAGE_SIZE - count - 2),
+ "%s ", df->governor_name);
+ goto out;
+ }
+
+ /*
+* The devfreq device show the registered governor except for
+* 'passive' governor.
+*/
+ list_for_each_entry(tmp_governor, _governor_list, node) {
+ if (is_passive_gov(tmp_governor->name))
+ continue;
+
count += scnprintf([count], (PAGE_SIZE - count - 2),
"%s ", tmp_governor->name);
+ }
+
+out:
mutex_unlock(_list_lock);
/* Truncate the trailing space */
--
1.9.1
