Re: [PATCH v6 2/5] powercap/drivers/dtpm: Add hierarchy creation

[Daniel Lezcano]

On 24/01/2022 21:00, Ulf Hansson wrote:
> On Wed, 19 Jan 2022 at 09:58, Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>>  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