[02/12] PM / OPP: Add 'struct kref' to OPP table

Message ID 3f23949c92492ea74a7a55cd04bcd41a37592ed5.1481106919.git.viresh.kumar@linaro.org
State New
Headers show

Commit Message

Viresh Kumar Dec. 7, 2016, 10:37 a.m.
Add kref to struct opp_table for easier accounting of the OPP table.

Note that the new routine dev_pm_opp_get_opp_table() takes the reference
from under the opp_table_lock, which guarantees that the OPP table
doesn't get freed unless dev_pm_opp_put_opp_table() is called for the
OPP table.

Two separate release mechanisms are added: locked and unlocked. In
unlocked version the routines aren't required to take/drop
opp_table_lock as the callers have already done that. This is required
to avoid breaking git bisect, otherwise we may get lockdeps between
commits. Once all the users of OPP table are updated the unlocked
version shall be removed.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

---
 drivers/base/power/opp/core.c | 51 +++++++++++++++++++++++++++++++++++++++++--
 drivers/base/power/opp/opp.h  |  3 +++
 include/linux/pm_opp.h        | 10 +++++++++
 3 files changed, 62 insertions(+), 2 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

Comments

Stephen Boyd Jan. 9, 2017, 11:36 p.m. | #1
On 12/07, Viresh Kumar wrote:
> @@ -894,8 +895,36 @@ static void _kfree_device_rcu(struct rcu_head *head)

>  	kfree_rcu(opp_table, rcu_head);

>  }

>  

> -static void _free_opp_table(struct opp_table *opp_table)

> +void _get_opp_table_kref(struct opp_table *opp_table)

>  {

> +	kref_get(&opp_table->kref);

> +}

> +

> +struct opp_table *dev_pm_opp_get_opp_table(struct device *dev)

> +{

> +	struct opp_table *opp_table;

> +

> +	/* Hold our table modification lock here */

> +	mutex_lock(&opp_table_lock);

> +

> +	opp_table = _find_opp_table(dev);

> +	if (!IS_ERR(opp_table)) {

> +		_get_opp_table_kref(opp_table);


It seems odd to have _get_opp_table_kref() take a pointer to
increment a kref on. It would be better to have _find_opp_table()
return the pointer with the reference already taken so that we
don't have to update callers with reference grabbing calls.
Typically if a function returns a reference counted pointer the
reference counting has already been done.

> +		goto unlock;

> +	}

> +

> +	opp_table = _allocate_opp_table(dev);

> +

> +unlock:

> +	mutex_unlock(&opp_table_lock);

> +

> +	return opp_table;

> +}

> +EXPORT_SYMBOL_GPL(dev_pm_opp_get_opp_table);

> +

> +static void _opp_table_kref_release_unlocked(struct kref *kref)

> +{

> +	struct opp_table *opp_table = container_of(kref, struct opp_table, kref);

>  	struct opp_device *opp_dev;

>  

>  	/* Release clk */


-- 
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
Viresh Kumar Jan. 10, 2017, 4:23 a.m. | #2
On 09-01-17, 15:36, Stephen Boyd wrote:
> On 12/07, Viresh Kumar wrote:

> > @@ -894,8 +895,36 @@ static void _kfree_device_rcu(struct rcu_head *head)

> >  	kfree_rcu(opp_table, rcu_head);

> >  }

> >  

> > -static void _free_opp_table(struct opp_table *opp_table)

> > +void _get_opp_table_kref(struct opp_table *opp_table)

> >  {

> > +	kref_get(&opp_table->kref);

> > +}

> > +

> > +struct opp_table *dev_pm_opp_get_opp_table(struct device *dev)

> > +{

> > +	struct opp_table *opp_table;

> > +

> > +	/* Hold our table modification lock here */

> > +	mutex_lock(&opp_table_lock);

> > +

> > +	opp_table = _find_opp_table(dev);

> > +	if (!IS_ERR(opp_table)) {

> > +		_get_opp_table_kref(opp_table);

> 

> It seems odd to have _get_opp_table_kref() take a pointer to

> increment a kref on.


This function is provided for better readability and passing opp_table to it is
the only option I had :)

> It would be better to have _find_opp_table()

> return the pointer with the reference already taken so that we

> don't have to update callers with reference grabbing calls.

> Typically if a function returns a reference counted pointer the

> reference counting has already been done.


Absolutely, but that happens with later patches in the series. I couldn't have
done it now, as something or the other would have broken.

-- 
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
Stephen Boyd Jan. 13, 2017, 8:54 a.m. | #3
On 01/10, Viresh Kumar wrote:
> On 09-01-17, 15:36, Stephen Boyd wrote:

> 

> > It would be better to have _find_opp_table()

> > return the pointer with the reference already taken so that we

> > don't have to update callers with reference grabbing calls.

> > Typically if a function returns a reference counted pointer the

> > reference counting has already been done.

> 

> Absolutely, but that happens with later patches in the series. I couldn't have

> done it now, as something or the other would have broken.

> 


Ok, if things get better later in the series then you can have my

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>


-- 
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

Patch

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 212b7dbecae2..0d9bc89de41a 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -855,6 +855,7 @@  static struct opp_table *_allocate_opp_table(struct device *dev)
 	srcu_init_notifier_head(&opp_table->srcu_head);
 	INIT_LIST_HEAD(&opp_table->opp_list);
 	mutex_init(&opp_table->lock);
+	kref_init(&opp_table->kref);
 
 	/* Secure the device table modification */
 	list_add_rcu(&opp_table->node, &opp_tables);
@@ -894,8 +895,36 @@  static void _kfree_device_rcu(struct rcu_head *head)
 	kfree_rcu(opp_table, rcu_head);
 }
 
