[PATCH v1 1/7] powercap/dtpm: Change locking scheme

[PATCH v1 1/7] powercap/dtpm: Change locking scheme

[Daniel Lezcano]

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.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 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

[PATCH v1 2/7] powercap/dtpm_cpu: Reset per_cpu variable in the release function

[Daniel Lezcano]

The release function does not reset the per cpu variable when it is
called. That will prevent creation again as the variable will be
already from the previous creation.

Fix it by resetting them.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/powercap/dtpm_cpu.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
index eed5ad688d46..71f45d2f5a60 100644
--- a/drivers/powercap/dtpm_cpu.c
+++ b/drivers/powercap/dtpm_cpu.c
@@ -151,10 +151,17 @@ static int update_pd_power_uw(struct dtpm *dtpm)
 static void pd_release(struct dtpm *dtpm)
 {
 	struct dtpm_cpu *dtpm_cpu = to_dtpm_cpu(dtpm);
+	struct cpufreq_policy *policy;
 
 	if (freq_qos_request_active(&dtpm_cpu->qos_req))
 		freq_qos_remove_request(&dtpm_cpu->qos_req);
 
+	policy = cpufreq_cpu_get(dtpm_cpu->cpu);
+	if (policy) {
+		for_each_cpu(dtpm_cpu->cpu, policy->related_cpus)
+			per_cpu(dtpm_per_cpu, dtpm_cpu->cpu) = NULL;
+	}
+	
 	kfree(dtpm_cpu);
 }
 
-- 
2.25.1

[PATCH v1 3/7] powercap/dtpm: Fixup kfree for virtual node

[Daniel Lezcano]

When the node is virtual there is no release function associated which
can free the memory.

Free the memory when no 'ops' exists.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/powercap/dtpm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
index 0b0121c37a1b..7bddd25a6767 100644
--- a/drivers/powercap/dtpm.c
+++ b/drivers/powercap/dtpm.c
@@ -181,12 +181,12 @@ int dtpm_release_zone(struct powercap_zone *pcz)
 
 	if (dtpm->ops)
 		dtpm->ops->release(dtpm);
+	else
+		kfree(dtpm);
 
 	if (root == dtpm)
 		root = NULL;
 
-	kfree(dtpm);
-
 	return 0;
 }
 
-- 
2.25.1

[PATCH v1 4/7] powercap/dtpm: Destroy hierarchy function

[Daniel Lezcano]

The hierarchy creation function exits but without a destroy hierarchy
function. Due to that, the modules creating the hierarchy can not be
unloaded properly because they don't have an exit callback.

Provide the dtpm_destroy_hierarchy() function to remove the previously
created hierarchy.

The function relies on all the release mechanisms implemented by the
underlying powercap framework.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/powercap/dtpm.c | 43 +++++++++++++++++++++++++++++++++++++++++
 include/linux/dtpm.h    |  3 +++
 2 files changed, 46 insertions(+)

diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
index 7bddd25a6767..d9d74f981118 100644
--- a/drivers/powercap/dtpm.c
+++ b/drivers/powercap/dtpm.c
@@ -617,3 +617,46 @@ int dtpm_create_hierarchy(struct of_device_id *dtpm_match_table)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(dtpm_create_hierarchy);
+
+static void __dtpm_destroy_hierarchy(struct dtpm *dtpm)
+{
+	struct dtpm *child, *aux;
+
+	list_for_each_entry_safe(child, aux, &dtpm->children, sibling)
+		__dtpm_destroy_hierarchy(child);
+
+	/*
+	 * At this point, we know all children were removed from the
+	 * recursive call before
+	 */
+	dtpm_unregister(dtpm);
+}
+
+void dtpm_destroy_hierarchy(void)
+{
+	int i;
+
+	mutex_lock(&dtpm_lock);
+
+	if (!pct)
+		goto out_unlock;
+
+	__dtpm_destroy_hierarchy(root);
+	
+
+	for (i = 0; i < ARRAY_SIZE(dtpm_subsys); i++) {
+
+		if (!dtpm_subsys[i]->exit)
+			continue;
+
+		dtpm_subsys[i]->exit();
+	}
+
+	powercap_unregister_control_type(pct);
+
+	pct = NULL;
+
+out_unlock:
+	mutex_unlock(&dtpm_lock);
+}
+EXPORT_SYMBOL_GPL(dtpm_destroy_hierarchy);
diff --git a/include/linux/dtpm.h b/include/linux/dtpm.h
index f7a25c70dd4c..a4a13514b730 100644
--- a/include/linux/dtpm.h
+++ b/include/linux/dtpm.h
@@ -37,6 +37,7 @@ struct device_node;
 struct dtpm_subsys_ops {
 	const char *name;
 	int (*init)(void);
+	void (*exit)(void);
 	int (*setup)(struct dtpm *, struct device_node *);
 };
 
