Message ID | 1374770011-22171-4-git-send-email-l.majewski@samsung.com (mailing list archive) |
---|---|
State | Changes Requested, archived |
Headers | show |
On 25 July 2013 22:03, Lukasz Majewski <l.majewski@samsung.com> wrote: > diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c > static void __init acpi_cpufreq_boost_init(void) > { > + acpi_cpufreq_driver.boost_supported = false; Would be better if we do this in else of below if. > if (boot_cpu_has(X86_FEATURE_CPB) || boot_cpu_has(X86_FEATURE_IDA)) { > msrs = msrs_alloc(); > @@ -1021,12 +995,11 @@ static int __init acpi_cpufreq_init(void) > *iter = &cpb; > } > #endif > + acpi_cpufreq_boost_init(); We are calling it before registering cpufreq driver. Will this have any complications? > ret = cpufreq_register_driver(&acpi_cpufreq_driver); > if (ret) > free_acpi_perf_data(); > - else > - acpi_cpufreq_boost_init(); > > return ret; -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 26 Jul 2013 12:58:02 +0530 Viresh Kumar wrote, > On 25 July 2013 22:03, Lukasz Majewski <l.majewski@samsung.com> wrote: > > diff --git a/drivers/cpufreq/acpi-cpufreq.c > > b/drivers/cpufreq/acpi-cpufreq.c > > > static void __init acpi_cpufreq_boost_init(void) > > { > > + acpi_cpufreq_driver.boost_supported = false; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [*] > > Would be better if we do this in else of below if. We need to set boost_supported = false at [*] for the case when: 1. msrs_alloc fails or 2. acpi_cpufreq is built as a module and can be inserted and removed several times. Without [*] we could end up with wrong (not false) initial state. > > > if (boot_cpu_has(X86_FEATURE_CPB) || > > boot_cpu_has(X86_FEATURE_IDA)) { msrs = msrs_alloc(); > > > > @@ -1021,12 +995,11 @@ static int __init acpi_cpufreq_init(void) > > *iter = &cpb; > > } > > #endif > > + acpi_cpufreq_boost_init(); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [**] > > We are calling it before registering cpufreq driver. Will this have > any complications? When we call [**] after the cpufreq_register_driver [***] we end up with sysfs boost attribute not exported at x86. The boost attribute is exported at [***] only when acpi_cpufreq.boost_supported = true. However support for boost at x86 is evaluated at acpi_cpufreq_boost_init(). > > > ret = cpufreq_register_driver(&acpi_cpufreq_driver); > > if (ret) > > free_acpi_perf_data(); > > - else > > - acpi_cpufreq_boost_init(); > > > > return ret;
On 26 July 2013 13:39, Lukasz Majewski <l.majewski@samsung.com> wrote: > On Fri, 26 Jul 2013 12:58:02 +0530 Viresh Kumar wrote, >> On 25 July 2013 22:03, Lukasz Majewski <l.majewski@samsung.com> wrote: >> > diff --git a/drivers/cpufreq/acpi-cpufreq.c >> > b/drivers/cpufreq/acpi-cpufreq.c >> >> > static void __init acpi_cpufreq_boost_init(void) >> > { >> > + acpi_cpufreq_driver.boost_supported = false; > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [*] >> >> Would be better if we do this in else of below if. > > We need to set boost_supported = false at [*] for the case when: > 1. msrs_alloc fails > or > 2. acpi_cpufreq is built as a module and can be inserted and removed > several times. Without [*] we could end up with wrong (not false) > initial state. Hmm.. Now that I see the code again, we don't need to set it to false as it is a global variable and this field is already set to false.. >> > if (boot_cpu_has(X86_FEATURE_CPB) || >> > boot_cpu_has(X86_FEATURE_IDA)) { msrs = msrs_alloc(); >> >> >> > @@ -1021,12 +995,11 @@ static int __init acpi_cpufreq_init(void) >> > *iter = &cpb; >> > } >> > #endif >> > + acpi_cpufreq_boost_init(); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [**] >> >> We are calling it before registering cpufreq driver. Will this have >> any complications? > > When we call [**] after the cpufreq_register_driver [***] we end up with > sysfs boost attribute not exported at x86. > The boost attribute is exported at [***] only when > acpi_cpufreq.boost_supported = true. However support for boost at x86 > is evaluated at acpi_cpufreq_boost_init(). I understand why you moved it above cpufreq driver register. I was thinking if there can be few side effects of this.. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 26 Jul 2013 14:54:07 +0530 Viresh Kumar viresh.kumar@linaro.org wrote, > On 26 July 2013 13:39, Lukasz Majewski <l.majewski@samsung.com> wrote: > > On Fri, 26 Jul 2013 12:58:02 +0530 Viresh Kumar wrote, > >> On 25 July 2013 22:03, Lukasz Majewski <l.majewski@samsung.com> > >> wrote: > >> > diff --git a/drivers/cpufreq/acpi-cpufreq.c > >> > b/drivers/cpufreq/acpi-cpufreq.c > >> > >> > static void __init acpi_cpufreq_boost_init(void) > >> > { > >> > + acpi_cpufreq_driver.boost_supported = false; > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [*] > >> > >> Would be better if we do this in else of below if. > > > > We need to set boost_supported = false at [*] for the case when: > > 1. msrs_alloc fails > > or > > 2. acpi_cpufreq is built as a module and can be inserted and removed > > several times. Without [*] we could end up with wrong (not false) > > initial state. > > Hmm.. Now that I see the code again, we don't need to set it to false > as it is a global variable and this field is already set to false.. Ok, so then I will remove this line [*]. > > >> > if (boot_cpu_has(X86_FEATURE_CPB) || > >> > boot_cpu_has(X86_FEATURE_IDA)) { msrs = msrs_alloc(); > >> > >> > >> > @@ -1021,12 +995,11 @@ static int __init acpi_cpufreq_init(void) > >> > *iter = &cpb; > >> > } > >> > #endif > >> > + acpi_cpufreq_boost_init(); > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [**] > >> > >> We are calling it before registering cpufreq driver. Will this have > >> any complications? > > > > When we call [**] after the cpufreq_register_driver [***] we end up > > with sysfs boost attribute not exported at x86. > > The boost attribute is exported at [***] only when > > acpi_cpufreq.boost_supported = true. However support for boost at > > x86 is evaluated at acpi_cpufreq_boost_init(). > > I understand why you moved it above cpufreq driver register. I was > thinking if there can be few side effects of this.. As fair as I've tested it, there weren't any side effects. Moreover the acpi_cpufreq_boost_init() mostly read state of the HW, so IMHO can be done before cpufreg_register_driver().
On Fri, 26 Jul 2013 14:54:07 +0530 Viresh Kumar viresh.kumar@linaro.org wrote, > On 26 July 2013 13:39, Lukasz Majewski <l.majewski@samsung.com> wrote: > > On Fri, 26 Jul 2013 12:58:02 +0530 Viresh Kumar wrote, > >> On 25 July 2013 22:03, Lukasz Majewski <l.majewski@samsung.com> > >> wrote: > >> > diff --git a/drivers/cpufreq/acpi-cpufreq.c > >> > b/drivers/cpufreq/acpi-cpufreq.c > >> > >> > static void __init acpi_cpufreq_boost_init(void) > >> > { > >> > + acpi_cpufreq_driver.boost_supported = false; > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [*] > >> > >> Would be better if we do this in else of below if. > > > > We need to set boost_supported = false at [*] for the case when: > > 1. msrs_alloc fails > > or > > 2. acpi_cpufreq is built as a module and can be inserted and removed > > several times. Without [*] we could end up with wrong (not false) > > initial state. > > Hmm.. Now that I see the code again, we don't need to set it to false > as it is a global variable and this field is already set to false.. Ok, I will delete the line at [*]. > > >> > if (boot_cpu_has(X86_FEATURE_CPB) || > >> > boot_cpu_has(X86_FEATURE_IDA)) { msrs = msrs_alloc(); > >> > >> > >> > @@ -1021,12 +995,11 @@ static int __init acpi_cpufreq_init(void) > >> > *iter = &cpb; > >> > } > >> > #endif > >> > + acpi_cpufreq_boost_init(); > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [**] > >> > >> We are calling it before registering cpufreq driver. Will this have > >> any complications? > > > > When we call [**] after the cpufreq_register_driver [***] we end up > > with sysfs boost attribute not exported at x86. > > The boost attribute is exported at [***] only when > > acpi_cpufreq.boost_supported = true. However support for boost at > > x86 is evaluated at acpi_cpufreq_boost_init(). > > I understand why you moved it above cpufreq driver register. I was > thinking if there can be few side effects of this.. Have any problem with the above change came to your mind?
On 12 August 2013 14:42, Lukasz Majewski <l.majewski@samsung.com> wrote:
> Have any problem with the above change came to your mind?
No.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c index 3926402..509764c 100644 --- a/drivers/cpufreq/acpi-cpufreq.c +++ b/drivers/cpufreq/acpi-cpufreq.c @@ -81,7 +81,6 @@ static struct acpi_processor_performance __percpu *acpi_perf_data; static struct cpufreq_driver acpi_cpufreq_driver; static unsigned int acpi_pstate_strict; -static bool boost_enabled, boost_supported; static struct msr __percpu *msrs; static bool boost_state(unsigned int cpu) @@ -134,49 +133,16 @@ static void boost_set_msrs(bool enable, const struct cpumask *cpumask) wrmsr_on_cpus(cpumask, msr_addr, msrs); } -static ssize_t _store_boost(const char *buf, size_t count) +static int _store_boost(int val) { - int ret; - unsigned long val = 0; - - if (!boost_supported) - return -EINVAL; - - ret = kstrtoul(buf, 10, &val); - if (ret || (val > 1)) - return -EINVAL; - - if ((val && boost_enabled) || (!val && !boost_enabled)) - return count; - get_online_cpus(); - boost_set_msrs(val, cpu_online_mask); - put_online_cpus(); - - boost_enabled = val; pr_debug("Core Boosting %sabled.\n", val ? "en" : "dis"); - return count; -} - -static ssize_t store_global_boost(struct kobject *kobj, struct attribute *attr, - const char *buf, size_t count) -{ - return _store_boost(buf, count); -} - -static ssize_t show_global_boost(struct kobject *kobj, - struct attribute *attr, char *buf) -{ - return sprintf(buf, "%u\n", boost_enabled); + return 0; } -static struct global_attr global_boost = __ATTR(boost, 0644, - show_global_boost, - store_global_boost); - static ssize_t show_freqdomain_cpus(struct cpufreq_policy *policy, char *buf) { struct acpi_cpufreq_data *data = per_cpu(acfreq_data, policy->cpu); @@ -187,15 +153,32 @@ static ssize_t show_freqdomain_cpus(struct cpufreq_policy *policy, char *buf) cpufreq_freq_attr_ro(freqdomain_cpus); #ifdef CONFIG_X86_ACPI_CPUFREQ_CPB +static ssize_t store_boost(const char *buf, size_t count) +{ + int ret; + unsigned long val = 0; + + if (!acpi_cpufreq_driver.boost_supported) + return -EINVAL; + + ret = kstrtoul(buf, 10, &val); + if (ret || (val > 1)) + return -EINVAL; + + _store_boost((int) val); + + return count; +} + static ssize_t store_cpb(struct cpufreq_policy *policy, const char *buf, size_t count) { - return _store_boost(buf, count); + return store_boost(buf, count); } static ssize_t show_cpb(struct cpufreq_policy *policy, char *buf) { - return sprintf(buf, "%u\n", boost_enabled); + return sprintf(buf, "%u\n", acpi_cpufreq_driver.boost_enabled); } static struct freq_attr cpb = __ATTR(cpb, 0644, show_cpb, store_cpb); @@ -583,7 +566,7 @@ static int boost_notify(struct notifier_block *nb, unsigned long action, switch (action) { case CPU_UP_PREPARE: case CPU_UP_PREPARE_FROZEN: - boost_set_msrs(boost_enabled, cpumask); + boost_set_msrs(acpi_cpufreq_driver.boost_enabled, cpumask); break; case CPU_DOWN_PREPARE: @@ -943,43 +926,34 @@ static struct cpufreq_driver acpi_cpufreq_driver = { .name = "acpi-cpufreq", .owner = THIS_MODULE, .attr = acpi_cpufreq_attr, + .set_boost = _store_boost, }; static void __init acpi_cpufreq_boost_init(void) { + acpi_cpufreq_driver.boost_supported = false; if (boot_cpu_has(X86_FEATURE_CPB) || boot_cpu_has(X86_FEATURE_IDA)) { msrs = msrs_alloc(); if (!msrs) return; - boost_supported = true; - boost_enabled = boost_state(0); - + acpi_cpufreq_driver.boost_supported = true; + acpi_cpufreq_driver.boost_enabled = boost_state(0); get_online_cpus(); /* Force all MSRs to the same value */ - boost_set_msrs(boost_enabled, cpu_online_mask); + boost_set_msrs(acpi_cpufreq_driver.boost_enabled, + cpu_online_mask); register_cpu_notifier(&boost_nb); put_online_cpus(); - } else - global_boost.attr.mode = 0444; - - /* We create the boost file in any case, though for systems without - * hardware support it will be read-only and hardwired to return 0. - */ - if (cpufreq_sysfs_create_file(&(global_boost.attr))) - pr_warn(PFX "could not register global boost sysfs file\n"); - else - pr_debug("registered global boost sysfs file\n"); + } } static void __exit acpi_cpufreq_boost_exit(void) { - cpufreq_sysfs_remove_file(&(global_boost.attr)); - if (msrs) { unregister_cpu_notifier(&boost_nb); @@ -1021,12 +995,11 @@ static int __init acpi_cpufreq_init(void) *iter = &cpb; } #endif + acpi_cpufreq_boost_init(); ret = cpufreq_register_driver(&acpi_cpufreq_driver); if (ret) free_acpi_perf_data(); - else - acpi_cpufreq_boost_init(); return ret; }