Re: [PATCH] PM / OPP: discard duplicate OPP additions
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 -- 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] PM / OPP: discard duplicate OPP additions
On 15 May 2014 14:16, Chander Kashyap 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 ? -- 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] PM / OPP: discard duplicate OPP additions
On 15 May 2014 13:57, Viresh Kumar wrote: > On 15 May 2014 13:55, Chander Kashyap 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 -- 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] PM / OPP: discard duplicate OPP additions
On 15 May 2014 13:55, Chander Kashyap 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. -- 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] PM / OPP: discard duplicate OPP additions
On 14 May 2014 19:57, Nishanth Menon wrote:
> On 05/14/2014 06:08 AM, Viresh Kumar wrote:
>> On 14 May 2014 15:01, Chander Kashyap 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
--
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] PM / OPP: discard duplicate OPP additions
On Thu, May 15, 2014 at 11:31 AM, Viresh Kumar wrote: > On 15 May 2014 11:16, Inderpal Singh 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 majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- 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] PM / OPP: discard duplicate OPP additions
On 15 May 2014 11:16, Inderpal Singh 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. -- 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] PM / OPP: discard duplicate OPP additions
On Thu, May 15, 2014 at 11:07 AM, Viresh Kumar wrote: > On 15 May 2014 11:00, Inderpal Singh wrote: >> On Thu, May 15, 2014 at 10:34 AM, Viresh Kumar >> wrote: >>> On 15 May 2014 10:22, Inderpal Singh 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 majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- 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] PM / OPP: discard duplicate OPP additions
On 15 May 2014 11:00, Inderpal Singh wrote: > On Thu, May 15, 2014 at 10:34 AM, Viresh Kumar > wrote: >> On 15 May 2014 10:22, Inderpal Singh 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. -- 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] PM / OPP: discard duplicate OPP additions
On Thu, May 15, 2014 at 10:34 AM, Viresh Kumar wrote: > On 15 May 2014 10:22, Inderpal Singh 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 majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- 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] PM / OPP: discard duplicate OPP additions
On 15 May 2014 10:22, Inderpal Singh 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... -- 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] PM / OPP: discard duplicate OPP additions
On Thu, May 15, 2014 at 10:06 AM, Viresh Kumar wrote: > On 15 May 2014 10:02, Inderpal Singh 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 majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- 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] PM / OPP: discard duplicate OPP additions
On 15 May 2014 10:02, Inderpal Singh 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. -- 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] PM / OPP: discard duplicate OPP additions
On 14 May 2014 20:32, Pavel Machek 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. -- 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] PM / OPP: discard duplicate OPP additions
Hi! > From: Chander Kashyap > > 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 -- 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] PM / OPP: discard duplicate OPP additions
On 05/14/2014 06:08 AM, Viresh Kumar wrote:
> On 14 May 2014 15:01, Chander Kashyap 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
--
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] PM / OPP: discard duplicate OPP additions
On 14 May 2014 15:01, Chander Kashyap 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..
--
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] PM / OPP: discard duplicate OPP additions
Hi Nishant,
On 13 May 2014 18:53, Nishanth Menon wrote:
> On Tue, May 13, 2014 at 2:41 AM, [Chander Kashyap
> wrote:
>> From: Chander Kashyap
>>
>> 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
>> Signed-off-by: Inderpal Singh
>> ---
>> 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 majord...@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
with warm regards,
Chander Kashyap
--
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] PM / OPP: discard duplicate OPP additions
On 05/13/2014 03:22 AM, Viresh Kumar wrote:
> On 13 May 2014 13:11, [Chander Kashyap wrote:
>
> What happened to your name ? "["
>
>> From: Chander Kashyap
>>
>> 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
>> Signed-off-by: Inderpal Singh
>> ---
>> 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
--
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] PM / OPP: discard duplicate OPP additions
On Tue, May 13, 2014 at 2:41 AM, [Chander Kashyap
wrote:
> From: Chander Kashyap
>
> 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
> Signed-off-by: Inderpal Singh
> ---
> 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 majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
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] PM / OPP: discard duplicate OPP additions
On 13/05/14 12:57, Chander Kashyap wrote: On 13 May 2014 16:35, Viresh Kumar wrote: On 13 May 2014 16:00, Sudeep Holla wrote: On Tue, May 13, 2014 at 9:22 AM, Viresh Kumar wrote: On 13 May 2014 13:11, [Chander Kashyap wrote: What happened to your name ? "[" From: Chander Kashyap 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 Signed-off-by: Inderpal Singh --- 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 -- 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] PM / OPP: discard duplicate OPP additions
On 13 May 2014 16:35, Viresh Kumar wrote: > On 13 May 2014 16:00, Sudeep Holla wrote: >> On Tue, May 13, 2014 at 9:22 AM, Viresh Kumar >> wrote: >>> On 13 May 2014 13:11, [Chander Kashyap wrote: >>> >>> What happened to your name ? "[" >>> From: Chander Kashyap 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 Signed-off-by: Inderpal Singh --- 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 -- 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] PM / OPP: discard duplicate OPP additions
Hi Viresh, On 13/05/14 12:05, Viresh Kumar wrote: On 13 May 2014 16:00, Sudeep Holla wrote: On Tue, May 13, 2014 at 9:22 AM, Viresh Kumar wrote: On 13 May 2014 13:11, [Chander Kashyap wrote: What happened to your name ? "[" From: Chander Kashyap 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 Signed-off-by: Inderpal Singh --- 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 -- 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] PM / OPP: discard duplicate OPP additions
On 13 May 2014 16:00, Sudeep Holla wrote: > On Tue, May 13, 2014 at 9:22 AM, Viresh Kumar wrote: >> On 13 May 2014 13:11, [Chander Kashyap wrote: >> >> What happened to your name ? "[" >> >>> From: Chander Kashyap >>> >>> 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 >>> Signed-off-by: Inderpal Singh >>> --- >>> 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 -- 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] PM / OPP: discard duplicate OPP additions
On Tue, May 13, 2014 at 9:22 AM, Viresh Kumar wrote: > On 13 May 2014 13:11, [Chander Kashyap wrote: > > What happened to your name ? "[" > >> From: Chander Kashyap >> >> 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 >> Signed-off-by: Inderpal Singh >> --- >> 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 -- 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] PM / OPP: discard duplicate OPP additions
On 13 May 2014 13:11, [Chander Kashyap wrote:
What happened to your name ? "["
> From: Chander Kashyap
>
> 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
> Signed-off-by: Inderpal Singh
> ---
> 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;
--
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] PM / OPP: discard duplicate OPP additions
From: Chander Kashyap
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
Signed-off-by: Inderpal Singh
---
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
--
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/
