Re: [PATCH v5 10/12] powercap: arm_scmi: Create synthetic parent node for multi-instance

[Philip Radford <philip.radford@arm.com>]

On Tue, May 05, 2026 at 11:03:34PM +0100, Cristian Marussi wrote:
> On Tue, Apr 28, 2026 at 10:09:19AM +0100, Philip Radford wrote:
> > SCMI powercap domains are exposed as a flat list and may include
> > multiple top-level domains without a parent. When registered with
> > the powercap framework, these appear as independent root zones
> > with no common hierarchy. The driver probes domains per SCMI
> > instance, but when registering with the powercap framework, these
> > would be combined into a single tree. This is particularly evident
> > in multi-instance setups where zone IDs and parent zones can only
> > coexist with other zones and parents from the same instance.
> 
> Hi,
> 

Hi, thanks for the review.

> I know the problem you are trying to solve is real and advised for this
> solution BUT saying that the domains are exposed in a flat list upfront
> is not correct and misleading I think...the problem revolves around the
> fact that...
> 
> SCMI powercap domains are represented with a hierarchical structure
> (if any such structure exists platform side) which can be comprised
> of multiple trees (a forest of powercap zones trees) each with its own
> top root powercap zone.
> On a system designed to expose multipe SCMI instances each of such
> instance could enumerate a pretty much similar/simmetrical hierarchy of
> powercap zomes, all sharing most probably the same IDs for children and
> parents...all of these simmetrical hierarchies can coexist as long as
> they are confined into their own instance 'space'.
> 
> Given that the Powercap framework does NOT have the concept of instances
> collating all of these zones into the same Linux Powercap top node leads
> to an artificially flattened tree of powercap where all the boudnaries
> between instances are squashes into one single structure....possibly
> combining multiple unrelated SCMI zones under a single unrelated Linux
> powercap zone (by virtue of simmetrical parent-child relations)
> 
> So the need for a (functionally limited) unifying synthetic top root for
> each instance...
> 
> I am sure you can summarize all of these better :P):
> 

Understood. I will re-work the explanation in a better way.

> > 
> > Create a synthetic root zone to act as a common parent for all
> > top-level domains. Creates a single hierarchy and unified entry
> > point per-instance for userspace.
> > 
> > Signed-off-by: Philip Radford <philip.radford@arm.com>
> > ---
> > ---
> >  drivers/powercap/arm_scmi_powercap.c | 67 ++++++++++++++++++++++++++--
> >  1 file changed, 64 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/powercap/arm_scmi_powercap.c b/drivers/powercap/arm_scmi_powercap.c
> > index 7f2bb162f96c..d74869af1633 100644
> > --- a/drivers/powercap/arm_scmi_powercap.c
> > +++ b/drivers/powercap/arm_scmi_powercap.c
> > @@ -36,6 +36,7 @@ struct scmi_powercap_root {
> >  	struct scmi_powercap_zone *spzones;
> >  	struct list_head *registered_zones;
> >  	struct list_head scmi_zones;
> > +	struct scmi_powercap_zone instance_root;
> >  };
> >  
> >  static struct powercap_control_type *scmi_top_pcntrl;
> > @@ -263,12 +264,47 @@ static const struct powercap_zone_constraint_ops constraint_ops  = {
> >  	.get_name = scmi_powercap_get_name,
> >  };
> >  
> > +/* Multi-instance constraints to meet driver requrements */
> > +static int instance_root_release(struct powercap_zone *pz)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int instance_root_get_power_uw(struct powercap_zone *pz, u64 *v)
> > +{
> > +	*v = 0;
> > +	return 0;
> > +}
> > +
> > +static int instance_root_set_constraint(struct powercap_zone *pz, int cid, u64 v)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > +static int instance_root_get_constraint(struct powercap_zone *pz, int cid, u64 *v)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> 
> I would explain here better WHY you cannot really implement all the
> fuctionalities on these toproot syntehtc zones...and WHY it is NOT an
> issue but STILL it is enough to solve our problem with the multiple
> instances configurations...
> 

No probs. I'll include the fact that full zone semantics aren't available for the
synthetic zone.

