diff mbox series

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

Message ID 302c014726dbd9fcde852985528c139d2214a1f2.1611038066.git.viresh.kumar@linaro.org
State Superseded
Headers show
Series opp: Prepare for ->set_opp() helper to work without regulators | expand

Commit Message

Viresh Kumar Jan. 19, 2021, 6:35 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>

---
Dmitry,

I haven't tested this patch, can you please help with that ?

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

-- 
2.25.0.rc1.19.g042ed3e048af

Comments

Dmitry Osipenko Jan. 19, 2021, 5:16 p.m. UTC | #1 
19.01.2021 09:35, Viresh Kumar пишет:
> +	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);


Why do we need all these locks in this patch?

The OPP API isn't thread-safe, these locks won't make the API
thread-safe. At least both sod_supplies and set_opp() pointers should be
set and unset under the lock.

If you're about to make OPP thread-safe, then I suggest to do it
separately from this change. Otherwise this patch looks good to me.
Dmitry Osipenko Jan. 19, 2021, 5:18 p.m. UTC | #2 
19.01.2021 09:35, Viresh Kumar пишет:
> 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>

> ---

> Dmitry,

> 

> I haven't tested this patch, can you please help with that ?

> 

>  drivers/opp/core.c | 81 ++++++++++++++++++++++++----------------------

>  drivers/opp/opp.h  |  2 ++

>  2 files changed, 45 insertions(+), 38 deletions(-)


Works good, thank you. It also almost looks good to me, please see my
other reply regarding the locks.

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


Now the set_opp() needs to be taught about the sod_supplies in order to
actually make it to work without the regulators, I'll make a patch on
top of this change.
Viresh Kumar Jan. 20, 2021, 7:39 a.m. UTC | #3 
On 19-01-21, 20:16, Dmitry Osipenko wrote:
> 19.01.2021 09:35, Viresh Kumar пишет:

> > +	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);

> 

> Why do we need all these locks in this patch?


In case dev_pm_opp_set_regulators() and
dev_pm_opp_register_set_opp_helper() get called at the same time.
Which can actually happen, though is a corner case.

> The OPP API isn't thread-safe, these locks won't make the API

> thread-safe.


I am not sure what you mean by that, can you please explain ?

> At least both sod_supplies and set_opp() pointers should be

> set and unset under the lock.


The ->set_opp pointer isn't getting used for a comparison and so
putting that inside a lock won't get us anything. We are only using
set_opp_data and sod_supplies for comparison at both the places and so
they need to be updated within the lock.

-- 
viresh
Viresh Kumar Jan. 20, 2021, 8:08 a.m. UTC | #4 
On 19-01-21, 12:05, Viresh Kumar wrote:
> 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>

> ---

> Dmitry,

> 

> I haven't tested this patch, can you please help with that ?

> 

>  drivers/opp/core.c | 81 ++++++++++++++++++++++++----------------------

>  drivers/opp/opp.h  |  2 ++

>  2 files changed, 45 insertions(+), 38 deletions(-)


Pushed with this diff.

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 1ed673334565..1dc5ca3f6d26 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1939,7 +1939,6 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
                                            unsigned int count)
 {
        struct dev_pm_opp_supply *supplies;
-       struct dev_pm_set_opp_data *data;
        struct opp_table *opp_table;
        struct regulator *reg;
        int ret, i;
@@ -1990,9 +1989,8 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
        mutex_lock(&opp_table->lock);
        opp_table->sod_supplies = supplies;
        if (opp_table->set_opp_data) {
-               data = opp_table->set_opp_data;
-               data->old_opp.supplies = supplies;
-               data->new_opp.supplies = supplies + count;
+               opp_table->set_opp_data->old_opp.supplies = supplies;
+               opp_table->set_opp_data->new_opp.supplies = supplies + count;
        }
        mutex_unlock(&opp_table->lock);
 
@@ -2038,7 +2036,7 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)
                regulator_put(opp_table->regulators[i]);
 
        mutex_lock(&opp_table->lock);
-       if (opp_table->sod_supplies) {
+       if (opp_table->set_opp_data) {
                opp_table->set_opp_data->old_opp.supplies = NULL;
                opp_table->set_opp_data->new_opp.supplies = NULL;
        }

-- 
viresh
Dmitry Osipenko Jan. 20, 2021, 2:50 p.m. UTC | #5 
20.01.2021 10:39, Viresh Kumar пишет:
> On 19-01-21, 20:16, Dmitry Osipenko wrote:

>> 19.01.2021 09:35, Viresh Kumar пишет:

>>> +	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);

>>

>> Why do we need all these locks in this patch?

> 

> In case dev_pm_opp_set_regulators() and

> dev_pm_opp_register_set_opp_helper() get called at the same time.

> Which can actually happen, though is a corner case.

> 