@@ -67,4 +68,6 @@ void dtpm_unregister(struct dtpm *dtpm);
 int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent);
 
 int dtpm_create_hierarchy(struct of_device_id *dtpm_match_table);
+
+void dtpm_destroy_hierarchy(void);
 #endif
-- 
2.25.1

[PATCH v1 5/7] powercap/dtpm: Move the 'root' reset place

[Daniel Lezcano]

The 'root' node is checked everytime a dtpm node is destroyed.

When we reach the end of the hierarchy destruction function, we can
unconditionnaly set the 'root' node to NULL again.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/powercap/dtpm.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
index d9d74f981118..ec931a06d90a 100644
--- a/drivers/powercap/dtpm.c
+++ b/drivers/powercap/dtpm.c
@@ -184,9 +184,6 @@ int dtpm_release_zone(struct powercap_zone *pcz)
 	else
 		kfree(dtpm);
 
-	if (root == dtpm)
-		root = NULL;
-
 	return 0;
 }
 
@@ -656,6 +653,8 @@ void dtpm_destroy_hierarchy(void)
 
 	pct = NULL;
 
+	root = NULL;
+
 out_unlock:
 	mutex_unlock(&dtpm_lock);
 }
-- 
2.25.1

[PATCH v1 6/7] powercap/dtpm/dtpm_cpu: Add exit function

[Daniel Lezcano]

Now that we can destroy the hierarchy, the code must remove what it
had put in place at the creation. In our case, the cpu hotplug
callbacks.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/powercap/dtpm_cpu.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
index 71f45d2f5a60..bca2f912d349 100644
--- a/drivers/powercap/dtpm_cpu.c
+++ b/drivers/powercap/dtpm_cpu.c
@@ -299,8 +299,15 @@ static int dtpm_cpu_init(void)
 	return 0;
 }
 
+static void dtpm_cpu_exit(void)
+{
+	cpuhp_remove_state_nocalls(CPUHP_AP_ONLINE_DYN);
+	cpuhp_remove_state_nocalls(CPUHP_AP_DTPM_CPU_DEAD);
+}
+
 struct dtpm_subsys_ops dtpm_cpu_ops = {
 	.name = KBUILD_MODNAME,
 	.init = dtpm_cpu_init,
+	.exit = dtpm_cpu_exit,
 	.setup = dtpm_cpu_setup,
 };
-- 
2.25.1

[PATCH v1 7/7] dtpm/soc/rk3399: Add the ability to unload the module

[Daniel Lezcano]

The dtpm hierarchy can now be removed with the
dtpm_destroy_hierarchy() function. Add the module_exit() callback so
the module can be unloaded by removing the previously created
hierarchy.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/soc/rockchip/dtpm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/soc/rockchip/dtpm.c b/drivers/soc/rockchip/dtpm.c
index ebebb748488b..5a23784b5221 100644
--- a/drivers/soc/rockchip/dtpm.c
+++ b/drivers/soc/rockchip/dtpm.c
@@ -52,6 +52,12 @@ static int __init rockchip_dtpm_init(void)
 }
 module_init(rockchip_dtpm_init);
 
+static void __exit rockchip_dtpm_exit(void)
+{
+	return dtpm_destroy_hierarchy();
+}
+module_exit(rockchip_dtpm_exit);
+
 MODULE_SOFTDEP("pre: panfrost cpufreq-dt");
 MODULE_DESCRIPTION("Rockchip DTPM driver");
 MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH v1 3/7] powercap/dtpm: Fixup kfree for virtual node

[Ulf Hansson]

On Sun, 30 Jan 2022 at 22:02, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> When the node is virtual there is no release function associated which
> can free the memory.
>
> Free the memory when no 'ops' exists.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/powercap/dtpm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
> index 0b0121c37a1b..7bddd25a6767 100644
> --- a/drivers/powercap/dtpm.c
> +++ b/drivers/powercap/dtpm.c
> @@ -181,12 +181,12 @@ int dtpm_release_zone(struct powercap_zone *pcz)
>
>         if (dtpm->ops)
>                 dtpm->ops->release(dtpm);
> +       else
> +               kfree(dtpm);
>

This doesn't look correct. Below you check dtpm against "root", which
may be after its memory has been freed.

If the ->release() function should be responsible for freeing the
dtpm, it needs to be called after the check below.

>         if (root == dtpm)
>                 root = NULL;
>
> -       kfree(dtpm);
> -
>         return 0;
>  }
>

Kind regards
Uffe

Re: [PATCH v1 1/7] powercap/dtpm: Change locking scheme

[Ulf Hansson]

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
>

Re: [PATCH v1 2/7] powercap/dtpm_cpu: Reset per_cpu variable in the release function

[Ulf Hansson]

