* [PATCH V5] PM/OPP: discard duplicate OPPs
@ 2014-05-20 14:53 Viresh Kumar
2014-05-20 15:19 ` Nishanth Menon
2014-05-20 21:09 ` Rafael J. Wysocki
0 siblings, 2 replies; 6+ messages in thread
From: Viresh Kumar @ 2014-05-20 14:53 UTC (permalink / raw)
To: rjw
Cc: linaro-kernel, linux-pm, linux-kernel, arvind.chauhan, inderpal.s,
nm, chander.kashyap, pavel, len.brown, Chander Kashyap,
Viresh Kumar
From: Chander Kashyap <k.chander@samsung.com>
We don't have any protection against addition of duplicate OPPs currently and
in case some code tries to add them it will end up corrupting OPP tables.
There can be many combinations in which we may end up trying duplicate OPPs:
- both freq and volt are same, but earlier OPP may or may not be active.
- only freq is same and volt is different.
This patch tries to implement below logic for these cases:
Return 0 if new OPP was duplicate of existing one (i.e. same freq and volt) and
return -EEXIST if new OPP had same freq but different volt as of an existing OPP
OR if both freq/volt were same but earlier OPP was disabled.
Signed-off-by: Chander Kashyap <k.chander@samsung.com>
Signed-off-by: Inderpal Singh <inderpal.s@samsung.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V4->V5:
- Mention Return values under 'Return:' clause of doc style comment.
- s/pr_warn/dev_warn
- s/linrao/linaro in my email id :(
drivers/base/power/opp.c | 27 +++++++++++++++++++++++++--
1 file changed, 25 insertions(+), 2 deletions(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 2553867..6a06d43 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -394,6 +394,13 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
* to keep the integrity of the internal data structures. Callers should ensure
* that this function is *NOT* called under RCU protection or in contexts where
* mutex cannot be locked.
+ *
+ * Returns:
+ * 0: On success OR
+ * Duplicate OPPs (both freq and volt are same) and opp->available
+ * -EEXIST: Freq are same and volt are different OR
+ * Duplicate OPPs (both freq and volt are same) and !opp->available
+ * -ENOMEM: Memory allocation failure
*/
int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
{
@@ -443,15 +450,31 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
new_opp->u_volt = u_volt;
new_opp->available = true;
- /* Insert new OPP in order of increasing frequency */
+ /*
+ * Insert new OPP in order of increasing frequency
+ * and discard if already present
+ */
head = &dev_opp->opp_list;
list_for_each_entry_rcu(opp, &dev_opp->opp_list, node) {
- if (new_opp->rate < opp->rate)
+ if (new_opp->rate <= opp->rate)
break;
else
head = &opp->node;
}
+ /* Duplicate OPPs ? */
+ if (new_opp->rate == opp->rate) {
+ int ret = (new_opp->u_volt == opp->u_volt) && opp->available ?
+ 0 : -EEXIST;
+
+ dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
+ __func__, opp->rate, opp->u_volt, opp->available,
+ new_opp->rate, new_opp->u_volt, new_opp->available);
+ mutex_unlock(&dev_opp_list_lock);
+ kfree(new_opp);
+ return ret;
+ }
+
list_add_rcu(&new_opp->node, head);
mutex_unlock(&dev_opp_list_lock);
--
2.0.0.rc2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V5] PM/OPP: discard duplicate OPPs
2014-05-20 14:53 [PATCH V5] PM/OPP: discard duplicate OPPs Viresh Kumar
@ 2014-05-20 15:19 ` Nishanth Menon
2014-05-20 21:09 ` Rafael J. Wysocki
1 sibling, 0 replies; 6+ messages in thread
From: Nishanth Menon @ 2014-05-20 15:19 UTC (permalink / raw)
To: Viresh Kumar, rjw
Cc: linaro-kernel, linux-pm, linux-kernel, arvind.chauhan, inderpal.s,
chander.kashyap, pavel, len.brown, Chander Kashyap
On 05/20/2014 09:53 AM, Viresh Kumar wrote:
> From: Chander Kashyap <k.chander@samsung.com>
>
> We don't have any protection against addition of duplicate OPPs currently and
> in case some code tries to add them it will end up corrupting OPP tables.
>
> There can be many combinations in which we may end up trying duplicate OPPs:
> - both freq and volt are same, but earlier OPP may or may not be active.
> - only freq is same and volt is different.
>
> This patch tries to implement below logic for these cases:
>
> Return 0 if new OPP was duplicate of existing one (i.e. same freq and volt) and
> return -EEXIST if new OPP had same freq but different volt as of an existing OPP
> OR if both freq/volt were same but earlier OPP was disabled.
>
> Signed-off-by: Chander Kashyap <k.chander@samsung.com>
> Signed-off-by: Inderpal Singh <inderpal.s@samsung.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V4->V5:
> - Mention Return values under 'Return:' clause of doc style comment.
> - s/pr_warn/dev_warn
> - s/linrao/linaro in my email id :(
>
> drivers/base/power/opp.c | 27 +++++++++++++++++++++++++--
> 1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index 2553867..6a06d43 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -394,6 +394,13 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
> * to keep the integrity of the internal data structures. Callers should ensure
> * that this function is *NOT* called under RCU protection or in contexts where
> * mutex cannot be locked.
> + *
> + * Returns:
s/Returns:/Return:/ -> sorry for being a nitpick.. scripts/kernel-doc
uses "Return:" in $section_return
> + * 0: On success OR
> + * Duplicate OPPs (both freq and volt are same) and opp->available
> + * -EEXIST: Freq are same and volt are different OR
> + * Duplicate OPPs (both freq and volt are same) and !opp->available
> + * -ENOMEM: Memory allocation failure
> */
> int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
> {
> @@ -443,15 +450,31 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
> new_opp->u_volt = u_volt;
> new_opp->available = true;
>
> - /* Insert new OPP in order of increasing frequency */
> + /*
> + * Insert new OPP in order of increasing frequency
> + * and discard if already present
> + */
> head = &dev_opp->opp_list;
> list_for_each_entry_rcu(opp, &dev_opp->opp_list, node) {
> - if (new_opp->rate < opp->rate)
> + if (new_opp->rate <= opp->rate)
> break;
> else
> head = &opp->node;
> }
>
> + /* Duplicate OPPs ? */
> + if (new_opp->rate == opp->rate) {
> + int ret = (new_opp->u_volt == opp->u_volt) && opp->available ?
> + 0 : -EEXIST;
> +
> + dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
> + __func__, opp->rate, opp->u_volt, opp->available,
> + new_opp->rate, new_opp->u_volt, new_opp->available);
checkpatch --strict showed:
--- /tmp/kernel-patch-verify.22670/ptest_check-start 2014-05-20
10:07:15.736147182 -0500
+++ /tmp/kernel-patch-verify.22670/ptest_check-end 2014-05-20
10:07:15.960149013 -0500
@@ -0,0 +1,6 @@
+CHECK: Alignment should match open parenthesis
+#68: FILE: drivers/base/power/opp.c:471:
++ dev_warn(dev, "%s: duplicate OPPs detected. Existing:
freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled:
%d\n",
++ __func__, opp->rate, opp->u_volt,
opp->available,
+If any of these errors are false positives, please report
+them to the maintainer, see CHECKPATCH in MAINTAINERS.
> + mutex_unlock(&dev_opp_list_lock);
> + kfree(new_opp);
> + return ret;
> + }
> +
> list_add_rcu(&new_opp->node, head);
> mutex_unlock(&dev_opp_list_lock);
>
>
Other than these minor fixes,
Acked-by: Nishanth Menon <nm@ti.com>
--
Regards,
Nishanth Menon
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V5] PM/OPP: discard duplicate OPPs
2014-05-20 14:53 [PATCH V5] PM/OPP: discard duplicate OPPs Viresh Kumar
2014-05-20 15:19 ` Nishanth Menon
@ 2014-05-20 21:09 ` Rafael J. Wysocki
2014-05-21 4:03 ` Viresh Kumar
1 sibling, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2014-05-20 21:09 UTC (permalink / raw)
To: Viresh Kumar
Cc: linaro-kernel, linux-pm, linux-kernel, arvind.chauhan, inderpal.s,
nm, chander.kashyap, pavel, len.brown, Chander Kashyap
On Tuesday, May 20, 2014 08:23:28 PM Viresh Kumar wrote:
> From: Chander Kashyap <k.chander@samsung.com>
>
> We don't have any protection against addition of duplicate OPPs currently and
> in case some code tries to add them it will end up corrupting OPP tables.
>
> There can be many combinations in which we may end up trying duplicate OPPs:
> - both freq and volt are same, but earlier OPP may or may not be active.
> - only freq is same and volt is different.
>
> This patch tries to implement below logic for these cases:
>
> Return 0 if new OPP was duplicate of existing one (i.e. same freq and volt) and
> return -EEXIST if new OPP had same freq but different volt as of an existing OPP
> OR if both freq/volt were same but earlier OPP was disabled.
>
> Signed-off-by: Chander Kashyap <k.chander@samsung.com>
> Signed-off-by: Inderpal Singh <inderpal.s@samsung.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V4->V5:
> - Mention Return values under 'Return:' clause of doc style comment.
> - s/pr_warn/dev_warn
> - s/linrao/linaro in my email id :(
>
> drivers/base/power/opp.c | 27 +++++++++++++++++++++++++--
> 1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index 2553867..6a06d43 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -394,6 +394,13 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
> * to keep the integrity of the internal data structures. Callers should ensure
> * that this function is *NOT* called under RCU protection or in contexts where
> * mutex cannot be locked.
> + *
> + * Returns:
> + * 0: On success OR
> + * Duplicate OPPs (both freq and volt are same) and opp->available
> + * -EEXIST: Freq are same and volt are different OR
> + * Duplicate OPPs (both freq and volt are same) and !opp->available
> + * -ENOMEM: Memory allocation failure
> */
> int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
> {
> @@ -443,15 +450,31 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
> new_opp->u_volt = u_volt;
> new_opp->available = true;
>
> - /* Insert new OPP in order of increasing frequency */
> + /*
> + * Insert new OPP in order of increasing frequency
> + * and discard if already present
> + */
> head = &dev_opp->opp_list;
> list_for_each_entry_rcu(opp, &dev_opp->opp_list, node) {
> - if (new_opp->rate < opp->rate)
> + if (new_opp->rate <= opp->rate)
> break;
> else
> head = &opp->node;
> }
>
> + /* Duplicate OPPs ? */
> + if (new_opp->rate == opp->rate) {
> + int ret = (new_opp->u_volt == opp->u_volt) && opp->available ?
> + 0 : -EEXIST;
The parens are not necessary. And is the direction correct?
> +
> + dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
> + __func__, opp->rate, opp->u_volt, opp->available,
> + new_opp->rate, new_opp->u_volt, new_opp->available);
> + mutex_unlock(&dev_opp_list_lock);
> + kfree(new_opp);
> + return ret;
> + }
> +
> list_add_rcu(&new_opp->node, head);
> mutex_unlock(&dev_opp_list_lock);
>
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V5] PM/OPP: discard duplicate OPPs
2014-05-20 21:09 ` Rafael J. Wysocki
@ 2014-05-21 4:03 ` Viresh Kumar
2014-05-21 23:48 ` Rafael J. Wysocki
0 siblings, 1 reply; 6+ messages in thread
From: Viresh Kumar @ 2014-05-21 4:03 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Lists linaro-kernel, linux-pm@vger.kernel.org,
Linux Kernel Mailing List, Arvind Chauhan, Ips Gandhi,
Nishanth Menon, Chander Kashyap, Pavel Machek, Brown, Len,
Chander Kashyap
On 21 May 2014 02:39, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> + /* Duplicate OPPs ? */
>> + if (new_opp->rate == opp->rate) {
>> + int ret = (new_opp->u_volt == opp->u_volt) && opp->available ?
>> + 0 : -EEXIST;
>
> The parens are not necessary. And is the direction correct?
What do you mean by direction here ?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V5] PM/OPP: discard duplicate OPPs
2014-05-21 4:03 ` Viresh Kumar
@ 2014-05-21 23:48 ` Rafael J. Wysocki
2014-05-22 4:05 ` Viresh Kumar
0 siblings, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2014-05-21 23:48 UTC (permalink / raw)
To: Viresh Kumar
Cc: Lists linaro-kernel, linux-pm@vger.kernel.org,
Linux Kernel Mailing List, Arvind Chauhan, Ips Gandhi,
Nishanth Menon, Chander Kashyap, Pavel Machek, Brown, Len,
Chander Kashyap
On Wednesday, May 21, 2014 09:33:42 AM Viresh Kumar wrote:
> On 21 May 2014 02:39, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> + /* Duplicate OPPs ? */
> >> + if (new_opp->rate == opp->rate) {
> >> + int ret = (new_opp->u_volt == opp->u_volt) && opp->available ?
> >> + 0 : -EEXIST;
> >
> > The parens are not necessary. And is the direction correct?
>
> What do you mean by direction here ?
The case in which we want to return 0. Never mind, it's OK. The parens are still
not necessary, though.
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V5] PM/OPP: discard duplicate OPPs
2014-05-21 23:48 ` Rafael J. Wysocki
@ 2014-05-22 4:05 ` Viresh Kumar
0 siblings, 0 replies; 6+ messages in thread
From: Viresh Kumar @ 2014-05-22 4:05 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Lists linaro-kernel, linux-pm@vger.kernel.org,
Linux Kernel Mailing List, Arvind Chauhan, Ips Gandhi,
Nishanth Menon, Chander Kashyap, Pavel Machek, Brown, Len,
Chander Kashyap
On 22 May 2014 05:18, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> The case in which we want to return 0. Never mind, it's OK.
Ahh yes, It was wrong earlier and fixed during this patch only :)
> The parens are still not necessary, though.
Already got rid of them and so didn't bother replying :)
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-05-22 4:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-05-20 14:53 [PATCH V5] PM/OPP: discard duplicate OPPs Viresh Kumar
2014-05-20 15:19 ` Nishanth Menon
2014-05-20 21:09 ` Rafael J. Wysocki
2014-05-21 4:03 ` Viresh Kumar
2014-05-21 23:48 ` Rafael J. Wysocki
2014-05-22 4:05 ` Viresh Kumar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).