| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 14 Dec 2015 09:49:48 +0000 (GMT)
From: MyungJoo Ham <myungjoo.ham@...sung.com>
To: 최찬우 <cw00.choi@...sung.com>,
크쉬시토프
<k.kozlowski@...sung.com>, "kgene@...nel.org" <kgene@...nel.org>
Cc: 박경민 <kyungmin.park@...sung.com>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"pawel.moll@....com" <pawel.moll@....com>,
"mark.rutland@....com" <mark.rutland@....com>,
"ijc+devicetree@...lion.org.uk" <ijc+devicetree@...lion.org.uk>,
"galak@...eaurora.org" <galak@...eaurora.org>,
"linux@....linux.org.uk" <linux@....linux.org.uk>,
"tjakobi@...h.uni-bielefeld.de" <tjakobi@...h.uni-bielefeld.de>,
"linux.amoon@...il.com" <linux.amoon@...il.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
"linux-samsung-soc@...r.kernel.org"
<linux-samsung-soc@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH v4 07/20] PM / devfreq: Show the related information
according to governor type
>
> This patch modifies the following sysfs entry of DEVFREQ framework
> because the devfreq device using passive governor don't need the same
> information of the devfreq device using rest governor.
> - polling_interval : passive gov don't use the sampling rate.
> - available_governors : passive gov don't be changed on runtime in this version.
> - trans_stat : passive governor don't support trans_stat in this version.
>
> Signed-off-by: Chanwoo Choi <cw00.choi@...sung.com>
> [linux.amoon: Tested on Odroid U3]
> Tested-by: Anand Moon <linux.amoon@...il.com>
You have a major update that is not mendtioned in the commit message.
(add governor "type")
> ---
> drivers/devfreq/devfreq.c | 31 +++++++++++++++++++++++++------
> drivers/devfreq/governor.h | 7 +++++++
> drivers/devfreq/governor_passive.c | 1 +
> drivers/devfreq/governor_performance.c | 1 +
> drivers/devfreq/governor_powersave.c | 1 +
> drivers/devfreq/governor_simpleondemand.c | 1 +
> drivers/devfreq/governor_userspace.c | 1 +
> include/linux/devfreq.h | 2 ++
> 8 files changed, 39 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 78ea4cdaa82c..18ad956fec93 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -597,7 +597,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
> goto err_init;
> }
>
> - if (!strncmp(devfreq->governor_name, "passive", 7)) {
> + if (devfreq->governor->type == DEVFREQ_GOV_PASSIVE) {
> struct devfreq *parent_devfreq =
> ((struct devfreq_passive_data *)data)->parent;
As mentioned in a previous reply, this code may be removed.
>
> @@ -963,13 +963,23 @@ static ssize_t available_governors_show(struct device *d,
> struct device_attribute *attr,
> char *buf)
> {
> - struct devfreq_governor *tmp_governor;
> + struct devfreq *devfreq = to_devfreq(d);
> ssize_t count = 0;
>
> mutex_lock(&devfreq_list_lock);
> - list_for_each_entry(tmp_governor, &devfreq_governor_list, node)
> + if (devfreq->governor->type == DEVFREQ_GOV_PASSIVE) {
> count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
> - "%s ", tmp_governor->name);
> + "%s ", devfreq->governor->name);
> + } else {
> + struct devfreq_governor *tmp_governor;
> +
> + list_for_each_entry(tmp_governor, &devfreq_governor_list, node) {
> + if (tmp_governor->type == DEVFREQ_GOV_PASSIVE)
> + continue;
> + count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
> + "%s ", tmp_governor->name);
> + }
> + }
Uh no. As long as we do not have a list of all possible governors
for each device, let's stick with what we've defined in ABI documentation:
show all available governors "in the system".
You MAY have a device that may run both PASSIVE and USERSPACE.
> mutex_unlock(&devfreq_list_lock);
>
> /* Truncate the trailing space */
> @@ -1006,6 +1016,11 @@ 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->governor->type == DEVFREQ_GOV_PASSIVE)
> + return sprintf(buf, "Not Supported.\n");
> +
> return sprintf(buf, "%d\n", to_devfreq(dev)->profile->polling_ms);
> }
When polling interval is irrlevent with the governor,
you don't need to print "Not Supported". Instead,
printing "0" is enough, (polling_ms=0 == no polling)
which is what devfreq is doing now.
>
> @@ -1020,6 +1035,9 @@ static ssize_t polling_interval_store(struct device *dev,
> if (!df->governor)
> return -EINVAL;
>
> + if (df->governor->type == DEVFREQ_GOV_PASSIVE)
> + return -EINVAL;
> +
Please simply return -EINVAL with governor's event_handler with DEVFREQ_GOV_INTERVAL
(I see the return value checking is missing at df->governor->event_handler().
You may want to add return value checking there to properly handle what you want.)
> ret = sscanf(buf, "%u", &value);
> if (ret != 1)
> return -EINVAL;
> @@ -1137,11 +1155,12 @@ static ssize_t trans_stat_show(struct device *dev,
> int i, j;
> unsigned int max_state = devfreq->profile->max_state;
>
> + if (max_state == 0 || devfreq->governor->type == DEVFREQ_GOV_PASSIVE)
> + return sprintf(buf, "Not Supported.\n");
> +
> if (!devfreq->stop_polling &&
> devfreq_update_status(devfreq, devfreq->previous_freq))
> return 0;
> - if (max_state == 0)
> - return sprintf(buf, "Not Supported.\n");
We have no reason to always disable this for PASSIVE.
>
> len = sprintf(buf, " From : To\n");
> len += sprintf(buf + len, " :");
> diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
> index fad7d6321978..43513a58f5bf 100644
> --- a/drivers/devfreq/governor.h
> +++ b/drivers/devfreq/governor.h
> @@ -18,6 +18,13 @@
>
> #define to_devfreq(DEV) container_of((DEV), struct devfreq, dev)
>
> +/* Devfreq governor type */
> +#define DEVFREQ_GOV_ONDEMAND 0x1
> +#define DEVFREQ_GOV_PERFORMANCE 0x2
> +#define DEVFREQ_GOV_POWERSAVE 0x3
> +#define DEVFREQ_GOV_USERSPACE 0x4
> +#define DEVFREQ_GOV_PASSIVE 0x4
Uh.. both USERSPACE AND PASSIVE are 0x4?
Powered by blists - more mailing lists