Messages in this thread | | | Subject | Re: [PATCH v5 2/6] powercap/drivers/dtpm: Add hierarchy creation | From | Daniel Lezcano <> | Date | Tue, 11 Jan 2022 18:52:07 +0100 |
| |
On 11/01/2022 09:28, Ulf Hansson wrote: > On Mon, 10 Jan 2022 at 16:55, Daniel Lezcano <daniel.lezcano@linaro.org> 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.
I agree, we could improve that but it is way too complex to be addressed in a single series and should be part of a specific change IMO.
> In any case, papering over the error seems quite scary to me. I would > much prefer if we instead could propagate the error code correctly to > the caller of dtpm_create_hierarchy(), to allow it to retry if > necessary.
It is really something we should be able to address later.
>> 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. > > I could quite figure out how that should work though, but I assume > that is what the ->release() callback in the struct dtpm_ops is there > to help with, in some way. > > Kind regards > Uffe >
-- <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
| |