| Message ID | 302c014726dbd9fcde852985528c139d2214a1f2.1611038066.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | opp: Prepare for ->set_opp() helper to work without regulators | expand |
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.
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.
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.
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; }
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.
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.
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);
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
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(-)