>> The OPP API isn't thread-safe, these locks won't make the API

>> thread-safe.

> 

> I am not sure what you mean by that, can you please explain ?

> 

>> At least both sod_supplies and set_opp() pointers should be

>> set and unset under the lock.

> 

> The ->set_opp pointer isn't getting used for a comparison and so

> putting that inside a lock won't get us anything. We are only using

> set_opp_data and sod_supplies for comparison at both the places and so

> they need to be updated within the lock.

> 

If OPP API was meant to be thread-safe, then the
dev_pm_opp_unregister_set_opp_helper() should unset the
opp_table->set_opp_data under the lock since it races with
dev_pm_opp_set_regulators().

Secondly, functions like dev_pm_opp_set_rate() don't have any locks at all.

It should be better not to add "random" locks into the code because it
only creates an illusion for an oblivious API user that OPP API cares
about thread safety, IMO.

Making OPP API thread-safe will take some effort and a careful review of
every lock will be needed.
Viresh Kumar Jan. 21, 2021, 11:25 a.m. UTC | #6 
On 20-01-21, 17:50, Dmitry Osipenko wrote:
> If OPP API was meant to be thread-safe, then the

> dev_pm_opp_unregister_set_opp_helper() should unset the

> opp_table->set_opp_data under the lock since it races with

> dev_pm_opp_set_regulators().


Right, I will fix that.

> Secondly, functions like dev_pm_opp_set_rate() don't have any locks at all.


It was on purpose. It is expected that this routine specially will
only have a single caller and calls will be in sequence. This gets
called a lot and we wanted to make it as much efficient as possible.

> It should be better not to add "random" locks into the code because it

> only creates an illusion for an oblivious API user that OPP API cares

> about thread safety, IMO.

> 

> Making OPP API thread-safe will take some effort and a careful review of

> every lock will be needed.


I agree, we have kept some part out of the lock intentionally, but
every other thing which can happen in parallel is well protected.
There maybe bugs, which I am not aware of.

Another reason you see less locks is because of the way I have used
the kref thing here. That allows us to take locks for very small
section of code and not big routines.

-- 
viresh
Viresh Kumar Jan. 21, 2021, 11:28 a.m. UTC | #7 
On 20-01-21, 13:38, Viresh Kumar wrote:
> On 19-01-21, 12:05, Viresh Kumar wrote:

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

> > ---

> > Dmitry,

> > 

> > I haven't tested this patch, can you please help with that ?

> > 

> >  drivers/opp/core.c | 81 ++++++++++++++++++++++++----------------------

> >  drivers/opp/opp.h  |  2 ++

> >  2 files changed, 45 insertions(+), 38 deletions(-)

> 

> Pushed with this diff.

> 

> diff --git a/drivers/opp/core.c b/drivers/opp/core.c

> index 1ed673334565..1dc5ca3f6d26 100644

> --- a/drivers/opp/core.c

> +++ b/drivers/opp/core.c

> @@ -1939,7 +1939,6 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,

>                                             unsigned int count)

>  {

>         struct dev_pm_opp_supply *supplies;

> -       struct dev_pm_set_opp_data *data;

>         struct opp_table *opp_table;

>         struct regulator *reg;

>         int ret, i;

> @@ -1990,9 +1989,8 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,

>         mutex_lock(&opp_table->lock);

>         opp_table->sod_supplies = supplies;

>         if (opp_table->set_opp_data) {

> -               data = opp_table->set_opp_data;

> -               data->old_opp.supplies = supplies;

> -               data->new_opp.supplies = supplies + count;

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

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

>         }

>         mutex_unlock(&opp_table->lock);

>  

> @@ -2038,7 +2036,7 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)

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

>  

>         mutex_lock(&opp_table->lock);

> -       if (opp_table->sod_supplies) {

> +       if (opp_table->set_opp_data) {

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

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

>         }


And some more as pointed out by Dmitry (I will resend it again
properly).

t a/drivers/opp/core.c b/drivers/opp/core.c
index d8ca15d96ce9..747bdc4338f3 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2118,8 +2118,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);

-- 
viresh
diff mbox series

Patch

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index b1a2296f3065..f80b6f1ca108 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,8 @@  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 dev_pm_set_opp_data *data;
 	struct opp_table *opp_table;
 	struct regulator *reg;
 	int ret, i;
@@ -1906,10 +1876,20 @@  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) {
+		data = opp_table->set_opp_data;
+		data->old_opp.supplies = supplies;
+		data->new_opp.supplies = supplies + count;
+	}
+	mutex_unlock(&opp_table->lock);
 
 	return opp_table;
 
@@ -1952,9 +1932,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->sod_supplies) {
+		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 +2033,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 +2050,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 +2088,8 @@  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;
+	kfree(opp_table->set_opp_data);
+	opp_table->set_opp_data = NULL;
 	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