Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20220119085719.1357874-1-daniel.lezcano@linaro.org> <20220119085719.1357874-3-daniel.lezcano@linaro.org>
In-Reply-To: <20220119085719.1357874-3-daniel.lezcano@linaro.org>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Mon, 24 Jan 2022 21:00:25 +0100
Message-ID: <CAPDyKFrtq0J2O0WBwLr7Zb+WkomhLcR1h+eDzzV-SxpiJmm_yQ@mail.gmail.com>
Subject: Re: [PATCH v6 2/5] powercap/drivers/dtpm: Add hierarchy creation
To:     Daniel Lezcano <daniel.lezcano@linaro.org>
Cc:     rjw@rjwysocki.net, robh@kernel.org, lukasz.luba@arm.com,
        heiko@sntech.de, arnd@linaro.org, linux-kernel@vger.kernel.org,
        linux-pm@vger.kernel.org, "Rafael J. Wysocki" <rafael@kernel.org>,
        Daniel Lezcano <daniel.lezcano@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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