Messages in this thread | | | Subject | Re: [PATCH v6 2/5] powercap/drivers/dtpm: Add hierarchy creation | From | Daniel Lezcano <> | Date | Tue, 25 Jan 2022 11:46:48 +0100 |
| |
On 24/01/2022 21:00, Ulf Hansson wrote: > 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!
With the above fixed, shall I add your reviewed-by ?
-- <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
| |