Re: [PATCH v10 01/13] powercap: Add enable disable control-type

[Cristian Marussi <cristian.marussi@arm.com>]

On Tue, Nov 03, 2026 at 11:09:55PM +0000, Philip Radford wrote:
> Add functionality to disable or enable the Powercap control-type by writing
> directly into sys/class/powercap/arm-scmi/enabled.
> 

Hi Philip,

see a some general remarks down below.

> Signed-off-by: Philip Radford <philip.radford@arm.com>
> ---
> V9->V10
> - changed control-type enable writes to snapshot per-zone state instead of
>   aggregate
> - cancel enable/disable writes if a root child zone is not configurable
> - roll back changed zones on failure
> - changed control-type enable reads to report enabled if any root child zone
>   is enabled
> ---
>  drivers/powercap/arm_scmi_powercap.c | 175 ++++++++++++++++++++++++++-
>  1 file changed, 174 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/powercap/arm_scmi_powercap.c b/drivers/powercap/arm_scmi_powercap.c
> index ab66e9a3b1e2..ba398048bf21 100644
> --- a/drivers/powercap/arm_scmi_powercap.c
> +++ b/drivers/powercap/arm_scmi_powercap.c
> @@ -33,13 +33,171 @@ struct scmi_powercap_zone {
>  
>  struct scmi_powercap_root {
>  	unsigned int num_zones;
> +	bool enabled;
> +	struct list_head node;
>  	struct scmi_powercap_zone *spzones;
>  	struct list_head *registered_zones;
>  	struct list_head scmi_zones;
>  };
>  
> +static LIST_HEAD(scmi_powercap_roots);
> +static DEFINE_MUTEX(scmi_powercap_roots_lock);
> +
>  static struct powercap_control_type *scmi_top_pcntrl;
>  
> +static bool scmi_powercap_is_control_type_child(const struct scmi_powercap_zone *spz)
> +{
> +	return spz->registered && !spz->invalid &&
> +	       spz->info->parent_id == SCMI_POWERCAP_ROOT_ZONE_ID;

Not sure to remember clearly the logic here, but is it possible for a
zone to be registered and invalid ?
I made some recent changes/fixes but the invariant that registered
implied !invalid seemed to apply before and after my fixes...

> +}
> +
> +static int
> +scmi_powercap_read_root_children_enable_state(struct scmi_powercap_root *pr, bool *mode)
> +{
> +	struct scmi_powercap_zone *spz;
> +	bool enabled;
> +	int i, ret;
> +
> +	*mode = false;
> +
> +	for (i = 0; i < pr->num_zones; i++) {

	for (int i = 0 ...

> +		spz = &pr->spzones[i];
> +
> +		if (!scmi_powercap_is_control_type_child(spz))
> +			continue;
> +
> +		ret = powercap_ops->cap_enable_get(spz->ph, spz->info->id, &enabled);
> +		if (ret)
> +			return ret;
> +
> +		if (enabled) {
> +			*mode = true;
> +			return 0;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +struct scmi_powercap_zone_state {
> +	struct scmi_powercap_zone *spz;
> +	bool prev_enabled;
> +	bool changed;
> +};
> +
> +static int
> +scmi_powercap_control_type_set_enable(struct powercap_control_type *pct, bool mode)
> +{
> +	struct scmi_powercap_zone_state *states;
> +	struct scmi_powercap_root *pr;
> +	struct scmi_powercap_zone *spz;
> +	int num_zones = 0, nr_states = 0;
> +	int i, ret = 0;

... while looking at some other previous Sashiko reported powercap issue
I had a superficial look at this, and beside the logic that seems to be
fine by itself, the code layout is deadly unreadable, thing which is
pretty normal to happen when there is a mix of allocations mutexes and
various error exit path....please refactor using cleanup handlers to
manage all of this allocations and mutexes, it should allow you to drop
all the gotos and keep a more linear logic across all of this fucntion..
(and NOTE that when using cleanup.h you MUST drop all of the gotos in
this function...all info inside cleanup.h too)
 
> +
> +	mutex_lock(&scmi_powercap_roots_lock);
> +	list_for_each_entry(pr, &scmi_powercap_roots, node) {
> +		for (i = 0; i < pr->num_zones; i++) {
> +			spz = &pr->spzones[i];
> +
> +			if (scmi_powercap_is_control_type_child(spz))
> +				num_zones++;
> +		}
> +	}
> +
> +	states = kcalloc(num_zones, sizeof(*states), GFP_KERNEL);
> +	if (!states) {
> +		ret = -ENOMEM;
> +		goto out_unlock;
> +	}
> +
> +	list_for_each_entry(pr, &scmi_powercap_roots, node)	{
> +		for (i = 0; i < pr->num_zones; i++) {
> +			spz = &pr->spzones[i];
> +
> +			if (!scmi_powercap_is_control_type_child(spz))
> +				continue;
> +
> +			if (!spz->info->powercap_cap_config) {
> +				ret = -EOPNOTSUPP;
> +				goto out_free;
> +			}
> +
> +			states[nr_states].spz = spz;
> +
> +			ret = powercap_ops->cap_enable_get(spz->ph, spz->info->id,
> +				&states[nr_states].prev_enabled);

also...I have not checked this myself, BUT do you use checkpatch --strict that I
can see a few strange indents...

> +
> +			if (ret)
> +				goto out_free;
> +
> +			nr_states++;
> +		}
> +	}
> +
> +	for (i = 0; i < nr_states; i++) {
> +		spz = states[i].spz;
> +
> +		if (states[i].prev_enabled == mode)
> +			continue;
> +
> +		ret = powercap_ops->cap_enable_set(spz->ph, spz->info->id, mode);
> +		if (ret)
> +			goto revert;
> +
> +		states[i].changed = true;
> +	}
> +
> +	goto out_free;
> +
> +revert:
> +	while (i--) {
> +		spz = states[i].spz;
> +
> +		if (!states[i].changed)
> +			continue;
> +
> +		powercap_ops->cap_enable_set(spz->ph, spz->info->id,
> +			states[i].prev_enabled);

ditto...checkpatch

> +	}
> +
> +out_free:
> +			kfree(states);
> +
ditto...checkpatch
> +out_unlock:
> +			mutex_unlock(&scmi_powercap_roots_lock);
ditto...checkpatch
> +
> +			return ret;
> +}
> +
> +static int
> +scmi_powercap_control_type_get_enable(struct powercap_control_type *pct, bool *mode)
> +{
> +	struct scmi_powercap_root *pr;
> +	int ret = 0;
> +

same here...even if it is a smaller function using cleanup guard helps
streamlining the code...

> +	*mode = false;
> +
> +	mutex_lock(&scmi_powercap_roots_lock);
> +	list_for_each_entry(pr, &scmi_powercap_roots, node) {
> +		ret = scmi_powercap_read_root_children_enable_state(pr, &pr->enabled);
> +
> +		if (ret)
> +			break;
> +		if (pr->enabled) {
> +			*mode = true;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&scmi_powercap_roots_lock);
> +
> +	return ret;
> +}
> +

Thanks,
Cristian