Messages in this thread | | | From | Ulf Hansson <> | Date | Wed, 16 Feb 2022 17:24:30 +0100 | Subject | Re: [PATCH v1 1/7] powercap/dtpm: Change locking scheme |
| |
On Sun, 30 Jan 2022 at 22:02, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > The different functions are all called through the > dtpm_create_hierarchy() which handle the mutex. The different > functions are used in this context, consequently with the lock always > held. > > Remove all locks taken in the function and add the lock in the > hierarchy creation function.
I have to admit that the locking scheme looks quite odd in dtpm.c. It's not really clear to me what the global "dtpm_lock" is there to protect in the first place (except the global "pct" variable). Nevertheless, this looks like a step in the right direction.
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Kind regards Uffe
> --- > drivers/powercap/dtpm.c | 95 ++++++++++++----------------------------- > 1 file changed, 27 insertions(+), 68 deletions(-) > > diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c > index 414826a1509b..0b0121c37a1b 100644 > --- a/drivers/powercap/dtpm.c > +++ b/drivers/powercap/dtpm.c > @@ -51,9 +51,7 @@ static int get_max_power_range_uw(struct powercap_zone *pcz, u64 *max_power_uw) > { > struct dtpm *dtpm = to_dtpm(pcz); > > - mutex_lock(&dtpm_lock); > *max_power_uw = dtpm->power_max - dtpm->power_min; > - mutex_unlock(&dtpm_lock); > > return 0; > } > @@ -83,14 +81,7 @@ static int __get_power_uw(struct dtpm *dtpm, u64 *power_uw) > > static int get_power_uw(struct powercap_zone *pcz, u64 *power_uw) > { > - struct dtpm *dtpm = to_dtpm(pcz); > - int ret; > - > - mutex_lock(&dtpm_lock); > - ret = __get_power_uw(dtpm, power_uw); > - mutex_unlock(&dtpm_lock); > - > - return ret; > + return __get_power_uw(to_dtpm(pcz), power_uw); > } > > static void __dtpm_rebalance_weight(struct dtpm *dtpm) > @@ -133,7 +124,16 @@ static void __dtpm_add_power(struct dtpm *dtpm) > } > } > > -static int __dtpm_update_power(struct dtpm *dtpm) > +/** > + * dtpm_update_power - Update the power on the dtpm > + * @dtpm: a pointer to a dtpm structure to update > + * > + * Function to update the power values of the dtpm node specified in > + * parameter. These new values will be propagated to the tree. > + * > + * Return: zero on success, -EINVAL if the values are inconsistent > + */ > +int dtpm_update_power(struct dtpm *dtpm) > { > int ret; > > @@ -155,26 +155,6 @@ static int __dtpm_update_power(struct dtpm *dtpm) > return ret; > } > > -/** > - * dtpm_update_power - Update the power on the dtpm > - * @dtpm: a pointer to a dtpm structure to update > - * > - * Function to update the power values of the dtpm node specified in > - * parameter. These new values will be propagated to the tree. > - * > - * Return: zero on success, -EINVAL if the values are inconsistent > - */ > -int dtpm_update_power(struct dtpm *dtpm) > -{ > - int ret; > - > - mutex_lock(&dtpm_lock); > - ret = __dtpm_update_power(dtpm); > - mutex_unlock(&dtpm_lock); > - > - return ret; > -} > - > /** > * dtpm_release_zone - Cleanup when the node is released > * @pcz: a pointer to a powercap_zone structure > @@ -191,20 +171,14 @@ int dtpm_release_zone(struct powercap_zone *pcz) > struct dtpm *dtpm = to_dtpm(pcz); > struct dtpm *parent = dtpm->parent; > > - mutex_lock(&dtpm_lock); > - > - if (!list_empty(&dtpm->children)) { > - mutex_unlock(&dtpm_lock); > + if (!list_empty(&dtpm->children)) > return -EBUSY; > - } > > if (parent) > list_del(&dtpm->sibling); > > __dtpm_sub_power(dtpm); > > - mutex_unlock(&dtpm_lock); > - > if (dtpm->ops) > dtpm->ops->release(dtpm); > > @@ -216,23 +190,12 @@ int dtpm_release_zone(struct powercap_zone *pcz) > return 0; > } > > -static int __get_power_limit_uw(struct dtpm *dtpm, int cid, u64 *power_limit) > -{ > - *power_limit = dtpm->power_limit; > - return 0; > -} > - > static int get_power_limit_uw(struct powercap_zone *pcz, > int cid, u64 *power_limit) > { > - struct dtpm *dtpm = to_dtpm(pcz); > - int ret; > - > - mutex_lock(&dtpm_lock); > - ret = __get_power_limit_uw(dtpm, cid, power_limit); > - mutex_unlock(&dtpm_lock); > - > - return ret; > + *power_limit = to_dtpm(pcz)->power_limit; > + > + return 0; > } > > /* > @@ -292,7 +255,7 @@ static int __set_power_limit_uw(struct dtpm *dtpm, int cid, u64 power_limit) > > ret = __set_power_limit_uw(child, cid, power); > if (!ret) > - ret = __get_power_limit_uw(child, cid, &power); > + ret = get_power_limit_uw(&child->zone, cid, &power); > > if (ret) > break; > @@ -310,8 +273,6 @@ static int set_power_limit_uw(struct powercap_zone *pcz, > struct dtpm *dtpm = to_dtpm(pcz); > int ret; > > - mutex_lock(&dtpm_lock); > - > /* > * Don't allow values outside of the power range previously > * set when initializing the power numbers. > @@ -323,8 +284,6 @@ static int set_power_limit_uw(struct powercap_zone *pcz, > pr_debug("%s: power limit: %llu uW, power max: %llu uW\n", > dtpm->zone.name, dtpm->power_limit, dtpm->power_max); > > - mutex_unlock(&dtpm_lock); > - > return ret; > } > > @@ -335,11 +294,7 @@ static const char *get_constraint_name(struct powercap_zone *pcz, int cid) > > static int get_max_power_uw(struct powercap_zone *pcz, int id, u64 *max_power) > { > - struct dtpm *dtpm = to_dtpm(pcz); > - > - mutex_lock(&dtpm_lock); > - *max_power = dtpm->power_max; > - mutex_unlock(&dtpm_lock); > + *max_power = to_dtpm(pcz)->power_max; > > return 0; > } > @@ -442,8 +397,6 @@ int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent) > if (IS_ERR(pcz)) > return PTR_ERR(pcz); > > - mutex_lock(&dtpm_lock); > - > if (parent) { > list_add_tail(&dtpm->sibling, &parent->children); > dtpm->parent = parent; > @@ -459,8 +412,6 @@ int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent) > pr_debug("Registered dtpm node '%s' / %llu-%llu uW, \n", > dtpm->zone.name, dtpm->power_min, dtpm->power_max); > > - mutex_unlock(&dtpm_lock); > - > return 0; > } > > @@ -605,8 +556,12 @@ int dtpm_create_hierarchy(struct of_device_id *dtpm_match_table) > struct device_node *np; > int i, ret; > > - if (pct) > - return -EBUSY; > + mutex_lock(&dtpm_lock); > + > + if (pct) { > + ret = -EBUSY; > + goto out_unlock; > + } > > pct = powercap_register_control_type(NULL, "dtpm", NULL); > if (IS_ERR(pct)) { > @@ -648,12 +603,16 @@ int dtpm_create_hierarchy(struct of_device_id *dtpm_match_table) > dtpm_subsys[i]->name, ret); > } > > + mutex_unlock(&dtpm_lock); > + > return 0; > > out_err: > powercap_unregister_control_type(pct); > out_pct: > pct = NULL; > +out_unlock: > + mutex_unlock(&dtpm_lock); > > return ret; > } > -- > 2.25.1 >
| |