On Sun, 30 Jan 2022 at 22:02, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> The release function does not reset the per cpu variable when it is
> called. That will prevent creation again as the variable will be
> already from the previous creation.
>
> Fix it by resetting them.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe


> ---
>  drivers/powercap/dtpm_cpu.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
> index eed5ad688d46..71f45d2f5a60 100644
> --- a/drivers/powercap/dtpm_cpu.c
> +++ b/drivers/powercap/dtpm_cpu.c
> @@ -151,10 +151,17 @@ static int update_pd_power_uw(struct dtpm *dtpm)
>  static void pd_release(struct dtpm *dtpm)
>  {
>         struct dtpm_cpu *dtpm_cpu = to_dtpm_cpu(dtpm);
> +       struct cpufreq_policy *policy;
>
>         if (freq_qos_request_active(&dtpm_cpu->qos_req))
>                 freq_qos_remove_request(&dtpm_cpu->qos_req);
>
> +       policy = cpufreq_cpu_get(dtpm_cpu->cpu);
> +       if (policy) {
> +               for_each_cpu(dtpm_cpu->cpu, policy->related_cpus)
> +                       per_cpu(dtpm_per_cpu, dtpm_cpu->cpu) = NULL;
> +       }
> +
>         kfree(dtpm_cpu);
>  }
>
> --
> 2.25.1
>

Re: [PATCH v1 4/7] powercap/dtpm: Destroy hierarchy function

[Ulf Hansson]

On Sun, 30 Jan 2022 at 22:02, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> The hierarchy creation function exits but without a destroy hierarchy
> function. Due to that, the modules creating the hierarchy can not be
> unloaded properly because they don't have an exit callback.
>
> Provide the dtpm_destroy_hierarchy() function to remove the previously
> created hierarchy.
>
> The function relies on all the release mechanisms implemented by the
> underlying powercap framework.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/powercap/dtpm.c | 43 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/dtpm.h    |  3 +++
>  2 files changed, 46 insertions(+)
>
> diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
> index 7bddd25a6767..d9d74f981118 100644
> --- a/drivers/powercap/dtpm.c
> +++ b/drivers/powercap/dtpm.c
> @@ -617,3 +617,46 @@ int dtpm_create_hierarchy(struct of_device_id *dtpm_match_table)
>         return ret;
>  }
>  EXPORT_SYMBOL_GPL(dtpm_create_hierarchy);
> +
> +static void __dtpm_destroy_hierarchy(struct dtpm *dtpm)
> +{
> +       struct dtpm *child, *aux;
> +
> +       list_for_each_entry_safe(child, aux, &dtpm->children, sibling)
> +               __dtpm_destroy_hierarchy(child);
> +
> +       /*
> +        * At this point, we know all children were removed from the
> +        * recursive call before
> +        */
> +       dtpm_unregister(dtpm);
> +}
> +
> +void dtpm_destroy_hierarchy(void)
> +{
> +       int i;
> +
> +       mutex_lock(&dtpm_lock);
> +
> +       if (!pct)

As I kind of indicated in one of the earlier replies, it looks like
dtpm_lock is being used to protect the global "pct". What else?

Rather than doing it like this, couldn't you instead let
dtpm_create_hiearchy() return a handle/cookie for a "dtpm hierarchy".
This handle then needs to be passed to dtpm_destroy_hierarchy().

In this way, the "pct" doesn't need to be protected and you wouldn't
need the global "pct" at all. Although, maybe there would be other
problems with this?

> +               goto out_unlock;
> +
> +       __dtpm_destroy_hierarchy(root);
> +
> +
> +       for (i = 0; i < ARRAY_SIZE(dtpm_subsys); i++) {
> +
> +               if (!dtpm_subsys[i]->exit)
> +                       continue;
> +
> +               dtpm_subsys[i]->exit();
> +       }
> +
> +       powercap_unregister_control_type(pct);
> +
> +       pct = NULL;
> +
> +out_unlock:
> +       mutex_unlock(&dtpm_lock);
> +}
> +EXPORT_SYMBOL_GPL(dtpm_destroy_hierarchy);
> diff --git a/include/linux/dtpm.h b/include/linux/dtpm.h
> index f7a25c70dd4c..a4a13514b730 100644
> --- a/include/linux/dtpm.h
> +++ b/include/linux/dtpm.h
> @@ -37,6 +37,7 @@ struct device_node;
>  struct dtpm_subsys_ops {
>         const char *name;
>         int (*init)(void);
> +       void (*exit)(void);
>         int (*setup)(struct dtpm *, struct device_node *);
>  };
>
> @@ -67,4 +68,6 @@ void dtpm_unregister(struct dtpm *dtpm);
>  int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent);
>
>  int dtpm_create_hierarchy(struct of_device_id *dtpm_match_table);
> +
> +void dtpm_destroy_hierarchy(void);
>  #endif

Kind regards
Uffe

