| 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: Wed, 12 Jun 2013 09:39:38 +0200
From: Lukasz Majewski <l.majewski@...sung.com>
To: Viresh Kumar <viresh.kumar@...aro.org>
Cc: "Rafael J. Wysocky" <rjw@...k.pl>,
"cpufreq@...r.kernel.org" <cpufreq@...r.kernel.org>,
Linux PM list <linux-pm@...r.kernel.org>,
Vincent Guittot <vincent.guittot@...aro.org>,
Jonghwa Lee <jonghwa3.lee@...sung.com>,
Myungjoo Ham <myungjoo.ham@...sung.com>,
linux-kernel <linux-kernel@...r.kernel.org>,
Lukasz Majewski <l.majewski@...ess.pl>,
Andre Przywara <andre.przywara@...aro.org>,
Daniel Lezcano <daniel.lezcano@...aro.org>,
Kukjin Kim <kgene.kim@...sung.com>
Subject: Re: [PATCH v2 1/3] cpufreq:boost: CPU frequency boost code unification
for software and hardware solutions
Hi Viresh,
> Hi,
>
> Change subject to: "cpufreq: Add boost frequency support in core"
Ok.
>
> On 11 June 2013 14:33, Lukasz Majewski <l.majewski@...sung.com> wrote:
> > This commit adds support for software based frequency boosting.
>
> No. It adds support for both software and hardware boosting. So just
> write: This commit adds boost frequency support in cpufreq core
> (Hardware & Software).
Ok.
>
> > Some SoC (like Exynos4 - e.g. 4x12) allow setting frequency above
> > its normal condition limits. Such a change shall be only done for a
> > short
>
> s/condition/operation
> s/change/mode
> s/done/used
>
Ok.
> > time.
> >
> > Overclocking (boost) support is essentially provided by platform
> > dependent cpufreq driver.
> >
> > This commit unifies support for SW and HW (Intel) over clocking
> > solutions in the core cpufreq driver. Previously the "boost" sysfs
> > attribute was defined at acpi driver code.
> > By default boost is disabled. One global attribute is available at:
> > /sys/devices/system/cpu/cpufreq/boost.
>
> Enter a blank line here.
Ok
>
> > It only shows up when cpufreq driver supports overclocking.
> > Under the hood frequencies dedicated for boosting are marked with a
> > special flag (CPUFREQ_BOOST_FREQ) at driver's frequency table.
> > It is the user's concern to enable/disable overclocking with proper
> > call to sysfs.
>
> Good.
>
> > Signed-off-by: Lukasz Majewski <l.majewski@...sung.com>
> > Signed-off-by: Myungjoo Ham <myungjoo.ham@...sung.com>
> >
> > ---
^^^^
[*] this --- was added manually by me.
> > Changes for v2:
> > - Removal of cpufreq_boost structure and move its fields to
> > cpufreq_driver structure
> > - Flag to indicate if global boost attribute is already defined
> > - Extent the pr_{err|debbug} functions to show current function
> > names ---
>
> You don't have to manually add "---" here. Just keep a blank line
> instead.
One "---" is added by git automatically. The [*] was added to distinct
the changelog from rest of the commit. At least older versions of GIT
required this to not include changelog to commit messages.
>
> > drivers/cpufreq/cpufreq.c | 69
> > ++++++++++++++++++++++++++++++++++++++++++
> > drivers/cpufreq/freq_table.c | 57
> > ++++++++++++++++++++++++++++++++-- include/linux/cpufreq.h |
> > 12 ++++++++ 3 files changed, 136 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 1b8a48e..98ba5f1 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -40,6 +40,7 @@
> > * also protects the cpufreq_cpu_data array.
> > */
> > static struct cpufreq_driver *cpufreq_driver;
> > +static bool cpufreq_boost_sysfs_defined;
> > static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
> > #ifdef CONFIG_HOTPLUG_CPU
> > /* This one keeps track of the previously set governor of a
> > removed CPU */ @@ -315,6 +316,33 @@
> > EXPORT_SYMBOL_GPL(cpufreq_notify_transition); /*********************************************************************
> > * SYSFS
> > INTERFACE *
> > *********************************************************************/
> > +ssize_t show_boost_status(struct kobject *kobj,
> > + struct attribute *attr, char *buf)
> > +{
> > + return sprintf(buf, "%d\n", cpufreq_driver->boost_en);
>
> This isn't correct. It shows if cpufreq driver supports boost or
> not and it should show if boost is enabled from sysfs when
> cpufreq driver supports boost.
The cpufreq_driver->low_level_boost() is only valid when cpufreq driver
supports boost. Otherwise it is NULL. Thereof you will not see those
attributes exported to /sysfs until cpufreq driver supports boost.
When "boost" attribute is exported - then it returns state of boosting
(if it is enabled or not).
>
> > +}
> > +
> > +static ssize_t store_boost_status(struct kobject *kobj, struct
> > attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + int ret, enable;
> > +
> > + ret = sscanf(buf, "%d", &enable);
> > + if (ret != 1 || enable < 0 || enable > 1)
> > + return -EINVAL;
> > +
> > + if (cpufreq_boost_trigger_state(enable)) {
> > + pr_err("%s: Cannot %sable boost!\n", __func__,
> > + enable ? "en" : "dis");
>
> %sable doesn't look very much readable. Use complete strings:
> "enable" and "disable".
Ok.
>
> > + return -EINVAL;
> > + }
> > +
> > + return count;
> > +}
> > +
> > +static struct global_attr global_boost = __ATTR(boost, 0644,
> > + show_boost_status,
> > + store_boost_status);
>
> User define_one_global_rw.
Ok, will reuse available macros (this code was taken directly from
acpi-cpufreq.c file).
>
> > static struct cpufreq_governor *__find_governor(const char
> > *str_governor) {
> > @@ -754,6 +782,17 @@ static int cpufreq_add_dev_interface(unsigned
> > int cpu,
>
> Why not do this in cpufreq_register_driver()?
Good point. I will move this code to cpufreq_register_driver.
>
> > goto err_out_kobj_put;
> > drv_attr++;
> > }
> > + if (cpufreq_driver->low_level_boost
> > && !cpufreq_boost_sysfs_defined) {
>
> I thought low_level_boost() is a function which will only be supported
> for drivers
> using hardware boost feature, like intel.
I think that we shall give users some flexibility and don't assume that
low_level_boost is only used for one solution/vendor.
It is also needed with software controlled boost. Please refer to patch
3/3.
> And so we must have used
> boost_en here.
boost_en has two meanings:
0 - either boost disabled or not supported (when low_level_boost=NULL).
1 - boost is enabled.
>
> > + ret = sysfs_create_file(cpufreq_global_kobject,
> > + &(global_boost.attr));
>
> cpufreq_sysfs_create_file(), check
> 2361be23666232dbb4851a527f466c4cbf5340fc for details.
Ok, I will rebase those changes to newest
kernel/git/rafael/linux-pm.git ,branch
kernel_pm/pm-cpufreq
>
> > + if (ret) {
> > + pr_err("%s: cannot register global boost
> > sysfs file\n",
> > + __func__);
> > + goto err_out_kobj_put;
> > + }
> > + cpufreq_boost_sysfs_defined = 1;
> > + }
> > +
> > if (cpufreq_driver->get) {
> > ret = sysfs_create_file(&policy->kobj,
> > &cpuinfo_cur_freq.attr); if (ret)
> > @@ -1853,6 +1892,30 @@ static struct notifier_block __refdata
> > cpufreq_cpu_notifier = { };
> >
> > /*********************************************************************
> > + *
> > BOOST *
> > +
> > *********************************************************************/
> > +int cpufreq_boost_trigger_state(int state) +{
> > + int ret = 0;
> > +
> > + if (!cpufreq_driver->low_level_boost)
> > + return -ENODEV;
>
> I am certainly not aligned with your design. What's the
> use of this field?
I had to rewrite a bit :-) patches since we decided to drop struct
cpufreq_boost.
I've added two fields to cpufreq_driver:
- low_level_boost:
* When boost is not supported (default) it is set to NULL. This
field has two purposes: Indicates if boost is available on
the system and provides address of low level callback
* When cpufreq driver assigns pointer to this field,
it means that it is supported
- boost_en:
* It shows if boost is enabled or disabled/not supported to the
rest of the system.
> And please update documentation too for these
> new entries in cpufreq_driver structure.
Ok I will extend it.
>
> > + if (cpufreq_driver->boost_en != state) {
>
> So, you are using boost_en to see if boost is enabled from sysfs?
> Then you have put it at wrong place.
I check if boost is already enabled.
>
> I thought there would be three variables:
> - cpufreq_driver->boost_supported: boost is enabled for driver
For this purpose I check the low_level_boost pointer if it's NULL or
not.
> - cpufreq_driver->low_level_boost(): to set desired boost state
> (Only for hardware boosting)
It has two purposes (as described above) and can be used (defined) by
software and hardware boost solutions.
> - boost_enabled: global variable in cpufreq.c file, used only if
> cpufreq_driver->boost_supported is true.
I think that boost enabled flag shall be defined at cpufreq driver
(please look into acpi_cpufreq.c - which used another set of global
flags).
It will then provide one flag for cpufreq.c (core) and drivers.
However I've added one global flag: cpufreq_boost_sysfs_defined
which indicates if /sys/devices/system/cpu/cpufreq/boost attribute has
been already defined (to prevent multiple definitions attempts).
>
>
> > diff --git a/drivers/cpufreq/freq_table.c
> > b/drivers/cpufreq/freq_table.c index d7a7966..4e4f692 100644
> > --- a/drivers/cpufreq/freq_table.c
> > +++ b/drivers/cpufreq/freq_table.c
> > @@ -20,6 +20,27 @@
> > * FREQUENCY TABLE
> > HELPERS *
> > *********************************************************************/
> >
> > +unsigned int
> > +cpufreq_frequency_table_max(struct cpufreq_frequency_table
> > *freq_table,
> > + int boost)
> > +{
> > + int i = 0, boost_freq_max = 0, freq_max = 0;
> > +
> > + for (; freq_table[i].frequency != CPUFREQ_TABLE_END; i++) {
> > + if (freq_table[i].index == CPUFREQ_BOOST_FREQ) {
> > + if (freq_table[i].frequency >
> > boost_freq_max)
> > + boost_freq_max =
> > freq_table[i].frequency;
>
> Do above only when boost==true and below when boost==false.
Ok.
>
> > + } else {
> > + if (freq_table[i].frequency > freq_max)
> > + freq_max = freq_table[i].frequency;
> > + }
> > + }
> > +
> > + return boost ? boost_freq_max : freq_max;
> > +
> > +}
> > +EXPORT_SYMBOL_GPL(cpufreq_frequency_table_max);
> > +
> > int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
> > struct cpufreq_frequency_table
> > *table) {
> > @@ -171,7 +192,8 @@ static DEFINE_PER_CPU(struct
> > cpufreq_frequency_table *, cpufreq_show_table); /**
> > * show_available_freqs - show available frequencies for the
> > specified CPU */
> > -static ssize_t show_available_freqs(struct cpufreq_policy *policy,
> > char *buf) +static ssize_t show_available_freqs(struct
> > cpufreq_policy *policy, char *buf,
> > + int show_boost)
> > {
> > unsigned int i = 0;
> > unsigned int cpu = policy->cpu;
> > @@ -186,22 +208,53 @@ static ssize_t show_available_freqs(struct
> > cpufreq_policy *policy, char *buf) for (i = 0;
> > (table[i].frequency != CPUFREQ_TABLE_END); i++) { if
> > (table[i].frequency == CPUFREQ_ENTRY_INVALID) continue;
> > + if (show_boost)
> > + if (table[i].index != CPUFREQ_BOOST_FREQ)
> > + continue;
> > +
>
> Looks wrong. You will show boost freqs when show_boost is false.
My purpose here is to display frequencies only tagged with
CPUFREQ_BOOST_FREQ and when show_boost is true.
When show_boost is false, the operation of the function is unchanged.
>
> > count += sprintf(&buf[count], "%d ",
> > table[i].frequency); }
> > count += sprintf(&buf[count], "\n");
> >
> > return count;
> > +}
> >
> > +/**
> > + * show_available_normal_freqs - show normal boost frequencies for
> > + * the specified CPU
> > + */
> > +static ssize_t show_available_normal_freqs(struct cpufreq_policy
> > *policy,
> > + char *buf)
> > +{
> > + return show_available_freqs(policy, buf, 0);
> > }
> >
> > struct freq_attr cpufreq_freq_attr_scaling_available_freqs = {
> > .attr = { .name = "scaling_available_frequencies",
> > .mode = 0444,
> > },
> > - .show = show_available_freqs,
> > + .show = show_available_normal_freqs,
> > };
> > EXPORT_SYMBOL_GPL(cpufreq_freq_attr_scaling_available_freqs);
> >
> > +/**
> > + * show_available_boost_freqs - show available boost frequencies
> > for
> > + * the specified CPU
> > + */
> > +static ssize_t show_available_boost_freqs(struct cpufreq_policy
> > *policy,
> > + char *buf)
> > +{
> > + return show_available_freqs(policy, buf, 1);
> > +}
> > +
> > +struct freq_attr cpufreq_freq_attr_boost_available_freqs = {
> > + .attr = { .name = "scaling_boost_frequencies",
> > + .mode = 0444,
> > + },
> > + .show = show_available_boost_freqs,
> > +};
> > +EXPORT_SYMBOL_GPL(cpufreq_freq_attr_boost_available_freqs);
> > +
>
> Code redundancy can be reduced by creating a macro for declaring
> **_availabe_freqs, its attributes and export symbol.
Yes, you are right here. Those two structures only differ with
different names.
>
> > /*
> > * if you use these, you must assure that the frequency table is
> > valid
> > * all the time between get_attr and put_attr!
>
> With this patch alone, we would be using boost frequencies even in
> normal cases where we haven't enabled boost.
Correct me if I'm wrong here, but the
cpufreq_freq_attr_boost_available_freqs will be added to cpufreq
driver's freq_attr table (i.e. *exynos_cpufreq_attr[]).
It is cpufreq driver's responsibility to add this attribute. By default
all other drivers add only cpufreq_freq_attr_boost_available_freqs.
--
Best regards,
Lukasz Majewski
Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
--
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