Re: [PATCH v5 2/6] powercap/drivers/dtpm: Add hierarchy creation

From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: Ulf Hansson <ulf.hansson@linaro.org>
Cc: rjw@rjwysocki.net, lukasz.luba@arm.com, robh@kernel.org,
	heiko@sntech.de, arnd@linaro.org, linux-kernel@vger.kernel.org,
	linux-pm@vger.kernel.org, "Rafael J. Wysocki" <rafael@kernel.org>,
	Daniel Lezcano <daniel.lezcano@kernel.org>
Subject: Re: [PATCH v5 2/6] powercap/drivers/dtpm: Add hierarchy creation
Date: Mon, 10 Jan 2022 16:55:31 +0100	[thread overview]
Message-ID: <cbc70ea7-39b4-b5e8-b5c0-45fb436f53eb@linaro.org> (raw)
In-Reply-To: <CAPDyKFqzxnfh0kow5mzoApFMQpKOAv=e1b7Cy9H-iEh99Wmagw@mail.gmail.com>

On 07/01/2022 16:54, Ulf Hansson wrote:
> [...]
> 
>>>> +static int dtpm_for_each_child(const struct dtpm_node *hierarchy,
>>>> +                              const struct dtpm_node *it, struct dtpm *parent)
>>>> +{
>>>> +       struct dtpm *dtpm;
>>>> +       int i, ret;
>>>> +
>>>> +       for (i = 0; hierarchy[i].name; i++) {
>>>> +
>>>> +               if (hierarchy[i].parent != it)
>>>> +                       continue;
>>>> +
>>>> +               dtpm = dtpm_node_callback[hierarchy[i].type](&hierarchy[i], parent);
>>>> +               if (!dtpm || IS_ERR(dtpm))
>>>> +                       continue;
>>>> +
>>>> +               ret = dtpm_for_each_child(hierarchy, &hierarchy[i], dtpm);
>>>
>>> Why do you need to recursively call dtpm_for_each_child() here?
>>>
>>> Is there a restriction on how the dtpm core code manages adding
>>> children/parents?
>>
>> [ ... ]
>>
>> The recursive call is needed given the structure of the tree in an array
>> in order to connect with the parent.
> 
> Right, I believe I understand what you are trying to do here, but I am
> not sure if this is the best approach to do this. Maybe it is.
> 
> The problem is that we are also allocating memory for a dtpm and we
> call dtpm_register() on it in this execution path - and this memory
> doesn't get freed up nor unregistered, if any of the later recursive
> calls to dtpm_for_each_child() fails.
> 
> The point is, it looks like it can get rather messy with the recursive
> calls to cope with the error path. Maybe it's easier to store the
> allocated dtpms in a list somewhere and use this to also find a
> reference of a parent?

I think it is better to continue the construction with other nodes even
some of them failed to create, it should be a non critical issue. As an
analogy, if one thermal zone fails to create, the other thermal zones
are not removed.

In addition, that should allow multiple nodes description for different
DT setup for the same platform. That should fix the issue pointed by Bjorn.

> Later on, when we may decide to implement "dtpm_destroy_hierarchy()"
> (or whatever we would call such interface), you probably need a list
> of the allocated dtpms anyway, don't you think?

No it is not necessary, the dtpms list is the dtpm tree itself and it
can be destroyed recursively.

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