| 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 |
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
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.
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
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 --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
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(+)