| Message ID | 1313663262-15308-2-git-send-email-myungjoo.ham@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Thu, Aug 18, 2011 at 3:27 AM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote: > The patch enables to register notifier_block for an OPP-device in order > to get notified for any changes in the availability of OPPs of the > device. For example, if a new OPP is inserted or enable/disable status > of an OPP is changed, the notifier is executed. > > This enables the usage of opp_add, opp_enable, and opp_disable to > directly take effect with any connected entities such as CPUFREQ or > DEVFREQ. > > Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > > --- > Added at devfreq patch set v6 replacing devfreq_update calls at OPP. > --- > drivers/base/power/opp.c | 28 ++++++++++++++++++++++++++++ > include/linux/opp.h | 12 ++++++++++++ > 2 files changed, 40 insertions(+), 0 deletions(-) > > diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c > index b23de18..39e16c3 100644 > --- a/drivers/base/power/opp.c > +++ b/drivers/base/power/opp.c > @@ -73,6 +73,7 @@ struct opp { > * RCU usage: nodes are not modified in the list of device_opp, > * however addition is possible and is secured by dev_opp_list_lock > * @dev: device pointer > + * @head: notifier head to notify the OPP availability changes. > * @opp_list: list of opps > * > * This is an internal data structure maintaining the link to opps attached to > @@ -83,6 +84,8 @@ struct device_opp { > struct list_head node; > > struct device *dev; > + struct srcu_notifier_head head; > + > struct list_head opp_list; > }; > > @@ -404,6 +407,7 @@ int opp_add(struct device *dev, unsigned long freq, unsigned long u_volt) > } > > dev_opp->dev = dev; > + srcu_init_notifier_head(&dev_opp->head); > INIT_LIST_HEAD(&dev_opp->opp_list); > > /* Secure the device list modification */ > @@ -428,6 +432,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 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; > } > > @@ -504,6 +513,11 @@ static int opp_set_availability(struct device *dev, unsigned long freq, > mutex_unlock(&dev_opp_list_lock); > synchronize_rcu(); I'm not an RCU expert. Is the above synchronize_rcu call still needed with the addition of the srcu_notifier_call_chain below? The newly added src_notifier_call_chain will result in the following call graph: srcu_notifier_call_chain -> __srcu_notifier_call_chain -> srcu_read_lock -> __srcu_read_lock -> srcu_barrier > > + /* Notify the change of the OPP availability */ > + srcu_notifier_call_chain(&dev_opp->head, availability_req ? > + OPP_EVENT_ENABLE : OPP_EVENT_DISABLE, Nitpick: I'm not a fan of conditional statements as func parameters. > + new_opp); > + > /* clean up old opp */ > new_opp = opp; > goto out; > @@ -512,6 +526,7 @@ unlock: > mutex_unlock(&dev_opp_list_lock); > out: > kfree(new_opp); > + Nitpick: unnecessary whitespace addition. > return r; > } > > @@ -643,3 +658,16 @@ void opp_free_cpufreq_table(struct device *dev, > *table = NULL; > } > #endif /* CONFIG_CPU_FREQ */ > + > +/** opp_get_notifier() - find notifier_head of the device with opp > + * @dev: device pointer used to lookup device OPPs. > + */ > +struct srcu_notifier_head *opp_get_notifier(struct device *dev) > +{ > + struct device_opp *dev_opp = find_device_opp(dev); > + > + if (IS_ERR(dev_opp)) > + return ERR_PTR(PTR_ERR(dev_opp)); /* matching type */ > + > + return &dev_opp->head; > +} > diff --git a/include/linux/opp.h b/include/linux/opp.h > index 7020e97..53d4538 100644 > --- a/include/linux/opp.h > +++ b/include/linux/opp.h > @@ -16,9 +16,14 @@ > > #include <linux/err.h> > #include <linux/cpufreq.h> > +#include <linux/notifier.h> > > struct opp; > > +enum opp_event { > + OPP_EVENT_ADD, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE, OPP_EVENT_MAX I don't see OPP_EVENT_MAX used anywhere else in the code. What is it's purpose? Regards, Mike > +}; > + > #if defined(CONFIG_PM_OPP) > > unsigned long opp_get_voltage(struct opp *opp); > @@ -40,6 +45,8 @@ int opp_enable(struct device *dev, unsigned long freq); > > int opp_disable(struct device *dev, unsigned long freq); > > +struct srcu_notifier_head *opp_get_notifier(struct device *dev); > + > #else > static inline unsigned long opp_get_voltage(struct opp *opp) > { > @@ -89,6 +96,11 @@ static inline int opp_disable(struct device *dev, unsigned long freq) > { > return 0; > } > + > +struct srcu_notifier_head *opp_get_notifier(struct device *dev) > +{ > + return ERR_PTR(-EINVAL); > +} > #endif /* CONFIG_PM */ > > #if defined(CONFIG_CPU_FREQ) && defined(CONFIG_PM_OPP) > -- > 1.7.4.1 > >
On Fri, Aug 19, 2011 at 7:06 AM, Turquette, Mike <mturquette@ti.com> wrote: > On Thu, Aug 18, 2011 at 3:27 AM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote: >> >> @@ -504,6 +513,11 @@ static int opp_set_availability(struct device *dev, unsigned long freq, >> mutex_unlock(&dev_opp_list_lock); >> synchronize_rcu(); > > I'm not an RCU expert. Is the above synchronize_rcu call still needed > with the addition of the srcu_notifier_call_chain below? The newly > added src_notifier_call_chain will result in the following call graph: > > srcu_notifier_call_chain > -> __srcu_notifier_call_chain > -> srcu_read_lock > -> __srcu_read_lock > -> srcu_barrier Though didn't read thorough kernel/rcutree_plugin.h, synchronize_rcu() appears to do a lot more than sleepable RCU's srcu_barrier. srcu_barrier() provides "barrier" for compiler at compile-time and synchronize_rcu() provides "barrier" for processes at run-time. For distributed systems (including multi-core systems) in general, I think using srcu_barrier() instead of synchronize_rcu() is very dangerous. > [] > > Nitpick: I'm not a fan of conditional statements as func parameters. > [] > > Nitpick: unnecessary whitespace addition. > Ok, I'll revise them. [] >> >> +enum opp_event { >> + OPP_EVENT_ADD, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE, OPP_EVENT_MAX > > I don't see OPP_EVENT_MAX used anywhere else in the code. What is it's purpose? > It was placed to mark "the end of the list" for opp notifier users. However, it seems useless. I'll remove it. I'll resubmit this part of the patchset. Thanks. MyungJoo
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index b23de18..39e16c3 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -73,6 +73,7 @@ struct opp { * RCU usage: nodes are not modified in the list of device_opp, * however addition is possible and is secured by dev_opp_list_lock * @dev: device pointer + * @head: notifier head to notify the OPP availability changes. * @opp_list: list of opps * * This is an internal data structure maintaining the link to opps attached to @@ -83,6 +84,8 @@ struct device_opp { struct list_head node; struct device *dev; + struct srcu_notifier_head head; + struct list_head opp_list; }; @@ -404,6 +407,7 @@ int opp_add(struct device *dev, unsigned long freq, unsigned long u_volt) } dev_opp->dev = dev; + srcu_init_notifier_head(&dev_opp->head); INIT_LIST_HEAD(&dev_opp->opp_list); /* Secure the device list modification */ @@ -428,6 +432,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 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; } @@ -504,6 +513,11 @@ static int opp_set_availability(struct device *dev, unsigned long freq, mutex_unlock(&dev_opp_list_lock); synchronize_rcu(); + /* Notify the change of the OPP availability */ + srcu_notifier_call_chain(&dev_opp->head, availability_req ? + OPP_EVENT_ENABLE : OPP_EVENT_DISABLE, + new_opp); + /* clean up old opp */ new_opp = opp; goto out; @@ -512,6 +526,7 @@ unlock: mutex_unlock(&dev_opp_list_lock); out: kfree(new_opp); + return r; } @@ -643,3 +658,16 @@ void opp_free_cpufreq_table(struct device *dev, *table = NULL; } #endif /* CONFIG_CPU_FREQ */ + +/** opp_get_notifier() - find notifier_head of the device with opp + * @dev: device pointer used to lookup device OPPs. + */ +struct srcu_notifier_head *opp_get_notifier(struct device *dev) +{ + struct device_opp *dev_opp = find_device_opp(dev); + + if (IS_ERR(dev_opp)) + return ERR_PTR(PTR_ERR(dev_opp)); /* matching type */ + + return &dev_opp->head; +} diff --git a/include/linux/opp.h b/include/linux/opp.h index 7020e97..53d4538 100644 --- a/include/linux/opp.h +++ b/include/linux/opp.h @@ -16,9 +16,14 @@ #include <linux/err.h> #include <linux/cpufreq.h> +#include <linux/notifier.h> struct opp; +enum opp_event { + OPP_EVENT_ADD, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE, OPP_EVENT_MAX +}; + #if defined(CONFIG_PM_OPP) unsigned long opp_get_voltage(struct opp *opp); @@ -40,6 +45,8 @@ int opp_enable(struct device *dev, unsigned long freq); int opp_disable(struct device *dev, unsigned long freq); +struct srcu_notifier_head *opp_get_notifier(struct device *dev); + #else static inline unsigned long opp_get_voltage(struct opp *opp) { @@ -89,6 +96,11 @@ static inline int opp_disable(struct device *dev, unsigned long freq) { return 0; } + +struct srcu_notifier_head *opp_get_notifier(struct device *dev) +{ + return ERR_PTR(-EINVAL); +} #endif /* CONFIG_PM */ #if defined(CONFIG_CPU_FREQ) && defined(CONFIG_PM_OPP)