Nishanth, Have your comments been addressed by the message below? Rafael On Monday, May 30, 2011, MyungJoo Ham wrote: > On Sat, May 28, 2011 at 5:57 PM, Nishanth Menon <nm@xxxxxx> wrote: > > On 13:53-20110527, MyungJoo Ham wrote: > > [..] > >> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c > >> index 56a6899..819c1b3 100644 > >> --- a/drivers/base/power/opp.c > >> +++ b/drivers/base/power/opp.c > >> @@ -21,6 +21,7 @@ > >> #include <linux/rculist.h> > >> #include <linux/rcupdate.h> > >> #include <linux/opp.h> > >> +#include <linux/devfreq.h> > >> > >> /* > >> * Internal data structure organization with the OPP layer library is as > >> @@ -428,6 +429,11 @@ int opp_add(struct device *dev, unsigned long freq, unsigned long u_volt) > >> list_add_rcu(&new_opp->node, head); > >> mutex_unlock(&dev_opp_list_lock); > >> > >> + /* > >> + * Notify generic dvfs for the change and ignore error > >> + * because the device may not have a devfreq entry > >> + */ > >> + devfreq_update(dev); > > I think I understand your thought about notifiers here - we had some > > initial thought that we may need notifiers, for e.g. clk change > > notifiers which could implement enable/disable on a dependent device, > > thermal management etc.. so far, we have'nt had a need to modify the > > lowest data store layer to handle this. In this case, I think you'd > > like a notifier mechanism in general for opp_add,enable or disable in > > the opp library. > > Yes, I wanted a notifier machnaism called by OPP for any potential > changes in available frequencies. For that purpose, I've added > devfreq_update() because, for now, DEVFREQ is the only one that needs > such a notifier and a notifier has some overhead. If there are > multiple entities that require notifications from OPP for such > changes, we'd need notifiers maintained by OPP. > > > However, with this change, an opp_add now means: > > a) opp_add now depends on devfreq_update() to be part of it's integral > > operation - I think the devfreq should maintain it's own notifier > > mechanisms and not make OPP library do the notification trigger job of > > devfreq. > > To let the entities depending on the list of available frequencies be > notified, I think it would be appropriate for OPP to have a notifier > chain and call the notifier chain whenever there is a change in the > list. In that way, we can always add and remove the depending entities > (including DEVFREQ) to and from OPP's. Otherwise, we'd need OPP users > that call OPP functions to call the notifier chain when it appears > that the call will change the list. > > As long as the updates in available frequencies affect other entities, > OPP library should be able to notify the others whenever such changes > occur. In this patch, I've used devfreq_update() directly because we > do not have other entities, yet. However, as you've mentioned, there > will be others, so I'm willing to implement notifier in OPP if other > people also agree. > > > b) By ignoring the error, how will the caller of opp_add now know if the > > failures were real - e.g. some device failed to act on the notification > > and the overall opp_add operation failed? And why should OPP library do > > recovery for on behalf of devfreq? > > Um... OPP does not need to do any recovery for errors from DEVFREQ. > Other than errors from find_device_devfreq() in devfreq_update(), the > errors seem to be better returned somehow to the caller of opp_add(). > However, would it be fine to let opp_add (and other > frequency-availability changing functions) return errors returned from > a notifier? (either devfreq_update() or srcu_notifier_call_chain()) > > > c) How will you ensure the lock accuracy - given that you need to keep > > to the constraints of opp_add/opp_set_availability here? For example: > > devfreq_do is called from devfreq_monitor(workqueue) and devfreq_update > > in addition to having to call a bunch of function pointers whose > > implementation you do not know, having a function that ensures security > > of it's data, handles all lock conditions that is imposed differently by > > add,enable and disable is not visible in this implementation. > > devfreq_update() waits for devfreq_monitor() with devfreq_list_lock. > Both has devfreq_list_lock locked during their operations. > > > c) How about considering the usage of OPP library with an SoC cpufreq > > implementation say for MPU AND using the same SoC using devfreq for > > controlling some devices like MALI in your example simultaneously? Lets > > say the thermal policy manager implementation for some reason did an > > opp_disable of a higher OPP of MPU - devfreq_update gets called with the > > device(mpu_dev).. devfreq_update needs to now invoke it's validation > > path and fails correctly - Vs devfreq_update being called by some module > > which actually uses devfreq and the update failed - How does one > > differentiate between the two? > > When OPP is being used by CPUFREQ w/ CPU and by MALI, the OPP list > should be attached to the two devices independently. > In other words, the frequency of CPU should be controlled > independently from the frequency of OPP. If both CPU and MALI should > use the same frequency, there is no point to implement devfreq for > MALI. However, if the condition is not fully-dependent; e.g., > Freq(MALI) <= Freq(CPU), we can implement two instances of OPP for > each of CPU and MALI and let the "target" function supplied to CPU to > run opp_enable/disable and devfreq_update() to MALI before adjusting > the frequency of itself. > > When we are implementing another entity that controls OPP/DEVFREQ such > as the thermal policy manager, it should tackle both CPU and MALI w/ > OPPs at the same time if they are fully independent (i.e., the > frequency of MALI is not affected by the frequency of CPU). If not, > the same things (discussed in the previous paragraph) are applied. > > In the two examples you've mentioned, the first one is about the > implementation of the current patch (let thermal manager handle OPP > and OPP handle devfreq_update) and the another is when DEVFREQ calls > are implemented through thermal manager (let thermal manager handle > both OPP and devfreq_update), right? > > For the two cases, I think that the first approach is more > appropriate. We will probably have more than just a thermal manager to > handle OPP (and devfreq that depends on OPP). The first approach > allows users to declare which frequencies are available and to let the > underlying structure do their work without intervention from users > (those who decide which frequencies are available). With the second > approach, we need to enforce every frequency affecting entity to > understand underlying frequency dependencies and the behavior of > devfreq, not jst to understand the behavior of the device according to > the frequency. DEVFREQ is going to be handled and implemented by the > device driver of the device controlled by the instance of DEVFREQ. By > using the first approach, we can prevent affecting entities other than > the device driver using DEVFREQ from thinking about DEVFREQ of that > device. > > > > > > just my 2 cents, I think these kind of notifiers should probably > > stay out of opp library OR notifiers be introduced in a generic and safe > > fashion. > > So, you want either > > to call devfreq_update from where opp_add/enable/disable/... are called. > > or > > to use notifier in OPP? > > > Well, the first one would become the source of messy problems as > mentioned earlier. The second one has some overhead, but it's a > general approach and does not incur the issues of the first approach. > > Therefore, I prefer either to keep the initial approach regarding > devfreq_update() or to use a generic notifier at OPP (probably, SRCU > notifier chains?). > > How about using a SRCU_NOTIFIER at OPP? > > The notifier.h says that SRCU has low overhead generally and the most > overhead of SRCU comes from "remove srcu" (probably when "device" is > being removed), which won't happen a lot with OPP. > > > > >> return 0; > >> } > >> > >> @@ -512,6 +518,9 @@ unlock: > >> mutex_unlock(&dev_opp_list_lock); > >> out: > >> kfree(new_opp); > >> + > >> + /* Notify generic dvfs for the change and ignore error */ > >> + devfreq_update(dev); > > Another place where I am confused - how does devfreq_update know in what > > context is it being called is the OPP getting enabled/disabled/added - > > devfreq_update has to do some additional work to make it's own decision > > - it is almost as if it uses devfreq_update as a trigger mechanism to reevalute > > the entire decision tree - without using information of the context > > opp_enable/disable/add has to maybe make better call. if this > > information is not needed, maybe some other mechanism implemented by > > devfreq layer might suffice.. > > No, DEVFREQ does not require the context (OPP being > enabled/disabled/...) with the devfreq_update() calls. It is > essentially a "notify devfreq to reevaluate" and DEVFREQ reevaluates > based on the available frequencies. > > Thus, a plain srcu_notifier_call_chain() should work for > DEVFREQ/devfreq_update from OPP. > > > > >> return r; > >> } > > > > [...] > > -- > > Regards, > > Nishanth Menon > > > > > Thank you for your comments, Nishanth. > > > Cheers! > - MyungJoo > > _______________________________________________ linux-pm mailing list linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/linux-pm
