[PATCH v3 0/2] PM / devfreq: Add governor feature and attribute flag

[PATCH v3 0/2] PM / devfreq: Add governor feature and attribute flag

[Chanwoo Choi]

Each devfreq governor can have the different sysfs attributes and features.
In order to provide the only available sysfs attribute to user-space,
add governor attribute flag with DEVFREQ_GOV_ATTR_[attribute name] defintion.

Also, each governor is able to have the specific flag in order to
support specific feature with DEVFREQ_GOV_FLAG_[feature name] defintion
like immutable governor.

According to each governor, can initiate the governor feature and attribute
flags.

[Common sysfs attributes for devfreq class]
And all devfreq governors have to support the following common attributes.
The common attributes are added to devfreq class by default.
- governor
- available_governors
- available_frequencies
- cur_freq
- target_freq
- min_freq
- max_freq
- trans_stat

[Definition for governor attribute flag]
- DEVFREQ_GOV_ATTR_POLLING_INTERVAL to update polling interval for timer.
  : /sys/class/devfreq/[devfreq dev name]/polling_interval
- DEVFREQ_GOV_ATTR_TIMER to change the type of timer on either deferrable
  or dealyed timer.
  : /sys/class/devfreq/[devfreq dev name]/timer

[Definition for governor feature flag]
- DEVFREQ_GOV_FLAG_IMMUTABLE
  : If immutable flag is set, governor is never changeable to other governors.
- DEVFREQ_GOV_FLAG_IRQ_DRIVEN
  : Devfreq core won't schedule polling work for this governor if value is set.

[Table of governor attribute flags for evfreq governors]
-----------------------------------------------------------------------------
                      | simple    | perfor | power | user | passive | tegra30
		      | ondemand  | mance  | save  | space|         |
------------------------------------------------------------------------------
governor              | O         | O      | O     | O    | O       | O
available_governors   | O         | O      | O     | O    | O       | O
available_frequencies | O         | O      | O     | O    | O       | O
cur_freq              | O         | O      | O     | O    | O       | O
target_freq           | O         | O      | O     | O    | O       | O
min_freq              | O         | O      | O     | O    | O       | O
max_freq              | O         | O      | O     | O    | O       | O
trans_stat            | O         | O      | O     | O    | O       | O
------------------------------------------------------------------------------
polling_interval      | O         | X      | X     | X    | X       | O
timer                 | O         | X      | X     | X    | X       | X
------------------------------------------------------------------------------
immutable             | X         | X      | X     | X    | O       | O
interrupt_driven      | X(polling)| X      | X     | X    | X       | O (irq)
------------------------------------------------------------------------------

Changes from v2:
- https://lkml.org/lkml/2020/7/13/285 ("[PATCH v2 0/2] PM / devfreq: Add governor flags")
- Hide unsupported sysfs node to user-space instead of checking the permission
of sysfs node.

Chanwoo Choi (2):
  PM / devfreq: Add governor feature flag
  PM / devfreq: Add governor attribute flag for specifc sysfs nodes

 Documentation/ABI/testing/sysfs-class-devfreq |  54 +++---
 drivers/devfreq/devfreq.c                     | 170 +++++++++++-------
 drivers/devfreq/governor.h                    |  30 +++-
 drivers/devfreq/governor_passive.c            |   2 +-
 drivers/devfreq/governor_simpleondemand.c     |   2 +
 drivers/devfreq/tegra30-devfreq.c             |   5 +-
 6 files changed, 171 insertions(+), 92 deletions(-)

-- 
2.17.1

[PATCH v3 1/2] PM / devfreq: Add governor feature flag

[Chanwoo Choi]

The devfreq governor is able to have the specific flag as follows
in order to implement the specific feature. For example, devfreq allows
user to change the governors on runtime via sysfs interface.
But, if devfreq device uses 'passive' governor, don't allow user to change
the governor. For this case, define the DEVFREQ_GOV_FLAT_IMMUTABLE
and set it to flag of passive governor.

[Definition for governor flag]
- DEVFREQ_GOV_FLAG_IMMUTABLE
  : If immutable flag is set, governor is never changeable to other governors.
- DEVFREQ_GOV_FLAG_IRQ_DRIVEN
  : Devfreq core won't schedule polling work for this governor if value is set.

[Table of governor flag for devfreq governors]
------------------------------------------------------------------------------
                      | simple    | perfor | power | user | passive | tegra30
		      | ondemand  | mance  | save  | space|         |
------------------------------------------------------------------------------
immutable             | X         | X      | X     | X    | O       | O
interrupt_driven      | X(polling)| X      | X     | X    | X       | O (irq)
------------------------------------------------------------------------------

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/devfreq.c          | 18 ++++++++++--------
 drivers/devfreq/governor.h         | 18 ++++++++++++------
 drivers/devfreq/governor_passive.c |  2 +-
 drivers/devfreq/tegra30-devfreq.c  |  4 ++--
 4 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 861c100f9fac..ce793fc9a558 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -31,6 +31,7 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/devfreq.h>
 
+#define IS_SUPPORTED_FLAG(f, name) ((f & DEVFREQ_GOV_FLAG_##name) ? true : false)
 #define HZ_PER_KHZ	1000
 
 static struct class *devfreq_class;