> > +
> > +static const struct powercap_zone_ops instance_root_ops = {
> > +	.get_max_power_range_uw = scmi_powercap_get_max_power_range_uw,
> > +	.get_power_uw = instance_root_get_power_uw,
> > +	.release = instance_root_release,
> > +};
> > +
> > +static const struct powercap_zone_constraint_ops instance_root_const_ops = {
> > +	.set_power_limit_uw = instance_root_set_constraint,
> > +	.get_power_limit_uw = instance_root_get_constraint,
> > +	.set_time_window_us = instance_root_set_constraint,
> > +	.get_time_window_us = instance_root_get_constraint,
> > +};
> > +
> >  static void scmi_powercap_unregister_all_zones(struct scmi_powercap_root *pr)
> >  {
> >  	int i;
> >  
> >  	/* Un-register children zones first starting from the leaves */
> > -	for (i = pr->num_zones - 1; i >= 0; i--) {
> > +	for (i = pr->num_zones; i >= 0; i--) {
> >  		if (!list_empty(&pr->registered_zones[i])) {
> >  			struct scmi_powercap_zone *spz;
> >  
> > @@ -313,7 +349,6 @@ static int scmi_powercap_register_zone(struct scmi_powercap_root *pr,
> >  				   parent ? &parent->zone : NULL,
> >  				   &zone_ops, spz->info->num_cpli, &constraint_ops);
> >  	if (!IS_ERR(z)) {
> > -		spz->height = scmi_powercap_get_zone_height(spz);
> 
> I have to admit I'd have to look back at how I implemneted this powercap zone
> walking at registration tome :D .. but HOW can you get rid of spz->height here ?
> 

I've overlooked this, sorry. I'd removed it to set spz to include the synthetic zone if it
was teh parent zone and ignore it if not. I can't rememeber removing the new logic but I'll
rectify it for V6.

> ...also because now spz->height is NOT set anywhere (presumable zeroed
> at init) and also why you have NOT removed also scmi_powercap_get_zone_height
> since nobody is calling it ?
> 
> ...also because I am pretty sure that all the management of the height of
> the nodes in the binary trees where crucial when tearing down the hierarchy of
> powercap zones ... (see explanation preceding scmi_zones_register())
> 
> ...mmmm.... have you tried to unload the arm_scmi_powercap driver with these
> new changes for the syntethic powercap zones ?
> 
> I just did :P
> 
> ---8<----
> root@deb-guest:~# modprobe -r arm_scmi_powercap
> [   28.939470] Unable to handle kernel paging request at virtual address fffffffffffffc90
> [   28.942910] Mem abort info:
> [   28.947866]   ESR = 0x0000000096000006
> [   28.953034]   EC = 0x25: DABT (current EL), IL = 32 bits
> [   28.957189]   SET = 0, FnV = 0
> [   28.957437]   EA = 0, S1PTW = 0
> [   28.958254]   FSC = 0x06: level 2 translation fault
> [   28.959025] Data abort info:
> [   28.959511]   ISV = 0, ISS = 0x00000006, ISS2 = 0x00000000
> [   28.960049]   CM = 0, WnR = 0, TnD = 0, TagAccess = 0
> [   28.961191]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> [   28.962227] swapper pgtable: 4k pages, 48-bit VAs, pgdp=000000010261b000
> [   28.963105] [fffffffffffffc90] pgd=0000000000000000, p4d=00000001030ff403, pud=0000000103100403, pmd=0000000000000000
> [   28.964189] Internal error: Oops: 0000000096000006 [#1]  SMP
> [   28.964994] Modules linked in: arm_scmi_powercap(-) cpufreq_powersave cpufreq_conservative
> [   28.966300] CPU: 2 UID: 0 PID: 227 Comm: modprobe Not tainted 7.1.0-rc1-00012-g383eb3db9548 #2 PREEMPT 
> [   28.967170] Hardware name: linux,dummy-virt (DT)
> [   28.967925] pstate: 61400005 (nZCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
> [   28.969245] pc : device_del+0x3c/0x3b0
> [   28.969963] lr : device_unregister+0x28/0x90
> [   28.970342] sp : ffff800083b73b30
> [   28.970753] x29: ffff800083b73b50 x28: ffff000005d86000 x27: 0000000000000000
> [   28.971448] x26: 0000000000000000 x25: 0000000000000000 x24: 0000000000000000
> [   28.972159] x23: fffffffffffffff0 x22: ffff80007b1e30c0 x21: ffff000006f3c080
> [   28.972701] x20: fffffffffffffc30 x19: fffffffffffffc50 x18: 0000000000000000
> [   28.973237] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
> [   28.973899] x14: 0000000000000000 x13: 0000000000000000 x12: ffff000007338880
> [   28.974795] x11: ffff0000056bbc10 x10: ffff000009ca0c00 x9 : ffff800080c44480
> [   28.975483] x8 : 0101010101010101 x7 : 7f7f7f7f7f7f7f7f x6 : fefefeff6364626d
> [   28.976161] x5 : 8080808000000000 x4 : 0000000000000020 x3 : ffff80008243c534
> [   28.976842] x2 : ffff000005d86000 x1 : 0000000000000000 x0 : fffffffffffffce0
> [   28.977511] Call trace:
> [   28.977738]  device_del+0x3c/0x3b0 (P)
> [   28.978047]  device_unregister+0x28/0x90
> [   28.978371]  powercap_unregister_zone+0x54/0x78
> [   28.978781]  scmi_powercap_unregister_all_zones+0x84/0xc8 [arm_scmi_powercap]
> [   28.979344]  scmi_powercap_remove+0x1c/0x38 [arm_scmi_powercap]
> [   28.979821]  scmi_dev_remove+0x28/0x40
> [   28.980136]  device_remove+0x54/0x90
> [   28.980428]  device_release_driver_internal+0x1cc/0x230
> [   28.980845]  driver_detach+0x54/0xc0
> [   28.981136]  bus_remove_driver+0x74/0x100
> [   28.981454]  driver_unregister+0x38/0x68
> [   28.981774]  scmi_driver_unregister+0x28/0x1a0
> [   28.982147]  scmi_powercap_exit+0x18/0xe20 [arm_scmi_powercap]
> [   28.982560]  __arm64_sys_delete_module+0x1b0/0x2e0
> [   28.982870]  invoke_syscall.constprop.0+0x48/0x120
> [   28.983219]  el0_svc_common.constprop.0+0x40/0xe8
> [   28.983628]  do_el0_svc+0x24/0x38
> [   28.983916]  el0_svc+0x38/0x138
> [   28.984201]  el0t_64_sync_handler+0xa0/0xe8
> [   28.984557]  el0t_64_sync+0x198/0x1a0
> [   28.984901] Code: f9429c01 f9000fe1 d2800001 91024260 (f9402276) 
> [   28.985443] ---[ end trace 0000000000000000 ]---
> Segmentation fault
> --->8----
> 
> This does NOT happen without your syntethic node changes since the driver on
> unload took care to remove/unregister powercap zones starting from the
> leaves so as not to remove parents that were still referenced ....
> 
> .... you should always test also as a loadable module and manually load/unload
> then testing...
> 
> I will review this again in V6...plesse double check to preserve the
> original zone-walking algo...

Understood. Sorry, I'll fully test for V6.