diff mbox series

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

Message ID 20220130210210.549877-4-daniel.lezcano@linaro.org
State Accepted
Commit c404c64d64bc31bebe8a2015103671f7cd282731
Headers show
Series [v1,1/7] powercap/dtpm: Change locking scheme | expand

Commit Message

Daniel Lezcano Jan. 30, 2022, 9:02 p.m. UTC
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(+)

Comments

Ulf Hansson Feb. 16, 2022, 4:31 p.m. UTC | #1
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
Daniel Lezcano Feb. 16, 2022, 7:25 p.m. UTC | #2
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.
Ulf Hansson Feb. 17, 2022, 1:12 p.m. UTC | #3
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
Ulf Hansson Feb. 17, 2022, 1:17 p.m. UTC | #4
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
>
diff mbox series

Patch

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