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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0e7bf391-08f1-88b3-14c2-8bbf45ad7491@linaro.org>
Date:   Wed, 16 Feb 2022 20:25:02 +0100
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Ulf Hansson <ulf.hansson@...aro.org>
Cc:     rjw@...ysocki.net, heiko@...ech.de, lukasz.luba@....com,
        linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        Daniel Lezcano <daniel.lezcano@...nel.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>
Subject: Re: [PATCH v1 4/7] powercap/dtpm: Destroy hierarchy function

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