Re: [PATCH v3] PM / OPP: discard duplicate OPPs
On 20 May 2014 17:35, Viresh Kumar wrote:
> Though after more thought into this I feel this must also be done:
>
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index bdf09f5..3f540d8 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -453,9 +453,13 @@ int dev_pm_opp_add(struct device *dev, unsigned
> long freq, unsigned long u_volt)
> }
>
> if (new_opp->rate == opp->rate) {
> + int ret = 0;
> +
> + if (new_opp->u_volt == opp->u_volt)
> + ret = -EEXIST;
> mutex_unlock(&dev_opp_list_lock);
> kfree(new_opp);
> - return 0;
> + return ret;
Ahh, sorry gentlemen. I have screwed up yet again.
I meant this instead:
> + if (new_opp->u_volt != opp->u_volt)
> + ret = -EEXIST;
Otherwise we are trying to add same OPP again and we can
return zero.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] PM / OPP: discard duplicate OPPs
On 20 May 2014 18:02, Nishanth Menon wrote:
>> + if (new_opp->u_volt == opp->u_volt)
>> + ret = -EEXIST;
As I mentioned in the other mail in same thread, I screwed up
again. I meant a s/==/!= here..
In words:
- If we are adding duplicate OPPs (both freq/volt same), return 0 as
we already have that.
- if we are adding OPPs with same key (same freq, diff volt), return
-EEXIST and also print both old and new values of both freq and volt.
> Else -> we now have two OPPs with the same key (same frequency, but
> different voltage) -> That does not make sense.
> Example: why would you add:
> If you already had {1GHz, 1.2V}
> and you attempted:
> {1GHz, 1.1V} (if you could do that, then you should added {1GHz, 1.1V}
> in the first place)
> OR
> {1GHz, 1.3V} (if you could do that, then you should add {1GHz, 1.3V}
> and the {1GHz, 1.2V} is wrong)
Exactly, this is why I wanted to return -EEXIST here with some prints.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] PM / OPP: discard duplicate OPPs
On 05/20/2014 08:31 AM, Viresh Kumar wrote:
> On 20 May 2014 17:35, Viresh Kumar wrote:
>> Though after more thought into this I feel this must also be done:
>>
>> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
>> index bdf09f5..3f540d8 100644
>> --- a/drivers/base/power/opp.c
>> +++ b/drivers/base/power/opp.c
>> @@ -453,9 +453,13 @@ int dev_pm_opp_add(struct device *dev, unsigned
>> long freq, unsigned long u_volt)
>> }
>>
>> if (new_opp->rate == opp->rate) {
>> + int ret = 0;
>> +
>> + if (new_opp->u_volt == opp->u_volt)
>> + ret = -EEXIST;
>> mutex_unlock(&dev_opp_list_lock);
>> kfree(new_opp);
>> - return 0;
>> + return ret;
>
> Ahh, sorry gentlemen. I have screwed up yet again.
>
> I meant this instead:
>
>> + if (new_opp->u_volt != opp->u_volt)
>> + ret = -EEXIST;
>
> Otherwise we are trying to add same OPP again and we can
> return zero.
>
if it was added and disabled? I suggest:
new_opp->u_volt != opp->u_volt || !opp->available
I still dont like the idea that we are allowing folks to do:
{
{1GHz 1.1V}
{1GHz 1.1V}
{1.2GHz 1.2V}
}
if you already had an OPP added and are trying to add it again, you
might want some debug ability. but anyways, with the mentioned check
above, my opposition is lower.
--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] PM / OPP: discard duplicate OPPs
On 20 May 2014 18:15, Rafael J. Wysocki wrote: > int ret = new_opp->u_volt == opp->u_volt ? -EEXIST : 0; > > would be slightly simpler IMO. Much better :) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] PM / OPP: discard duplicate OPPs
On Tue, May 20, 2014 at 7:05 AM, Viresh Kumar wrote:
> On 20 May 2014 16:56, Viresh Kumar wrote:
>> 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.
>
> Though after more thought into this I feel this must also be done:
>
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index bdf09f5..3f540d8 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -453,9 +453,13 @@ int dev_pm_opp_add(struct device *dev, unsigned
> long freq, unsigned long u_volt)
> }
>
> if (new_opp->rate == opp->rate) {
> + int ret = 0;
> +
> + if (new_opp->u_volt == opp->u_volt)
> + ret = -EEXIST;
Else -> we now have two OPPs with the same key (same frequency, but
different voltage) -> That does not make sense.
Example: why would you add:
If you already had {1GHz, 1.2V}
and you attempted:
{1GHz, 1.1V} (if you could do that, then you should added {1GHz, 1.1V}
in the first place)
OR
{1GHz, 1.3V} (if you could do that, then you should add {1GHz, 1.3V}
and the {1GHz, 1.2V} is wrong)
In addition, if old OPP was disabled, that new OPP would be in enabled
state -> which also does not make sense either - since we disabled
that frequency for some reason.
> mutex_unlock(&dev_opp_list_lock);
> kfree(new_opp);
> - return 0;
> + return ret;
> }
>
> list_add_rcu(&new_opp->node, head);
The reason I ask to return error is because if we attempt to add two
OPPs with the same key (frequency), then it breaks the logic in
remaining search logic.
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] PM / OPP: discard duplicate OPPs
On Tuesday, May 20, 2014 05:35:04 PM Viresh Kumar wrote:
> On 20 May 2014 16:56, Viresh Kumar wrote:
> > 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.
>
> Though after more thought into this I feel this must also be done:
>
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index bdf09f5..3f540d8 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -453,9 +453,13 @@ int dev_pm_opp_add(struct device *dev, unsigned
> long freq, unsigned long u_volt)
> }
>
> if (new_opp->rate == opp->rate) {
> + int ret = 0;
int ret = new_opp->u_volt == opp->u_volt ? -EEXIST : 0;
would be slightly simpler IMO.
> +
> + if (new_opp->u_volt == opp->u_volt)
> + ret = -EEXIST;
> mutex_unlock(&dev_opp_list_lock);
> kfree(new_opp);
> - return 0;
> + return ret;
> }
>
> list_add_rcu(&new_opp->node, head);
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] PM / OPP: discard duplicate OPPs
On 20 May 2014 16:56, Viresh Kumar wrote:
> 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.
Though after more thought into this I feel this must also be done:
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index bdf09f5..3f540d8 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -453,9 +453,13 @@ int dev_pm_opp_add(struct device *dev, unsigned
long freq, unsigned long u_volt)
}
if (new_opp->rate == opp->rate) {
+ int ret = 0;
+
+ if (new_opp->u_volt == opp->u_volt)
+ ret = -EEXIST;
mutex_unlock(&dev_opp_list_lock);
kfree(new_opp);
- return 0;
+ return ret;
}
list_add_rcu(&new_opp->node, head);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] PM / OPP: discard duplicate OPPs
On 20 May 2014 16:54, Nishanth Menon 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] PM / OPP: discard duplicate OPPs
On Mon, May 19, 2014 at 11:00 PM, Viresh Kumar 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] PM / OPP: discard duplicate OPPs
On 19 May 2014 18:38, Nishanth Menon 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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] PM / OPP: discard duplicate OPPs
On 05/16/2014 04:00 AM, Chander Kashyap wrote:
> From: Chander Kashyap
>
> This patch detects the duplicate OPP entries and discards them
>
> Signed-off-by: Chander Kashyap
> Signed-off-by: Inderpal Singh
> ---
> 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
--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] PM / OPP: discard duplicate OPPs
On 16 May 2014 14:30, Chander Kashyap wrote: > From: Chander Kashyap > > 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 > Signed-off-by: Inderpal Singh > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3] PM / OPP: discard duplicate OPPs
From: Chander Kashyap
This patch detects the duplicate OPP entries and discards them
Signed-off-by: Chander Kashyap
Signed-off-by: Inderpal Singh
---
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;
+ }
+
list_add_rcu(&new_opp->node, head);
mutex_unlock(&dev_opp_list_lock);
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
