[PATCH V4] PM/OPP: discard duplicate OPPs

[PATCH V4] PM/OPP: discard duplicate OPPs

[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@linrao.org>
---
V3->V4:
- handle duplicate OPPs more appropriately
- update comment over routine and enhance commit log

@Chander: I have kept your authorship as is, hope you don't mind me sending it
on your behalf :)

 drivers/base/power/opp.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 2553867..cd9af42 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -389,6 +389,11 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
  * The opp is made available by default and it can be controlled using
  * dev_pm_opp_enable/disable functions.
  *
+ * Duplicate OPPs are discarded. Will return 0 if new OPP was duplicate of
+ * existing one (i.e. same freq and volt) and -EEXIST would be returned if new
+ * OPP had same freq but different volt as of an existing OPP OR if both were
+ * same but earlier OPP was disabled.
+ *
  * Locking: The internal device_opp and opp structures are RCU protected.
  * Hence this function internally uses RCU updater strategy with mutex locks
  * to keep the integrity of the internal data structures. Callers should ensure
@@ -443,15 +448,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;
+
+		pr_warn("%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

Re: [PATCH V4] PM/OPP: discard duplicate OPPs

[Nishanth Menon]

On 05/20/2014 09:17 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@linrao.org>
> ---
> V3->V4:
> - handle duplicate OPPs more appropriately
> - update comment over routine and enhance commit log
> 
> @Chander: I have kept your authorship as is, hope you don't mind me sending it
> on your behalf :)
> 
>  drivers/base/power/opp.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index 2553867..cd9af42 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -389,6 +389,11 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
>   * The opp is made available by default and it can be controlled using
>   * dev_pm_opp_enable/disable functions.
>   *
> + * Duplicate OPPs are discarded. Will return 0 if new OPP was duplicate of
> + * existing one (i.e. same freq and volt) and -EEXIST would be returned if new
> + * OPP had same freq but different volt as of an existing OPP OR if both were
> + * same but earlier OPP was disabled.
How about we use the kernel-doc's "Return:"
Return: Returns 0 if new OPP was successfully added OR if the new OPP
was exact duplicate of existing one (i.e. same frequency and volt).
-EEXIST would be returned if new
OPP had same freq but different volt as of an existing OPP OR if both
were same but earlier OPP was disabled. -ENOMEM is returned if there
is no memory available to allocate requisite internal structures.

> + *
>   * Locking: The internal device_opp and opp structures are RCU protected.
>   * Hence this function internally uses RCU updater strategy with mutex locks
>   * to keep the integrity of the internal data structures. Callers should ensure
> @@ -443,15 +448,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;
> +
> +		pr_warn("%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
dev_warn please? we already know the dev pointer. Also can we reduce
this down to 80 character limit if possible?

> +			__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);
>  
> 

Otherwise, this looks fine to me.

-- 
Regards,
Nishanth Menon

Re: [PATCH V4] PM/OPP: discard duplicate OPPs

[Viresh Kumar]

On 20 May 2014 19:57, Nishanth Menon <nm@ti.com> wrote:
>> +             pr_warn("%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
> dev_warn please? we already know the dev pointer. Also can we reduce
> this down to 80 character limit if possible?

breaking printk string into multiple line isn't suggested as it affects
readability and so I kept it this way. Another was was to break printk
itself intro two-three printk lines, but that would have looked bad
on console.

So, will retain it :(

Re: [PATCH V4] PM/OPP: discard duplicate OPPs

[Nishanth Menon]

On Tue, May 20, 2014 at 9:49 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> breaking printk string into multiple line isn't suggested as it affects
> readability and so I kept it this way. Another was was to break printk
> itself intro two-three printk lines, but that would have looked bad
> on console.
>
> So, will retain it :(
OK, dev_warn at least should be done.. we use dev_warn everywhere else.

Regards,
Nishanth Menon

Re: [PATCH V4] PM/OPP: discard duplicate OPPs

[Viresh Kumar]

On 20 May 2014 20:24, Nishanth Menon <nm@ti.com> wrote:
> On Tue, May 20, 2014 at 9:49 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>
>> breaking printk string into multiple line isn't suggested as it affects
>> readability and so I kept it this way. Another was was to break printk
>> itself intro two-three printk lines, but that would have looked bad
>> on console.
>>
>> So, will retain it :(
> OK, dev_warn at least should be done.. we use dev_warn everywhere else.

Already done. :)

TIP: One tip from my side :), Please use a blank line before and after
your reply.
It makes it much more readable.