Re: [PATCH v4 4/6] powercap: dtpm: Use generic powercap hierarchy helpers

[Lukasz Luba]

Hi Daniel,

On 10/29/26 16:56, Daniel Lezcano wrote:
> DTPM implements its own hierarchy creation and destruction logic while
> the powercap core now provides generic helpers for this purpose.
>
> Convert DTPM to use the generic powercap hierarchy infrastructure.
> Duplicate the platform hierarchy before creating it, and use
> powercap_hierarchy_create() and powercap_hierarchy_destroy() to manage
> the DTPM zones.
>
> Add DTPM callbacks to create and destroy the zones associated with each
> powercap hierarchy node. Adapt the CPU and devfreq backends to return
> the created DTPM instance and use the node name provided by the
> hierarchy.
>
> Convert the Rockchip RK3399 description to struct powercap_hierarchy
> and keep the device tree path as DTPM-specific node data.
>
> This removes the DTPM-specific hierarchy traversal and destruction code
> and leaves the hierarchy management to the powercap core.
>
> Preserve best-effort creation: an unavailable backend returns NULL to
> skip its branch, and setup failures are warned about and skipped rather
> than rolling back independent zones. Invalid node descriptors remain
> fatal.
>
> When the representative CPU is offline, find an active policy through an
> online sibling whose related_cpus mask contains it. Hold the CPU hotplug
> read lock during selection and setup, and initialize the zone through the
> policy's active CPU while keeping the hierarchy-provided name. An entirely
> offline policy still has no active CPU to initialize through.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxxxxxxxx>
> ---
> Changes in v4 (since v3):
> - Rebase on v7.3; no code or commit-description changes.
>
> Changes in v3 (since v2):
> - Rebase on v7.3-rc7.
> - Preserve best-effort branch creation using the generic NULL result.
> - Find an online policy sibling for offline CPU representatives and
>    protect policy selection and setup against CPU hotplug.
> - Keep generic API behavior and documentation in patch 1.
> - Fix __initdata placement; match-table indentation is handled in patch 3.
> - Name setup callback arguments and remove whitespace-only lines.
> - Update the description.
>
>   drivers/powercap/dtpm.c         | 177 +++++++++++++++-----------------
>   drivers/powercap/dtpm_cpu.c     |  62 ++++++++---
>   drivers/powercap/dtpm_devfreq.c |  22 ++--
>   drivers/soc/rockchip/dtpm.c     |  67 +++++++++---
>   include/linux/dtpm.h            |   6 +-
>   5 files changed, 195 insertions(+), 139 deletions(-)

Tha patch in general looks good. I only have one
comment related to the future usage of this mechanism.
For this implementation it's safe, but I would add
some extra safety net around the of_find_node_by_path().
See comments below.
[snip]

>   
> -static struct dtpm *dtpm_setup_dt(const struct dtpm_node *hierarchy,
> +static struct dtpm *dtpm_setup_dt(const char *name, const char *path,
>   				  struct dtpm *parent)
>   {
>   	struct device_node *np;
> -	int i, ret;
> +	struct dtpm *dtpm = NULL;
> +	int i;
>   
> -	np = of_find_node_by_path(hierarchy->name);


I would add a NULL check for 'path' here, or below
where dtpm_setup_dt() is called.

Usually it's up the the code which is calling that API
to make sure that the NULL is not sneaked in.
We probably can force drivers code for that, right?

> +	np = of_find_node_by_path(path);
>   	if (!np) {
> -		pr_err("Failed to find '%s'\n", hierarchy->name);
> +		pr_err("Failed to find '%s'\n", path);
>   		return ERR_PTR(-ENXIO);
>   	}
>   
>   	for (i = 0; i < ARRAY_SIZE(dtpm_subsys); i++) {
> +		struct dtpm *tmp;
>   
>   		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);
> +		tmp = dtpm_subsys[i]->setup(parent, np, name);
> +		if (IS_ERR(tmp)) {
> +			pr_err("Failed to setup '%s': %ld\n",
> +			       dtpm_subsys[i]->name, PTR_ERR(tmp));
> +			dtpm = tmp;
> +			break;
> +		}
> +
> +		if (tmp) {
> +			dtpm = tmp;
> +			break;
>   		}
>   	}
>   
>   	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;
> +	return dtpm;
>   }
>   
> -typedef struct dtpm * (*dtpm_node_callback_t)(const struct dtpm_node *, struct dtpm *);
> -
> -static 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)
> +static struct powercap_zone *
> +dtpm_node_create(struct powercap_control_type *pct, const char *name,
> +		 void *data, struct powercap_zone *parent)
>   {
> +	struct dtpm_node *node = data;
> +	struct dtpm *dtpm_parent = parent ? to_dtpm(parent) : NULL;
>   	struct dtpm *dtpm;
> -	int i, ret;
>   
> -	for (i = 0; hierarchy[i].name; i++) {
> +	if (!node)
> +		return ERR_PTR(-EINVAL);
> +
> +	switch (node->type) {
> +	case DTPM_NODE_VIRTUAL:
> +		dtpm = dtpm_setup_virtual(name, dtpm_parent);
> +		break;
> +	case DTPM_NODE_DT:

... or a NULL check here.

> +		dtpm = dtpm_setup_dt(name, node->path, dtpm_parent);
> +		break;
> +	default:
> +		return ERR_PTR(-EINVAL);
> +	}


The rest looks good.

If we are sure we would never experience that NULL ptr coming to those
registration callbacks - then OK.

Regards,
Lukasz