Re: [PATCH v1 3/7] powercap/dtpm: Fixup kfree for virtual node

[Daniel Lezcano]

On 16/02/2022 17:22, Ulf Hansson wrote:
> On Sun, 30 Jan 2022 at 22:02, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>> When the node is virtual there is no release function associated which
>> can free the memory.
>>
>> Free the memory when no 'ops' exists.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>   drivers/powercap/dtpm.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
>> index 0b0121c37a1b..7bddd25a6767 100644
>> --- a/drivers/powercap/dtpm.c
>> +++ b/drivers/powercap/dtpm.c
>> @@ -181,12 +181,12 @@ int dtpm_release_zone(struct powercap_zone *pcz)
>>
>>          if (dtpm->ops)
>>                  dtpm->ops->release(dtpm);
>> +       else
>> +               kfree(dtpm);
>>
> 
> This doesn't look correct. Below you check dtpm against "root", which
> may be after its memory has been freed.
> 
> If the ->release() function should be responsible for freeing the
> dtpm, it needs to be called after the check below.

It is harmless, 'root' is not dereferenced but used as an ID

Moreover, in the patch 5/7 it is moved out this function.


>>          if (root == dtpm)
>>                  root = NULL;
>>
>> -       kfree(dtpm);
>> -
>>          return 0;
>>   }
>>
> 
> Kind regards
> Uffe


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v1 4/7] powercap/dtpm: Destroy hierarchy function

[Daniel Lezcano]

On 16/02/2022 17:31, Ulf Hansson wrote:
> On Sun, 30 Jan 2022 at 22:02, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>> The hierarchy creation function exits but without a destroy hierarchy
>> function. Due to that, the modules creating the hierarchy can not be
>> unloaded properly because they don't have an exit callback.
>>
>> Provide the dtpm_destroy_hierarchy() function to remove the previously
>> created hierarchy.
>>
>> The function relies on all the release mechanisms implemented by the
>> underlying powercap framework.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>   drivers/powercap/dtpm.c | 43 +++++++++++++++++++++++++++++++++++++++++
>>   include/linux/dtpm.h    |  3 +++
>>   2 files changed, 46 insertions(+)
>>
>> diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
>> index 7bddd25a6767..d9d74f981118 100644
>> --- a/drivers/powercap/dtpm.c
>> +++ b/drivers/powercap/dtpm.c
>> @@ -617,3 +617,46 @@ int dtpm_create_hierarchy(struct of_device_id *dtpm_match_table)
>>          return ret;
>>   }
>>   EXPORT_SYMBOL_GPL(dtpm_create_hierarchy);
>> +
>> +static void __dtpm_destroy_hierarchy(struct dtpm *dtpm)
>> +{
>> +       struct dtpm *child, *aux;
>> +
>> +       list_for_each_entry_safe(child, aux, &dtpm->children, sibling)
>> +               __dtpm_destroy_hierarchy(child);
>> +
>> +       /*
>> +        * At this point, we know all children were removed from the
>> +        * recursive call before
>> +        */
>> +       dtpm_unregister(dtpm);
>> +}
>> +
>> +void dtpm_destroy_hierarchy(void)
>> +{
>> +       int i;
>> +
>> +       mutex_lock(&dtpm_lock);
>> +
>> +       if (!pct)
> 
> As I kind of indicated in one of the earlier replies, it looks like
> dtpm_lock is being used to protect the global "pct". What else?

The root node pointer and the lists describing the hierarchy

> Rather than doing it like this, couldn't you instead let
> dtpm_create_hiearchy() return a handle/cookie for a "dtpm hierarchy".
> This handle then needs to be passed to dtpm_destroy_hierarchy().
> 
> In this way, the "pct" doesn't need to be protected and you wouldn't
> need the global "pct" at all. Although, maybe there would be other
> problems with this?

The only problem I see with this approach is the dtpm framework is 
designed to be a singleton, no other instance of the hierarchy can exists.

By allowing returning a pointer or whatever, that implies multiple 
controller can be created.

In addition that would mean duplicating a global variable for each 
module to store the pointer at init time in order to reuse it at exit time.


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v1 4/7] powercap/dtpm: Destroy hierarchy function

[Ulf Hansson]

