| 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: Fri, 18 Dec 2015 10:27:33 +0900
From: Chanwoo Choi <cw00.choi@...sung.com>
To: myungjoo.ham@...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 05/20] PM / devfreq: Add new passive governor
On 2015년 12월 14일 18:04, MyungJoo Ham wrote:
>>
>> This patch adds the new passive governor for DEVFREQ framework. The following
>> governors are already present and used for DVFS (Dynamic Voltage and Frequency
>> Scaling) drivers. The following governors are independently used for one device
>> driver which don't give the influence to other device drviers and also don't
>> receive the effect from other device drivers.
>> - ondemand / performance / powersave / userspace
>>
>> The passive governor depends on operation of parent driver with specific
>> governos extremely and is not able to decide the new frequency by oneself.
>> According to the decided new frequency of parent driver with governor,
>> the passive governor uses it to decide the appropriate frequency for own
>> device driver. The passive governor must need the following information
>> from device tree:
>> - the source clock and OPP tables
>> - the instance of parent device
>>
>> For exameple,
>> there are one more devfreq device drivers which need to change their source
>> clock according to their utilization on runtime. But, they share the same
>> power line (e.g., regulator). So, specific device driver is operated as parent
>> with ondemand governor and then the rest device driver with passive governor
>> is influenced by parent device.
>>
>> Suggested-by: Myungjoo Ham <myungjoo.ham@...sung.com>
>> Signed-off-by: Chanwoo Choi <cw00.choi@...sung.com>
>> [linux.amoon: Tested on Odroid U3]
>> Tested-by: Anand Moon <linux.amoon@...il.com>
>> ---
>> drivers/devfreq/Kconfig | 9 ++++
>> drivers/devfreq/Makefile | 1 +
>> drivers/devfreq/devfreq.c | 47 ++++++++++++++++
>> drivers/devfreq/governor_passive.c | 108 +++++++++++++++++++++++++++++++++++++
>> include/linux/devfreq.h | 15 ++++++
>> 5 files changed, 180 insertions(+)
>> create mode 100644 drivers/devfreq/governor_passive.c
>>
>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>> index 55ec774f794c..d03f635a93e1 100644
>> --- a/drivers/devfreq/Kconfig
>> +++ b/drivers/devfreq/Kconfig
>> @@ -64,6 +64,15 @@ config DEVFREQ_GOV_USERSPACE
>> Otherwise, the governor does not change the frequnecy
>> given at the initialization.
>>
>> +config DEVFREQ_GOV_PASSIVE
>> + tristate "Passive"
>> + help
>> + Sets the frequency by other governors (simple_ondemand, performance,
>> + powersave, usersapce) of a parent devfreq device. This governor
>> + always has the dependency on the chosen frequency from paired
>> + governor. This governor does not change the frequency by oneself
>> + through sysfs entry.
>
> Sets the frequency based on the frequency of its parent devfreq
> device. This governor does not change the frequency by itself
> through sysfs entries.
OK. I'll modify it.
>
>> +
>> comment "DEVFREQ Drivers"
>>
>> config ARM_EXYNOS_BUS_DEVFREQ
>> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
>> index 375ebbb4fcfb..f81c313b4b79 100644
>> --- a/drivers/devfreq/Makefile
>> +++ b/drivers/devfreq/Makefile
> []
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 984c5e9e7bdd..15e58779e4c0 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -190,6 +190,31 @@ static struct devfreq_governor *find_devfreq_governor(const char *name)
>>
>> /* Load monitoring helper functions for governors use */
>>
>> +static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq)
>> +{
>> + struct devfreq *passive;
>> + unsigned long rate;
>> + int ret;
>> +
>> + list_for_each_entry(passive, &devfreq->passive_dev_list, passive_node) {
>> + if (!passive->governor)
>> + continue;
>> + rate = freq;
>> +
>> + ret = passive->governor->get_target_freq(passive, &rate);
>> + if (ret)
>> + return ret;
>> +
>> + ret = passive->profile->target(passive->dev.parent, &rate, 0);
>> + if (ret)
>> + return ret;
>> +
>> + passive->previous_freq = rate;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> /**
>> * update_devfreq() - Reevaluate the device and configure frequency.
>> * @devfreq: the devfreq instance.
>> @@ -233,10 +258,18 @@ int update_devfreq(struct devfreq *devfreq)
>> flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */
>> }
>>
>> + if (!list_empty(&devfreq->passive_dev_list)
>> + && devfreq->previous_freq > freq)
>> + update_devfreq_passive(devfreq, freq);
>> +
>
> Could you please comment somewhere appropriate
> that the dependent is going to be changed
> before its parent if the frequency is going down.
> (and after if going up)
> And state why as well.
I use the DEVFREQ_TRANSITION_NOTIFIER instead of this implementation.
>
> And, is this viable universally?
>
>> err = devfreq->profile->target(devfreq->dev.parent, &freq, flags);
>> if (err)
>> return err;
>>
>> + if (!list_empty(&devfreq->passive_dev_list)
>> + && devfreq->previous_freq < freq)
>> + update_devfreq_passive(devfreq, freq);
>> +
>> if (devfreq->profile->freq_table)
>> if (devfreq_update_status(devfreq, freq))
>> dev_err(&devfreq->dev,
>> @@ -442,6 +475,10 @@ static void _remove_devfreq(struct devfreq *devfreq)
>> return;
>> }
>> list_del(&devfreq->node);
>> + list_del(&devfreq->passive_node);
>> + if (!list_empty(&devfreq->passive_dev_list))
>> + list_del_init(&devfreq->passive_dev_list);
>> +
>> mutex_unlock(&devfreq_list_lock);
>>
>> if (devfreq->governor)
>> @@ -559,6 +596,16 @@ struct devfreq *devfreq_add_device(struct device *dev,
>> goto err_init;
>> }
>>
>> + if (!strncmp(devfreq->governor_name, "passive", 7)) {
>> + struct devfreq *parent_devfreq =
>> + ((struct devfreq_passive_data *)data)->parent;
>> +
>> + list_add(&devfreq->passive_node,
>> + &parent_devfreq->passive_dev_list);
>> + } else {
>> + INIT_LIST_HEAD(&devfreq->passive_dev_list);
>> + }
>> +
>> return devfreq;
>>
>> err_init:
>
> This code has become too much invasive to devfreq.c
> while being too special for the passive governor.
I agree. I'll implement again with notifier.
>
> Why don't you add notifier chain to devfreq.c, which can be used
> by anyone else as well, and use that notifier for passive governor?
> You may refer to "cpufreq_register_notifier()" with
> CPUFREQ_TRANSITION_NOTIFIER.
OK. I'll add the new notifier of DEVFREQ_TRANSITION_NOTIFIER
The list of supported notification:
DEVFREQ_PRECHANGE
DEVFREQ_POSTCHANGE
>
>> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
>> new file mode 100644
>
> Then, utilizing notifier-block at governor_passive.c becomes possible.
>
> You will also be able to write any frequency deciding code
> inside governor_passive.c as well, not in devfreq.c.
OK, I'll use DEVFREQ_TRANSITION_NOTIFIER for passive governor.
>
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index 6fa02a20eb63..95c54578a1b4 100644
>> --- a/include/linux/devfreq.h
>> +++ b/include/linux/devfreq.h
>> @@ -177,6 +177,9 @@ struct devfreq {
>> unsigned int *trans_table;
>> unsigned long *time_in_state;
>> unsigned long last_stat_updated;
>> +
>> + struct list_head passive_dev_list;
>> + struct list_head passive_node;
>> };
>
> You will need only one notifier head here.
OK.
>
>>
>> #if defined(CONFIG_PM_DEVFREQ)
>> @@ -241,6 +244,18 @@ struct devfreq_simple_ondemand_data {
>> };
>> #endif
>>
>> +/**
>> + * struct devfreq_passive_data - void *data fed to struct devfreq
>> + * and devfreq_add_device
>> + * @parent: The parent devfreq device.
>> + *
>> + * If the fed devfreq_passive_data pointer is NULL to the governor,
>> + * the governor return ERROR.
>> + */
>> +struct devfreq_passive_data {
>> + struct devfreq *parent;
>> +};
>> +
>
> Please enclose the above with #if
OK.
Thanks,
Chanwoo Choi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists