| Message ID | 1399966890-22926-1-git-send-email-k.chander@samsung.com |
|---|---|
| State | New |
| Headers | show |
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 -- 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
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
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.. -- 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
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.
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
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.
--
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/
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. -- 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
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 -- 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
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... -- 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/
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 -- 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
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. -- 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/
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 -- 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
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. -- 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
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 -- 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
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
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. -- 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/
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,
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 ? -- 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
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
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);