On Wed, 16 Feb 2022 at 20:25, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On 16/02/2022 17:31, Ulf Hansson wrote:
> > On Sun, 30 Jan 2022 at 22:02, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >>
> >> The hierarchy creation function exits but without a destroy hierarchy
> >> function. Due to that, the modules creating the hierarchy can not be
> >> unloaded properly because they don't have an exit callback.
> >>
> >> Provide the dtpm_destroy_hierarchy() function to remove the previously
> >> created hierarchy.
> >>
> >> The function relies on all the release mechanisms implemented by the
> >> underlying powercap framework.
> >>
> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >> ---
> >>   drivers/powercap/dtpm.c | 43 +++++++++++++++++++++++++++++++++++++++++
> >>   include/linux/dtpm.h    |  3 +++
> >>   2 files changed, 46 insertions(+)
> >>
> >> diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
> >> index 7bddd25a6767..d9d74f981118 100644
> >> --- a/drivers/powercap/dtpm.c
> >> +++ b/drivers/powercap/dtpm.c
> >> @@ -617,3 +617,46 @@ int dtpm_create_hierarchy(struct of_device_id *dtpm_match_table)
> >>          return ret;
> >>   }
> >>   EXPORT_SYMBOL_GPL(dtpm_create_hierarchy);
> >> +
> >> +static void __dtpm_destroy_hierarchy(struct dtpm *dtpm)
> >> +{
> >> +       struct dtpm *child, *aux;
> >> +
> >> +       list_for_each_entry_safe(child, aux, &dtpm->children, sibling)
> >> +               __dtpm_destroy_hierarchy(child);
> >> +
> >> +       /*
> >> +        * At this point, we know all children were removed from the
> >> +        * recursive call before
> >> +        */
> >> +       dtpm_unregister(dtpm);
> >> +}
> >> +
> >> +void dtpm_destroy_hierarchy(void)
> >> +{
> >> +       int i;
> >> +
> >> +       mutex_lock(&dtpm_lock);
> >> +
> >> +       if (!pct)
> >
> > As I kind of indicated in one of the earlier replies, it looks like
> > dtpm_lock is being used to protect the global "pct". What else?
>
> The root node pointer and the lists describing the hierarchy
>
> > Rather than doing it like this, couldn't you instead let
> > dtpm_create_hiearchy() return a handle/cookie for a "dtpm hierarchy".
> > This handle then needs to be passed to dtpm_destroy_hierarchy().
> >
> > In this way, the "pct" doesn't need to be protected and you wouldn't
> > need the global "pct" at all. Although, maybe there would be other
> > problems with this?
>
> The only problem I see with this approach is the dtpm framework is
> designed to be a singleton, no other instance of the hierarchy can exists.

Right. So, it's not likely that we need to change this in future then,
I assume!?

>
> By allowing returning a pointer or whatever, that implies multiple
> controller can be created.

Yes.

>
> In addition that would mean duplicating a global variable for each
> module to store the pointer at init time in order to reuse it at exit time.

Yes, that seems unnecessary. At least as long as the dtpm framework
remains to be a singleton.

Kind regards
Uffe

Re: [PATCH v1 3/7] powercap/dtpm: Fixup kfree for virtual node

[Ulf Hansson]

On Wed, 16 Feb 2022 at 19:10, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On 16/02/2022 17:22, Ulf Hansson wrote:
> > On Sun, 30 Jan 2022 at 22:02, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >>
> >> When the node is virtual there is no release function associated which
> >> can free the memory.
> >>
> >> Free the memory when no 'ops' exists.
> >>
> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >> ---
> >>   drivers/powercap/dtpm.c | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
> >> index 0b0121c37a1b..7bddd25a6767 100644
> >> --- a/drivers/powercap/dtpm.c
> >> +++ b/drivers/powercap/dtpm.c
> >> @@ -181,12 +181,12 @@ int dtpm_release_zone(struct powercap_zone *pcz)
> >>
> >>          if (dtpm->ops)
> >>                  dtpm->ops->release(dtpm);
> >> +       else
> >> +               kfree(dtpm);
> >>
> >
> > This doesn't look correct. Below you check dtpm against "root", which
> > may be after its memory has been freed.
> >
> > If the ->release() function should be responsible for freeing the
> > dtpm, it needs to be called after the check below.
>
> It is harmless, 'root' is not dereferenced but used as an ID
>
> Moreover, in the patch 5/7 it is moved out this function.

Right. It just looks a bit odd here.

>
>
> >>          if (root == dtpm)
> >>                  root = NULL;
> >>
> >> -       kfree(dtpm);

So then why doesn't this kfree do the job already?

kfree(NULL) works fine, if dtpm->ops->release(dtpm) already freed the data.

> >> -
> >>          return 0;
> >>   }
> >>

Kind regards
Uffe

Re: [PATCH v1 4/7] powercap/dtpm: Destroy hierarchy function

[Ulf Hansson]

On Sun, 30 Jan 2022 at 22:02, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> The hierarchy creation function exits but without a destroy hierarchy
> function. Due to that, the modules creating the hierarchy can not be
> unloaded properly because they don't have an exit callback.
>
> Provide the dtpm_destroy_hierarchy() function to remove the previously
> created hierarchy.
>
> The function relies on all the release mechanisms implemented by the
> underlying powercap framework.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/powercap/dtpm.c | 43 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/dtpm.h    |  3 +++
>  2 files changed, 46 insertions(+)
>
> diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
> index 7bddd25a6767..d9d74f981118 100644
> --- a/drivers/powercap/dtpm.c
> +++ b/drivers/powercap/dtpm.c
> @@ -617,3 +617,46 @@ int dtpm_create_hierarchy(struct of_device_id *dtpm_match_table)
>         return ret;
>  }
>  EXPORT_SYMBOL_GPL(dtpm_create_hierarchy);
> +
> +static void __dtpm_destroy_hierarchy(struct dtpm *dtpm)
> +{
> +       struct dtpm *child, *aux;
> +
> +       list_for_each_entry_safe(child, aux, &dtpm->children, sibling)
> +               __dtpm_destroy_hierarchy(child);
> +
> +       /*
> +        * At this point, we know all children were removed from the
> +        * recursive call before
> +        */
> +       dtpm_unregister(dtpm);
> +}
> +
> +void dtpm_destroy_hierarchy(void)
> +{
> +       int i;
> +
> +       mutex_lock(&dtpm_lock);
> +
> +       if (!pct)
> +               goto out_unlock;
> +
> +       __dtpm_destroy_hierarchy(root);
> +
> +
> +       for (i = 0; i < ARRAY_SIZE(dtpm_subsys); i++) {
> +
> +               if (!dtpm_subsys[i]->exit)
> +                       continue;
> +
> +               dtpm_subsys[i]->exit();
> +       }
> +
> +       powercap_unregister_control_type(pct);
> +
> +       pct = NULL;
> +
> +out_unlock:
> +       mutex_unlock(&dtpm_lock);
> +}
> +EXPORT_SYMBOL_GPL(dtpm_destroy_hierarchy);
> diff --git a/include/linux/dtpm.h b/include/linux/dtpm.h
> index f7a25c70dd4c..a4a13514b730 100644
> --- a/include/linux/dtpm.h
> +++ b/include/linux/dtpm.h
> @@ -37,6 +37,7 @@ struct device_node;
>  struct dtpm_subsys_ops {
>         const char *name;
>         int (*init)(void);
> +       void (*exit)(void);
>         int (*setup)(struct dtpm *, struct device_node *);
>  };
>
> @@ -67,4 +68,6 @@ void dtpm_unregister(struct dtpm *dtpm);
>  int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent);
>
>  int dtpm_create_hierarchy(struct of_device_id *dtpm_match_table);
> +
> +void dtpm_destroy_hierarchy(void);
>  #endif
> --
> 2.25.1
>

Re: [PATCH v1 5/7] powercap/dtpm: Move the 'root' reset place

[Ulf Hansson]

On Sun, 30 Jan 2022 at 22:02, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> The 'root' node is checked everytime a dtpm node is destroyed.
>
> When we reach the end of the hierarchy destruction function, we can
> unconditionnaly set the 'root' node to NULL again.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/powercap/dtpm.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
> index d9d74f981118..ec931a06d90a 100644
> --- a/drivers/powercap/dtpm.c
> +++ b/drivers/powercap/dtpm.c
> @@ -184,9 +184,6 @@ int dtpm_release_zone(struct powercap_zone *pcz)
>         else
>                 kfree(dtpm);
>
> -       if (root == dtpm)
> -               root = NULL;
> -
>         return 0;
>  }
>
> @@ -656,6 +653,8 @@ void dtpm_destroy_hierarchy(void)
>
>         pct = NULL;
>
> +       root = NULL;
> +
>  out_unlock:
>         mutex_unlock(&dtpm_lock);
>  }
> --
> 2.25.1
>

Re: [PATCH v1 6/7] powercap/dtpm/dtpm_cpu: Add exit function

[Ulf Hansson]

On Sun, 30 Jan 2022 at 22:02, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> Now that we can destroy the hierarchy, the code must remove what it
> had put in place at the creation. In our case, the cpu hotplug
> callbacks.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/powercap/dtpm_cpu.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
> index 71f45d2f5a60..bca2f912d349 100644
> --- a/drivers/powercap/dtpm_cpu.c
> +++ b/drivers/powercap/dtpm_cpu.c
> @@ -299,8 +299,15 @@ static int dtpm_cpu_init(void)
>         return 0;
>  }
>
> +static void dtpm_cpu_exit(void)
> +{
> +       cpuhp_remove_state_nocalls(CPUHP_AP_ONLINE_DYN);
> +       cpuhp_remove_state_nocalls(CPUHP_AP_DTPM_CPU_DEAD);
> +}
> +
>  struct dtpm_subsys_ops dtpm_cpu_ops = {
>         .name = KBUILD_MODNAME,
>         .init = dtpm_cpu_init,
> +       .exit = dtpm_cpu_exit,
>         .setup = dtpm_cpu_setup,
>  };
> --
> 2.25.1
>

