On 17 December 2014 at 04:39, Dmitry Torokhov <[email protected]> wrote: > A lot of callers are missing the fact that dev_pm_opp_get_opp_count > needs to be called under RCU lock. Given that RCU locks can safely be > nested, instead of providing *_locked() API, let's take RCU lock inside
Hmm, I asked for a *_locked() API because many users of dev_pm_opp_get_opp_count() are already calling it from rcu read side critical sections. Now, there are two questions: - Can rcu-read side critical sections be nested ? Yes, this is what the comment over rcu_read_lock() says * RCU read-side critical sections may be nested. Any deferred actions * will be deferred until the outermost RCU read-side critical section * completes. - Would it be better to drop these double rcu_read_locks() ? i.e. either get a *_locked() API or fix the callers of dev_pm_opp_get_opp_count(). @Paul: What do you say ? > dev_pm_opp_get_opp_count() and leave callers as is. > > Signed-off-by: Dmitry Torokhov <[email protected]> > --- > drivers/base/power/opp.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c > index 413c7fe..ee5eca2 100644 > --- a/drivers/base/power/opp.c > +++ b/drivers/base/power/opp.c > @@ -216,9 +216,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_get_freq); > * This function returns the number of available opps if there are any, > * else returns 0 if none or the corresponding error value. > * > - * Locking: This function must be called under rcu_read_lock(). This function > - * internally references two RCU protected structures: device_opp and opp > which > - * are safe as long as we are under a common RCU locked section. > + * Locking: This function takes rcu_read_lock(). > */ > int dev_pm_opp_get_opp_count(struct device *dev) > { > @@ -226,13 +224,14 @@ int dev_pm_opp_get_opp_count(struct device *dev) > struct dev_pm_opp *temp_opp; > int count = 0; > > - opp_rcu_lockdep_assert(); > + rcu_read_lock(); > > dev_opp = find_device_opp(dev); > if (IS_ERR(dev_opp)) { > - int r = PTR_ERR(dev_opp); > - dev_err(dev, "%s: device OPP not found (%d)\n", __func__, r); > - return r; > + count = PTR_ERR(dev_opp); > + dev_err(dev, "%s: device OPP not found (%d)\n", > + __func__, count); > + goto out_unlock; > } > > list_for_each_entry_rcu(temp_opp, &dev_opp->opp_list, node) { > @@ -240,6 +239,8 @@ int dev_pm_opp_get_opp_count(struct device *dev) > count++; > } > > +out_unlock: > + rcu_read_unlock(); > return count; > } Looked fine otherwise: Acked-by: Viresh Kumar <[email protected]> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