@@ -456,7 +457,7 @@ static void devfreq_monitor(struct work_struct *work)
  */
 void devfreq_monitor_start(struct devfreq *devfreq)
 {
-	if (devfreq->governor->interrupt_driven)
+	if (IS_SUPPORTED_FLAG(devfreq->governor->flag, IRQ_DRIVEN))
 		return;
 
 	switch (devfreq->profile->timer) {
@@ -486,7 +487,7 @@ EXPORT_SYMBOL(devfreq_monitor_start);
  */
 void devfreq_monitor_stop(struct devfreq *devfreq)
 {
-	if (devfreq->governor->interrupt_driven)
+	if (IS_SUPPORTED_FLAG(devfreq->governor->flag, IRQ_DRIVEN))
 		return;
 
 	cancel_delayed_work_sync(&devfreq->work);
@@ -517,7 +518,7 @@ void devfreq_monitor_suspend(struct devfreq *devfreq)
 	devfreq->stop_polling = true;
 	mutex_unlock(&devfreq->lock);
 
-	if (devfreq->governor->interrupt_driven)
+	if (IS_SUPPORTED_FLAG(devfreq->governor->flag, IRQ_DRIVEN))
 		return;
 
 	cancel_delayed_work_sync(&devfreq->work);
@@ -540,7 +541,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
 	if (!devfreq->stop_polling)
 		goto out;
 
-	if (devfreq->governor->interrupt_driven)
+	if (IS_SUPPORTED_FLAG(devfreq->governor->flag, IRQ_DRIVEN))
 		goto out_update;
 
 	if (!delayed_work_pending(&devfreq->work) &&
@@ -580,7 +581,7 @@ void devfreq_update_interval(struct devfreq *devfreq, unsigned int *delay)
 	if (devfreq->stop_polling)
 		goto out;
 
-	if (devfreq->governor->interrupt_driven)
+	if (IS_SUPPORTED_FLAG(devfreq->governor->flag, IRQ_DRIVEN))
 		goto out;
 
 	/* if new delay is zero, stop polling */
@@ -1347,7 +1348,8 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
 	if (df->governor == governor) {
 		ret = 0;
 		goto out;
-	} else if (df->governor->immutable || governor->immutable) {
+	} else if (IS_SUPPORTED_FLAG(df->governor->flag, IMMUTABLE)
+		|| IS_SUPPORTED_FLAG(governor->flag, IMMUTABLE)) {
 		ret = -EINVAL;
 		goto out;
 	}
@@ -1402,7 +1404,7 @@ static ssize_t available_governors_show(struct device *d,
 	 * The devfreq with immutable governor (e.g., passive) shows
 	 * only own governor.
 	 */
-	if (df->governor->immutable) {
+	if (IS_SUPPORTED_FLAG(df->governor->flag, IMMUTABLE)) {
 		count = scnprintf(&buf[count], DEVFREQ_NAME_LEN,
 				  "%s ", df->governor_name);
 	/*
@@ -1413,7 +1415,7 @@ static ssize_t available_governors_show(struct device *d,
 		struct devfreq_governor *governor;
 
 		list_for_each_entry(governor, &devfreq_governor_list, node) {
-			if (governor->immutable)
+			if (IS_SUPPORTED_FLAG(governor->flag, IMMUTABLE))
 				continue;
 			count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
 					   "%s ", governor->name);
diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
index ae4d0cc18359..540e59ef467c 100644
--- a/drivers/devfreq/governor.h
+++ b/drivers/devfreq/governor.h
@@ -25,14 +25,21 @@
 #define DEVFREQ_MIN_FREQ			0
 #define DEVFREQ_MAX_FREQ			ULONG_MAX
 
+/*
+ * Definition of the governor feature flags
+ * - DEVFREQ_GOV_FLAG_IMMUTABLE
+ *   : This governor is never changeable to other governors.
+ * - DEVFREQ_GOV_FLAG_IRQ_DRIVEN
+ *   : The devfreq won't schedule the work for this governor.
+ */
+#define DEVFREQ_GOV_FLAG_IMMUTABLE			BIT(0)
+#define DEVFREQ_GOV_FLAG_IRQ_DRIVEN			BIT(1)
+
 /**
  * struct devfreq_governor - Devfreq policy governor
  * @node:		list node - contains registered devfreq governors
  * @name:		Governor's name
- * @immutable:		Immutable flag for governor. If the value is 1,
- *			this governor is never changeable to other governor.
- * @interrupt_driven:	Devfreq core won't schedule polling work for this
- *			governor if value is set to 1.
+ * @flag:		Governor's feature flag
  * @get_target_freq:	Returns desired operating frequency for the device.
  *			Basically, get_target_freq will run
  *			devfreq_dev_profile.get_dev_status() to get the
@@ -50,8 +57,7 @@ struct devfreq_governor {
 	struct list_head node;
 
 	const char name[DEVFREQ_NAME_LEN];
-	const unsigned int immutable;
-	const unsigned int interrupt_driven;
+	const u64 flag;
 	int (*get_target_freq)(struct devfreq *this, unsigned long *freq);
 	int (*event_handler)(struct devfreq *devfreq,
 				unsigned int event, void *data);
diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
index be6eeab9c814..432a4cc683f7 100644
--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -180,7 +180,7 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
 
 static struct devfreq_governor devfreq_passive = {
 	.name = DEVFREQ_GOV_PASSIVE,
-	.immutable = 1,
+	.flag = DEVFREQ_GOV_FLAG_IMMUTABLE,
 	.get_target_freq = devfreq_passive_get_target_freq,
 	.event_handler = devfreq_passive_event_handler,
 };
diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index f5e74c2ede85..868c72cc0276 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -765,10 +765,10 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
 
 static struct devfreq_governor tegra_devfreq_governor = {
 	.name = "tegra_actmon",
+	.flag = DEVFREQ_GOV_FLAG_IMMUTABLE
+		| DEVFREQ_GOV_FLAG_IRQ_DRIVEN,
 	.get_target_freq = tegra_governor_get_target,
 	.event_handler = tegra_governor_event_handler,
-	.immutable = true,
-	.interrupt_driven = true,
 };
 
 static int tegra_devfreq_probe(struct platform_device *pdev)
-- 
2.17.1

Re: [PATCH v3 1/2] PM / devfreq: Add governor feature flag

[Dmitry Osipenko]

07.10.2020 08:07, Chanwoo Choi пишет:
> The devfreq governor is able to have the specific flag as follows
> in order to implement the specific feature. For example, devfreq allows
> user to change the governors on runtime via sysfs interface.
> But, if devfreq device uses 'passive' governor, don't allow user to change
> the governor. For this case, define the DEVFREQ_GOV_FLAT_IMMUTABLE

s/DEVFREQ_GOV_FLAT/DEVFREQ_GOV_FLAG/

...
>  /**
>   * struct devfreq_governor - Devfreq policy governor
>   * @node:		list node - contains registered devfreq governors
>   * @name:		Governor's name
> - * @immutable:		Immutable flag for governor. If the value is 1,
> - *			this governor is never changeable to other governor.
> - * @interrupt_driven:	Devfreq core won't schedule polling work for this
> - *			governor if value is set to 1.
> + * @flag:		Governor's feature flag
>   * @get_target_freq:	Returns desired operating frequency for the device.
>   *			Basically, get_target_freq will run
>   *			devfreq_dev_profile.get_dev_status() to get the
> @@ -50,8 +57,7 @@ struct devfreq_governor {
>  	struct list_head node;
>  
>  	const char name[DEVFREQ_NAME_LEN];
> -	const unsigned int immutable;
> -	const unsigned int interrupt_driven;
> +	const u64 flag;
A plural form of flag(s) is more common, IMO.

It's also possible to use a single bit:1 for the struct members. Thus,
could you please explain what are the benefits of the "flag"?

Re: [PATCH v3 1/2] PM / devfreq: Add governor feature flag

[Chanwoo Choi]

On 10/19/20 9:57 AM, Dmitry Osipenko wrote:
> 07.10.2020 08:07, Chanwoo Choi пишет:
>> The devfreq governor is able to have the specific flag as follows
>> in order to implement the specific feature. For example, devfreq allows
>> user to change the governors on runtime via sysfs interface.
>> But, if devfreq device uses 'passive' governor, don't allow user to change
>> the governor. For this case, define the DEVFREQ_GOV_FLAT_IMMUTABLE
> 
> s/DEVFREQ_GOV_FLAT/DEVFREQ_GOV_FLAG/
> 
> ...
>>  /**
>>   * struct devfreq_governor - Devfreq policy governor
>>   * @node:		list node - contains registered devfreq governors
>>   * @name:		Governor's name
>> - * @immutable:		Immutable flag for governor. If the value is 1,
>> - *			this governor is never changeable to other governor.
>> - * @interrupt_driven:	Devfreq core won't schedule polling work for this
>> - *			governor if value is set to 1.
>> + * @flag:		Governor's feature flag
>>   * @get_target_freq:	Returns desired operating frequency for the device.
>>   *			Basically, get_target_freq will run
>>   *			devfreq_dev_profile.get_dev_status() to get the
>> @@ -50,8 +57,7 @@ struct devfreq_governor {
>>  	struct list_head node;
>>  
>>  	const char name[DEVFREQ_NAME_LEN];
>> -	const unsigned int immutable;
>> -	const unsigned int interrupt_driven;
>> +	const u64 flag;
> A plural form of flag(s) is more common, IMO.

When need to add more feature flag, I prefer to add
the definition instead of changing the structure.
I think it is better.

> 
> It's also possible to use a single bit:1 for the struct members. Thus,
> could you please explain what are the benefits of the "flag"?

I think that anyone might add the some optional
feature. So, I used 'flag' for the extensibility.



-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v3 1/2] PM / devfreq: Add governor feature flag

[Dmitry Osipenko]

19.10.2020 06:53, Chanwoo Choi пишет:
...
>>> +	const u64 flag;
>> A plural form of flag(s) is more common, IMO.
> 
> When need to add more feature flag, I prefer to add
> the definition instead of changing the structure.
> I think it is better.

I meant to rename the new member "flag" to "flags".

Re: [PATCH v3 1/2] PM / devfreq: Add governor feature flag

[Chanwoo Choi]

On 10/20/20 7:39 AM, Dmitry Osipenko wrote:
> 19.10.2020 06:53, Chanwoo Choi пишет:
> ...
>>>> +	const u64 flag;
>>> A plural form of flag(s) is more common, IMO.
>>
>> When need to add more feature flag, I prefer to add
>> the definition instead of changing the structure.
>> I think it is better.
> 
> I meant to rename the new member "flag" to "flags".

I misunderstood. I'll edit it.


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

[PATCH v3 2/2] PM / devfreq: Add governor attribute flag for specifc sysfs nodes

[Chanwoo Choi]

DEVFREQ supports the default governors like performance, simple_ondemand and
also allows the devfreq driver to add their own governor like tegra30-devfreq.c
according to their requirement. In result, some sysfs attributes are useful
or not useful. Prior to that the user can access all sysfs attributes
regardless of the available attributes.

So, clarify the access permission of sysfs attributes according to governor.
When adding the devfreq governor, can specify the available attribute
information by using DEVFREQ_GOV_ATTR_* constant variable. The user can
read or write the sysfs attributes in accordance to the specified attributes.

When adding the governor, can add the following attributes
according to the governor feature.

[Definition for speific sysfs attributes]
- DEVFREQ_GOV_ATTR_POLLING_INTERVAL to update polling interval for timer.
  : /sys/class/devfreq/[devfreq dev name]/polling_interval
- DEVFREQ_GOV_ATTR_TIMER to change the type of timer on either deferrable
  or dealyed timer.
  : /sys/class/devfreq/[devfreq dev name]/timer

And all devfreq governors have to support the following common attributes.
The common attributes are added to devfreq class by default.
- governor
- available_governors
- available_frequencies
- cur_freq
- target_freq
- min_freq
- max_freq
- trans_stat

[Table of governor attribute flags for devfreq governors]
------------------------------------------------------------------------------
                      | simple    | perfor | power | user | passive | tegra30
		      | ondemand  | mance  | save  | space|         |
------------------------------------------------------------------------------
governor              | O         | O      | O     | O    | O       | O
available_governors   | O         | O      | O     | O    | O       | O
available_frequencies | O         | O      | O     | O    | O       | O
cur_freq              | O         | O      | O     | O    | O       | O
target_freq           | O         | O      | O     | O    | O       | O
min_freq              | O         | O      | O     | O    | O       | O
max_freq              | O         | O      | O     | O    | O       | O
trans_stat            | O         | O      | O     | O    | O       | O
                      --------------------------------------------------------
polling_interval      | O         | X      | X     | X    | X       | O
timer                 | O         | X      | X     | X    | X       | X
------------------------------------------------------------------------------

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 Documentation/ABI/testing/sysfs-class-devfreq |  54 +++---
 drivers/devfreq/devfreq.c                     | 154 ++++++++++++------
 drivers/devfreq/governor.h                    |  12 ++
 drivers/devfreq/governor_simpleondemand.c     |   2 +
 drivers/devfreq/tegra30-devfreq.c             |   1 +
 5 files changed, 147 insertions(+), 76 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-devfreq b/Documentation/ABI/testing/sysfs-class-devfreq
index deefffb3bbe4..67af3f31e17c 100644
--- a/Documentation/ABI/testing/sysfs-class-devfreq
+++ b/Documentation/ABI/testing/sysfs-class-devfreq
@@ -37,20 +37,6 @@ Description:
 		The /sys/class/devfreq/.../target_freq shows the next governor
 		predicted target frequency of the corresponding devfreq object.
 
-What:		/sys/class/devfreq/.../polling_interval
-Date:		September 2011
-Contact:	MyungJoo Ham <myungjoo.ham@samsung.com>
-Description:
-		The /sys/class/devfreq/.../polling_interval shows and sets
-		the requested polling interval of the corresponding devfreq
-		object. The values are represented in ms. If the value is
-		less than 1 jiffy, it is considered to be 0, which means
-		no polling. This value is meaningless if the governor is
-		not polling; thus. If the governor is not using
-		devfreq-provided central polling
-		(/sys/class/devfreq/.../central_polling is 0), this value
-		may be useless.
-
 What:		/sys/class/devfreq/.../trans_stat
 Date:		October 2012
 Contact:	MyungJoo Ham <myungjoo.ham@samsung.com>
@@ -65,14 +51,6 @@ Description:
 		as following:
 			echo 0 > /sys/class/devfreq/.../trans_stat
 
-What:		/sys/class/devfreq/.../userspace/set_freq
-Date:		September 2011
-Contact:	MyungJoo Ham <myungjoo.ham@samsung.com>
-Description:
-		The /sys/class/devfreq/.../userspace/set_freq shows and
-		sets the requested frequency for the devfreq object if
-		userspace governor is in effect.
-
 What:		/sys/class/devfreq/.../available_frequencies
 Date:		October 2012
 Contact:	Nishanth Menon <nm@ti.com>
@@ -109,6 +87,35 @@ Description:
 		The max_freq overrides min_freq because max_freq may be
 		used to throttle devices to avoid overheating.
 
+What:		/sys/class/devfreq/.../polling_interval
+Date:		September 2011
+Contact:	MyungJoo Ham <myungjoo.ham@samsung.com>
+Description:
+		The /sys/class/devfreq/.../polling_interval shows and sets
+		the requested polling interval of the corresponding devfreq
+		object. The values are represented in ms. If the value is
+		less than 1 jiffy, it is considered to be 0, which means
+		no polling. This value is meaningless if the governor is
+		not polling; thus. If the governor is not using
+		devfreq-provided central polling
+		(/sys/class/devfreq/.../central_polling is 0), this value
+		may be useless.
+
+		A list of governors that support the node:
+		- simple_ondmenad
+		- tegra_actmon
+
+What:		/sys/class/devfreq/.../userspace/set_freq
+Date:		September 2011
+Contact:	MyungJoo Ham <myungjoo.ham@samsung.com>
+Description:
+		The /sys/class/devfreq/.../userspace/set_freq shows and
+		sets the requested frequency for the devfreq object if
+		userspace governor is in effect.
+
+		A list of governors that support the node:
+		- userspace
+
 What:		/sys/class/devfreq/.../timer
 Date:		July 2020
 Contact:	Chanwoo Choi <cw00.choi@samsung.com>
@@ -120,3 +127,6 @@ Description:
 		as following:
 			echo deferrable > /sys/class/devfreq/.../timer
 			echo delayed > /sys/class/devfreq/.../timer
+
+		A list of governors that support the node:
+		- simple_ondemand
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index ce793fc9a558..379aaaabf25d 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -32,6 +32,7 @@
 #include <trace/events/devfreq.h>
 
 #define IS_SUPPORTED_FLAG(f, name) ((f & DEVFREQ_GOV_FLAG_##name) ? true : false)
+#define IS_SUPPORTED_ATTR(f, name) ((f & DEVFREQ_GOV_ATTR_##name) ? true : false)
 #define HZ_PER_KHZ	1000
 
 static struct class *devfreq_class;
@@ -538,12 +539,13 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
 	unsigned long freq;
 
 	mutex_lock(&devfreq->lock);
-	if (!devfreq->stop_polling)
-		goto out;
 
 	if (IS_SUPPORTED_FLAG(devfreq->governor->flag, IRQ_DRIVEN))
 		goto out_update;
 
+	if (!devfreq->stop_polling)
+		goto out;
+
 	if (!delayed_work_pending(&devfreq->work) &&
 			devfreq->profile->polling_ms)
 		queue_delayed_work(devfreq_wq, &devfreq->work,
@@ -578,10 +580,10 @@ void devfreq_update_interval(struct devfreq *devfreq, unsigned int *delay)
 	mutex_lock(&devfreq->lock);
 	devfreq->profile->polling_ms = new_delay;
 
-	if (devfreq->stop_polling)
+	if (DEVFREQ_GOV_FLAG_IRQ_DRIVEN & devfreq->governor->flag)
 		goto out;
 
-	if (IS_SUPPORTED_FLAG(devfreq->governor->flag, IRQ_DRIVEN))
+	if (devfreq->stop_polling)
 		goto out;
 
 	/* if new delay is zero, stop polling */
@@ -736,6 +738,11 @@ static void devfreq_dev_release(struct device *dev)
 	kfree(devfreq);
 }
 
+static void create_sysfs_files(struct devfreq *devfreq,
+				const struct devfreq_governor *gov);
+static void remove_sysfs_files(struct devfreq *devfreq,
+				const struct devfreq_governor *gov);
+
 /**
  * devfreq_add_device() - Add devfreq feature to the device
  * @dev:	the device to add devfreq feature.
@@ -885,6 +892,8 @@ struct devfreq *devfreq_add_device(struct device *dev,
 		goto err_init;
 	}
 
+	create_sysfs_files(devfreq, governor);
+
 	devfreq->governor = governor;
 	err = devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_START,
 						NULL);
@@ -923,9 +932,12 @@ int devfreq_remove_device(struct devfreq *devfreq)
 	if (!devfreq)
 		return -EINVAL;
 
-	if (devfreq->governor)
+	if (devfreq->governor) {
 		devfreq->governor->event_handler(devfreq,
 						 DEVFREQ_GOV_STOP, NULL);
+		remove_sysfs_files(devfreq, devfreq->governor);
+	}
+
 	device_unregister(&devfreq->dev);
 
 	return 0;
@@ -1361,6 +1373,9 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
 		goto out;
 	}
 
+	remove_sysfs_files(df, df->governor);
+	create_sysfs_files(df, governor);
+
 	prev_governor = df->governor;
 	df->governor = governor;
 	strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN);
@@ -1460,39 +1475,6 @@ static ssize_t target_freq_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(target_freq);
 
-static ssize_t polling_interval_show(struct device *dev,
-				     struct device_attribute *attr, char *buf)
-{
-	struct devfreq *df = to_devfreq(dev);
-
-	if (!df->profile)
-		return -EINVAL;
-
-	return sprintf(buf, "%d\n", df->profile->polling_ms);
-}
-
-static ssize_t polling_interval_store(struct device *dev,
-				      struct device_attribute *attr,
-				      const char *buf, size_t count)
-{
-	struct devfreq *df = to_devfreq(dev);
-	unsigned int value;
-	int ret;
-
-	if (!df->governor)
-		return -EINVAL;
-
-	ret = sscanf(buf, "%u", &value);
-	if (ret != 1)
-		return -EINVAL;
-
-	df->governor->event_handler(df, DEVFREQ_GOV_UPDATE_INTERVAL, &value);
-	ret = count;
-
-	return ret;
-}
-static DEVICE_ATTR_RW(polling_interval);
-
 static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
 			      const char *buf, size_t count)
 {
@@ -1700,6 +1682,53 @@ static ssize_t trans_stat_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(trans_stat);
 
+static struct attribute *devfreq_attrs[] = {
+	&dev_attr_name.attr,
+	&dev_attr_governor.attr,
+	&dev_attr_available_governors.attr,
+	&dev_attr_cur_freq.attr,
+	&dev_attr_available_frequencies.attr,
+	&dev_attr_target_freq.attr,
+	&dev_attr_min_freq.attr,
+	&dev_attr_max_freq.attr,
+	&dev_attr_trans_stat.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(devfreq);
+
+static ssize_t polling_interval_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct devfreq *df = to_devfreq(dev);
+
+	if (!df->profile)
+		return -EINVAL;
+
+	return sprintf(buf, "%d\n", df->profile->polling_ms);
+}
+
+static ssize_t polling_interval_store(struct device *dev,
+				      struct device_attribute *attr,
+				      const char *buf, size_t count)
+{
+	struct devfreq *df = to_devfreq(dev);
+	unsigned int value;
+	int ret;
+
+	if (!df->governor)
+		return -EINVAL;
+
+	ret = sscanf(buf, "%u", &value);
+	if (ret != 1)
+		return -EINVAL;
+
+	df->governor->event_handler(df, DEVFREQ_GOV_UPDATE_INTERVAL, &value);
+	ret = count;
+
+	return ret;
+}
+static DEVICE_ATTR_RW(polling_interval);
+
 static ssize_t timer_show(struct device *dev,
 			     struct device_attribute *attr, char *buf)
 {
@@ -1763,21 +1792,34 @@ static ssize_t timer_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RW(timer);
 
-static struct attribute *devfreq_attrs[] = {
-	&dev_attr_name.attr,
-	&dev_attr_governor.attr,
-	&dev_attr_available_governors.attr,
-	&dev_attr_cur_freq.attr,
-	&dev_attr_available_frequencies.attr,
-	&dev_attr_target_freq.attr,
-	&dev_attr_polling_interval.attr,
-	&dev_attr_min_freq.attr,
-	&dev_attr_max_freq.attr,
-	&dev_attr_trans_stat.attr,
-	&dev_attr_timer.attr,
-	NULL,
-};
-ATTRIBUTE_GROUPS(devfreq);
+#define CREATE_SYSFS_FILE(df, name)					\
+{									\
+	int ret;							\
+	ret = sysfs_create_file(&df->dev.kobj, &dev_attr_##name.attr);	\
+	if (ret < 0) {							\
+		dev_warn(&df->dev,					\
+			"Unable to create attr(%s)\n", "##name");	\
+	}								\
+}									\
+
+static void create_sysfs_files(struct devfreq *devfreq,
+				const struct devfreq_governor *gov)
+{
+	if (IS_SUPPORTED_ATTR(gov->attr, POLLING_INTERVAL))
+		CREATE_SYSFS_FILE(devfreq, polling_interval);
+	if (IS_SUPPORTED_ATTR(gov->attr, TIMER))
+		CREATE_SYSFS_FILE(devfreq, timer);
+}
+
+static void remove_sysfs_files(struct devfreq *devfreq,
+				const struct devfreq_governor *gov)
+{
+	if (IS_SUPPORTED_ATTR(gov->attr, POLLING_INTERVAL))
+		sysfs_remove_file(&devfreq->dev.kobj,
+				&dev_attr_polling_interval.attr);
+	if (IS_SUPPORTED_ATTR(gov->attr, TIMER))
+		sysfs_remove_file(&devfreq->dev.kobj, &dev_attr_timer.attr);
+}
 
 /**
  * devfreq_summary_show() - Show the summary of the devfreq devices
@@ -1834,8 +1876,12 @@ static int devfreq_summary_show(struct seq_file *s, void *data)
 		mutex_lock(&devfreq->lock);
 		cur_freq = devfreq->previous_freq;
 		get_freq_range(devfreq, &min_freq, &max_freq);
-		polling_ms = devfreq->profile->polling_ms;
 		timer = devfreq->profile->timer;
+
+		if (IS_SUPPORTED_ATTR(devfreq->governor->attr, POLLING_INTERVAL))
+			polling_ms = devfreq->profile->polling_ms;
+		else
+			polling_ms = 0;
 		mutex_unlock(&devfreq->lock);
 
 		seq_printf(s,
diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
index 540e59ef467c..eb6392d397b3 100644
--- a/drivers/devfreq/governor.h
+++ b/drivers/devfreq/governor.h
@@ -35,10 +35,21 @@
 #define DEVFREQ_GOV_FLAG_IMMUTABLE			BIT(0)
 #define DEVFREQ_GOV_FLAG_IRQ_DRIVEN			BIT(1)
 
+/*
+ * Definition of governor attribute flags except for common sysfs attributes
+ * - DEVFREQ_GOV_ATTR_POLLING_INTERVAL
+ *   : Indicate polling_interal sysfs attribute
+ * - DEVFREQ_GOV_ATTR_TIMER
+ *   : Indicate timer sysfs attribute
+ */
+#define DEVFREQ_GOV_ATTR_POLLING_INTERVAL		BIT(0)
+#define DEVFREQ_GOV_ATTR_TIMER				BIT(1)
+
 /**
  * struct devfreq_governor - Devfreq policy governor
  * @node:		list node - contains registered devfreq governors
  * @name:		Governor's name
+ * @attr:		Governor's sysfs attribute flag
  * @flag:		Governor's feature flag
  * @get_target_freq:	Returns desired operating frequency for the device.
  *			Basically, get_target_freq will run
@@ -57,6 +68,7 @@ struct devfreq_governor {
 	struct list_head node;
 
 	const char name[DEVFREQ_NAME_LEN];
+	const u64 attr;
 	const u64 flag;
 	int (*get_target_freq)(struct devfreq *this, unsigned long *freq);
 	int (*event_handler)(struct devfreq *devfreq,
diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c
index 1b314e1df028..28b7f9d8fb4e 100644
--- a/drivers/devfreq/governor_simpleondemand.c
+++ b/drivers/devfreq/governor_simpleondemand.c
@@ -117,6 +117,8 @@ static int devfreq_simple_ondemand_handler(struct devfreq *devfreq,
 
 static struct devfreq_governor devfreq_simple_ondemand = {
 	.name = DEVFREQ_GOV_SIMPLE_ONDEMAND,
+	.attr = DEVFREQ_GOV_ATTR_POLLING_INTERVAL
+		| DEVFREQ_GOV_ATTR_TIMER,
 	.get_target_freq = devfreq_simple_ondemand_func,
 	.event_handler = devfreq_simple_ondemand_handler,
 };
diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index 868c72cc0276..150eb70e62db 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -765,6 +765,7 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
 
 static struct devfreq_governor tegra_devfreq_governor = {
 	.name = "tegra_actmon",
+	.attr = DEVFREQ_GOV_ATTR_POLLING_INTERVAL,
 	.flag = DEVFREQ_GOV_FLAG_IMMUTABLE
 		| DEVFREQ_GOV_FLAG_IRQ_DRIVEN,
 	.get_target_freq = tegra_governor_get_target,
-- 
2.17.1

Re: [PATCH v3 2/2] PM / devfreq: Add governor attribute flag for specifc sysfs nodes

[Dmitry Osipenko]

...
> diff --git a/Documentation/ABI/testing/sysfs-class-devfreq b/Documentation/ABI/testing/sysfs-class-devfreq
> index deefffb3bbe4..67af3f31e17c 100644
> --- a/Documentation/ABI/testing/sysfs-class-devfreq
> +++ b/Documentation/ABI/testing/sysfs-class-devfreq
> @@ -37,20 +37,6 @@ Description:
>  		The /sys/class/devfreq/.../target_freq shows the next governor
>  		predicted target frequency of the corresponding devfreq object.
>  
> -What:		/sys/class/devfreq/.../polling_interval
> -Date:		September 2011
> -Contact:	MyungJoo Ham <myungjoo.ham@samsung.com>
> -Description:
> -		The /sys/class/devfreq/.../polling_interval shows and sets
> -		the requested polling interval of the corresponding devfreq
> -		object. The values are represented in ms. If the value is
> -		less than 1 jiffy, it is considered to be 0, which means
> -		no polling. This value is meaningless if the governor is
> -		not polling; thus. If the governor is not using
> -		devfreq-provided central polling
> -		(/sys/class/devfreq/.../central_polling is 0), this value
> -		may be useless.
> -
>  What:		/sys/class/devfreq/.../trans_stat
>  Date:		October 2012
>  Contact:	MyungJoo Ham <myungjoo.ham@samsung.com>
> @@ -65,14 +51,6 @@ Description:
>  		as following:
>  			echo 0 > /sys/class/devfreq/.../trans_stat
>  
> -What:		/sys/class/devfreq/.../userspace/set_freq
> -Date:		September 2011
> -Contact:	MyungJoo Ham <myungjoo.ham@samsung.com>
> -Description:
> -		The /sys/class/devfreq/.../userspace/set_freq shows and
> -		sets the requested frequency for the devfreq object if
> -		userspace governor is in effect.
> -
>  What:		/sys/class/devfreq/.../available_frequencies
>  Date:		October 2012
>  Contact:	Nishanth Menon <nm@ti.com>
> @@ -109,6 +87,35 @@ Description:
>  		The max_freq overrides min_freq because max_freq may be
>  		used to throttle devices to avoid overheating.
>  
> +What:		/sys/class/devfreq/.../polling_interval
> +Date:		September 2011
> +Contact:	MyungJoo Ham <myungjoo.ham@samsung.com>
> +Description:
> +		The /sys/class/devfreq/.../polling_interval shows and sets
> +		the requested polling interval of the corresponding devfreq
> +		object. The values are represented in ms. If the value is
> +		less than 1 jiffy, it is considered to be 0, which means
> +		no polling. This value is meaningless if the governor is
> +		not polling; thus. If the governor is not using
> +		devfreq-provided central polling
> +		(/sys/class/devfreq/.../central_polling is 0), this value
> +		may be useless.
> +
> +		A list of governors that support the node:
> +		- simple_ondmenad
> +		- tegra_actmon
> +
> +What:		/sys/class/devfreq/.../userspace/set_freq
> +Date:		September 2011
> +Contact:	MyungJoo Ham <myungjoo.ham@samsung.com>
> +Description:
> +		The /sys/class/devfreq/.../userspace/set_freq shows and
> +		sets the requested frequency for the devfreq object if
> +		userspace governor is in effect.
> +
> +		A list of governors that support the node:
> +		- userspace
> +
>  What:		/sys/class/devfreq/.../timer
>  Date:		July 2020
>  Contact:	Chanwoo Choi <cw00.choi@samsung.com>
> @@ -120,3 +127,6 @@ Description:
>  		as following:
>  			echo deferrable > /sys/class/devfreq/.../timer
>  			echo delayed > /sys/class/devfreq/.../timer
> +
> +		A list of governors that support the node:
> +		- simple_ondemand

Hello, Chanwoo!

Could you please explain the reason of changing the doc? It looks like
you only added the lists of governors, but is it a really useful change?
Are you going to keep these lists up-to-date?

Re: [PATCH v3 2/2] PM / devfreq: Add governor attribute flag for specifc sysfs nodes

[Chanwoo Choi]

On 10/19/20 9:38 AM, Dmitry Osipenko wrote:
> ...
>> diff --git a/Documentation/ABI/testing/sysfs-class-devfreq b/Documentation/ABI/testing/sysfs-class-devfreq
>> index deefffb3bbe4..67af3f31e17c 100644
>> --- a/Documentation/ABI/testing/sysfs-class-devfreq
>> +++ b/Documentation/ABI/testing/sysfs-class-devfreq
>> @@ -37,20 +37,6 @@ Description:
>>  		The /sys/class/devfreq/.../target_freq shows the next governor
>>  		predicted target frequency of the corresponding devfreq object.
>>  
>> -What:		/sys/class/devfreq/.../polling_interval
>> -Date:		September 2011
>> -Contact:	MyungJoo Ham <myungjoo.ham@samsung.com>
>> -Description:
>> -		The /sys/class/devfreq/.../polling_interval shows and sets
>> -		the requested polling interval of the corresponding devfreq
>> -		object. The values are represented in ms. If the value is
>> -		less than 1 jiffy, it is considered to be 0, which means
>> -		no polling. This value is meaningless if the governor is
>> -		not polling; thus. If the governor is not using
>> -		devfreq-provided central polling
>> -		(/sys/class/devfreq/.../central_polling is 0), this value
>> -		may be useless.
>> -
>>  What:		/sys/class/devfreq/.../trans_stat
>>  Date:		October 2012
>>  Contact:	MyungJoo Ham <myungjoo.ham@samsung.com>
>> @@ -65,14 +51,6 @@ Description:
>>  		as following:
>>  			echo 0 > /sys/class/devfreq/.../trans_stat
>>  
>> -What:		/sys/class/devfreq/.../userspace/set_freq
>> -Date:		September 2011
>> -Contact:	MyungJoo Ham <myungjoo.ham@samsung.com>
>> -Description:
>> -		The /sys/class/devfreq/.../userspace/set_freq shows and
>> -		sets the requested frequency for the devfreq object if
>> -		userspace governor is in effect.
>> -
>>  What:		/sys/class/devfreq/.../available_frequencies
>>  Date:		October 2012
>>  Contact:	Nishanth Menon <nm@ti.com>
>> @@ -109,6 +87,35 @@ Description:
>>  		The max_freq overrides min_freq because max_freq may be
>>  		used to throttle devices to avoid overheating.
>>  
>> +What:		/sys/class/devfreq/.../polling_interval
>> +Date:		September 2011
>> +Contact:	MyungJoo Ham <myungjoo.ham@samsung.com>
>> +Description:
>> +		The /sys/class/devfreq/.../polling_interval shows and sets
>> +		the requested polling interval of the corresponding devfreq
>> +		object. The values are represented in ms. If the value is
>> +		less than 1 jiffy, it is considered to be 0, which means
>> +		no polling. This value is meaningless if the governor is
>> +		not polling; thus. If the governor is not using
>> +		devfreq-provided central polling
>> +		(/sys/class/devfreq/.../central_polling is 0), this value
>> +		may be useless.
>> +
>> +		A list of governors that support the node:
>> +		- simple_ondmenad
>> +		- tegra_actmon
>> +
>> +What:		/sys/class/devfreq/.../userspace/set_freq
>> +Date:		September 2011
>> +Contact:	MyungJoo Ham <myungjoo.ham@samsung.com>
>> +Description:
>> +		The /sys/class/devfreq/.../userspace/set_freq shows and
>> +		sets the requested frequency for the devfreq object if
>> +		userspace governor is in effect.
>> +
>> +		A list of governors that support the node:
>> +		- userspace
>> +
>>  What:		/sys/class/devfreq/.../timer
>>  Date:		July 2020
>>  Contact:	Chanwoo Choi <cw00.choi@samsung.com>
>> @@ -120,3 +127,6 @@ Description:
>>  		as following:
>>  			echo deferrable > /sys/class/devfreq/.../timer
>>  			echo delayed > /sys/class/devfreq/.../timer
>> +
>> +		A list of governors that support the node:
>> +		- simple_ondemand
> 
> Hello, Chanwoo!
> 
> Could you please explain the reason of changing the doc? It looks like
> you only added the lists of governors, but is it a really useful change?
> Are you going to keep these lists up-to-date?

I think that is is useful. Because user cannot know why specific sysfs node
(like 'timer') is absence according to governor. So, in order to remove
the user confusion, better to add the information to documentation.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v3 2/2] PM / devfreq: Add governor attribute flag for specifc sysfs nodes

[Dmitry Osipenko]

...
> @@ -1361,6 +1373,9 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>  		goto out;
>  	}
>  
> +	remove_sysfs_files(df, df->governor);
> +	create_sysfs_files(df, governor);
> +
>  	prev_governor = df->governor;
>  	df->governor = governor;
>  	strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN);
> @@ -1460,39 +1475,6 @@ static ssize_t target_freq_show(struct device *dev,
>  }

The further code may revert df->governor to the prev_governor or set it
to NULL. The create_sysfs_files(df->governor) should be invoked at the
very end of the governor_store() and only in a case of success.

Re: [PATCH v3 2/2] PM / devfreq: Add governor attribute flag for specifc sysfs nodes

[Chanwoo Choi]

On 10/19/20 9:39 AM, Dmitry Osipenko wrote:
> ...
>> @@ -1361,6 +1373,9 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>  		goto out;
>>  	}
>>  
>> +	remove_sysfs_files(df, df->governor);
>> +	create_sysfs_files(df, governor);
>> +
>>  	prev_governor = df->governor;
>>  	df->governor = governor;
>>  	strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN);
>> @@ -1460,39 +1475,6 @@ static ssize_t target_freq_show(struct device *dev,
>>  }
> 
> The further code may revert df->governor to the prev_governor or set it

prev_governor is better. I'll change it.

> to NULL. The create_sysfs_files(df->governor) should be invoked at the
> very end of the governor_store() and only in a case of success.

OK. I'll add more exception handling code.


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v3 2/2] PM / devfreq: Add governor attribute flag for specifc sysfs nodes

[Chanwoo Choi]

On 10/19/20 1:11 PM, Chanwoo Choi wrote:
> On 10/19/20 9:39 AM, Dmitry Osipenko wrote:
>> ...
>>> @@ -1361,6 +1373,9 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>>  		goto out;
>>>  	}
>>>  
>>> +	remove_sysfs_files(df, df->governor);
>>> +	create_sysfs_files(df, governor);
>>> +
>>>  	prev_governor = df->governor;
>>>  	df->governor = governor;
>>>  	strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN);
>>> @@ -1460,39 +1475,6 @@ static ssize_t target_freq_show(struct device *dev,
>>>  }
>>
>> The further code may revert df->governor to the prev_governor or set it
> 
> prev_governor is better. I'll change it.
> 
>> to NULL. The create_sysfs_files(df->governor) should be invoked at the

Also, when creating and removing the sysfs files, devfreq instance is needed
because of df->dev.kobj. So, *_sysfs_files need the two parameters.

>> very end of the governor_store() and only in a case of success.
> 
> OK. I'll add more exception handling code.
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics