2020-10-23 10:20:56

by Chanwoo Choi

[permalink] [raw]
Subject: [PATCH v2] PM / devfreq: Remove redundant governor_name from struct devfreq

The devfreq structure instance contains the governor_name and a governor
instance. When need to show the governor name, better to use the name
of devfreq_governor structure. So, governor_name variable in struct devfreq
is a redundant and unneeded variable. Remove the redundant governor_name
of struct devfreq and then use the name of devfreq_governor instance.

Signed-off-by: Chanwoo Choi <[email protected]>
---
Changes from v1:
- Rebase it on "[PATCH v5 0/2] PM / devfreq: Add governor feature and attribute flag"

drivers/devfreq/devfreq.c | 18 +++++++-----------
drivers/devfreq/governor.h | 2 ++
include/linux/devfreq.h | 4 ----
3 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index f6c55cf498ed..74f71588ace5 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -811,7 +811,6 @@ struct devfreq *devfreq_add_device(struct device *dev,
devfreq->dev.release = devfreq_dev_release;
INIT_LIST_HEAD(&devfreq->node);
devfreq->profile = profile;
- strscpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN);
devfreq->previous_freq = profile->initial_freq;
devfreq->last_status.current_frequency = profile->initial_freq;
devfreq->data = data;
@@ -907,7 +906,7 @@ struct devfreq *devfreq_add_device(struct device *dev,

mutex_lock(&devfreq_list_lock);

- governor = try_then_request_governor(devfreq->governor_name);
+ governor = try_then_request_governor(governor_name);
if (IS_ERR(governor)) {
dev_err(dev, "%s: Unable to find governor for the device\n",
__func__);
@@ -1250,7 +1249,7 @@ int devfreq_add_governor(struct devfreq_governor *governor)
int ret = 0;
struct device *dev = devfreq->dev.parent;

- if (!strncmp(devfreq->governor_name, governor->name,
+ if (!strncmp(devfreq->governor->name, governor->name,
DEVFREQ_NAME_LEN)) {
/* The following should never occur */
if (devfreq->governor) {
@@ -1312,7 +1311,7 @@ int devfreq_remove_governor(struct devfreq_governor *governor)
int ret;
struct device *dev = devfreq->dev.parent;

- if (!strncmp(devfreq->governor_name, governor->name,
+ if (!strncmp(devfreq->governor->name, governor->name,
DEVFREQ_NAME_LEN)) {
/* we should have a devfreq governor! */
if (!devfreq->governor) {
@@ -1407,7 +1406,6 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
*/
prev_governor = df->governor;
df->governor = governor;
- strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN);
ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
if (ret) {
dev_warn(dev, "%s: Governor %s not started(%d)\n",
@@ -1415,13 +1413,11 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,

/* Restore previous governor */
df->governor = prev_governor;
- strncpy(df->governor_name, prev_governor->name,
- DEVFREQ_NAME_LEN);
ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
if (ret) {
dev_err(dev,
"%s: reverting to Governor %s failed (%d)\n",
- __func__, df->governor_name, ret);
+ __func__, prev_governor->name, ret);
df->governor = NULL;
goto out;
}
@@ -1457,7 +1453,7 @@ static ssize_t available_governors_show(struct device *d,
*/
if (IS_SUPPORTED_FLAG(df->governor->flags, IMMUTABLE)) {
count = scnprintf(&buf[count], DEVFREQ_NAME_LEN,
- "%s ", df->governor_name);
+ "%s ", df->governor->name);
/*
* The devfreq device shows the registered governor except for
* immutable governors such as passive governor .
@@ -1900,7 +1896,7 @@ static int devfreq_summary_show(struct seq_file *s, void *data)

list_for_each_entry_reverse(devfreq, &devfreq_list, node) {
#if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
- if (!strncmp(devfreq->governor_name, DEVFREQ_GOV_PASSIVE,
+ if (!strncmp(devfreq->governor->name, DEVFREQ_GOV_PASSIVE,
DEVFREQ_NAME_LEN)) {
struct devfreq_passive_data *data = devfreq->data;

@@ -1926,7 +1922,7 @@ static int devfreq_summary_show(struct seq_file *s, void *data)
"%-30s %-30s %-15s %-10s %10d %12ld %12ld %12ld\n",
dev_name(&devfreq->dev),
p_devfreq ? dev_name(&p_devfreq->dev) : "null",
- devfreq->governor_name,
+ devfreq->governor->name,
polling_ms ? timer_name[timer] : "null",
polling_ms,
cur_freq,
diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
index df413b851bb2..2a52f97b542d 100644
--- a/drivers/devfreq/governor.h
+++ b/drivers/devfreq/governor.h
@@ -13,6 +13,8 @@

#include <linux/devfreq.h>

+#define DEVFREQ_NAME_LEN 16
+
#define to_devfreq(DEV) container_of((DEV), struct devfreq, dev)

/* Devfreq events */
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 2f4a74efa6be..82156721f459 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -15,8 +15,6 @@
#include <linux/pm_opp.h>
#include <linux/pm_qos.h>

-#define DEVFREQ_NAME_LEN 16
-
/* DEVFREQ governor name */
#define DEVFREQ_GOV_SIMPLE_ONDEMAND "simple_ondemand"
#define DEVFREQ_GOV_PERFORMANCE "performance"
@@ -139,7 +137,6 @@ struct devfreq_stats {
* using devfreq.
* @profile: device-specific devfreq profile
* @governor: method how to choose frequency based on the usage.
- * @governor_name: devfreq governor name for use with this devfreq
* @nb: notifier block used to notify devfreq object that it should
* reevaluate operable frequencies. Devfreq users may use
* devfreq.nb to the corresponding register notifier call chain.
@@ -176,7 +173,6 @@ struct devfreq {
struct device dev;
struct devfreq_dev_profile *profile;
const struct devfreq_governor *governor;
- char governor_name[DEVFREQ_NAME_LEN];
struct notifier_block nb;
struct delayed_work work;

--
2.17.1


2020-10-26 06:41:35

by Chanwoo Choi

[permalink] [raw]
Subject: Re: [PATCH v2] PM / devfreq: Remove redundant governor_name from struct devfreq

On 10/23/20 7:32 PM, Chanwoo Choi wrote:
> The devfreq structure instance contains the governor_name and a governor
> instance. When need to show the governor name, better to use the name
> of devfreq_governor structure. So, governor_name variable in struct devfreq
> is a redundant and unneeded variable. Remove the redundant governor_name
> of struct devfreq and then use the name of devfreq_governor instance.
>
> Signed-off-by: Chanwoo Choi <[email protected]>
> ---
> Changes from v1:
> - Rebase it on "[PATCH v5 0/2] PM / devfreq: Add governor feature and attribute flag"
>
> drivers/devfreq/devfreq.c | 18 +++++++-----------
> drivers/devfreq/governor.h | 2 ++
> include/linux/devfreq.h | 4 ----
> 3 files changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index f6c55cf498ed..74f71588ace5 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -811,7 +811,6 @@ struct devfreq *devfreq_add_device(struct device *dev,
> devfreq->dev.release = devfreq_dev_release;
> INIT_LIST_HEAD(&devfreq->node);
> devfreq->profile = profile;
> - strscpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN);
> devfreq->previous_freq = profile->initial_freq;
> devfreq->last_status.current_frequency = profile->initial_freq;
> devfreq->data = data;
> @@ -907,7 +906,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
>
> mutex_lock(&devfreq_list_lock);
>
> - governor = try_then_request_governor(devfreq->governor_name);
> + governor = try_then_request_governor(governor_name);
> if (IS_ERR(governor)) {
> dev_err(dev, "%s: Unable to find governor for the device\n",
> __func__);
> @@ -1250,7 +1249,7 @@ int devfreq_add_governor(struct devfreq_governor *governor)
> int ret = 0;
> struct device *dev = devfreq->dev.parent;
>
> - if (!strncmp(devfreq->governor_name, governor->name,
> + if (!strncmp(devfreq->governor->name, governor->name,
> DEVFREQ_NAME_LEN)) {
> /* The following should never occur */
> if (devfreq->governor) {
> @@ -1312,7 +1311,7 @@ int devfreq_remove_governor(struct devfreq_governor *governor)
> int ret;
> struct device *dev = devfreq->dev.parent;
>
> - if (!strncmp(devfreq->governor_name, governor->name,
> + if (!strncmp(devfreq->governor->name, governor->name,
> DEVFREQ_NAME_LEN)) {
> /* we should have a devfreq governor! */
> if (!devfreq->governor) {
> @@ -1407,7 +1406,6 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
> */
> prev_governor = df->governor;
> df->governor = governor;
> - strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN);
> ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
> if (ret) {
> dev_warn(dev, "%s: Governor %s not started(%d)\n",
> @@ -1415,13 +1413,11 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>
> /* Restore previous governor */
> df->governor = prev_governor;
> - strncpy(df->governor_name, prev_governor->name,
> - DEVFREQ_NAME_LEN);
> ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
> if (ret) {
> dev_err(dev,
> "%s: reverting to Governor %s failed (%d)\n",
> - __func__, df->governor_name, ret);
> + __func__, prev_governor->name, ret);
> df->governor = NULL;
> goto out;
> }
> @@ -1457,7 +1453,7 @@ static ssize_t available_governors_show(struct device *d,
> */
> if (IS_SUPPORTED_FLAG(df->governor->flags, IMMUTABLE)) {
> count = scnprintf(&buf[count], DEVFREQ_NAME_LEN,
> - "%s ", df->governor_name);
> + "%s ", df->governor->name);
> /*
> * The devfreq device shows the registered governor except for
> * immutable governors such as passive governor .
> @@ -1900,7 +1896,7 @@ static int devfreq_summary_show(struct seq_file *s, void *data)
>
> list_for_each_entry_reverse(devfreq, &devfreq_list, node) {
> #if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
> - if (!strncmp(devfreq->governor_name, DEVFREQ_GOV_PASSIVE,
> + if (!strncmp(devfreq->governor->name, DEVFREQ_GOV_PASSIVE,
> DEVFREQ_NAME_LEN)) {
> struct devfreq_passive_data *data = devfreq->data;
>
> @@ -1926,7 +1922,7 @@ static int devfreq_summary_show(struct seq_file *s, void *data)
> "%-30s %-30s %-15s %-10s %10d %12ld %12ld %12ld\n",
> dev_name(&devfreq->dev),
> p_devfreq ? dev_name(&p_devfreq->dev) : "null",
> - devfreq->governor_name,
> + devfreq->governor->name,
> polling_ms ? timer_name[timer] : "null",
> polling_ms,
> cur_freq,
> diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
> index df413b851bb2..2a52f97b542d 100644
> --- a/drivers/devfreq/governor.h
> +++ b/drivers/devfreq/governor.h
> @@ -13,6 +13,8 @@
>
> #include <linux/devfreq.h>
>
> +#define DEVFREQ_NAME_LEN 16
> +
> #define to_devfreq(DEV) container_of((DEV), struct devfreq, dev)
>
> /* Devfreq events */
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 2f4a74efa6be..82156721f459 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -15,8 +15,6 @@
> #include <linux/pm_opp.h>
> #include <linux/pm_qos.h>
>
> -#define DEVFREQ_NAME_LEN 16
> -
> /* DEVFREQ governor name */
> #define DEVFREQ_GOV_SIMPLE_ONDEMAND "simple_ondemand"
> #define DEVFREQ_GOV_PERFORMANCE "performance"
> @@ -139,7 +137,6 @@ struct devfreq_stats {
> * using devfreq.
> * @profile: device-specific devfreq profile
> * @governor: method how to choose frequency based on the usage.
> - * @governor_name: devfreq governor name for use with this devfreq
> * @nb: notifier block used to notify devfreq object that it should
> * reevaluate operable frequencies. Devfreq users may use
> * devfreq.nb to the corresponding register notifier call chain.
> @@ -176,7 +173,6 @@ struct devfreq {
> struct device dev;
> struct devfreq_dev_profile *profile;
> const struct devfreq_governor *governor;
> - char governor_name[DEVFREQ_NAME_LEN];
> struct notifier_block nb;
> struct delayed_work work;
>
>

Applied it.

--
Best Regards,
Chanwoo Choi
Samsung Electronics