[PATCH] PM / OPP: discard duplicate OPP additions

[PATCH] PM / OPP: discard duplicate OPP additions

[[Chander Kashyap]

From: Chander Kashyap <k.chander@samsung.com>

It may be possible to unregister and re-register the cpufreq driver.
One such example is arm big-little IKS cpufreq driver. While
re-registering the driver, same OPPs may get added again.

This patch detects the duplicacy and discards them.

Signed-off-by: Chander Kashyap <k.chander@samsung.com>
Signed-off-by: Inderpal Singh <inderpal.s@samsung.com>
---
 drivers/base/power/opp.c |   28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 2553867..2e803a9 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -443,23 +443,33 @@ 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;
 	}
 
-	list_add_rcu(&new_opp->node, head);
-	mutex_unlock(&dev_opp_list_lock);
+	if (new_opp->rate != opp->rate) {
+		list_add_rcu(&new_opp->node, head);
+		mutex_unlock(&dev_opp_list_lock);
+
+		/*
+		 * Notify the changes in the availability of the operable
+		 * frequency/voltage list.
+		 */
+		srcu_notifier_call_chain(&dev_opp->head,
+						OPP_EVENT_ADD, new_opp);
+	} else {
+		mutex_unlock(&dev_opp_list_lock);
+		kfree(new_opp);
+	}
 
-	/*
-	 * Notify the changes in the availability of the operable
-	 * frequency/voltage list.
-	 */
-	srcu_notifier_call_chain(&dev_opp->head, OPP_EVENT_ADD, new_opp);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_add);
-- 
1.7.9.5

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Viresh Kumar]

On 13 May 2014 13:11, [Chander Kashyap <chander.kashyap@linaro.org> wrote:

What happened to your name ? "["

> From: Chander Kashyap <k.chander@samsung.com>
>
> It may be possible to unregister and re-register the cpufreq driver.
> One such example is arm big-little IKS cpufreq driver. While
> re-registering the driver, same OPPs may get added again.
>
> This patch detects the duplicacy and discards them.
>
> Signed-off-by: Chander Kashyap <k.chander@samsung.com>
> Signed-off-by: Inderpal Singh <inderpal.s@samsung.com>
> ---
>  drivers/base/power/opp.c |   28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)

I wouldn't say that this approach is particularly bad or wrong, but what
about this instead?

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 2553867..7efdaf3 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -713,6 +713,11 @@ int of_init_opp_table(struct device *dev)
        const __be32 *val;
        int nr;

+       if (!IS_ERR(find_device_opp(dev))) {
+               dev_warn("%s: opp table already exists\n", __func__);
+               return -EEXIST;
+       }
+
        prop = of_find_property(dev->of_node, "operating-points", NULL);
        if (!prop)
                return -ENODEV;

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Sudeep Holla]

On Tue, May 13, 2014 at 9:22 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 13 May 2014 13:11, [Chander Kashyap <chander.kashyap@linaro.org> wrote:
>
> What happened to your name ? "["
>
>> From: Chander Kashyap <k.chander@samsung.com>
>>
>> It may be possible to unregister and re-register the cpufreq driver.
>> One such example is arm big-little IKS cpufreq driver. While
>> re-registering the driver, same OPPs may get added again.
>>
>> This patch detects the duplicacy and discards them.
>>
>> Signed-off-by: Chander Kashyap <k.chander@samsung.com>
>> Signed-off-by: Inderpal Singh <inderpal.s@samsung.com>
>> ---
>>  drivers/base/power/opp.c |   28 +++++++++++++++++++---------
>>  1 file changed, 19 insertions(+), 9 deletions(-)
>
> I wouldn't say that this approach is particularly bad or wrong, but what
> about this instead?
>

Yes I prefer this and this exactly what I had[1] in my OPP DT series which
we could not conclude on the bindings. You also need patch[2] for DT version.

Regards,
Sudeep

[1] http://www.spinics.net/lists/cpufreq/msg07914.html
[2] http://www.spinics.net/lists/cpufreq/msg07916.html

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Viresh Kumar]

On 13 May 2014 16:00, Sudeep Holla <sudeep.holla@arm.com> wrote:
> On Tue, May 13, 2014 at 9:22 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> On 13 May 2014 13:11, [Chander Kashyap <chander.kashyap@linaro.org> wrote:
>>
>> What happened to your name ? "["
>>
>>> From: Chander Kashyap <k.chander@samsung.com>
>>>
>>> It may be possible to unregister and re-register the cpufreq driver.
>>> One such example is arm big-little IKS cpufreq driver. While
>>> re-registering the driver, same OPPs may get added again.
>>>
>>> This patch detects the duplicacy and discards them.
>>>
>>> Signed-off-by: Chander Kashyap <k.chander@samsung.com>
>>> Signed-off-by: Inderpal Singh <inderpal.s@samsung.com>
>>> ---
>>>  drivers/base/power/opp.c |   28 +++++++++++++++++++---------
>>>  1 file changed, 19 insertions(+), 9 deletions(-)
>>
>> I wouldn't say that this approach is particularly bad or wrong, but what
>> about this instead?
>>
>
> Yes I prefer this and this exactly what I had[1] in my OPP DT series which
> we could not conclude on the bindings. You also need patch[2] for DT version.

Ahh, I have just reinvented the wheel. Though I can see now that I have
Acked those patches as well :)

So, what are the plans for those patches then? As Chander also needs few
of those.

Probably split the series to get the non-blockers upstream Atleast ?

Another thing that I thought later, though the problem can be fixed by
your version of patches, the version from chander had something good as
well. It would get rid of duplicate entries coming from dtb. Would it make
sense to get that part in as well?

--
viresh

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Sudeep Holla]

Hi Viresh,

On 13/05/14 12:05, Viresh Kumar wrote:
> On 13 May 2014 16:00, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> On Tue, May 13, 2014 at 9:22 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>> On 13 May 2014 13:11, [Chander Kashyap <chander.kashyap@linaro.org> wrote:
>>>
>>> What happened to your name ? "["
>>>
>>>> From: Chander Kashyap <k.chander@samsung.com>
>>>>
>>>> It may be possible to unregister and re-register the cpufreq driver.
>>>> One such example is arm big-little IKS cpufreq driver. While
>>>> re-registering the driver, same OPPs may get added again.
>>>>
>>>> This patch detects the duplicacy and discards them.
>>>>
>>>> Signed-off-by: Chander Kashyap <k.chander@samsung.com>
>>>> Signed-off-by: Inderpal Singh <inderpal.s@samsung.com>
>>>> ---
>>>>   drivers/base/power/opp.c |   28 +++++++++++++++++++---------
>>>>   1 file changed, 19 insertions(+), 9 deletions(-)
>>>
>>> I wouldn't say that this approach is particularly bad or wrong, but what
>>> about this instead?
>>>
>>
>> Yes I prefer this and this exactly what I had[1] in my OPP DT series which
>> we could not conclude on the bindings. You also need patch[2] for DT version.
>
> Ahh, I have just reinvented the wheel. Though I can see now that I have
> Acked those patches as well :)
>

Yes correct, it was stalled due to no discussions on the bindings.

> So, what are the plans for those patches then? As Chander also needs few
> of those.
>
> Probably split the series to get the non-blockers upstream Atleast ?
>

I can do that.

> Another thing that I thought later, though the problem can be fixed by
> your version of patches, the version from chander had something good as
> well. It would get rid of duplicate entries coming from dtb. Would it make
> sense to get that part in as well?
>

That requires agreement on the bindings. The main issue with shared opp
bindings is that in general we don't want to modify the standard bindings
without covering all aspects. It will avoid carrying around buggy bindings
forever as its ABI.

There was another thread from Samsung modifying bindings[0] and Rob insisted to
join the discussion[1] I started, but I never got any feedback.

Regards,
Sudeep

[0] http://marc.info/?l=devicetree&m=139152254008892&w=2
[1] http://www.spinics.net/lists/arm-kernel/msg303977.html

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Chander Kashyap]

