[PATCH v2 0/2] PM / devfreq: Add governor flags

[PATCH v2 0/2] PM / devfreq: Add governor flags

[Chanwoo Choi]

Devfreq provides the multiple governors and sysfs interface for user.
But, some sysfs attributes are useful or not useful. Prior to that
the user can access all sysfs attributes regardless of availability.

So, clarify the access permission of sysfs attributes according to governor.
Provide the governor flag to specify what is necessary or not.
When adding the devfreq governor, governor can specify the available
attributes with DEVFREQ_GOV_ATTR_ATTR_* defintion as following.

[Definition for sysfs attributes]
- DEVFREQ_GOV_ATTR_GOVERNOR
- DEVFREQ_GOV_ATTR_AVAIL_GOVERNORS
- DEVFREQ_GOV_ATTR_AVAIL_FREQUENCIES
- DEVFREQ_GOV_ATTR_CUR_FREQ
- DEVFREQ_GOV_ATTR_TARGET_FREQ
- DEVFREQ_GOV_ATTR_MIN_FREQ
- DEVFREQ_GOV_ATTR_MAX_FREQ
- DEVFREQ_GOV_ATTR_TRANS_STAT
- DEVFREQ_GOV_ATTR_POLLING_INTERVAL
- DEVFREQ_GOV_ATTR_TIMER

Also, the devfreq governor is able to have the specific flag as follows
in order to implement the specific feature with DEVFREQ_GOV_FLA_*.
For exmaple, the devfreq deivce using passive governor cannot change
their own governor because passive governor requires the 'immutable'
feature with DEVFREQ_GOV_FLAG_IMMUTABLE.