Re: [PATCH v1 7/7] dtpm/soc/rk3399: Add the ability to unload the module

[Ulf Hansson]

On Sun, 30 Jan 2022 at 22:02, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> The dtpm hierarchy can now be removed with the
> dtpm_destroy_hierarchy() function. Add the module_exit() callback so
> the module can be unloaded by removing the previously created
> hierarchy.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/soc/rockchip/dtpm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/soc/rockchip/dtpm.c b/drivers/soc/rockchip/dtpm.c
> index ebebb748488b..5a23784b5221 100644
> --- a/drivers/soc/rockchip/dtpm.c
> +++ b/drivers/soc/rockchip/dtpm.c
> @@ -52,6 +52,12 @@ static int __init rockchip_dtpm_init(void)
>  }
>  module_init(rockchip_dtpm_init);
>
> +static void __exit rockchip_dtpm_exit(void)
> +{
> +       return dtpm_destroy_hierarchy();
> +}
> +module_exit(rockchip_dtpm_exit);
> +
>  MODULE_SOFTDEP("pre: panfrost cpufreq-dt");
>  MODULE_DESCRIPTION("Rockchip DTPM driver");
>  MODULE_LICENSE("GPL");
> --
> 2.25.1
>

Re: [PATCH v1 3/7] powercap/dtpm: Fixup kfree for virtual node

[Daniel Lezcano]

On 17/02/2022 14:17, Ulf Hansson wrote:
> On Wed, 16 Feb 2022 at 19:10, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>> On 16/02/2022 17:22, Ulf Hansson wrote:
>>> On Sun, 30 Jan 2022 at 22:02, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> When the node is virtual there is no release function associated which
>>>> can free the memory.
>>>>
>>>> Free the memory when no 'ops' exists.
>>>>
>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>> ---
>>>>    drivers/powercap/dtpm.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
>>>> index 0b0121c37a1b..7bddd25a6767 100644
>>>> --- a/drivers/powercap/dtpm.c
>>>> +++ b/drivers/powercap/dtpm.c
>>>> @@ -181,12 +181,12 @@ int dtpm_release_zone(struct powercap_zone *pcz)
>>>>
>>>>           if (dtpm->ops)
>>>>                   dtpm->ops->release(dtpm);
>>>> +       else
>>>> +               kfree(dtpm);
>>>>
>>>
>>> This doesn't look correct. Below you check dtpm against "root", which
>>> may be after its memory has been freed.
>>>
>>> If the ->release() function should be responsible for freeing the
>>> dtpm, it needs to be called after the check below.
>>
>> It is harmless, 'root' is not dereferenced but used as an ID
>>
>> Moreover, in the patch 5/7 it is moved out this function.
> 
> Right. It just looks a bit odd here.
> 
>>
>>
>>>>           if (root == dtpm)
>>>>                   root = NULL;
>>>>
>>>> -       kfree(dtpm);
> 
> So then why doesn't this kfree do the job already?
> 
> kfree(NULL) works fine, if dtpm->ops->release(dtpm) already freed the data.

The description is confusing.

Actually, there is a double kfree. When there is a ops->release, the 
kfree is done there and again a few lines after.

The issue was introduced with the change where dtpm had a private data 
field to store the backend specific structure and was converted to a 
backend specific structure containing a dtpm node [1].

So this function was calling release from the dtpm backend which was 
freeing the specific data in the dtpm->private and then here was freeing 
the dtpm. Now, the backend frees the structure which contains the dtpm 
structure, so when returning from ops->release(), dtpm is already free.

I should change the description and add a Fixes tag to the change 
described above.

[1] 
https://lore.kernel.org/r/20210312130411.29833-4-daniel.lezcano@linaro.org



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v1 3/7] powercap/dtpm: Fixup kfree for virtual node

[Ulf Hansson]

