Messages in this thread | | | Subject | Re: [PATCH v5 2/6] powercap/drivers/dtpm: Add hierarchy creation | From | Daniel Lezcano <> | Date | Fri, 14 Jan 2022 20:15:43 +0100 |
| |
On 12/01/2022 13:00, Ulf Hansson wrote: > On Tue, 11 Jan 2022 at 18:52, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: >> >> 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. > > 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
| |