Re: [PATCH v5 12/12] powercap: arm_scmi: Synthetic zone enable/disable

[Philip Radford]

On Tue, May 05, 2026 at 11:28:09PM +0100, Cristian Marussi wrote:
> On Tue, Apr 28, 2026 at 10:09:21AM +0100, Philip Radford wrote:
> > Add functionality to disable and enable the synthetic zone which
> > also affects the immediate children of the synthetic zone by applying
> > the same command to them.
> > 

Hi,

> > Signed-off-by: Philip Radford <philip.radford@xxxxxxx>
> > ---
> >  drivers/powercap/arm_scmi_powercap.c | 81 ++++++++++++++++++++++++++++
> >  1 file changed, 81 insertions(+)
> > 
> > diff --git a/drivers/powercap/arm_scmi_powercap.c b/drivers/powercap/arm_scmi_powercap.c
> > index 81b5214acda4..1ed2949b06cb 100644
> > --- a/drivers/powercap/arm_scmi_powercap.c
> > +++ b/drivers/powercap/arm_scmi_powercap.c
> > @@ -270,6 +270,85 @@ static int instance_root_release(struct powercap_zone *pz)
> >  	return 0;
> >  }
> >  
> > +static int instance_root_set_enable_state(struct powercap_zone *pz, bool enable)
> > +{
> > +	struct scmi_powercap_zone *root;
> > +	struct scmi_powercap_root *pr;
> > +	struct scmi_powercap_zone *child;
> 
> ...child and root on the same line of declarations...
> 

Noted.

> > +	int ret, first_err = 0;
> > +
> > +	if (!pz)
> > +		return -EINVAL;
> > +
> > +	root = to_scmi_powercap_zone(pz);
> > +	pr = container_of(root, struct scmi_powercap_root, instance_root);
> 
> ...another user of you new macro !
> 

:)

> > +
> > +	list_for_each_entry(child, &pr->registered_zones[0], node) {
> > +		if (child == &pr->instance_root)
> > +			continue;
> > +
> > +		if (child->info->parent_id != SCMI_POWERCAP_ROOT_ZONE_ID)
> > +			continue;
> > +
> > +		if (!child->info->cpli[0].cap_config)
> > +			continue;
> > +
> > +		ret = powercap_ops->cap_enable_set(child->ph, child->info->id, enable);
> > +
> > +		if (ret && !first_err) {
> 
> ...mmm what is the logic here ? why not bailing out on any error ?
> 

Good point. I was initially trying to prevent one error stopping everything without
checking the other children, but even as I'm writing this out I can see the fault in
that logic.

> > +			first_err = ret;
> > +			dev_err(child->dev, "failed to %s zone %s: %d\n",
> > +				enable ? "enable" : "disable",
> > +				child->info->name, ret);
> > +		}
> > +	}
> > +
> > +	return first_err;
> 
> ...especially if you anyway fails globally on any error....
> ..I am not completely sure bit given that youare operating on a
> synthetic zone that you enable as a whole, while acting on its children
> in the backstage...I would say that if any enable/disable fails on a
> chidlren you should revert the enable status of the children thate were
> succesffull and report the error...I mean the state of top synthatic
> zones AND the states of the children MUST remain consistent...
> ...it CANNOT be that some chidlren fails, some succeeds and you report
> an error..it must be all or nothing...
> ...example..top syntethic zone is OFF if all children were successfully
> disabled...on a failure with one of the children you shoudl revert the
> already successfully set children and report the global error...
> 

Point taken, this needs reworking.

> > +}
> > +
> > +static int instance_root_set_enable(struct powercap_zone *pz, bool mode)
> > +{
> > +	return instance_root_set_enable_state(pz, mode);
> > +}
> > +
> > +static int instance_root_get_enable(struct powercap_zone *pz, bool *mode)
> > +{
> > +	struct scmi_powercap_zone *root;
> > +	struct scmi_powercap_root *pr;
> > +	struct scmi_powercap_zone *child;
> > +	bool enabled;
> > +	int ret;
> > +
> > +	if (!pz || !mode)
> > +		return -EINVAL;
> > +
> > +	root = to_scmi_powercap_zone(pz);
> > +	pr = container_of(root, struct scmi_powercap_root, instance_root);
> > +
> > +	*mode = true;
> > +
> > +	list_for_each_entry(child, &pr->registered_zones[0], node) {
> 
> mmm...what is the point here of scanning the children to GET the
> state...you should report the top syntethic zone state right ?
> You could have disable children directly...that wont be reflected in the
> Linux powercap hiearcrhy right ?
> I mean should you NOT simply return the stae of the top syntethic zone
> which is should have saved in the previous state_set operation above ?
> 
> I think that anyway if you disable a zone...any zone...ONLY that zone is
> marked as disable in Lnux powercap ... am I right ? 
> 
> Then probably our SCMI fw will do much more on all the children..
> 

Good point, I'll simplify it to report a saved state for the synthetic root.

Regards,
Phil