[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.


Changes from v1:
- Rebase it on latest devfreq-next branch
- Fix typo issue of tegra30-devfreq.c and test it with COMPILE_TEST

Chanwoo Choi (2):
  PM / devfreq: Clean up the devfreq instance name in sysfs attr
  PM / devfreq: Add governor flags to clarify the features

 drivers/devfreq/devfreq.c                 | 186 +++++++++++++++++-----
 drivers/devfreq/governor.h                |  44 ++++-
 drivers/devfreq/governor_passive.c        |   3 +-
 drivers/devfreq/governor_performance.c    |   1 +
 drivers/devfreq/governor_powersave.c      |   1 +
 drivers/devfreq/governor_simpleondemand.c |   3 +
 drivers/devfreq/governor_userspace.c      |   1 +
 drivers/devfreq/tegra30-devfreq.c         |   6 +-
 8 files changed, 192 insertions(+), 53 deletions(-)

-- 
2.17.1

[PATCH v2 1/2] PM / devfreq: Clean up the devfreq instance name in sysfs attr

[Chanwoo Choi]

The sysfs attr interface used eithere 'df' or 'devfreq' for devfreq instance
name. In order to keep the consistency and to improve the readabilty,
unify the instance name as 'df'. Add add the missing conditional statement
to prevent the fault.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/devfreq.c | 94 +++++++++++++++++++++++++--------------
 1 file changed, 60 insertions(+), 34 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 5320c3b37f35..286957f760f1 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -1280,18 +1280,20 @@ EXPORT_SYMBOL(devfreq_remove_governor);
 static ssize_t name_show(struct device *dev,
 			struct device_attribute *attr, char *buf)
 {
-	struct devfreq *devfreq = to_devfreq(dev);
-	return sprintf(buf, "%s\n", dev_name(devfreq->dev.parent));
+	struct devfreq *df = to_devfreq(dev);
+	return sprintf(buf, "%s\n", dev_name(df->dev.parent));
 }
 static DEVICE_ATTR_RO(name);
 
 static ssize_t governor_show(struct device *dev,
 			     struct device_attribute *attr, char *buf)
 {
-	if (!to_devfreq(dev)->governor)
+	struct devfreq *df = to_devfreq(dev);
+
+	if (!df->governor)
 		return -EINVAL;
 
-	return sprintf(buf, "%s\n", to_devfreq(dev)->governor->name);
+	return sprintf(buf, "%s\n", df->governor->name);
 }
 
 static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
@@ -1302,6 +1304,9 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
 	char str_governor[DEVFREQ_NAME_LEN + 1];
 	const struct devfreq_governor *governor, *prev_governor;
 
+	if (!df->governor)
+		return -EINVAL;
+
 	ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s", str_governor);
 	if (ret != 1)
 		return -EINVAL;
@@ -1315,20 +1320,18 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
 	if (df->governor == governor) {
 		ret = 0;
 		goto out;
-	} else if ((df->governor && df->governor->immutable) ||
-					governor->immutable) {
+	} else if (df->governor->immutable || governor->immutable) {
 		ret = -EINVAL;
 		goto out;
 	}
 
-	if (df->governor) {
-		ret = df->governor->event_handler(df, DEVFREQ_GOV_STOP, NULL);
-		if (ret) {
-			dev_warn(dev, "%s: Governor %s not stopped(%d)\n",
-				 __func__, df->governor->name, ret);
-			goto out;
-		}
+	ret = df->governor->event_handler(df, DEVFREQ_GOV_STOP, NULL);
+	if (ret) {
+		dev_warn(dev, "%s: Governor %s not stopped(%d)\n",
+			 __func__, df->governor->name, ret);
+		goto out;
 	}
+
 	prev_governor = df->governor;
 	df->governor = governor;
 	strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN);
@@ -1363,13 +1366,16 @@ static ssize_t available_governors_show(struct device *d,
 	struct devfreq *df = to_devfreq(d);
 	ssize_t count = 0;
 
+	if (!df->governor)
+		return -EINVAL;
+
 	mutex_lock(&devfreq_list_lock);
 
 	/*
 	 * The devfreq with immutable governor (e.g., passive) shows
 	 * only own governor.
 	 */
-	if (df->governor && df->governor->immutable) {
+	if (df->governor->immutable) {
 		count = scnprintf(&buf[count], DEVFREQ_NAME_LEN,
 				  "%s ", df->governor_name);
 	/*
@@ -1403,27 +1409,37 @@ static ssize_t cur_freq_show(struct device *dev, struct device_attribute *attr,
 			     char *buf)
 {
 	unsigned long freq;
-	struct devfreq *devfreq = to_devfreq(dev);
+	struct devfreq *df = to_devfreq(dev);
 
-	if (devfreq->profile->get_cur_freq &&
-		!devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq))
+	if (!df->profile)
+		return -EINVAL;
+
+	if (df->profile->get_cur_freq &&
+		!df->profile->get_cur_freq(df->dev.parent, &freq))
 		return sprintf(buf, "%lu\n", freq);
 
-	return sprintf(buf, "%lu\n", devfreq->previous_freq);
+	return sprintf(buf, "%lu\n", df->previous_freq);
 }
 static DEVICE_ATTR_RO(cur_freq);
 
 static ssize_t target_freq_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
-	return sprintf(buf, "%lu\n", to_devfreq(dev)->previous_freq);
+	struct devfreq *df = to_devfreq(dev);
+
+	return sprintf(buf, "%lu\n", df->previous_freq);
 }
 static DEVICE_ATTR_RO(target_freq);
 
 static ssize_t polling_interval_show(struct device *dev,
 				     struct device_attribute *attr, char *buf)
 {
-	return sprintf(buf, "%d\n", to_devfreq(dev)->profile->polling_ms);
+	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,
@@ -1551,6 +1567,9 @@ static ssize_t available_frequencies_show(struct device *d,
 	ssize_t count = 0;
 	int i;
 
+	if (!df->profile)
+		return -EINVAL;
+
 	mutex_lock(&df->lock);
 
 	for (i = 0; i < df->profile->max_state; i++)
@@ -1571,49 +1590,53 @@ static DEVICE_ATTR_RO(available_frequencies);
 static ssize_t trans_stat_show(struct device *dev,
 			       struct device_attribute *attr, char *buf)
 {
-	struct devfreq *devfreq = to_devfreq(dev);
+	struct devfreq *df = to_devfreq(dev);
 	ssize_t len;
 	int i, j;
-	unsigned int max_state = devfreq->profile->max_state;
+	unsigned int max_state;
+
+	if (!df->profile)
+		return -EINVAL;
+	max_state = df->profile->max_state;
 
 	if (max_state == 0)
 		return sprintf(buf, "Not Supported.\n");
 
-	mutex_lock(&devfreq->lock);
-	if (!devfreq->stop_polling &&
-			devfreq_update_status(devfreq, devfreq->previous_freq)) {
-		mutex_unlock(&devfreq->lock);
+	mutex_lock(&df->lock);
+	if (!df->stop_polling &&
+			devfreq_update_status(df, df->previous_freq)) {
+		mutex_unlock(&df->lock);
 		return 0;
 	}
-	mutex_unlock(&devfreq->lock);
+	mutex_unlock(&df->lock);
 
 	len = sprintf(buf, "     From  :   To\n");
 	len += sprintf(buf + len, "           :");
 	for (i = 0; i < max_state; i++)
 		len += sprintf(buf + len, "%10lu",
-				devfreq->profile->freq_table[i]);
+				df->profile->freq_table[i]);
 
 	len += sprintf(buf + len, "   time(ms)\n");
 
 	for (i = 0; i < max_state; i++) {
-		if (devfreq->profile->freq_table[i]
-					== devfreq->previous_freq) {
+		if (df->profile->freq_table[i]
+					== df->previous_freq) {
 			len += sprintf(buf + len, "*");
 		} else {
 			len += sprintf(buf + len, " ");
 		}
 		len += sprintf(buf + len, "%10lu:",
-				devfreq->profile->freq_table[i]);
+				df->profile->freq_table[i]);
 		for (j = 0; j < max_state; j++)
 			len += sprintf(buf + len, "%10u",
-				devfreq->stats.trans_table[(i * max_state) + j]);
+				df->stats.trans_table[(i * max_state) + j]);
 
 		len += sprintf(buf + len, "%10llu\n", (u64)
-			jiffies64_to_msecs(devfreq->stats.time_in_state[i]));
+			jiffies64_to_msecs(df->stats.time_in_state[i]));
 	}
 
 	len += sprintf(buf + len, "Total transition : %u\n",
-					devfreq->stats.total_trans);
+					df->stats.total_trans);
 	return len;
 }
 
@@ -1624,6 +1647,9 @@ static ssize_t trans_stat_store(struct device *dev,
 	struct devfreq *df = to_devfreq(dev);
 	int err, value;
 
+	if (!df->profile)
+		return -EINVAL;
+
 	if (df->profile->max_state == 0)
 		return count;
 
-- 
2.17.1

Re: [PATCH v2 1/2] PM / devfreq: Clean up the devfreq instance name in sysfs attr

[Chanwoo Choi]

On 7/13/20 5:31 PM, Chanwoo Choi wrote:
> The sysfs attr interface used eithere 'df' or 'devfreq' for devfreq instance
> name. In order to keep the consistency and to improve the readabilty,
> unify the instance name as 'df'. Add add the missing conditional statement
> to prevent the fault.
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  drivers/devfreq/devfreq.c | 94 +++++++++++++++++++++++++--------------
>  1 file changed, 60 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 5320c3b37f35..286957f760f1 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -1280,18 +1280,20 @@ EXPORT_SYMBOL(devfreq_remove_governor);
>  static ssize_t name_show(struct device *dev,
>  			struct device_attribute *attr, char *buf)
>  {
> -	struct devfreq *devfreq = to_devfreq(dev);
> -	return sprintf(buf, "%s\n", dev_name(devfreq->dev.parent));
> +	struct devfreq *df = to_devfreq(dev);
> +	return sprintf(buf, "%s\n", dev_name(df->dev.parent));
>  }
>  static DEVICE_ATTR_RO(name);
>  
>  static ssize_t governor_show(struct device *dev,
>  			     struct device_attribute *attr, char *buf)
>  {
> -	if (!to_devfreq(dev)->governor)
> +	struct devfreq *df = to_devfreq(dev);
> +
> +	if (!df->governor)
>  		return -EINVAL;
>  
> -	return sprintf(buf, "%s\n", to_devfreq(dev)->governor->name);
> +	return sprintf(buf, "%s\n", df->governor->name);
>  }
>  
>  static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
> @@ -1302,6 +1304,9 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>  	char str_governor[DEVFREQ_NAME_LEN + 1];
>  	const struct devfreq_governor *governor, *prev_governor;
>  
> +	if (!df->governor)
> +		return -EINVAL;
> +
>  	ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s", str_governor);
>  	if (ret != 1)
>  		return -EINVAL;
> @@ -1315,20 +1320,18 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>  	if (df->governor == governor) {
>  		ret = 0;
>  		goto out;
> -	} else if ((df->governor && df->governor->immutable) ||
> -					governor->immutable) {
> +	} else if (df->governor->immutable || governor->immutable) {
>  		ret = -EINVAL;
>  		goto out;
>  	}
>  
> -	if (df->governor) {
> -		ret = df->governor->event_handler(df, DEVFREQ_GOV_STOP, NULL);
> -		if (ret) {
> -			dev_warn(dev, "%s: Governor %s not stopped(%d)\n",
> -				 __func__, df->governor->name, ret);
> -			goto out;
> -		}
> +	ret = df->governor->event_handler(df, DEVFREQ_GOV_STOP, NULL);
> +	if (ret) {
> +		dev_warn(dev, "%s: Governor %s not stopped(%d)\n",
> +			 __func__, df->governor->name, ret);
> +		goto out;
>  	}
> +
>  	prev_governor = df->governor;
>  	df->governor = governor;
>  	strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN);
> @@ -1363,13 +1366,16 @@ static ssize_t available_governors_show(struct device *d,
>  	struct devfreq *df = to_devfreq(d);
>  	ssize_t count = 0;
>  
> +	if (!df->governor)
> +		return -EINVAL;
> +
>  	mutex_lock(&devfreq_list_lock);
>  
>  	/*
>  	 * The devfreq with immutable governor (e.g., passive) shows
>  	 * only own governor.
>  	 */
> -	if (df->governor && df->governor->immutable) {
> +	if (df->governor->immutable) {
>  		count = scnprintf(&buf[count], DEVFREQ_NAME_LEN,
>  				  "%s ", df->governor_name);
>  	/*
> @@ -1403,27 +1409,37 @@ static ssize_t cur_freq_show(struct device *dev, struct device_attribute *attr,
>  			     char *buf)
>  {
>  	unsigned long freq;
> -	struct devfreq *devfreq = to_devfreq(dev);
> +	struct devfreq *df = to_devfreq(dev);
>  
> -	if (devfreq->profile->get_cur_freq &&
> -		!devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq))
> +	if (!df->profile)
> +		return -EINVAL;
> +
> +	if (df->profile->get_cur_freq &&
> +		!df->profile->get_cur_freq(df->dev.parent, &freq))
>  		return sprintf(buf, "%lu\n", freq);
>  
> -	return sprintf(buf, "%lu\n", devfreq->previous_freq);
> +	return sprintf(buf, "%lu\n", df->previous_freq);
>  }
>  static DEVICE_ATTR_RO(cur_freq);
>  
>  static ssize_t target_freq_show(struct device *dev,
>  				struct device_attribute *attr, char *buf)
>  {
> -	return sprintf(buf, "%lu\n", to_devfreq(dev)->previous_freq);
> +	struct devfreq *df = to_devfreq(dev);
> +
> +	return sprintf(buf, "%lu\n", df->previous_freq);
>  }
>  static DEVICE_ATTR_RO(target_freq);
>  
>  static ssize_t polling_interval_show(struct device *dev,
>  				     struct device_attribute *attr, char *buf)
>  {
> -	return sprintf(buf, "%d\n", to_devfreq(dev)->profile->polling_ms);
> +	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,
> @@ -1551,6 +1567,9 @@ static ssize_t available_frequencies_show(struct device *d,
>  	ssize_t count = 0;
>  	int i;
>  
> +	if (!df->profile)
> +		return -EINVAL;
> +
>  	mutex_lock(&df->lock);
>  
>  	for (i = 0; i < df->profile->max_state; i++)
> @@ -1571,49 +1590,53 @@ static DEVICE_ATTR_RO(available_frequencies);
>  static ssize_t trans_stat_show(struct device *dev,
>  			       struct device_attribute *attr, char *buf)
>  {
> -	struct devfreq *devfreq = to_devfreq(dev);
> +	struct devfreq *df = to_devfreq(dev);
>  	ssize_t len;
>  	int i, j;
> -	unsigned int max_state = devfreq->profile->max_state;
> +	unsigned int max_state;
> +
> +	if (!df->profile)
> +		return -EINVAL;
> +	max_state = df->profile->max_state;
>  
>  	if (max_state == 0)
>  		return sprintf(buf, "Not Supported.\n");
>  
> -	mutex_lock(&devfreq->lock);
> -	if (!devfreq->stop_polling &&
> -			devfreq_update_status(devfreq, devfreq->previous_freq)) {
> -		mutex_unlock(&devfreq->lock);
> +	mutex_lock(&df->lock);
> +	if (!df->stop_polling &&
> +			devfreq_update_status(df, df->previous_freq)) {
> +		mutex_unlock(&df->lock);
>  		return 0;
>  	}
> -	mutex_unlock(&devfreq->lock);
> +	mutex_unlock(&df->lock);
>  
>  	len = sprintf(buf, "     From  :   To\n");
>  	len += sprintf(buf + len, "           :");
>  	for (i = 0; i < max_state; i++)
>  		len += sprintf(buf + len, "%10lu",
> -				devfreq->profile->freq_table[i]);
> +				df->profile->freq_table[i]);
>  
>  	len += sprintf(buf + len, "   time(ms)\n");
>  
>  	for (i = 0; i < max_state; i++) {
> -		if (devfreq->profile->freq_table[i]
> -					== devfreq->previous_freq) {
> +		if (df->profile->freq_table[i]
> +					== df->previous_freq) {
>  			len += sprintf(buf + len, "*");
>  		} else {
>  			len += sprintf(buf + len, " ");
>  		}
>  		len += sprintf(buf + len, "%10lu:",
> -				devfreq->profile->freq_table[i]);
> +				df->profile->freq_table[i]);
>  		for (j = 0; j < max_state; j++)
>  			len += sprintf(buf + len, "%10u",
> -				devfreq->stats.trans_table[(i * max_state) + j]);
> +				df->stats.trans_table[(i * max_state) + j]);
>  
>  		len += sprintf(buf + len, "%10llu\n", (u64)
> -			jiffies64_to_msecs(devfreq->stats.time_in_state[i]));
> +			jiffies64_to_msecs(df->stats.time_in_state[i]));
>  	}
>  
>  	len += sprintf(buf + len, "Total transition : %u\n",
> -					devfreq->stats.total_trans);
> +					df->stats.total_trans);
>  	return len;
>  }
>  
> @@ -1624,6 +1647,9 @@ static ssize_t trans_stat_store(struct device *dev,
>  	struct devfreq *df = to_devfreq(dev);
>  	int err, value;
>  
> +	if (!df->profile)
> +		return -EINVAL;
> +
>  	if (df->profile->max_state == 0)
>  		return count;
>  
> 

Applied it. It is just clean-up patch for patch2.
patch2 needs more discussion. So, only apply patch1.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

[PATCH v2 2/2] PM / devfreq: Add governor flags to clarify the features

[Chanwoo Choi]

DEVFREQ supports the default governors like performance, powersave 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 availability.

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.

/* Devfreq governor flags for attributes and features */
[Definition for sysfs attributes]
- DEVFREQ_GOV_ATTR_GOVERNOR
- DEVFREQ_GOV_ATTR_AVAIL_GOVERNORS
- DEVFREQ_GOV_ATTR_AVAIL_FREQUENCIES
- DEVFREQ_GOV_ATTR_CUR_FREQ
- DEVFREQ_GOV_ATTR_TARGET_FREQ
- DEVFREQ_GOV_ATTR_MIN_FREQ
- DEVFREQ_GOV_ATTR_MAX_FREQ
- DEVFREQ_GOV_ATTR_TRANS_STAT
- DEVFREQ_GOV_ATTR_POLLING_INTERVAL
- DEVFREQ_GOV_ATTR_TIMER

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

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/devfreq.c                 | 106 ++++++++++++++++++----
 drivers/devfreq/governor.h                |  44 +++++++--
 drivers/devfreq/governor_passive.c        |   3 +-
 drivers/devfreq/governor_performance.c    |   1 +
 drivers/devfreq/governor_powersave.c      |   1 +
 drivers/devfreq/governor_simpleondemand.c |   3 +
 drivers/devfreq/governor_userspace.c      |   1 +
 drivers/devfreq/tegra30-devfreq.c         |   6 +-
 8 files changed, 139 insertions(+), 26 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 286957f760f1..ce82bdb5fa5c 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -456,7 +456,7 @@ static void devfreq_monitor(struct work_struct *work)
  */
 void devfreq_monitor_start(struct devfreq *devfreq)
 {
-	if (devfreq->governor->interrupt_driven)
+	if (DEVFREQ_GOV_FLAG_IRQ_DRIVEN & devfreq->governor->flag)
 		return;
 
 	switch (devfreq->profile->timer) {
@@ -486,7 +486,7 @@ EXPORT_SYMBOL(devfreq_monitor_start);
  */
 void devfreq_monitor_stop(struct devfreq *devfreq)
 {
-	if (devfreq->governor->interrupt_driven)
+	if (DEVFREQ_GOV_FLAG_IRQ_DRIVEN & devfreq->governor->flag)
 		return;
 
 	cancel_delayed_work_sync(&devfreq->work);
@@ -517,7 +517,7 @@ void devfreq_monitor_suspend(struct devfreq *devfreq)
 	devfreq->stop_polling = true;
 	mutex_unlock(&devfreq->lock);
 
-	if (devfreq->governor->interrupt_driven)
+	if (DEVFREQ_GOV_FLAG_IRQ_DRIVEN & devfreq->governor->flag)
 		return;
 
 	cancel_delayed_work_sync(&devfreq->work);
@@ -537,12 +537,13 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
 	unsigned long freq;
 
 	mutex_lock(&devfreq->lock);
-	if (!devfreq->stop_polling)
-		goto out;
 
-	if (devfreq->governor->interrupt_driven)
+	if (DEVFREQ_GOV_FLAG_IRQ_DRIVEN & devfreq->governor->flag)
 		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,
@@ -577,10 +578,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 (devfreq->governor->interrupt_driven)
+	if (devfreq->stop_polling)
 		goto out;
 
 	/* if new delay is zero, stop polling */
@@ -1281,6 +1282,13 @@ static ssize_t name_show(struct device *dev,
 			struct device_attribute *attr, char *buf)
 {
 	struct devfreq *df = to_devfreq(dev);
+
+	if (!df->governor)
+		return -EINVAL;
+
+	if (!(DEVFREQ_GOV_ATTR_NAME & df->governor->attr))
+		return -EACCES;
+
 	return sprintf(buf, "%s\n", dev_name(df->dev.parent));
 }
 static DEVICE_ATTR_RO(name);
@@ -1293,6 +1301,9 @@ static ssize_t governor_show(struct device *dev,
 	if (!df->governor)
 		return -EINVAL;
 
+	if (!(DEVFREQ_GOV_ATTR_GOVERNOR & df->governor->attr))
+		return -EACCES;
+
 	return sprintf(buf, "%s\n", df->governor->name);
 }
 
@@ -1307,6 +1318,9 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
 	if (!df->governor)
 		return -EINVAL;
 
+	if (!(DEVFREQ_GOV_ATTR_GOVERNOR & df->governor->attr))
+		return -EACCES;
+
 	ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s", str_governor);
 	if (ret != 1)
 		return -EINVAL;
@@ -1320,7 +1334,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 ((DEVFREQ_GOV_FLAG_IMMUTABLE & df->governor->flag)
+		|| (DEVFREQ_GOV_FLAG_IMMUTABLE & governor->flag)) {
 		ret = -EINVAL;
 		goto out;
 	}
@@ -1369,13 +1384,16 @@ static ssize_t available_governors_show(struct device *d,
 	if (!df->governor)
 		return -EINVAL;
 
+	if (!(DEVFREQ_GOV_ATTR_AVAIL_GOVERNORS & df->governor->attr))
+		return -EACCES;
+
 	mutex_lock(&devfreq_list_lock);
 
 	/*
 	 * The devfreq with immutable governor (e.g., passive) shows
 	 * only own governor.
 	 */
-	if (df->governor->immutable) {
+	if (DEVFREQ_GOV_FLAG_IMMUTABLE & df->governor->flag) {
 		count = scnprintf(&buf[count], DEVFREQ_NAME_LEN,
 				  "%s ", df->governor_name);
 	/*
@@ -1386,7 +1404,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 (DEVFREQ_GOV_FLAG_IMMUTABLE & governor->flag)
 				continue;
 			count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
 					   "%s ", governor->name);
@@ -1411,9 +1429,12 @@ static ssize_t cur_freq_show(struct device *dev, struct device_attribute *attr,
 	unsigned long freq;
 	struct devfreq *df = to_devfreq(dev);
 
-	if (!df->profile)
+	if (!df->governor || !df->profile)
 		return -EINVAL;
 
+	if (!(DEVFREQ_GOV_ATTR_CUR_FREQ & df->governor->attr))
+		return -EACCES;
+
 	if (df->profile->get_cur_freq &&
 		!df->profile->get_cur_freq(df->dev.parent, &freq))
 		return sprintf(buf, "%lu\n", freq);
@@ -1427,6 +1448,12 @@ static ssize_t target_freq_show(struct device *dev,
 {
 	struct devfreq *df = to_devfreq(dev);
 
+	if (!df->governor)
+		return -EINVAL;
+
+	if (!(DEVFREQ_GOV_ATTR_TARGET_FREQ & df->governor->attr))
+		return -EACCES;
+
 	return sprintf(buf, "%lu\n", df->previous_freq);
 }
 static DEVICE_ATTR_RO(target_freq);
@@ -1436,9 +1463,12 @@ static ssize_t polling_interval_show(struct device *dev,
 {
 	struct devfreq *df = to_devfreq(dev);
 
-	if (!df->profile)
+	if (!df->governor || !df->profile)
 		return -EINVAL;
 
+	if (!(DEVFREQ_GOV_ATTR_POLLING_INTERVAL & df->governor->attr))
+		return -EACCES;
+
 	return sprintf(buf, "%d\n", df->profile->polling_ms);
 }
 
@@ -1453,6 +1483,9 @@ static ssize_t polling_interval_store(struct device *dev,
 	if (!df->governor)
 		return -EINVAL;
 
+	if (!(DEVFREQ_GOV_ATTR_POLLING_INTERVAL & df->governor->attr))
+		return -EACCES;
+
 	ret = sscanf(buf, "%u", &value);
 	if (ret != 1)
 		return -EINVAL;
@@ -1471,6 +1504,12 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
 	unsigned long value;
 	int ret;
 
+	if (!df->governor)
+		return -EINVAL;
+
+	if (!(DEVFREQ_GOV_ATTR_MIN_FREQ & df->governor->attr))
+		return -EACCES;
+
 	/*
 	 * Protect against theoretical sysfs writes between
 	 * device_add and dev_pm_qos_add_request
@@ -1497,6 +1536,12 @@ static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
 	struct devfreq *df = to_devfreq(dev);
 	unsigned long min_freq, max_freq;
 
+	if (!df->governor)
+		return -EINVAL;
+
+	if (!(DEVFREQ_GOV_ATTR_MIN_FREQ & df->governor->attr))
+		return -EACCES;
+
 	mutex_lock(&df->lock);
 	get_freq_range(df, &min_freq, &max_freq);
 	mutex_unlock(&df->lock);
@@ -1512,6 +1557,12 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
 	unsigned long value;
 	int ret;
 
+	if (!df->governor)
+		return -EINVAL;
+
+	if (!(DEVFREQ_GOV_ATTR_MAX_FREQ & df->governor->attr))
+		return -EACCES;
+
 	/*
 	 * Protect against theoretical sysfs writes between
 	 * device_add and dev_pm_qos_add_request
@@ -1551,6 +1602,12 @@ static ssize_t max_freq_show(struct device *dev, struct device_attribute *attr,
 	struct devfreq *df = to_devfreq(dev);
 	unsigned long min_freq, max_freq;
 
+	if (!df->governor)
+		return -EINVAL;
+
+	if (!(DEVFREQ_GOV_ATTR_MAX_FREQ & df->governor->attr))
+		return -EACCES;
+
 	mutex_lock(&df->lock);
 	get_freq_range(df, &min_freq, &max_freq);
 	mutex_unlock(&df->lock);
@@ -1567,9 +1624,12 @@ static ssize_t available_frequencies_show(struct device *d,
 	ssize_t count = 0;
 	int i;
 
-	if (!df->profile)
+	if (!df->governor || !df->profile)
 		return -EINVAL;
 
+	if (!(DEVFREQ_GOV_ATTR_AVAIL_FREQUENCIES & df->governor->attr))
+		return -EACCES;
+
 	mutex_lock(&df->lock);
 
 	for (i = 0; i < df->profile->max_state; i++)
@@ -1595,10 +1655,13 @@ static ssize_t trans_stat_show(struct device *dev,
 	int i, j;
 	unsigned int max_state;
 
-	if (!df->profile)
+	if (!df->governor || !df->profile)
 		return -EINVAL;
 	max_state = df->profile->max_state;
 
+	if (!(DEVFREQ_GOV_ATTR_TRANS_STAT & df->governor->attr))
+		return -EACCES;
+
 	if (max_state == 0)
 		return sprintf(buf, "Not Supported.\n");
 
@@ -1647,9 +1710,12 @@ static ssize_t trans_stat_store(struct device *dev,
 	struct devfreq *df = to_devfreq(dev);
 	int err, value;
 
-	if (!df->profile)
+	if (!df->governor || !df->profile)
 		return -EINVAL;
 
+	if (!(DEVFREQ_GOV_ATTR_TRANS_STAT & df->governor->attr))
+		return -EACCES;
+
 	if (df->profile->max_state == 0)
 		return count;
 
@@ -1676,9 +1742,12 @@ static ssize_t timer_show(struct device *dev,
 {
 	struct devfreq *df = to_devfreq(dev);
 
-	if (!df->profile)
+	if (!df->governor || !df->profile)
 		return -EINVAL;
 
+	if (!(DEVFREQ_GOV_ATTR_TIMER & df->governor->attr))
+		return -EACCES;
+
 	return sprintf(buf, "%s\n", timer_name[df->profile->timer]);
 }
 
@@ -1693,6 +1762,9 @@ static ssize_t timer_store(struct device *dev, struct device_attribute *attr,
 	if (!df->governor || !df->profile)
 		return -EINVAL;
 
+	if (!(DEVFREQ_GOV_ATTR_TIMER & df->governor->attr))
+		return -EACCES;
+
 	ret = sscanf(buf, "%16s", str_timer);
 	if (ret != 1)
 		return -EINVAL;
diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
index ae4d0cc18359..7d1ed5933b9a 100644
--- a/drivers/devfreq/governor.h
+++ b/drivers/devfreq/governor.h
@@ -25,14 +25,46 @@
 #define DEVFREQ_MIN_FREQ			0
 #define DEVFREQ_MAX_FREQ			ULONG_MAX
 
+/* Devfreq governor flags for attribtures and features */
+#define DEVFREQ_GOV_ATTR_NAME				BIT(0)
+#define DEVFREQ_GOV_ATTR_GOVERNOR			BIT(1)
+#define DEVFREQ_GOV_ATTR_AVAIL_GOVERNORS		BIT(2)
+#define DEVFREQ_GOV_ATTR_AVAIL_FREQUENCIES		BIT(3)
+#define DEVFREQ_GOV_ATTR_CUR_FREQ			BIT(4)
+#define DEVFREQ_GOV_ATTR_TARGET_FREQ			BIT(5)
+#define DEVFREQ_GOV_ATTR_MIN_FREQ			BIT(6)
+#define DEVFREQ_GOV_ATTR_MAX_FREQ			BIT(7)
+#define DEVFREQ_GOV_ATTR_TRANS_STAT			BIT(8)
+#define DEVFREQ_GOV_ATTR_POLLING_INTERVAL		BIT(9)
+#define DEVFREQ_GOV_ATTR_TIMER				BIT(10)
+
+#define DEVFREQ_GOV_ATTR_COMMON (DEVFREQ_GOV_ATTR_NAME			\
+				| DEVFREQ_GOV_ATTR_GOVERNOR		\
+				| DEVFREQ_GOV_ATTR_AVAIL_GOVERNORS	\
+				| DEVFREQ_GOV_ATTR_AVAIL_FREQUENCIES	\
+				| DEVFREQ_GOV_ATTR_CUR_FREQ		\
+				| DEVFREQ_GOV_ATTR_TARGET_FREQ		\
+				| DEVFREQ_GOV_ATTR_MIN_FREQ		\
+				| DEVFREQ_GOV_ATTR_MAX_FREQ		\
+				| DEVFREQ_GOV_ATTR_TRANS_STAT )
+
+/*
+ * If immutable flag is set, this governor is never changeable
+ * to other governors.
+ */
+#define DEVFREQ_GOV_FLAG_IMMUTABLE			BIT(0)
+
+/*
+ * If irq_drive flag is set, devfreq won't schedule the work for this governor.
+ */
+#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.
+ * @attr:		Governor's sysfs attribute flag
+ * @flag:		Governor's 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 +82,8 @@ struct devfreq_governor {
 	struct list_head node;
 
 	const char name[DEVFREQ_NAME_LEN];
-	const unsigned int immutable;
-	const unsigned int interrupt_driven;
+	const unsigned long attr;
+	const unsigned long 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..671a5f8f774e 100644
--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -180,7 +180,8 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
 
 static struct devfreq_governor devfreq_passive = {
 	.name = DEVFREQ_GOV_PASSIVE,
-	.immutable = 1,
+	.attr = DEVFREQ_GOV_ATTR_COMMON,
+	.flag = DEVFREQ_GOV_FLAG_IMMUTABLE,
 	.get_target_freq = devfreq_passive_get_target_freq,
 	.event_handler = devfreq_passive_event_handler,
 };
diff --git a/drivers/devfreq/governor_performance.c b/drivers/devfreq/governor_performance.c
index 5dbc1e56ec08..7ae9829775b8 100644
--- a/drivers/devfreq/governor_performance.c
+++ b/drivers/devfreq/governor_performance.c
@@ -37,6 +37,7 @@ static int devfreq_performance_handler(struct devfreq *devfreq,
 
 static struct devfreq_governor devfreq_performance = {
 	.name = DEVFREQ_GOV_PERFORMANCE,
+	.attr = DEVFREQ_GOV_ATTR_COMMON,
 	.get_target_freq = devfreq_performance_func,
 	.event_handler = devfreq_performance_handler,
 };
diff --git a/drivers/devfreq/governor_powersave.c b/drivers/devfreq/governor_powersave.c
index 4746af2435b0..8761cc400f8a 100644
--- a/drivers/devfreq/governor_powersave.c
+++ b/drivers/devfreq/governor_powersave.c
@@ -37,6 +37,7 @@ static int devfreq_powersave_handler(struct devfreq *devfreq,
 
 static struct devfreq_governor devfreq_powersave = {
 	.name = DEVFREQ_GOV_POWERSAVE,
+	.attr = DEVFREQ_GOV_ATTR_COMMON,
 	.get_target_freq = devfreq_powersave_func,
 	.event_handler = devfreq_powersave_handler,
 };
diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c
index 1b314e1df028..57083b83a094 100644
--- a/drivers/devfreq/governor_simpleondemand.c
+++ b/drivers/devfreq/governor_simpleondemand.c
@@ -117,6 +117,9 @@ 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_COMMON
+		| 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/governor_userspace.c b/drivers/devfreq/governor_userspace.c
index 0fd6c4851071..a32bdea49fa3 100644
--- a/drivers/devfreq/governor_userspace.c
+++ b/drivers/devfreq/governor_userspace.c
@@ -132,6 +132,7 @@ static int devfreq_userspace_handler(struct devfreq *devfreq,
 
 static struct devfreq_governor devfreq_userspace = {
 	.name = DEVFREQ_GOV_USERSPACE,
+	.attr = DEVFREQ_GOV_ATTR_COMMON,
 	.get_target_freq = devfreq_userspace_func,
 	.event_handler = devfreq_userspace_handler,
 };
diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index e94a27804c20..ad5bd8e7b24d 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -765,10 +765,12 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
 
 static struct devfreq_governor tegra_devfreq_governor = {
 	.name = "tegra_actmon",
+	.attr = DEVFREQ_GOV_ATTR_COMMON
+		| DEVFREQ_GOV_ATTR_POLLING_INTERVAL,
+	.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 v2 2/2] PM / devfreq: Add governor flags to clarify the features

[Dmitry Osipenko]

13.07.2020 11:31, Chanwoo Choi пишет:
> DEVFREQ supports the default governors like performance, powersave 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 availability.
> 
> 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.
> 
> /* Devfreq governor flags for attributes and features */
> [Definition for sysfs attributes]
> - DEVFREQ_GOV_ATTR_GOVERNOR
> - DEVFREQ_GOV_ATTR_AVAIL_GOVERNORS
> - DEVFREQ_GOV_ATTR_AVAIL_FREQUENCIES
> - DEVFREQ_GOV_ATTR_CUR_FREQ
> - DEVFREQ_GOV_ATTR_TARGET_FREQ
> - DEVFREQ_GOV_ATTR_MIN_FREQ
> - DEVFREQ_GOV_ATTR_MAX_FREQ
> - DEVFREQ_GOV_ATTR_TRANS_STAT
> - DEVFREQ_GOV_ATTR_POLLING_INTERVAL
> - DEVFREQ_GOV_ATTR_TIMER
> 
> Also, 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|         |
> ------------------------------------------------------------------------------
> 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)
> ------------------------------------------------------------------------------
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---

Hello, Chanwoo! I tested this series on NVIDIA Tegra30 and everything
working fine!

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
Tested-by: Dmitry Osipenko <digetx@gmail.com>


BTW, I'm curious what do you think about hiding the unsupported debugfs
attributes per-device instead of returning the -EACCES?

Re: [PATCH v2 2/2] PM / devfreq: Add governor flags to clarify the features

[Chanwoo Choi]

On 7/13/20 7:37 PM, Dmitry Osipenko wrote:
> 13.07.2020 11:31, Chanwoo Choi пишет:
>> DEVFREQ supports the default governors like performance, powersave 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 availability.
>>
>> 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.
>>
>> /* Devfreq governor flags for attributes and features */
>> [Definition for sysfs attributes]
>> - DEVFREQ_GOV_ATTR_GOVERNOR
>> - DEVFREQ_GOV_ATTR_AVAIL_GOVERNORS
>> - DEVFREQ_GOV_ATTR_AVAIL_FREQUENCIES
>> - DEVFREQ_GOV_ATTR_CUR_FREQ
>> - DEVFREQ_GOV_ATTR_TARGET_FREQ
>> - DEVFREQ_GOV_ATTR_MIN_FREQ
>> - DEVFREQ_GOV_ATTR_MAX_FREQ
>> - DEVFREQ_GOV_ATTR_TRANS_STAT
>> - DEVFREQ_GOV_ATTR_POLLING_INTERVAL
>> - DEVFREQ_GOV_ATTR_TIMER
>>
>> Also, 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|         |
>> ------------------------------------------------------------------------------
>> 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)
>> ------------------------------------------------------------------------------
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
> 
> Hello, Chanwoo! I tested this series on NVIDIA Tegra30 and everything
> working fine!
> 
> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
> Tested-by: Dmitry Osipenko <digetx@gmail.com>
> 
> 
> BTW, I'm curious what do you think about hiding the unsupported debugfs

Do you mean that sysfs?

> attributes per-device instead of returning the -EACCES?

I considered the hiding of sysfs node too instead of -EACCES.


But,
For a long time, devfreq showed the sysfs interface of all devfreq devices
regardless of the kind of devfreq governor. It means that devfreq keeps
the ABI interface. If devfreq hides the unsupported sysfs node
according to the type of governor, it will break the ABI.

Although I knew that maybe performance/powersave/userspace didn't use
the 'polling_interval' node, I just returned -EACCESS.

Thanks for suggesting your opinion. We can discuss it more.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 2/2] PM / devfreq: Add governor flags to clarify the features

[Dmitry Osipenko]

13.07.2020 15:26, Chanwoo Choi пишет:
...
>> BTW, I'm curious what do you think about hiding the unsupported debugfs
> 
> Do you mean that sysfs?

Yes, sysfs :)

>> attributes per-device instead of returning the -EACCES?
> 
> I considered the hiding of sysfs node too instead of -EACCES.

If there is no real userspace (used by a non-developer crowd) that
relies on the attributes presence, then it could be fine to change the
behaviour, IMO.

I know that PowerTOP utility uses the 'trans_stat' attribute, but not
sure about the other attributes.

> But,
> For a long time, devfreq showed the sysfs interface of all devfreq devices
> regardless of the kind of devfreq governor. It means that devfreq keeps
> the ABI interface. If devfreq hides the unsupported sysfs node
> according to the type of governor, it will break the ABI.

I didn't notice that it's an ABI already [1]. Should be better not to
change the ABI if there is userspace already relying on the old
behaviour, otherwise it may be okay to make changes until it will be too
late, also given that this is still a "testing" ABI.

[1] https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-class-devfreq

Although, the doc doesn't say anything about -EACCES/-EINVAL, so isn't
it an ABI change already? Doesn't doc need to be updated in order to
reflect the ABI change?

For example, doc says that userspace shouldn't care about attribute
values which are irrelevant for a selected governor, like in the case of
the 'polling_interval' attribute. The doc doesn't say that userspace may
get a error.

> Although I knew that maybe performance/powersave/userspace didn't use
> the 'polling_interval' node, I just returned -EACCESS.

The 'polling_interval', 'min/max_freq' and the new 'timer' attributes
are all the governor attributes.

Would be nice to have a per-device `governor/` directory containing all
the governor-specific attributes (without the unrelated attributes), but
perhaps it's a bit too late to change it now?