On 13 May 2014 16:35, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 13 May 2014 16:00, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> On Tue, May 13, 2014 at 9:22 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>> On 13 May 2014 13:11, [Chander Kashyap <chander.kashyap@linaro.org> wrote:
>>>
>>> What happened to your name ? "["
>>>
>>>> From: Chander Kashyap <k.chander@samsung.com>
>>>>
>>>> It may be possible to unregister and re-register the cpufreq driver.
>>>> One such example is arm big-little IKS cpufreq driver. While
>>>> re-registering the driver, same OPPs may get added again.
>>>>
>>>> This patch detects the duplicacy and discards them.
>>>>
>>>> Signed-off-by: Chander Kashyap <k.chander@samsung.com>
>>>> Signed-off-by: Inderpal Singh <inderpal.s@samsung.com>
>>>> ---
>>>>  drivers/base/power/opp.c |   28 +++++++++++++++++++---------
>>>>  1 file changed, 19 insertions(+), 9 deletions(-)
>>>
>>> I wouldn't say that this approach is particularly bad or wrong, but what
>>> about this instead?
>>>
>>
>> Yes I prefer this and this exactly what I had[1] in my OPP DT series which
>> we could not conclude on the bindings. You also need patch[2] for DT version.
>
> Ahh, I have just reinvented the wheel. Though I can see now that I have
> Acked those patches as well :)
>
> So, what are the plans for those patches then? As Chander also needs few
> of those.
>
> Probably split the series to get the non-blockers upstream Atleast ?
>
> Another thing that I thought later, though the problem can be fixed by
> your version of patches, the version from chander had something good as
> well. It would get rid of duplicate entries coming from dtb. Would it make
> sense to get that part in as well?

This patch takes care for duplicate entries even without dt. Hence i
feel it can go as separate patch.


>
> --
> viresh



-- 
with warm regards,
Chander Kashyap

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Sudeep Holla]

On 13/05/14 12:57, Chander Kashyap wrote:
> On 13 May 2014 16:35, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> On 13 May 2014 16:00, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>> On Tue, May 13, 2014 at 9:22 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>>> On 13 May 2014 13:11, [Chander Kashyap <chander.kashyap@linaro.org> wrote:
>>>>
>>>> What happened to your name ? "["
>>>>
>>>>> From: Chander Kashyap <k.chander@samsung.com>
>>>>>
>>>>> It may be possible to unregister and re-register the cpufreq driver.
>>>>> One such example is arm big-little IKS cpufreq driver. While
>>>>> re-registering the driver, same OPPs may get added again.
>>>>>
>>>>> This patch detects the duplicacy and discards them.
>>>>>
>>>>> Signed-off-by: Chander Kashyap <k.chander@samsung.com>
>>>>> Signed-off-by: Inderpal Singh <inderpal.s@samsung.com>
>>>>> ---
>>>>>   drivers/base/power/opp.c |   28 +++++++++++++++++++---------
>>>>>   1 file changed, 19 insertions(+), 9 deletions(-)
>>>>
>>>> I wouldn't say that this approach is particularly bad or wrong, but what
>>>> about this instead?
>>>>
>>>
>>> Yes I prefer this and this exactly what I had[1] in my OPP DT series which
>>> we could not conclude on the bindings. You also need patch[2] for DT version.
>>
>> Ahh, I have just reinvented the wheel. Though I can see now that I have
>> Acked those patches as well :)
>>
>> So, what are the plans for those patches then? As Chander also needs few
>> of those.
>>
>> Probably split the series to get the non-blockers upstream Atleast ?
>>
>> Another thing that I thought later, though the problem can be fixed by
>> your version of patches, the version from chander had something good as
>> well. It would get rid of duplicate entries coming from dtb. Would it make
>> sense to get that part in as well?
>
> This patch takes care for duplicate entries even without dt. Hence i
> feel it can go as separate patch.
>

Sorry if I added any confusion. My original patch series addressed both the
issues:

1. duplicate entries added by OPP library(same as addressed by your patch) and
2. another to avoid duplication in the devicetree for OPPs

IIUC Exynos don't use ST for OPPs(yet), so you need to address only (1).
But we still need (2) before any bL platforms use DT for OPPs and need CPUFreq 
support. And yes we can address this separately.

Regards,
Sudeep

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Nishanth Menon]

On 05/13/2014 03:22 AM, Viresh Kumar wrote:
> On 13 May 2014 13:11, [Chander Kashyap <chander.kashyap@linaro.org> wrote:
> 
> What happened to your name ? "["
> 
>> From: Chander Kashyap <k.chander@samsung.com>
>>
>> It may be possible to unregister and re-register the cpufreq driver.
>> One such example is arm big-little IKS cpufreq driver. While
>> re-registering the driver, same OPPs may get added again.
>>
>> This patch detects the duplicacy and discards them.
>>
>> Signed-off-by: Chander Kashyap <k.chander@samsung.com>
>> Signed-off-by: Inderpal Singh <inderpal.s@samsung.com>
>> ---
>>  drivers/base/power/opp.c |   28 +++++++++++++++++++---------
>>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> I wouldn't say that this approach is particularly bad or wrong, but what
> about this instead?
> 
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index 2553867..7efdaf3 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -713,6 +713,11 @@ int of_init_opp_table(struct device *dev)
>         const __be32 *val;
>         int nr;
> 
> +       if (!IS_ERR(find_device_opp(dev))) {
> +               dev_warn("%s: opp table already exists\n", __func__);
> +               return -EEXIST;
> +       }
> +
>         prop = of_find_property(dev->of_node, "operating-points", NULL);
>         if (!prop)
>                 return -ENODEV;

yes - this is good but should be an additional patch IMHO, since it
solves a different issue:"prevent opp table re-creation attempt". the
$subject patch addresses an issue where dev_pm_opp_add can be invoked
independently as well -> So, we seem to have have three issues to solve:
a) do we continue to ensure that OPP table is created one time?
b) while creating OPP table, duplicate entries should be rejected
($subject)
c) prevent recreation of OPP table once created (if we decide not to
have a cleanup logic as part of (a)) - this is what you propose.

Now, considering that OPP table definition is an SoC behavior, that
behavior is NOT going to change just because we are reinserting driver
modules - so if cpufreq drivers are resulting in that behavior, then
we should fix that. and consider OPP tables are created one time (at
boot) and do the necessary changes for the same.

just my 2 cents.

-- 
Regards,
Nishanth Menon

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Nishanth Menon]

On Tue, May 13, 2014 at 2:41 AM, [Chander Kashyap
<chander.kashyap@linaro.org> wrote:
> From: Chander Kashyap <k.chander@samsung.com>
>
> It may be possible to unregister and re-register the cpufreq driver.
> One such example is arm big-little IKS cpufreq driver. While
> re-registering the driver, same OPPs may get added again.
>
> This patch detects the duplicacy and discards them.

Nice catch. Thanks for the same.

That said, instead of ignoring it (skipping addition), should we do
the following:
a) if we find the same OPP being added, return error
b) add a cleanup routine dev_pm_opp_remove ?

Original design required OPP entries added by platform code and used
by driver code, but things have changed over time.

>
> Signed-off-by: Chander Kashyap <k.chander@samsung.com>
> Signed-off-by: Inderpal Singh <inderpal.s@samsung.com>
> ---
>  drivers/base/power/opp.c |   28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index 2553867..2e803a9 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -443,23 +443,33 @@ 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;
>         }
>

say we do at this point:
if (new_opp->rate == opp->rate) {
  dev_err(dev, "%s: attempt to add duplicate OPP entry (rate=%ld)\n",
__func__, new_opp->rate)
   kfree(new_opp);
    return -EINVAL;
}
we could avoid the change below, right?

> -       list_add_rcu(&new_opp->node, head);
> -       mutex_unlock(&dev_opp_list_lock);
> +       if (new_opp->rate != opp->rate) {
> +               list_add_rcu(&new_opp->node, head);
> +               mutex_unlock(&dev_opp_list_lock);
> +
> +               /*
> +                * Notify the changes in the availability of the operable
> +                * frequency/voltage list.
> +                */
> +               srcu_notifier_call_chain(&dev_opp->head,
> +                                               OPP_EVENT_ADD, new_opp);
> +       } else {
> +               mutex_unlock(&dev_opp_list_lock);
> +               kfree(new_opp);
> +       }
>
> -       /*
> -        * Notify the changes in the availability of the operable
> -        * frequency/voltage list.
> -        */
> -       srcu_notifier_call_chain(&dev_opp->head, OPP_EVENT_ADD, new_opp);
>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_add);
> --
> 1.7.9.5
>
> --
> 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

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Chander Kashyap]

Hi Nishant,

On 13 May 2014 18:53, Nishanth Menon <nm@ti.com> wrote:
> On Tue, May 13, 2014 at 2:41 AM, [Chander Kashyap
> <chander.kashyap@linaro.org> wrote:
>> From: Chander Kashyap <k.chander@samsung.com>
>>
>> It may be possible to unregister and re-register the cpufreq driver.
>> One such example is arm big-little IKS cpufreq driver. While
>> re-registering the driver, same OPPs may get added again.
>>
>> This patch detects the duplicacy and discards them.
>
> Nice catch. Thanks for the same.
>
> That said, instead of ignoring it (skipping addition), should we do
> the following:
> a) if we find the same OPP being added, return error
> b) add a cleanup routine dev_pm_opp_remove ?
>
> Original design required OPP entries added by platform code and used
> by driver code, but things have changed over time.
>
>>
>> Signed-off-by: Chander Kashyap <k.chander@samsung.com>
>> Signed-off-by: Inderpal Singh <inderpal.s@samsung.com>
>> ---
>>  drivers/base/power/opp.c |   28 +++++++++++++++++++---------
>>  1 file changed, 19 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
>> index 2553867..2e803a9 100644
>> --- a/drivers/base/power/opp.c
>> +++ b/drivers/base/power/opp.c
>> @@ -443,23 +443,33 @@ 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;
>>         }
>>
>
> say we do at this point:
> if (new_opp->rate == opp->rate) {
>   dev_err(dev, "%s: attempt to add duplicate OPP entry (rate=%ld)\n",
> __func__, new_opp->rate)
>    kfree(new_opp);
>     return -EINVAL;
> }

Yes this is more cleaner.
But instead of dev_err,  we should use dev_warn and secondly
return 0 rather than EINVAL, as there are independent users for this function

> we could avoid the change below, right?
>
>> -       list_add_rcu(&new_opp->node, head);
>> -       mutex_unlock(&dev_opp_list_lock);
>> +       if (new_opp->rate != opp->rate) {
>> +               list_add_rcu(&new_opp->node, head);
>> +               mutex_unlock(&dev_opp_list_lock);
>> +
>> +               /*
>> +                * Notify the changes in the availability of the operable
>> +                * frequency/voltage list.
>> +                */
>> +               srcu_notifier_call_chain(&dev_opp->head,
>> +                                               OPP_EVENT_ADD, new_opp);
>> +       } else {
>> +               mutex_unlock(&dev_opp_list_lock);
>> +               kfree(new_opp);
>> +       }
>>
>> -       /*
>> -        * Notify the changes in the availability of the operable
>> -        * frequency/voltage list.
>> -        */
>> -       srcu_notifier_call_chain(&dev_opp->head, OPP_EVENT_ADD, new_opp);
>>         return 0;
>>  }
>>  EXPORT_SYMBOL_GPL(dev_pm_opp_add);
>> --
>> 1.7.9.5
>>
>> --
>> 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



-- 
with warm regards,
Chander Kashyap

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Viresh Kumar]

On 14 May 2014 15:01, Chander Kashyap <chander.kashyap@linaro.org> wrote:
>> say we do at this point:
>> if (new_opp->rate == opp->rate) {
>>   dev_err(dev, "%s: attempt to add duplicate OPP entry (rate=%ld)\n",
>> __func__, new_opp->rate)
>>    kfree(new_opp);
>>     return -EINVAL;
>> }
>
> Yes this is more cleaner.
> But instead of dev_err,  we should use dev_warn and secondly

Correct

> return 0 rather than EINVAL, as there are independent users for this function

Why? We should actually use EEXIST here instead of EINVAL though..

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Nishanth Menon]

