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@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.

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