On 25 July 2013 22:03, Lukasz Majewski l.majewski@samsung.com wrote:
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
/*********************************************************************
BOOST *- *********************************************************************/
+static int cpufreq_boost_set_sw(int state) +{
struct cpufreq_frequency_table *freq_table;struct cpufreq_policy *policy;int ret = -EINVAL;list_for_each_entry(policy, &cpufreq_policy_list, policy_list) {freq_table = cpufreq_frequency_get_table(policy->cpu);if (freq_table) {ret = cpufreq_frequency_table_cpuinfo(policy,freq_table);if (!ret) {policy->user_policy.max = policy->max;__cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);}}}return ret;+}
+int cpufreq_boost_trigger_state(int state) +{
unsigned long flags;int ret = 0;if (cpufreq_driver->boost_enabled == state)return 0;write_lock_irqsave(&cpufreq_driver_lock, flags);cpufreq_driver->boost_enabled = state;write_unlock_irqrestore(&cpufreq_driver_lock, flags);
Not sure if we should leave the lock at this point of time, as we haven't enabled boost until now.
If somebody tries to use this variable at this point of time, then it would get the wrong information about it.
ret = cpufreq_driver->set_boost(state);if (ret) {write_lock_irqsave(&cpufreq_driver_lock, flags);cpufreq_driver->boost_enabled = 0;
should be: cpufreq_driver->boost_enabled = !state;
write_unlock_irqrestore(&cpufreq_driver_lock, flags);pr_err("%s: BOOST cannot %s\n", __func__,
s/BOOST cannot %s/Cannot %s BOOST
state ? "enabled" : "disabled");}return ret;+}
+int cpufreq_boost_supported(void) +{
if (cpufreq_driver)
This routine is always called from places where cpufreq_driver can't be NULL..
--contd--
return cpufreq_driver->boost_supported;return 0;+} +EXPORT_SYMBOL_GPL(cpufreq_boost_supported);
+int cpufreq_boost_enabled(void) +{
return cpufreq_driver->boost_enabled;
And if above check is necessary, then don't you need to check it here as well?
+} +EXPORT_SYMBOL_GPL(cpufreq_boost_enabled);
+/*********************************************************************
REGISTER / UNREGISTER CPUFREQ DRIVER **********************************************************************/
@@ -2008,9 +2099,25 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data) cpufreq_driver = driver_data; write_unlock_irqrestore(&cpufreq_driver_lock, flags);
if (cpufreq_boost_supported()) {/** Check if boost driver provides function to enable boost -
s/boost driver/driver
* if not, use cpufreq_boost_set_sw as default*/if (!cpufreq_driver->set_boost)cpufreq_driver->set_boost = cpufreq_boost_set_sw;ret = cpufreq_sysfs_create_file(&(boost.attr));
You don't need braces around boost.attr.
if (ret) {pr_err("%s: cannot register global BOOST sysfs file\n",__func__);goto err_null_driver;}}ret = subsys_interface_register(&cpufreq_interface); if (ret)
goto err_null_driver;
goto err_boost_unreg; if (!(cpufreq_driver->flags & CPUFREQ_STICKY)) { int i;@@ -2037,6 +2144,9 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data) return 0; err_if_unreg: subsys_interface_unregister(&cpufreq_interface); +err_boost_unreg:
if (cpufreq_boost_supported())cpufreq_sysfs_remove_file(&(boost.attr));
same here.
err_null_driver: write_lock_irqsave(&cpufreq_driver_lock, flags); cpufreq_driver = NULL; @@ -2063,6 +2173,9 @@ int cpufreq_unregister_driver(struct cpufreq_driver *driver) pr_debug("unregistering driver %s\n", driver->name);
subsys_interface_unregister(&cpufreq_interface);
if (cpufreq_boost_supported())cpufreq_sysfs_remove_file(&(boost.attr));
here too.
unregister_hotcpu_notifier(&cpufreq_cpu_notifier); write_lock_irqsave(&cpufreq_driver_lock, flags);
+static ssize_t scaling_available_frequencies_show(struct cpufreq_policy *policy,
char *buf)+{
return show_available_freqs(policy, buf, 0);
s/0/false
+}
+static ssize_t scaling_boost_frequencies_show(struct cpufreq_policy *policy,
char *buf)+{
return show_available_freqs(policy, buf, 1);
s/1/true
+}
Looks good mostly.. We Should be to get it in 3.12 :)
On Fri, 26 Jul 2013 12:47:15 +0530 Viresh Kumar wrote,
On 25 July 2013 22:03, Lukasz Majewski l.majewski@samsung.com wrote:
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
/*********************************************************************
BOOST *
*********************************************************************/ +static int cpufreq_boost_set_sw(int state) +{
struct cpufreq_frequency_table *freq_table;struct cpufreq_policy *policy;int ret = -EINVAL;list_for_each_entry(policy, &cpufreq_policy_list,policy_list) {
freq_table =cpufreq_frequency_get_table(policy->cpu);
if (freq_table) {ret =cpufreq_frequency_table_cpuinfo(policy,
freq_table);if (!ret) {policy->user_policy.max =policy->max;
__cpufreq_governor(policy,CPUFREQ_GOV_LIMITS);
}}}return ret;+}
+int cpufreq_boost_trigger_state(int state) +{
unsigned long flags;int ret = 0;if (cpufreq_driver->boost_enabled == state)return 0;write_lock_irqsave(&cpufreq_driver_lock, flags);cpufreq_driver->boost_enabled = state;write_unlock_irqrestore(&cpufreq_driver_lock, flags);
^^^^^^^^^^^^^^^^^^^^ [*]
Not sure if we should leave the lock at this point of time, as we haven't enabled boost until now.
The problem here is with the cpufreq_driver->set_boost() call.
I tried to avoid acquiring lock at one function and release it at another (in this case cpufreq_boost_set_sw), especially since the __cpufreq_governor() acquires its own lock - good place for deadlock.
Is it OK for you to grab lock at one function (cpufreq_boost_trigger_state()) and then at other function (cpufreq_boost_set_sw) release it before calling __cpufreq_governor() and grab it again after its completion?
If somebody tries to use this variable at this point of time, then it would get the wrong information about it.
ret = cpufreq_driver->set_boost(state);if (ret) {write_lock_irqsave(&cpufreq_driver_lock, flags);cpufreq_driver->boost_enabled = 0;should be: cpufreq_driver->boost_enabled = !state;
For me = 0 (or = false) is more readable. If you wish, I will change it to = !state.
write_unlock_irqrestore(&cpufreq_driver_lock,flags); +
pr_err("%s: BOOST cannot %s\n", __func__,s/BOOST cannot %s/Cannot %s BOOST
Ok.
state ? "enabled" : "disabled");}return ret;+}
+int cpufreq_boost_supported(void) +{
if (cpufreq_driver)This routine is always called from places where cpufreq_driver can't be NULL..
It is also called from thermal. And it happens that thermal is initialized earlier. Then "NULL pointer dereference" happens.
--contd--
return cpufreq_driver->boost_supported;return 0;+} +EXPORT_SYMBOL_GPL(cpufreq_boost_supported);
+int cpufreq_boost_enabled(void) +{
return cpufreq_driver->boost_enabled;
^^^^^^^^^^^^^^ [1]
And if above check is necessary, then don't you need to check it here as well?
Because on thermal I check first if cpufreq_boost_supported() is true. If boost is not supported then check for cpufreq_boost_enabled() is not performed.
In my opinion at [1] we don't need the if (cpufreq_driver) check. But it is up to you to decide.
+} +EXPORT_SYMBOL_GPL(cpufreq_boost_enabled);
+/*********************************************************************
REGISTER / UNREGISTER CPUFREQDRIVER * *********************************************************************/
@@ -2008,9 +2099,25 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data) cpufreq_driver = driver_data; write_unlock_irqrestore(&cpufreq_driver_lock, flags);
if (cpufreq_boost_supported()) {/** Check if boost driver provides function toenable boost -
s/boost driver/driver
Ok.
* if not, use cpufreq_boost_set_sw as default*/if (!cpufreq_driver->set_boost)cpufreq_driver->set_boost =cpufreq_boost_set_sw; +
ret = cpufreq_sysfs_create_file(&(boost.attr));You don't need braces around boost.attr.
Ok.
if (ret) {pr_err("%s: cannot register global BOOSTsysfs file\n",
__func__);goto err_null_driver;}}ret = subsys_interface_register(&cpufreq_interface); if (ret)
goto err_null_driver;
goto err_boost_unreg; if (!(cpufreq_driver->flags & CPUFREQ_STICKY)) { int i;@@ -2037,6 +2144,9 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data) return 0; err_if_unreg: subsys_interface_unregister(&cpufreq_interface); +err_boost_unreg:
if (cpufreq_boost_supported())cpufreq_sysfs_remove_file(&(boost.attr));same here.
Ok.
err_null_driver: write_lock_irqsave(&cpufreq_driver_lock, flags); cpufreq_driver = NULL; @@ -2063,6 +2173,9 @@ int cpufreq_unregister_driver(struct cpufreq_driver *driver) pr_debug("unregistering driver %s\n", driver->name);
subsys_interface_unregister(&cpufreq_interface);
if (cpufreq_boost_supported())cpufreq_sysfs_remove_file(&(boost.attr));here too.
Ok.
unregister_hotcpu_notifier(&cpufreq_cpu_notifier); write_lock_irqsave(&cpufreq_driver_lock, flags);+static ssize_t scaling_available_frequencies_show(struct cpufreq_policy *policy,
char *buf)+{
return show_available_freqs(policy, buf, 0);s/0/false
Ok.
+}
+static ssize_t scaling_boost_frequencies_show(struct cpufreq_policy *policy,
char *buf)+{
return show_available_freqs(policy, buf, 1);s/1/true
Ok.
+}
Looks good mostly.. We Should be to get it in 3.12 :)
If we agree about above comments, I will post v7 ASAP.
On 26 July 2013 14:03, Lukasz Majewski l.majewski@samsung.com wrote:
On Fri, 26 Jul 2013 12:47:15 +0530 Viresh Kumar wrote,
On 25 July 2013 22:03, Lukasz Majewski l.majewski@samsung.com wrote:
+int cpufreq_boost_trigger_state(int state) +{
unsigned long flags;int ret = 0;if (cpufreq_driver->boost_enabled == state)return 0;write_lock_irqsave(&cpufreq_driver_lock, flags);cpufreq_driver->boost_enabled = state;write_unlock_irqrestore(&cpufreq_driver_lock, flags);^^^^^^^^^^^^^^^^^^^^ [*]Not sure if we should leave the lock at this point of time, as we haven't enabled boost until now.
The problem here is with the cpufreq_driver->set_boost() call.
I tried to avoid acquiring lock at one function and release it at another (in this case cpufreq_boost_set_sw), especially since the __cpufreq_governor() acquires its own lock - good place for deadlock.
Is it OK for you to grab lock at one function (cpufreq_boost_trigger_state()) and then at other function (cpufreq_boost_set_sw) release it before calling __cpufreq_governor() and grab it again after its completion?
ret = cpufreq_driver->set_boost(state);if (ret) {write_lock_irqsave(&cpufreq_driver_lock, flags);cpufreq_driver->boost_enabled = 0;should be: cpufreq_driver->boost_enabled = !state;
For me = 0 (or = false) is more readable. If you wish, I will change it to = !state.
Its not about readability but logic... What if boost was enabled earlier and we are disabling it now.. and we reach here.. We need to enable boost again, whereas you are disabling it.
+int cpufreq_boost_supported(void) +{
if (cpufreq_driver)This routine is always called from places where cpufreq_driver can't be NULL..
It is also called from thermal. And it happens that thermal is initialized earlier. Then "NULL pointer dereference" happens.
Ok.. Put a likely() around this check for cpufreq_driver..
In my opinion at [1] we don't need the if (cpufreq_driver) check. But it is up to you to decide.
leave it as is.
If we agree about above comments, I will post v7 ASAP.
Don't post it ASAP, wait for few more days for others to give comments.. And also I haven't finished reviewing it until now.
On Fri, 26 Jul 2013 15:03:34 +0530 Viresh Kumar wrote,
On 26 July 2013 14:03, Lukasz Majewski l.majewski@samsung.com wrote:
On Fri, 26 Jul 2013 12:47:15 +0530 Viresh Kumar wrote,
On 25 July 2013 22:03, Lukasz Majewski l.majewski@samsung.com wrote:
+int cpufreq_boost_trigger_state(int state) +{
unsigned long flags;int ret = 0;if (cpufreq_driver->boost_enabled == state)return 0;write_lock_irqsave(&cpufreq_driver_lock, flags);cpufreq_driver->boost_enabled = state;write_unlock_irqrestore(&cpufreq_driver_lock, flags);^^^^^^^^^^^^^^^^^^^^ [*]Not sure if we should leave the lock at this point of time, as we haven't enabled boost until now.
The problem here is with the cpufreq_driver->set_boost() call.
I tried to avoid acquiring lock at one function and release it at another (in this case cpufreq_boost_set_sw), especially since the __cpufreq_governor() acquires its own lock - good place for deadlock.
Is it OK for you to grab lock at one function (cpufreq_boost_trigger_state()) and then at other function (cpufreq_boost_set_sw) release it before calling __cpufreq_governor() and grab it again after its completion?
ret = cpufreq_driver->set_boost(state);if (ret) {write_lock_irqsave(&cpufreq_driver_lock, flags);cpufreq_driver->boost_enabled = 0;should be: cpufreq_driver->boost_enabled = !state;
For me = 0 (or = false) is more readable. If you wish, I will change it to = !state.
Its not about readability but logic... What if boost was enabled earlier and we are disabling it now.. and we reach here.. We need to enable boost again, whereas you are disabling it.
You are right here. I will change this to = !state
+int cpufreq_boost_supported(void) +{
if (cpufreq_driver)This routine is always called from places where cpufreq_driver can't be NULL..
It is also called from thermal. And it happens that thermal is initialized earlier. Then "NULL pointer dereference" happens.
Ok.. Put a likely() around this check for cpufreq_driver..
Ok.
In my opinion at [1] we don't need the if (cpufreq_driver) check. But it is up to you to decide.
leave it as is.
Ok.
If we agree about above comments, I will post v7 ASAP.
Don't post it ASAP, wait for few more days for others to give comments.. And also I haven't finished reviewing it until now.
Ok.
On 26 July 2013 14:03, Lukasz Majewski l.majewski@samsung.com wrote:
The problem here is with the cpufreq_driver->set_boost() call.
I tried to avoid acquiring lock at one function and release it at another (in this case cpufreq_boost_set_sw), especially since the __cpufreq_governor() acquires its own lock - good place for deadlock.
Is it OK for you to grab lock at one function (cpufreq_boost_trigger_state()) and then at other function (cpufreq_boost_set_sw) release it before calling __cpufreq_governor() and grab it again after its completion?
Problem is not only that.. but we shouldn't call boost_set() of drivers like acpi-cpufreq with this lock..... Leave it as it is for now.. Let me see if I can think of any problems that can happen due to this.
On Fri, 26 Jul 2013 15:06:45 +0530 Viresh Kumar wrote,
On 26 July 2013 14:03, Lukasz Majewski l.majewski@samsung.com wrote:
The problem here is with the cpufreq_driver->set_boost() call.
I tried to avoid acquiring lock at one function and release it at another (in this case cpufreq_boost_set_sw), especially since the __cpufreq_governor() acquires its own lock - good place for deadlock.
Is it OK for you to grab lock at one function (cpufreq_boost_trigger_state()) and then at other function (cpufreq_boost_set_sw) release it before calling __cpufreq_governor() and grab it again after its completion?
Problem is not only that.. but we shouldn't call boost_set() of drivers like acpi-cpufreq with this lock..... Leave it as it is for now.. Let me see if I can think of any problems that can happen due to this.
Ok. No problem.
On Fri, 26 Jul 2013 15:06:45 +0530 Viresh Kumar viresh.kumar@linaro.org wrote,
On 26 July 2013 14:03, Lukasz Majewski l.majewski@samsung.com wrote:
The problem here is with the cpufreq_driver->set_boost() call.
I tried to avoid acquiring lock at one function and release it at another (in this case cpufreq_boost_set_sw), especially since the __cpufreq_governor() acquires its own lock - good place for deadlock.
Is it OK for you to grab lock at one function (cpufreq_boost_trigger_state()) and then at other function (cpufreq_boost_set_sw) release it before calling __cpufreq_governor() and grab it again after its completion?
Problem is not only that.. but we shouldn't call boost_set() of drivers like acpi-cpufreq with this lock..... Leave it as it is for now.. Let me see if I can think of any problems that can happen due to this.
Do you have any second thoughts about this? Shall I leave it as it is now?
On 12 August 2013 14:37, Lukasz Majewski l.majewski@samsung.com wrote:
Do you have any second thoughts about this? Shall I leave it as it is now?
Honestly speaking I didn't had a chance to look into this.. Leave it as is, in case there is some problem we can patch it later.
On Friday, July 26, 2013 10:33:21 AM Lukasz Majewski wrote:
On Fri, 26 Jul 2013 12:47:15 +0530 Viresh Kumar wrote,
On 25 July 2013 22:03, Lukasz Majewski l.majewski@samsung.com wrote:
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
/*********************************************************************
BOOST *
*********************************************************************/ +static int cpufreq_boost_set_sw(int state) +{
struct cpufreq_frequency_table *freq_table;struct cpufreq_policy *policy;int ret = -EINVAL;list_for_each_entry(policy, &cpufreq_policy_list,policy_list) {
freq_table =cpufreq_frequency_get_table(policy->cpu);
if (freq_table) {ret =cpufreq_frequency_table_cpuinfo(policy,
freq_table);if (!ret) {policy->user_policy.max =policy->max;
__cpufreq_governor(policy,CPUFREQ_GOV_LIMITS);
}}}return ret;+}
+int cpufreq_boost_trigger_state(int state) +{
unsigned long flags;int ret = 0;if (cpufreq_driver->boost_enabled == state)return 0;write_lock_irqsave(&cpufreq_driver_lock, flags);cpufreq_driver->boost_enabled = state;write_unlock_irqrestore(&cpufreq_driver_lock, flags);^^^^^^^^^^^^^^^^^^^^ [*]Not sure if we should leave the lock at this point of time, as we haven't enabled boost until now.
The problem here is with the cpufreq_driver->set_boost() call.
I tried to avoid acquiring lock at one function and release it at another (in this case cpufreq_boost_set_sw), especially since the __cpufreq_governor() acquires its own lock - good place for deadlock.
Is it OK for you to grab lock at one function (cpufreq_boost_trigger_state()) and then at other function (cpufreq_boost_set_sw) release it before calling __cpufreq_governor() and grab it again after its completion?
It generally is better to avoid doing that, although it is not unheard of.
Thanks, Rafael
On Fri, 26 Jul 2013 14:36:08 +0200 Rafael J. Wysocki rjw@sisk.pl wrote,
On Friday, July 26, 2013 10:33:21 AM Lukasz Majewski wrote:
On Fri, 26 Jul 2013 12:47:15 +0530 Viresh Kumar wrote,
On 25 July 2013 22:03, Lukasz Majewski l.majewski@samsung.com wrote:
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
/*********************************************************************
BOOST *
*********************************************************************/ +static int cpufreq_boost_set_sw(int state) +{
struct cpufreq_frequency_table *freq_table;struct cpufreq_policy *policy;int ret = -EINVAL;list_for_each_entry(policy, &cpufreq_policy_list,policy_list) {
freq_table =cpufreq_frequency_get_table(policy->cpu);
if (freq_table) {ret =cpufreq_frequency_table_cpuinfo(policy,
freq_table);
if (!ret) {policy->user_policy.max =policy->max;
__cpufreq_governor(policy,CPUFREQ_GOV_LIMITS);
}}}return ret;+}
+int cpufreq_boost_trigger_state(int state) +{
unsigned long flags;int ret = 0;if (cpufreq_driver->boost_enabled == state)return 0;write_lock_irqsave(&cpufreq_driver_lock, flags);cpufreq_driver->boost_enabled = state;write_unlock_irqrestore(&cpufreq_driver_lock, flags);^^^^^^^^^^^^^^^^^^^^ [*]Not sure if we should leave the lock at this point of time, as we haven't enabled boost until now.
The problem here is with the cpufreq_driver->set_boost() call.
I tried to avoid acquiring lock at one function and release it at another (in this case cpufreq_boost_set_sw), especially since the __cpufreq_governor() acquires its own lock - good place for deadlock.
Is it OK for you to grab lock at one function (cpufreq_boost_trigger_state()) and then at other function (cpufreq_boost_set_sw) release it before calling __cpufreq_governor() and grab it again after its completion?
It generally is better to avoid doing that, although it is not unheard of.
In this particular case, one also needs to pass the "flags" parameter to the set_boost() function. This looks a bit unnatural to mix lock layer with the boost.
Thanks, Rafael
linaro-kernel@lists.linaro.org
-
Lukasz Majewski -
Rafael J. Wysocki -
Viresh Kumar