| Message ID | 1374770011-22171-3-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/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 :) -- 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: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 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 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 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. 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. -- 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 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. -- 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 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 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 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 > >
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. -- 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/cpufreq.c b/drivers/cpufreq/cpufreq.c index 172a25e..15f8d79 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -377,6 +377,33 @@ EXPORT_SYMBOL_GPL(cpufreq_notify_transition); /********************************************************************* * SYSFS INTERFACE * *********************************************************************/ +ssize_t show_boost(struct kobject *kobj, + struct attribute *attr, char *buf) +{ + return sprintf(buf, "%d\n", cpufreq_driver->boost_enabled); +} + +static ssize_t store_boost(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 %s BOOST!\n", __func__, + enable ? "enable" : "disable"); + return -EINVAL; + } + + pr_debug("%s: cpufreq BOOST %s\n", __func__, + enable ? "enabled" : "disabled"); + + return count; +} +define_one_global_rw(boost); static struct cpufreq_governor *__find_governor(const char *str_governor) { @@ -1970,6 +1997,70 @@ static struct notifier_block __refdata cpufreq_cpu_notifier = { }; /********************************************************************* + * 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); + + ret = cpufreq_driver->set_boost(state); + if (ret) { + write_lock_irqsave(&cpufreq_driver_lock, flags); + cpufreq_driver->boost_enabled = 0; + write_unlock_irqrestore(&cpufreq_driver_lock, flags); + + pr_err("%s: BOOST cannot %s\n", __func__, + state ? "enabled" : "disabled"); + } + + return ret; +} + +int cpufreq_boost_supported(void) +{ + if (cpufreq_driver) + return cpufreq_driver->boost_supported; + + return 0; +} +EXPORT_SYMBOL_GPL(cpufreq_boost_supported); + +int cpufreq_boost_enabled(void) +{ + return cpufreq_driver->boost_enabled; +} +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 - + * 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)); + 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)); 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)); + unregister_hotcpu_notifier(&cpufreq_cpu_notifier); write_lock_irqsave(&cpufreq_driver_lock, flags); diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c index f0d8741..17d03fc 100644 --- a/drivers/cpufreq/freq_table.c +++ b/drivers/cpufreq/freq_table.c @@ -34,6 +34,10 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy, continue; } + if (!cpufreq_boost_enabled() + && table[i].driver_data == CPUFREQ_BOOST_FREQ) + continue; + pr_debug("table entry %u: %u kHz, %u driver_data\n", i, freq, table[i].driver_data); if (freq < min_freq) @@ -171,7 +175,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, + bool show_boost) { unsigned int i = 0; unsigned int cpu = policy->cpu; @@ -186,6 +191,20 @@ 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; + /* + * show_boost = true and driver_data = BOOST freq + * display BOOST freqs + * + * show_boost = false and driver_data = BOOST freq + * show_boost = true and driver_data != BOOST freq + * continue - do not display anything + * + * show_boost = false and driver_data != BOOST freq + * display NON BOOST freqs + */ + if (show_boost ^ (table[i].driver_data == CPUFREQ_BOOST_FREQ)) + continue; + count += sprintf(&buf[count], "%d ", table[i].frequency); } count += sprintf(&buf[count], "\n"); @@ -194,14 +213,34 @@ static ssize_t show_available_freqs(struct cpufreq_policy *policy, char *buf) } -struct freq_attr cpufreq_freq_attr_scaling_available_freqs = { - .attr = { .name = "scaling_available_frequencies", - .mode = 0444, - }, - .show = show_available_freqs, -}; +#define cpufreq_attr_available_freq(_name) \ +struct freq_attr cpufreq_freq_attr_##_name##_freqs = \ +__ATTR_RO(_name##_frequencies) + +/** + * show_scaling_available_frequencies - show available normal frequencies for + * the specified CPU + */ +static ssize_t scaling_available_frequencies_show(struct cpufreq_policy *policy, + char *buf) +{ + return show_available_freqs(policy, buf, 0); +} +cpufreq_attr_available_freq(scaling_available); EXPORT_SYMBOL_GPL(cpufreq_freq_attr_scaling_available_freqs); +/** + * show_available_boost_freqs - show available boost frequencies for + * the specified CPU + */ +static ssize_t scaling_boost_frequencies_show(struct cpufreq_policy *policy, + char *buf) +{ + return show_available_freqs(policy, buf, 1); +} +cpufreq_attr_available_freq(scaling_boost); +EXPORT_SYMBOL_GPL(cpufreq_freq_attr_scaling_boost_freqs); + /* * if you use these, you must assure that the frequency table is valid * all the time between get_attr and put_attr! diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index d8e30fc..49a73c9 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -267,6 +267,11 @@ struct cpufreq_driver { int (*suspend) (struct cpufreq_policy *policy); int (*resume) (struct cpufreq_policy *policy); struct freq_attr **attr; + + /* platform specific boost support code */ + bool boost_supported; + bool boost_enabled; + int (*set_boost) (int state); }; /* flags */ @@ -410,6 +415,7 @@ extern struct cpufreq_governor cpufreq_gov_conservative; #define CPUFREQ_ENTRY_INVALID ~0 #define CPUFREQ_TABLE_END ~1 +#define CPUFREQ_BOOST_FREQ ~2 struct cpufreq_frequency_table { unsigned int driver_data; /* driver specific data, not used by core */ @@ -429,11 +435,15 @@ int cpufreq_frequency_table_target(struct cpufreq_policy *policy, unsigned int relation, unsigned int *index); +int cpufreq_boost_trigger_state(int state); +int cpufreq_boost_supported(void); +int cpufreq_boost_enabled(void); /* the following 3 funtions are for cpufreq core use only */ struct cpufreq_frequency_table *cpufreq_frequency_get_table(unsigned int cpu); /* the following are really really optional */ extern struct freq_attr cpufreq_freq_attr_scaling_available_freqs; +extern struct freq_attr cpufreq_freq_attr_scaling_boost_freqs; void cpufreq_frequency_table_get_attr(struct cpufreq_frequency_table *table, unsigned int cpu);