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

[Ulf Hansson]

On Wed, 16 Feb 2022 at 20:25, Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote:
>
> On 16/02/2022 17:31, Ulf Hansson wrote:
> > On Sun, 30 Jan 2022 at 22:02, Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> ---
> >>   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