On 05/14/2014 06:08 AM, Viresh Kumar wrote:
> On 14 May 2014 15:01, Chander Kashyap <chander.kashyap@linaro.org> wrote:
>>> say we do at this point:
>>> if (new_opp->rate == opp->rate) {
>>>   dev_err(dev, "%s: attempt to add duplicate OPP entry (rate=%ld)\n",
>>> __func__, new_opp->rate)
>>>    kfree(new_opp);
>>>     return -EINVAL;
>>> }
>>
>> Yes this is more cleaner.
>> But instead of dev_err,  we should use dev_warn and secondly
> 
> Correct
> 
>> return 0 rather than EINVAL, as there are independent users for this function
> 
> Why? We should actually use EEXIST here instead of EINVAL though..
> 
Yep -EEXIST is the right return value here. As Viresh indicated,
reporting back 0 when the requested operation actually was not
performed is wrong. Caller is supposed to know when it makes an error
- hiding it is not correct.

-- 
Regards,
Nishanth Menon

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Chander Kashyap]

On 14 May 2014 19:57, Nishanth Menon <nm@ti.com> wrote:
> On 05/14/2014 06:08 AM, Viresh Kumar wrote:
>> On 14 May 2014 15:01, Chander Kashyap <chander.kashyap@linaro.org> wrote:
>>>> say we do at this point:
>>>> if (new_opp->rate == opp->rate) {
>>>>   dev_err(dev, "%s: attempt to add duplicate OPP entry (rate=%ld)\n",
>>>> __func__, new_opp->rate)
>>>>    kfree(new_opp);
>>>>     return -EINVAL;
>>>> }
>>>
>>> Yes this is more cleaner.
>>> But instead of dev_err,  we should use dev_warn and secondly
>>
>> Correct
>>
>>> return 0 rather than EINVAL, as there are independent users for this function
>>
>> Why? We should actually use EEXIST here instead of EINVAL though..
>>
> Yep -EEXIST is the right return value here. As Viresh indicated,
> reporting back 0 when the requested operation actually was not
> performed is wrong. Caller is supposed to know when it makes an error
> - hiding it is not correct.
>

Then in that case the caller must take care for two type of errors:
-EEXIST and -ENOMEM

> --
> Regards,
> Nishanth Menon



-- 
with warm regards,
Chander Kashyap

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Viresh Kumar]

On 15 May 2014 13:55, Chander Kashyap <chander.kashyap@linaro.org> wrote:
> Then in that case the caller must take care for two type of errors:
> -EEXIST and -ENOMEM

Actually, success: (0 or -EEXIST), failure: Anything else.

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Chander Kashyap]

On 15 May 2014 13:57, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 15 May 2014 13:55, Chander Kashyap <chander.kashyap@linaro.org> wrote:
>> Then in that case the caller must take care for two type of errors:
>> -EEXIST and -ENOMEM
>
> Actually, success: (0 or -EEXIST), failure: Anything else.

Yes exactly. All users of this API need to be modified to handle
EEXIST as success.

To avoid this returning 0 was suggested,

-- 
with warm regards,
Chander Kashyap

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Viresh Kumar]

On 15 May 2014 14:16, Chander Kashyap <chander.kashyap@linaro.org> wrote:
> Yes exactly. All users of this API need to be modified to handle
> EEXIST as success.

There are very few:

arch/arm/mach-omap2/opp.c
arch/arm/mach-vexpress/spc.c
drivers/devfreq/exynos/exynos4_bus.c
drivers/devfreq/exynos/exynos5_bus.c

So shouldn't be a problem fixing them..

> To avoid this returning 0 was suggested

But the bigger problem is that all new users have to know about this
and must take care of it, would also result in code duplication.

So, if I think this way:

The purpose of dev_pm_opp_add() is to make sure the OPP is
present with the device, when this function returns..

And with a duplicate entry, we can still confirm that. Over that, I couldn't
think of any situation where the caller wants to react to -EXIST separately.
They will still consider it as success.

So, maybe returning '0' isn't that bad of an idea :)

@Nishanth ?

[PATCH v2] PM / OPP: discard duplicate OPP additions

[Chander Kashyap]

From: Chander Kashyap <k.chander@samsung.com>

It may be possible to unregister and re-register the cpufreq driver.
One such example is arm big-little IKS cpufreq driver. While
re-registering the driver, same OPPs may get added again.

This patch detects the duplicacy and discards them.

Signed-off-by: Chander Kashyap <k.chander@samsung.com>
Signed-off-by: Inderpal Singh <inderpal.s@samsung.com>
---
 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

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Pavel Machek]

Hi!

> From: Chander Kashyap <k.chander@samsung.com>
> 
> It may be possible to unregister and re-register the cpufreq driver.
> One such example is arm big-little IKS cpufreq driver. While
> re-registering the driver, same OPPs may get added again.

Hmm. Would it be better to actually delete stale OPPs?

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Viresh Kumar]

On 14 May 2014 20:32, Pavel Machek <pavel@ucw.cz> wrote:
> Hmm. Would it be better to actually delete stale OPPs?

I wouldn't call them stale as dtb isn't going to change after boot
and we will always get the same list again, unless some arch has
added few more after boot.

So, there is no point freeing opp's once created. Just make sure
they aren't added again.

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Viresh Kumar]

On 15 May 2014 10:02, Inderpal Singh <inderpal.s@samsung.com> wrote:
> I feel freeing of opps are needed at least at the driver unregistration
> time, like we free cpufreq_table.
> Otherwise it amounts to memory leak unless we assume that the same driver is
> going to re-register and re-use the same opps.

Its memory leak only if we have lost the pointer to allocated memory, which
we haven't. Yes, it will keep occupying some space but there is only
one instance
of that per 'cluster' and is very much affordable instead of building it again..

There is a high chance that it will be used again by this or any other driver,
cpufreq or outside of it.

But, yes I do agree that the OPPs not added from dts, i.e. added from
platform should be freed when they don't make a sense. But that's a different
issue altogether.

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Inderpal Singh]

On Thu, May 15, 2014 at 10:06 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 15 May 2014 10:02, Inderpal Singh <inderpal.s@samsung.com> wrote:
>> I feel freeing of opps are needed at least at the driver unregistration
>> time, like we free cpufreq_table.
>> Otherwise it amounts to memory leak unless we assume that the same driver is
>> going to re-register and re-use the same opps.
>
> Its memory leak only if we have lost the pointer to allocated memory, which
> we haven't. Yes, it will keep occupying some space but there is only
> one instance
> of that per 'cluster' and is very much affordable instead of building it again..
>
> There is a high chance that it will be used again by this or any other driver,
> cpufreq or outside of it.
>
> But, yes I do agree that the OPPs not added from dts, i.e. added from
> platform should be freed when they don't make a sense. But that's a different
> issue altogether.

What i am saying that "what if we are not going to re-use again ? " I
am not sure if its practical.
Also, I feel the driver who created the opp table at its registration
time should free it at its unregistration. Isn't it true in general?


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

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Viresh Kumar]

On 15 May 2014 10:22, Inderpal Singh <inderpal.s@samsung.com> wrote:

> What i am saying that "what if we are not going to re-use again ? "

That's what I said:

Yes, it will keep occupying some space but there is only one instance
of that per 'cluster' and is very much affordable instead of building it again..

So, we may not need to free it.

> Also, I feel the driver who created the opp table at its registration
> time should free it at its unregistration. Isn't it true in general?

Probably case to case basis. You must free resources for two reasons:
- They are not lost, like memory leak ..
- They can be used by others ..

And both these doesn't happen in this case. OPP tables can be used
by any other framework and is more or less a core thing..

Now, with this discussion I have another idea here..

Why don't we build these tables automatically on boot from some core
code, rather than asking drivers to do it ? That will get rid of duplication
from all drivers and would also reduce confusion

@Rafael/Nishanth ??

If things look right, then I can write a patch for fixing it quickly...

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Inderpal Singh]

On Thu, May 15, 2014 at 10:34 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 15 May 2014 10:22, Inderpal Singh <inderpal.s@samsung.com> wrote:
>
>> What i am saying that "what if we are not going to re-use again ? "
>
> That's what I said:
>
> Yes, it will keep occupying some space but there is only one instance
> of that per 'cluster' and is very much affordable instead of building it again..
>
> So, we may not need to free it.

Its not just about cpufreq. There may be others using OPPs as well.
For example devfreq.
The OPPs for cpu devices may be used again but for others I am not sure.

>
>> Also, I feel the driver who created the opp table at its registration
>> time should free it at its unregistration. Isn't it true in general?
>
> Probably case to case basis. You must free resources for two reasons:
> - They are not lost, like memory leak ..
> - They can be used by others ..
>
> And both these doesn't happen in this case. OPP tables can be used
> by any other framework and is more or less a core thing..
>
> Now, with this discussion I have another idea here..
>
> Why don't we build these tables automatically on boot from some core
> code, rather than asking drivers to do it ? That will get rid of duplication
> from all drivers and would also reduce confusion
>

I also felt the same at least for OPPs of cpu devices.

> @Rafael/Nishanth ??
>
> If things look right, then I can write a patch for fixing it quickly...
> --
> 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

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Viresh Kumar]

On 15 May 2014 11:00, Inderpal Singh <inderpal.s@samsung.com> wrote:
> On Thu, May 15, 2014 at 10:34 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> On 15 May 2014 10:22, Inderpal Singh <inderpal.s@samsung.com> wrote:
>>
>>> What i am saying that "what if we are not going to re-use again ? "
>>
>> That's what I said:
>>
>> Yes, it will keep occupying some space but there is only one instance
>> of that per 'cluster' and is very much affordable instead of building it again..
>>
>> So, we may not need to free it.
>
> Its not just about cpufreq. There may be others using OPPs as well.
> For example devfreq.

And who is stopping these to use the already built ones? Exactly for
this reason I have been saying that lets not free OPPs already built.
Devfreq can simply use the ones built by cpufreq, even if cpufreq isn't
using it anymore.

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Inderpal Singh]

On Thu, May 15, 2014 at 11:07 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 15 May 2014 11:00, Inderpal Singh <inderpal.s@samsung.com> wrote:
>> On Thu, May 15, 2014 at 10:34 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>> On 15 May 2014 10:22, Inderpal Singh <inderpal.s@samsung.com> wrote:
>>>
>>>> What i am saying that "what if we are not going to re-use again ? "
>>>
>>> That's what I said:
>>>
>>> Yes, it will keep occupying some space but there is only one instance
>>> of that per 'cluster' and is very much affordable instead of building it again..
>>>
>>> So, we may not need to free it.
>>
>> Its not just about cpufreq. There may be others using OPPs as well.
>> For example devfreq.
>
> And who is stopping these to use the already built ones? Exactly for
> this reason I have been saying that lets not free OPPs already built.
> Devfreq can simply use the ones built by cpufreq, even if cpufreq isn't
> using it anymore.

I think I did not make myself clear.
Devfreq will have its own opp table associated with its own device. It
does not uses the opp table of cpus.
Hence there may be need to free the table if driver (at least devfreq)
getting un-registered.

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

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Viresh Kumar]

On 15 May 2014 11:16, Inderpal Singh <inderpal.s@samsung.com> wrote:
> I think I did not make myself clear.

Probably I was the one who got confused :)

> Devfreq will have its own opp table associated with its own device. It
> does not uses the opp table of cpus.
> Hence there may be need to free the table if driver (at least devfreq)
> getting un-registered.

We may have an unregister routine routine, I am not arguing about that.
But we don't need to call that for CPU's opp, that's it.. For devices it might
make sense to free memory.

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Inderpal Singh]

On Thu, May 15, 2014 at 11:31 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 15 May 2014 11:16, Inderpal Singh <inderpal.s@samsung.com> wrote:
>> I think I did not make myself clear.
>
> Probably I was the one who got confused :)
>
>> Devfreq will have its own opp table associated with its own device. It
>> does not uses the opp table of cpus.
>> Hence there may be need to free the table if driver (at least devfreq)
>> getting un-registered.
>
> We may have an unregister routine routine, I am not arguing about that.
> But we don't need to call that for CPU's opp, that's it.. For devices it might
> make sense to free memory.

Yes the provision should be there in the OPP framework and let the
individual drivers decide whether to invoke or not.


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

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Pavel Machek]

Hi!

> And both these doesn't happen in this case. OPP tables can be used
> by any other framework and is more or less a core thing..
> 
> Now, with this discussion I have another idea here..
> 
> Why don't we build these tables automatically on boot from some core
> code, rather than asking drivers to do it ? That will get rid of duplication
> from all drivers and would also reduce confusion

Yes please.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html