-static void _free_opp_table(struct opp_table *opp_table)
+void _get_opp_table_kref(struct opp_table *opp_table)
 {
+	kref_get(&opp_table->kref);
+}
+
+struct opp_table *dev_pm_opp_get_opp_table(struct device *dev)
+{
+	struct opp_table *opp_table;
+
+	/* Hold our table modification lock here */
+	mutex_lock(&opp_table_lock);
+
+	opp_table = _find_opp_table(dev);
+	if (!IS_ERR(opp_table)) {
+		_get_opp_table_kref(opp_table);
+		goto unlock;
+	}
+
+	opp_table = _allocate_opp_table(dev);
+
+unlock:
+	mutex_unlock(&opp_table_lock);
+
+	return opp_table;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_get_opp_table);
+
+static void _opp_table_kref_release_unlocked(struct kref *kref)
+{
+	struct opp_table *opp_table = container_of(kref, struct opp_table, kref);
 	struct opp_device *opp_dev;
 
 	/* Release clk */
@@ -916,6 +945,24 @@  static void _free_opp_table(struct opp_table *opp_table)
 		  _kfree_device_rcu);
 }
 
+static void dev_pm_opp_put_opp_table_unlocked(struct opp_table *opp_table)
+{
+	kref_put(&opp_table->kref, _opp_table_kref_release_unlocked);
+}
+
+static void _opp_table_kref_release(struct kref *kref)
+{
+	_opp_table_kref_release_unlocked(kref);
+	mutex_unlock(&opp_table_lock);
+}
+
+void dev_pm_opp_put_opp_table(struct opp_table *opp_table)
+{
+	kref_put_mutex(&opp_table->kref, _opp_table_kref_release,
+		       &opp_table_lock);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_put_opp_table);
+
 /**
  * _remove_opp_table() - Removes a OPP table
  * @opp_table: OPP table to be removed.
@@ -939,7 +986,7 @@  static void _remove_opp_table(struct opp_table *opp_table)
 	if (opp_table->set_opp)
 		return;
 
-	_free_opp_table(opp_table);
+	dev_pm_opp_put_opp_table_unlocked(opp_table);
 }
 
 void _opp_free(struct dev_pm_opp *opp)
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index 81eb6cee7295..596f361fbe70 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -131,6 +131,7 @@  enum opp_table_access {
  * @rcu_head:	RCU callback head used for deferred freeing
  * @dev_list:	list of devices that share these OPPs
  * @opp_list:	table of opps
+ * @kref:	for reference count of the table.
  * @lock:	mutex protecting the opp_list.
  * @np:		struct device_node pointer for opp's DT node.
  * @clock_latency_ns_max: Max clock latency in nanoseconds.
@@ -164,6 +165,7 @@  struct opp_table {
 	struct rcu_head rcu_head;
 	struct list_head dev_list;
 	struct list_head opp_list;
+	struct kref kref;
 	struct mutex lock;
 
 	struct device_node *np;
@@ -192,6 +194,7 @@  struct opp_table {
 };
 
 /* Routines internal to opp core */
+void _get_opp_table_kref(struct opp_table *opp_table);
 struct opp_table *_find_opp_table(struct device *dev);
 struct opp_table *_add_opp_table(struct device *dev);
 struct opp_device *_add_opp_dev(const struct device *dev, struct opp_table *opp_table);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 66a02deeb03f..d867c6b25f9a 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -78,6 +78,9 @@  struct dev_pm_set_opp_data {
 
 #if defined(CONFIG_PM_OPP)
 
+struct opp_table *dev_pm_opp_get_opp_table(struct device *dev);
+void dev_pm_opp_put_opp_table(struct opp_table *opp_table);
+
 unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp);
 
 unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp);
@@ -126,6 +129,13 @@  int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask)
 void dev_pm_opp_remove_table(struct device *dev);
 void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask);
 #else
+static inline struct opp_table *dev_pm_opp_get_opp_table(struct device *dev)
+{
+	return ERR_PTR(-ENOTSUPP);
+}
+
+static inline void dev_pm_opp_put_opp_table(struct opp_table *opp_table) {}
+
 static inline unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
 {
 	return 0;