On Thu, 17 Feb 2022 at 14:54, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On 17/02/2022 14:17, Ulf Hansson wrote:
> > On Wed, 16 Feb 2022 at 19:10, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 16/02/2022 17:22, Ulf Hansson wrote:
> >>> On Sun, 30 Jan 2022 at 22:02, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >>>>
> >>>> When the node is virtual there is no release function associated which
> >>>> can free the memory.
> >>>>
> >>>> Free the memory when no 'ops' exists.
> >>>>
> >>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >>>> ---
> >>>>    drivers/powercap/dtpm.c | 4 ++--
> >>>>    1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
> >>>> index 0b0121c37a1b..7bddd25a6767 100644
> >>>> --- a/drivers/powercap/dtpm.c
> >>>> +++ b/drivers/powercap/dtpm.c
> >>>> @@ -181,12 +181,12 @@ int dtpm_release_zone(struct powercap_zone *pcz)
> >>>>
> >>>>           if (dtpm->ops)
> >>>>                   dtpm->ops->release(dtpm);
> >>>> +       else
> >>>> +               kfree(dtpm);
> >>>>
> >>>
> >>> This doesn't look correct. Below you check dtpm against "root", which
> >>> may be after its memory has been freed.
> >>>
> >>> If the ->release() function should be responsible for freeing the
> >>> dtpm, it needs to be called after the check below.
> >>
> >> It is harmless, 'root' is not dereferenced but used as an ID
> >>
> >> Moreover, in the patch 5/7 it is moved out this function.
> >
> > Right. It just looks a bit odd here.
> >
> >>
> >>
> >>>>           if (root == dtpm)
> >>>>                   root = NULL;
> >>>>
> >>>> -       kfree(dtpm);
> >
> > So then why doesn't this kfree do the job already?
> >
> > kfree(NULL) works fine, if dtpm->ops->release(dtpm) already freed the data.
>
> The description is confusing.
>
> Actually, there is a double kfree. When there is a ops->release, the
> kfree is done there and again a few lines after.
>
> The issue was introduced with the change where dtpm had a private data
> field to store the backend specific structure and was converted to a
> backend specific structure containing a dtpm node [1].
>
> So this function was calling release from the dtpm backend which was
> freeing the specific data in the dtpm->private and then here was freeing
> the dtpm. Now, the backend frees the structure which contains the dtpm
> structure, so when returning from ops->release(), dtpm is already free.
>
> I should change the description and add a Fixes tag to the change
> described above.

Does ops->release() also resets the "dtpm" pointer to NULL? If not,
it's good practice that it should, right?

In that case, we would be calling "kfree(NULL);" the second time,
which is perfectly fine.

>
> [1]
> https://lore.kernel.org/r/20210312130411.29833-4-daniel.lezcano@linaro.org
>

Kind regards
Uffe

Re: [PATCH v1 3/7] powercap/dtpm: Fixup kfree for virtual node

[Daniel Lezcano]

On 17/02/2022 16:45, Ulf Hansson wrote:

[ ... ]

> Does ops->release() also resets the "dtpm" pointer to NULL? If not,
> it's good practice that it should, right?
>
> In that case, we would be calling "kfree(NULL);" the second time,
> which is perfectly fine.

So you suggest to replace:

if (ops->release)
	ops->release(dtpm);
else
	kfree(dtpm);

By:

if (ops->release) {
	ops->release(dtpm);
	dtpm = NULL;
}

kfree(dtpm);

?

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v1 3/7] powercap/dtpm: Fixup kfree for virtual node

[Ulf Hansson]

On Fri, 18 Feb 2022 at 14:18, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On 17/02/2022 16:45, Ulf Hansson wrote:
>
> [ ... ]
>
> > Does ops->release() also resets the "dtpm" pointer to NULL? If not,
> > it's good practice that it should, right?
> >
> > In that case, we would be calling "kfree(NULL);" the second time,
> > which is perfectly fine.
>
> So you suggest to replace:
>
> if (ops->release)
>         ops->release(dtpm);
> else
>         kfree(dtpm);
>
> By:
>
> if (ops->release) {
>         ops->release(dtpm);
>         dtpm = NULL;
> }
>

I don't have a strong opinion how to code this.

What I was trying to point out was that if ->ops->release() frees the
memory it could/should also reset the pointer to NULL.

And if that is already done, the kfree below is harmless and there
would be nothing to "fix".

> kfree(dtpm);
>
> ?

Kind regards
Uffe

Re: [PATCH v1 3/7] powercap/dtpm: Fixup kfree for virtual node

[Daniel Lezcano]

On 22/02/2022 16:55, Ulf Hansson wrote:
> On Fri, 18 Feb 2022 at 14:18, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>> On 17/02/2022 16:45, Ulf Hansson wrote:
>>
>> [ ... ]
>>
>>> Does ops->release() also resets the "dtpm" pointer to NULL? If not,
>>> it's good practice that it should, right?
>>>
>>> In that case, we would be calling "kfree(NULL);" the second time,
>>> which is perfectly fine.
>>
>> So you suggest to replace:
>>
>> if (ops->release)
>>          ops->release(dtpm);
>> else
>>          kfree(dtpm);
>>
>> By:
>>
>> if (ops->release) {
>>          ops->release(dtpm);
>>          dtpm = NULL;
>> }
>>
> 
> I don't have a strong opinion how to code this.
> 
> What I was trying to point out was that if ->ops->release() frees the
> memory it could/should also reset the pointer to NULL

No it can't because it is not a pointer, it is contained by the backend 
specific structure.

eg.

struct dtpm_cpu {
	struct dtpm dtpm;
};

the release frees a dtpm_cpu structure.



> And if that is already done, the kfree below is harmless and there
> would be nothing to "fix".
> 
>> kfree(dtpm);
>>
>> ?
> 
> Kind regards
> Uffe


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog