diff mbox series

[V2] opp: Prepare for ->set_opp() helper to work without regulators

Message ID d60bc79ed2ac3fd2f2911449ba994b4d55bd1bfd.1611228541.git.viresh.kumar@linaro.org
State Accepted
Commit 38bb34393804b79eff647bdf96762db5efce392c
Headers show
Series [V2] opp: Prepare for ->set_opp() helper to work without regulators | expand

Commit Message

Viresh Kumar Jan. 21, 2021, 11:30 a.m. UTC 
Until now the ->set_opp() helper (i.e. special implementation for
setting the OPPs for platforms) was implemented only to take care of
multiple regulators case, but going forward we would need that for other
use cases as well.

This patch prepares for that by allocating the regulator specific part
from dev_pm_opp_set_regulators() and the opp helper part from
dev_pm_opp_register_set_opp_helper().

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

Tested-by: Dmitry Osipenko <digetx@gmail.com>

---
V2:
- Fixed a bug where we accessed the wrong pointer
- Add locks in dev_pm_opp_unregister_set_opp_helper()

 drivers/opp/core.c | 83 +++++++++++++++++++++++++---------------------
 drivers/opp/opp.h  |  2 ++
 2 files changed, 47 insertions(+), 38 deletions(-)

-- 
2.25.0.rc1.19.g042ed3e048af

Comments

Dmitry Osipenko Jan. 21, 2021, 9:02 p.m. UTC | #1 
21.01.2021 14:30, Viresh Kumar пишет:
> @@ -1952,9 +1930,16 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)

>  	for (i = opp_table->regulator_count - 1; i >= 0; i--)

>  		regulator_put(opp_table->regulators[i]);

>  

> -	_free_set_opp_data(opp_table);

> +	mutex_lock(&opp_table->lock);

> +	if (opp_table->set_opp_data) {

> +		opp_table->set_opp_data->old_opp.supplies = NULL;

> +		opp_table->set_opp_data->new_opp.supplies = NULL;

> +	}

> +	mutex_unlock(&opp_table->lock);

>  

> +	kfree(opp_table->sod_supplies);

>  	kfree(opp_table->regulators);

> +	opp_table->sod_supplies = NULL;

>  	opp_table->regulators = NULL;

>  	opp_table->regulator_count = -1;


The sod_supplies should be unset under the lock.
Viresh Kumar Jan. 22, 2021, 3:05 a.m. UTC | #2 
On 22-01-21, 00:02, Dmitry Osipenko wrote:
> 21.01.2021 14:30, Viresh Kumar пишет:

> > @@ -1952,9 +1930,16 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)

> >  	for (i = opp_table->regulator_count - 1; i >= 0; i--)

> >  		regulator_put(opp_table->regulators[i]);

> >  

> > -	_free_set_opp_data(opp_table);

> > +	mutex_lock(&opp_table->lock);

> > +	if (opp_table->set_opp_data) {

> > +		opp_table->set_opp_data->old_opp.supplies = NULL;

> > +		opp_table->set_opp_data->new_opp.supplies = NULL;

> > +	}

> > +	mutex_unlock(&opp_table->lock);

> >  

> > +	kfree(opp_table->sod_supplies);

> >  	kfree(opp_table->regulators);

> > +	opp_table->sod_supplies = NULL;

> >  	opp_table->regulators = NULL;

> >  	opp_table->regulator_count = -1;

> 

> The sod_supplies should be unset under the lock.


Fixed, thanks.

-- 
viresh
diff mbox series

Patch

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 846390c9434a..dff939ed5118 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1817,38 +1817,6 @@  void dev_pm_opp_put_prop_name(struct opp_table *opp_table)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
 
-static int _allocate_set_opp_data(struct opp_table *opp_table)
-{
-	struct dev_pm_set_opp_data *data;
-	int len, count = opp_table->regulator_count;
-
-	if (WARN_ON(!opp_table->regulators))
-		return -EINVAL;
-
-	/* space for set_opp_data */
-	len = sizeof(*data);
-
-	/* space for old_opp.supplies and new_opp.supplies */
-	len += 2 * sizeof(struct dev_pm_opp_supply) * count;
-
-	data = kzalloc(len, GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	data->old_opp.supplies = (void *)(data + 1);
-	data->new_opp.supplies = data->old_opp.supplies + count;
-
-	opp_table->set_opp_data = data;
-
-	return 0;
-}
-
-static void _free_set_opp_data(struct opp_table *opp_table)
-{
-	kfree(opp_table->set_opp_data);
-	opp_table->set_opp_data = NULL;
-}
-
 /**
  * dev_pm_opp_set_regulators() - Set regulator names for the device
  * @dev: Device for which regulator name is being set.
@@ -1865,6 +1833,7 @@  struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
 					    const char * const names[],
 					    unsigned int count)
 {
+	struct dev_pm_opp_supply *supplies;
 	struct opp_table *opp_table;
 	struct regulator *reg;
 	int ret, i;
@@ -1906,10 +1875,19 @@  struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
 
 	opp_table->regulator_count = count;
 
-	/* Allocate block only once to pass to set_opp() routines */
-	ret = _allocate_set_opp_data(opp_table);
-	if (ret)
+	supplies = kmalloc_array(count * 2, sizeof(*supplies), GFP_KERNEL);
+	if (!supplies) {
+		ret = -ENOMEM;
 		goto free_regulators;
+	}
+
+	mutex_lock(&opp_table->lock);
+	opp_table->sod_supplies = supplies;
+	if (opp_table->set_opp_data) {
+		opp_table->set_opp_data->old_opp.supplies = supplies;
+		opp_table->set_opp_data->new_opp.supplies = supplies + count;
+	}
+	mutex_unlock(&opp_table->lock);
 
 	return opp_table;
 
@@ -1952,9 +1930,16 @@  void dev_pm_opp_put_regulators(struct opp_table *opp_table)
 	for (i = opp_table->regulator_count - 1; i >= 0; i--)
 		regulator_put(opp_table->regulators[i]);
 
-	_free_set_opp_data(opp_table);
+	mutex_lock(&opp_table->lock);
+	if (opp_table->set_opp_data) {
+		opp_table->set_opp_data->old_opp.supplies = NULL;
+		opp_table->set_opp_data->new_opp.supplies = NULL;
+	}
+	mutex_unlock(&opp_table->lock);
 
+	kfree(opp_table->sod_supplies);
 	kfree(opp_table->regulators);
+	opp_table->sod_supplies = NULL;
 	opp_table->regulators = NULL;
 	opp_table->regulator_count = -1;
 
@@ -2046,6 +2031,7 @@  EXPORT_SYMBOL_GPL(dev_pm_opp_put_clkname);
 struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev,
 			int (*set_opp)(struct dev_pm_set_opp_data *data))
 {
+	struct dev_pm_set_opp_data *data;
 	struct opp_table *opp_table;
 
 	if (!set_opp)
@@ -2062,8 +2048,23 @@  struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev,
 	}
 
 	/* Another CPU that shares the OPP table has set the helper ? */
-	if (!opp_table->set_opp)
-		opp_table->set_opp = set_opp;
+	if (opp_table->set_opp)
+		return opp_table;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return ERR_PTR(-ENOMEM);
+
+	mutex_lock(&opp_table->lock);
+	opp_table->set_opp_data = data;
+	if (opp_table->sod_supplies) {
+		data->old_opp.supplies = opp_table->sod_supplies;
+		data->new_opp.supplies = opp_table->sod_supplies +
+					 opp_table->regulator_count;
+	}
+	mutex_unlock(&opp_table->lock);
+
+	opp_table->set_opp = set_opp;
 
 	return opp_table;
 }
@@ -2085,6 +2086,12 @@  void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table)
 	WARN_ON(!list_empty(&opp_table->opp_list));
 
 	opp_table->set_opp = NULL;
+
+	mutex_lock(&opp_table->lock);
+	kfree(opp_table->set_opp_data);
+	opp_table->set_opp_data = NULL;
+	mutex_unlock(&opp_table->lock);
+
 	dev_pm_opp_put_opp_table(opp_table);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_unregister_set_opp_helper);
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 4ced7ffa8158..4408cfcb0f31 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -155,6 +155,7 @@  enum opp_table_access {
  * @genpd_performance_state: Device's power domain support performance state.
  * @is_genpd: Marks if the OPP table belongs to a genpd.
  * @set_opp: Platform specific set_opp callback
+ * @sod_supplies: Set opp data supplies
  * @set_opp_data: Data to be passed to set_opp callback
  * @dentry:	debugfs dentry pointer of the real device directory (not links).
  * @dentry_name: Name of the real dentry.
@@ -202,6 +203,7 @@  struct opp_table {
 	bool is_genpd;
 
 	int (*set_opp)(struct dev_pm_set_opp_data *data);
+	struct dev_pm_opp_supply *sod_supplies;
 	struct dev_pm_set_opp_data *set_opp_data;
 
 #ifdef CONFIG_DEBUG_FS