| Message ID | 1d5f61440dbfb640c330f77c9090e2ac23482ebc.1481106919.git.viresh.kumar@linaro.org |
|---|---|
| State | Accepted |
| Commit | 7034764a1e4a6edbb60914e89aad8384e3fe5d17 |
| Headers | show |
On 12/07, Viresh Kumar wrote: > Add kref to struct dev_pm_opp for easier accounting of the OPPs. > > Note that the OPPs are freed under the opp_table->lock mutex only. I'm lost. Why add another level of krefs? -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- 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 01/10, Viresh Kumar wrote: > On 09-01-17, 15:44, Stephen Boyd wrote: > > On 12/07, Viresh Kumar wrote: > > > Add kref to struct dev_pm_opp for easier accounting of the OPPs. > > > > > > Note that the OPPs are freed under the opp_table->lock mutex only. > > > > I'm lost. Why add another level of krefs? > > Heh. The earlier krefs were for the OPP table itself, so that it gets freed once > there are no more users of it. > > The kref introduced now is for individual OPPs, so that they don't disappear > while being used and gets freed once all are done. > > Also note that the OPP table will get freed only after all the OPPs are freed, > plus there are no more users left, like platform code which might have set > suppoerted-hw property. > What still doesn't make sense is how an individual OPP could go away without the table that the OPP lives in also going away. If an OPP is going away while a driver has a reference to it, then the driver using that OPP should probably not be using it. TL;DR letting drivers use OPP pointers outside of the OPP core feels racy. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- 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 13-01-17, 00:52, Stephen Boyd wrote: > What still doesn't make sense is how an individual OPP could go > away without the table that the OPP lives in also going away. dev_pm_opp_remove() is one such option, which can remove OPPs individually. Over that, while remove tables we remove all the OPPs one by one. So that really does happen. > If > an OPP is going away while a driver has a reference to it, then > the driver using that OPP should probably not be using it. That is being protected with this patch now and the drivers can use them freely. > TL;DR > letting drivers use OPP pointers outside of the OPP core feels > racy. Hmm, we don't update the OPP a lot after creating it today. But that's a different problem to solve, if we really see a race there. -- viresh -- 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
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index ec833a8f7aa5..12be0f29f2ad 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -949,20 +949,10 @@ static void _kfree_opp_rcu(struct rcu_head *head) kfree_rcu(opp, rcu_head); } -/** - * _opp_remove() - Remove an OPP from a table definition - * @opp_table: points back to the opp_table struct this opp belongs to - * @opp: pointer to the OPP to remove - * - * This function removes an opp definition from the opp table. - * - * Locking: The internal opp_table and opp structures are RCU protected. - * It is assumed that the caller holds required mutex for an RCU updater - * strategy. - */ -static void _opp_remove(struct opp_table *opp_table, struct dev_pm_opp *opp) +static void _opp_kref_release(struct kref *kref) { - mutex_lock(&opp_table->lock); + struct dev_pm_opp *opp = container_of(kref, struct dev_pm_opp, kref); + struct opp_table *opp_table = opp->opp_table; /* * Notify the changes in the availability of the operable @@ -977,6 +967,12 @@ static void _opp_remove(struct opp_table *opp_table, struct dev_pm_opp *opp) dev_pm_opp_put_opp_table(opp_table); } +void dev_pm_opp_put(struct dev_pm_opp *opp) +{ + kref_put_mutex(&opp->kref, _opp_kref_release, &opp->opp_table->lock); +} +EXPORT_SYMBOL_GPL(dev_pm_opp_put); + /** * dev_pm_opp_remove() - Remove an OPP from OPP table * @dev: device for which we do this operation @@ -1020,7 +1016,7 @@ void dev_pm_opp_remove(struct device *dev, unsigned long freq) goto unlock; } - _opp_remove(opp_table, opp); + dev_pm_opp_put(opp); unlock: mutex_unlock(&opp_table_lock); } @@ -1125,6 +1121,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, mutex_unlock(&opp_table->lock); new_opp->opp_table = opp_table; + kref_init(&new_opp->kref); /* Get a reference to the OPP table */ _get_opp_table_kref(opp_table); @@ -1820,7 +1817,7 @@ void _dev_pm_opp_remove_table(struct opp_table *opp_table, struct device *dev, /* Free static OPPs */ list_for_each_entry_safe(opp, tmp, &opp_table->opp_list, node) { if (remove_all || !opp->dynamic) - _opp_remove(opp_table, opp); + dev_pm_opp_put(opp); } } else { _remove_opp_dev(_find_opp_dev(dev, opp_table), opp_table); diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h index 8435a0eb27be..bd929ba6efaf 100644 --- a/drivers/base/power/opp/opp.h +++ b/drivers/base/power/opp/opp.h @@ -16,6 +16,7 @@ #include <linux/device.h> #include <linux/kernel.h> +#include <linux/kref.h> #include <linux/list.h> #include <linux/limits.h> #include <linux/pm_opp.h> @@ -56,6 +57,7 @@ extern struct list_head opp_tables; * are protected by the opp_table_lock for integrity. * IMPORTANT: the opp nodes should be maintained in increasing * order. + * @kref: for reference count of the OPP. * @available: true/false - marks if this OPP as available or not * @dynamic: not-created from static DT entries. * @turbo: true if turbo (boost) OPP @@ -73,6 +75,7 @@ extern struct list_head opp_tables; */ struct dev_pm_opp { struct list_head node; + struct kref kref; bool available; bool dynamic; diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index 99787cbcaab2..731d548657aa 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -102,6 +102,7 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev, struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev, unsigned long *freq); +void dev_pm_opp_put(struct dev_pm_opp *opp); int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt); @@ -193,6 +194,8 @@ static inline struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev, return ERR_PTR(-ENOTSUPP); } +static inline void dev_pm_opp_put(struct dev_pm_opp *opp) {} + static inline int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt) {
Add kref to struct dev_pm_opp for easier accounting of the OPPs. Note that the OPPs are freed under the opp_table->lock mutex only. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/base/power/opp/core.c | 27 ++++++++++++--------------- drivers/base/power/opp/opp.h | 3 +++ include/linux/pm_opp.h | 3 +++ 3 files changed, 18 insertions(+), 15 deletions(-) -- 2.7.1.410.g6faf27b -- 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