Messages in this thread | | | From | Ulf Hansson <> | Date | Fri, 7 Jan 2022 16:54:01 +0100 | Subject | Re: [PATCH v5 2/6] powercap/drivers/dtpm: Add hierarchy creation |
| |
[...]
> >> +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?
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?
[...]
Kind regards Uffe
| |