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

Philip Radford
2026-05-06T10:51:43Z

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@arm.com>
> > ---
> > 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