[v3] PM / OPP: discard duplicate OPPs

Message ID 1400230809-11401-1-git-send-email-chander.kashyap@linaro.org
State New
Headers show

Commit Message

Chander Kashyap May 16, 2014, 9 a.m.
From: Chander Kashyap <k.chander@samsung.com>

This patch detects the duplicate OPP entries and discards them

Signed-off-by: Chander Kashyap <k.chander@samsung.com>
Signed-off-by: Inderpal Singh <inderpal.s@samsung.com>
---
 Changes in v3:
	- Modify the commit log
 Changes in v2:
	- Reorder check for duplicate opp

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

Comments

Viresh Kumar May 16, 2014, 10:03 a.m. | #1
On 16 May 2014 14:30, Chander Kashyap <chander.kashyap@linaro.org> wrote:
> From: Chander Kashyap <k.chander@samsung.com>
>
> This patch detects the duplicate OPP entries and discards them

I wish this would have been a bit more explanatory :)

> Signed-off-by: Chander Kashyap <k.chander@samsung.com>
> Signed-off-by: Inderpal Singh <inderpal.s@samsung.com>
> ---
>  Changes in v3:
>         - Modify the commit log
>  Changes in v2:
>         - Reorder check for duplicate opp
>
>  drivers/base/power/opp.c |   13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
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
Nishanth Menon May 19, 2014, 1:08 p.m. | #2
On 05/16/2014 04:00 AM, Chander Kashyap wrote:
> From: Chander Kashyap <k.chander@samsung.com>
> 
> This patch detects the duplicate OPP entries and discards them
> 
> Signed-off-by: Chander Kashyap <k.chander@samsung.com>
> Signed-off-by: Inderpal Singh <inderpal.s@samsung.com>
> ---
>  Changes in v3:
> 	- Modify the commit log
>  Changes in v2:
> 	- Reorder check for duplicate opp
> 
>  drivers/base/power/opp.c |   13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index ca521e1..973da78 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -443,15 +443,24 @@ 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;
>  	}
>  
> +	if (new_opp->rate == opp->rate) {
> +		mutex_unlock(&dev_opp_list_lock);
> +		kfree(new_opp);
> +		return 0;

IF we decide on ensuring that the OPP additions are done one time[1] -
then returning -EEXIST is appropriate here. we want to be able to
catch warnings of sequencing errors, and returning 0 is not the way to
do it.

> +	}
> +
>  	list_add_rcu(&new_opp->node, head);
>  	mutex_unlock(&dev_opp_list_lock);
>  
> 

[1] http://marc.info/?l=linux-pm&m=140034777229205&w=2
Viresh Kumar May 20, 2014, 4 a.m. | #3
On 19 May 2014 18:38, Nishanth Menon <nm@ti.com> wrote:

>> +     if (new_opp->rate == opp->rate) {
>> +             mutex_unlock(&dev_opp_list_lock);
>> +             kfree(new_opp);
>> +             return 0;
>
> IF we decide on ensuring that the OPP additions are done one time[1] -

Fingers crossed :)

But that doesn't mean we covered everything. First of all platforms can
still add OPPs directly and then there are other OPPs than CPU's.

> then returning -EEXIST is appropriate here. we want to be able to
> catch warnings of sequencing errors, and returning 0 is not the way to
> do it.

I have asked this on the earlier thread as well, let me ask it again.
What would callers do on return value of EEXIST ? Is there anything
special we may want to handle ?

Yes, we shouldn't fix everything silently and so a pr_warn() can/should
be added here. But returning is zero is better in order not to complicate
error handling at callers side.

Isn't it ?
--
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
Nishanth Menon May 20, 2014, 11:24 a.m. | #4
On Mon, May 19, 2014 at 11:00 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> I have asked this on the earlier thread as well, let me ask it again.
> What would callers do on return value of EEXIST ? Is there anything
> special we may want to handle ?

That is upto the caller. returning 0 for an operation we were supposed
to do, but due to error checks, did not do, implies we need to provide
appropriate error back to caller. caller may choose to act upon the
error and do something OR not - depending on what the caller is (for
example, caller may choose to abort the full sequence as it does not
trust the entries anymore, OR maybe trying to add optional OPP - whose
failure is ignored) - it is NOT upto the this code to implement that
policy.

Regards,
Nishanth Menon
--
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
Viresh Kumar May 20, 2014, 11:26 a.m. | #5
On 20 May 2014 16:54, Nishanth Menon <nm@ti.com> wrote:
> That is upto the caller. returning 0 for an operation we were supposed
> to do, but due to error checks, did not do, implies we need to provide
> appropriate error back to caller. caller may choose to act upon the
> error and do something OR not - depending on what the caller is (for
> example, caller may choose to abort the full sequence as it does not
> trust the entries anymore, OR maybe trying to add optional OPP - whose
> failure is ignored) - it is NOT upto the this code to implement that
> policy.

But we aren't talking about failure here. Its not failure. The operation
we are trying to do is already done and nothing should break if the
OPP was already there or its added now. Its all the same.
--
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

Patch

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index ca521e1..973da78 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -443,15 +443,24 @@  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;
 	}
 
+	if (new_opp->rate == opp->rate) {
+		mutex_unlock(&dev_opp_list_lock);
+		kfree(new_opp);
+		return 0;
+	}
+
 	list_add_rcu(&new_opp->node, head);
 	mutex_unlock(&dev_opp_list_lock);