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

[Daniel Lezcano]

On 12/01/2022 13:00, Ulf Hansson wrote:
> On Tue, 11 Jan 2022 at 18:52, Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote:
>>
>> On 11/01/2022 09:28, Ulf Hansson wrote:
>>> On Mon, 10 Jan 2022 at 16:55, Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote:
>>>>
>>>> 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.
>>>
>>> Well, what if it fails because its "consumer part" is waiting for some
>>> resource to become available?
>>>
>>> Maybe the devfreq driver/subsystem isn't available yet and causes
>>> -EPROBE_DEFER, for example. Perhaps this isn't the way the dtpm
>>> registration works currently, but sure it's worth considering when
>>> going forward, no?
>>
>> It should be solved by the fact that the DTPM description is a module
>> and loaded after the system booted. The module loading ordering is
>> solved by userspace.
> 
> Ideally, yes. However, drivers/subsystems in the kernel should respect
> -EPROBE_DEFER. It's good practice to do that.

Certainly.

However, it does not make sense because dtpm is not a device and I don't
see a device returning EPROBE_DEFER right now.

Wanting to handle EPROBE_DEFER will make the code a gaz factory:
 - shall we destroy the hierarchy each time a device is returning a
EPROBE_DEFER ?
     * yes : then we need to recreate it every time we recall it and we
end with an empty tree in case of error
     * no : we have to keep track of what was created or not, in order
to attach the newly device to the tree with a the parent, etc ...

So an incredible complexity for actually having no device returning
EPROBE_DEFER.

In addition, let's imagine one of the component like cpufreq is a
module, no EPROBE_DEFER handling will prevent the description being
created before the cpufreq driver is loaded.

But... I agree the hierarchy creation function should be called after
all the devices were created. For that, I think the kernel is providing
what is needed:

1. We compile the SoC specific dtpm always as a module

	depends on ... && m

2. In the module we add the dependencies to other modules

	MODULE_SOFTDEP(post: panfrost)

And with that, all dependencies are explicitly described and the
hierarchy creation is safe.

Does it make sense ?



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