Messages in this thread | | | From | Ulf Hansson <> | Date | Mon, 24 Jan 2022 21:00:25 +0100 | Subject | Re: [PATCH v6 2/5] powercap/drivers/dtpm: Add hierarchy creation |
| |
On Wed, 19 Jan 2022 at 09:58, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > The DTPM framework is available but without a way to configure it. > > This change provides a way to create a hierarchy of DTPM node where > the power consumption reflects the sum of the children's power > consumption. > > It is up to the platform to specify an array of dtpm nodes where each > element has a pointer to its parent, except the top most one. The type > of the node gives the indication of which initialization callback to > call. At this time, we can create a virtual node, where its purpose is > to be a parent in the hierarchy, and a DT node where the name > describes its path. > > In order to ensure a nice self-encapsulation, the DTPM subsys array > contains a couple of initialization functions, one to setup the DTPM > backend and one to initialize it up. With this approach, the DTPM > framework has a very few material to export. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > drivers/powercap/Kconfig | 1 + > drivers/powercap/dtpm.c | 168 ++++++++++++++++++++++++++++++++++++++- > include/linux/dtpm.h | 15 ++++ > 3 files changed, 181 insertions(+), 3 deletions(-) > > diff --git a/drivers/powercap/Kconfig b/drivers/powercap/Kconfig > index 8242e8c5ed77..b1ca339957e3 100644 > --- a/drivers/powercap/Kconfig > +++ b/drivers/powercap/Kconfig > @@ -46,6 +46,7 @@ config IDLE_INJECT > > config DTPM > bool "Power capping for Dynamic Thermal Power Management (EXPERIMENTAL)" > + depends on OF > help > This enables support for the power capping for the dynamic > thermal power management userspace engine. > diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c > index 0e5c93443c70..10032f7132c4 100644 > --- a/drivers/powercap/dtpm.c > +++ b/drivers/powercap/dtpm.c > @@ -23,6 +23,7 @@ > #include <linux/powercap.h> > #include <linux/slab.h> > #include <linux/mutex.h> > +#include <linux/of.h> > > #include "dtpm_subsys.h" > > @@ -463,14 +464,175 @@ int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent) > return 0; > } > > -static int __init init_dtpm(void) > +static struct dtpm *dtpm_setup_virtual(const struct dtpm_node *hierarchy, > + struct dtpm *parent) > { > + struct dtpm *dtpm; > + int ret; > + > + dtpm = kzalloc(sizeof(*dtpm), GFP_KERNEL); > + if (!dtpm) > + return ERR_PTR(-ENOMEM); > + dtpm_init(dtpm, NULL); > + > + ret = dtpm_register(hierarchy->name, dtpm, parent); > + if (ret) { > + pr_err("Failed to register dtpm node '%s': %d\n", > + hierarchy->name, ret); > + kfree(dtpm); > + return ERR_PTR(ret); > + } > + > + return dtpm; > +} > + > +static struct dtpm *dtpm_setup_dt(const struct dtpm_node *hierarchy, > + struct dtpm *parent) > +{ > + struct device_node *np; > + int i, ret; > + > + np = of_find_node_by_path(hierarchy->name); > + if (!np) { > + pr_err("Failed to find '%s'\n", hierarchy->name); > + return ERR_PTR(-ENXIO); > + } > + > + for (i = 0; i < ARRAY_SIZE(dtpm_subsys); i++) { > + > + if (!dtpm_subsys[i]->setup) > + continue; > + > + ret = dtpm_subsys[i]->setup(parent, np); > + if (ret) { > + pr_err("Failed to setup '%s': %d\n", dtpm_subsys[i]->name, ret); > + of_node_put(np); > + return ERR_PTR(ret); > + } > + } > + > + of_node_put(np); > + > + /* > + * By returning a NULL pointer, we let know the caller there > + * is no child for us as we are a leaf of the tree > + */ > + return NULL; > +} > + > +typedef struct dtpm * (*dtpm_node_callback_t)(const struct dtpm_node *, struct dtpm *); > + > +dtpm_node_callback_t dtpm_node_callback[] = { > + [DTPM_NODE_VIRTUAL] = dtpm_setup_virtual, > + [DTPM_NODE_DT] = dtpm_setup_dt, > +}; > + > +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))
This can be tested with the "IS_ERR_OR_NULL()" macro.
> + continue;
We have discussed the error path previously. Just ignoring errors here and continuing with the initialization, isn't normally how we design good kernel code.
However, you have also explained that the error path is special and somewhat non-trivial to manage in this case. I get that now and thanks for clarifying.
Nevertheless, I think it deserves to be explained a bit with a comment in the code of what goes on here. Otherwise another developer that looks at this code in the future, may think it looks suspicious too.
> + > + ret = dtpm_for_each_child(hierarchy, &hierarchy[i], dtpm); > + if (ret) > + return ret; > + } > + > + return 0; > +}
[...]
Other than the above, this looks good to me!
Kind regards Uffe
| |