2020-10-23 10:14:34

by Chanwoo Choi

[permalink] [raw]
Subject: [PATCH v5 0/2] PM / devfreq: Add governor feature and attribute flag

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 v4:
- Rename from 'attr' to 'attrs'
- Restore the variable name in governor_store because it is enought to explain
the previous or new governor with detailed comments instead of variable name
changes.

Changes from v3:
- Fix typo
- Rename from 'flag' to 'flags'
- Add more exception handling code and add comments on governor_store()

Changes from v2:
- 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 | 185 ++++++++++++------
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, 186 insertions(+), 92 deletions(-)

--
2.17.1


2020-10-23 10:15:24

by Chanwoo Choi

[permalink] [raw]
Subject: [PATCH v5 1/2] PM / devfreq: Add governor feature flag

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_FLAG_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 <[email protected]>
---
drivers/devfreq/devfreq.c | 25 ++++++++++++++-----------
drivers/devfreq/governor.h | 18 ++++++++++++------
drivers/devfreq/governor_passive.c | 2 +-
drivers/devfreq/tegra30-devfreq.c | 4 ++--
4 files changed, 29 insertions(+), 20 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 1b236b9e4d9e..a862acfe987e 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;
@@ -479,7 +480,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->flags, IRQ_DRIVEN))
return;

switch (devfreq->profile->timer) {
@@ -509,7 +510,7 @@ EXPORT_SYMBOL(devfreq_monitor_start);
*/
void devfreq_monitor_stop(struct devfreq *devfreq)
{
- if (devfreq->governor->interrupt_driven)
+ if (IS_SUPPORTED_FLAG(devfreq->governor->flags, IRQ_DRIVEN))
return;

cancel_delayed_work_sync(&devfreq->work);
@@ -540,7 +541,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->flags, IRQ_DRIVEN))
return;

cancel_delayed_work_sync(&devfreq->work);
@@ -560,12 +561,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 (IS_SUPPORTED_FLAG(devfreq->governor->flags, 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,
@@ -600,10 +602,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 (IS_SUPPORTED_FLAG(devfreq->governor->flags, IRQ_DRIVEN))
goto out;

- if (devfreq->governor->interrupt_driven)
+ if (devfreq->stop_polling)
goto out;

/* if new delay is zero, stop polling */
@@ -1370,7 +1372,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->flags, IMMUTABLE)
+ || IS_SUPPORTED_FLAG(governor->flags, IMMUTABLE)) {
ret = -EINVAL;
goto out;
}
@@ -1425,7 +1428,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->flags, IMMUTABLE)) {
count = scnprintf(&buf[count], DEVFREQ_NAME_LEN,
"%s ", df->governor_name);
/*
@@ -1436,7 +1439,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->flags, 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 02cf876244d6..7dbb110a869e 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.
+ * @flags: Governor's feature flags
* @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 flags;
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 53a1b1596232..63332e4a65ae 100644
--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -158,7 +158,7 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,

static struct devfreq_governor devfreq_passive = {
.name = DEVFREQ_GOV_PASSIVE,
- .immutable = 1,
+ .flags = 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..faa1aecf2a31 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",
+ .flags = 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

2020-10-25 17:52:35

by Dmitry Osipenko

[permalink] [raw]
Subject: Re: [PATCH v5 1/2] PM / devfreq: Add governor feature flag

23.10.2020 13:26, 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_FLAG_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 <[email protected]>
> ---

Reviewed-by: Dmitry Osipenko <[email protected]>
Tested-by: Dmitry Osipenko <[email protected]>