[v3] PM / OPP: discard duplicate OPPs

Message ID CAKohpomKvPcioNmfzvx7vqJHqs_TrhnHH6yYb5w_0f+YvA5hvA@mail.gmail.com
State New
Headers show

Commit Message

Viresh Kumar May 20, 2014, 12:05 p.m.
On 20 May 2014 16:56, Viresh Kumar <viresh.kumar@linaro.org> 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:

        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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Comments

Nishanth Menon May 20, 2014, 12:32 p.m. | #1
On Tue, May 20, 2014 at 7:05 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 20 May 2014 16:56, Viresh Kumar <viresh.kumar@linaro.org> 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-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki May 20, 2014, 12:45 p.m. | #2
On Tuesday, May 20, 2014 05:35:04 PM Viresh Kumar wrote:
> On 20 May 2014 16:56, Viresh Kumar <viresh.kumar@linaro.org> 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);
Viresh Kumar May 20, 2014, 1:29 p.m. | #3
On 20 May 2014 18:15, Rafael J. Wysocki <rjw@rjwysocki.net> 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-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, 1:31 p.m. | #4
On 20 May 2014 17:35, Viresh Kumar <viresh.kumar@linaro.org> 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-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, 1:36 p.m. | #5
On 05/20/2014 08:31 AM, Viresh Kumar wrote:
> On 20 May 2014 17:35, Viresh Kumar <viresh.kumar@linaro.org> 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.
Viresh Kumar May 20, 2014, 1:36 p.m. | #6
On 20 May 2014 18:02, Nishanth Menon <nm@ti.com> 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 majordomo@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

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;
        }