Message-ID: <4f523696-54ab-427d-974a-3c5ffb865fed@arm.com>
Date: Fri, 19 Sep 2025 17:10:25 +0100
From: James Morse <james.morse@....com>
To: Jonathan Cameron <jonathan.cameron@...wei.com>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-acpi@...r.kernel.org,
 D Scott Phillips OS <scott@...amperecomputing.com>,
 carl@...amperecomputing.com, lcherian@...vell.com,
 bobo.shaobowang@...wei.com, tan.shaopeng@...itsu.com,
 baolin.wang@...ux.alibaba.com, Jamie Iles <quic_jiles@...cinc.com>,
 Xin Hao <xhao@...ux.alibaba.com>, peternewman@...gle.com,
 dfustini@...libre.com, amitsinght@...vell.com,
 David Hildenbrand <david@...hat.com>, Dave Martin <dave.martin@....com>,
 Koba Ko <kobak@...dia.com>, Shanker Donthineni <sdonthineni@...dia.com>,
 fenghuay@...dia.com, baisheng.gao@...soc.com, Rob Herring <robh@...nel.org>,
 Rohit Mathew <rohit.mathew@....com>, Rafael Wysocki <rafael@...nel.org>,
 Len Brown <lenb@...nel.org>, Lorenzo Pieralisi <lpieralisi@...nel.org>,
 Hanjun Guo <guohanjun@...wei.com>, Sudeep Holla <sudeep.holla@....com>,
 Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Danilo Krummrich <dakr@...nel.org>
Subject: Re: [PATCH v2 01/29] ACPI / PPTT: Add a helper to fill a cpumask from
 a processor container

Hi Jonathan,

On 11/09/2025 11:43, Jonathan Cameron wrote:
> On Wed, 10 Sep 2025 20:42:41 +0000
> James Morse <james.morse@....com> wrote:
> 
>> The ACPI MPAM table uses the UID of a processor container specified in
>> the PPTT to indicate the subset of CPUs and cache topology that can
>> access each MPAM System Component (MSC).
>>
>> This information is not directly useful to the kernel. The equivalent
>> cpumask is needed instead.
>>
>> Add a helper to find the processor container by its id, then walk
>> the possible CPUs to fill a cpumask with the CPUs that have this
>> processor container as a parent.
>>
>> CC: Dave Martin <dave.martin@....com>
>> Reviewed-by: Sudeep Holla <sudeep.holla@....com>
>> Signed-off-by: James Morse <james.morse@....com>
> 
> Hi James,
> 
> Sorry I missed v1.  Busy few weeks.
> 
> I think one resource leak plus a few suggested changes that
> I'm not that bothered about.



>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
>> index 54676e3d82dd..1728545d90b2 100644
>> --- a/drivers/acpi/pptt.c
>> +++ b/drivers/acpi/pptt.c
>> @@ -817,3 +817,86 @@ int find_acpi_cpu_topology_hetero_id(unsigned int cpu)
>>  	return find_acpi_cpu_topology_tag(cpu, PPTT_ABORT_PACKAGE,
>>  					  ACPI_PPTT_ACPI_IDENTICAL);
>>  }
> 
>> +/**
>> + * acpi_pptt_get_cpus_from_container() - Populate a cpumask with all CPUs in a
>> + *                                       processor container
>> + * @acpi_cpu_id:	The UID of the processor container.
>> + * @cpus:		The resulting CPU mask.
>> + *
>> + * Find the specified Processor Container, and fill @cpus with all the cpus
>> + * below it.
>> + *
>> + * Not all 'Processor' entries in the PPTT are either a CPU or a Processor
>> + * Container, they may exist purely to describe a Private resource. CPUs
>> + * have to be leaves, so a Processor Container is a non-leaf that has the
>> + * 'ACPI Processor ID valid' flag set.
>> + *
>> + * Return: 0 for a complete walk, or an error if the mask is incomplete.
>> + */
>> +void acpi_pptt_get_cpus_from_container(u32 acpi_cpu_id, cpumask_t *cpus)
>> +{
>> +	struct acpi_pptt_processor *cpu_node;
>> +	struct acpi_table_header *table_hdr;
>> +	struct acpi_subtable_header *entry;
>> +	unsigned long table_end;
>> +	u32 proc_sz;
>> +
>> +	cpumask_clear(cpus);
>> +
>> +	table_hdr = acpi_get_pptt();
> 
> This calls acpi_get_table() so you need to put it again or every call
> to this leaks a reference count.  I messed around with DEFINE_FREE() for this
> but it doesn't fit that well as the underlying call doesn't return the table.
> This one does though so you could do a pptt specific one.  
> 
> Or just acpi_put_table(table_hdr); at exit path from this function.

This is a strange one - you spotted it on your follow up email.


>> +	if (!table_hdr)
>> +		return;
>> +
>> +	table_end = (unsigned long)table_hdr + table_hdr->length;
>> +	entry = ACPI_ADD_PTR(struct acpi_subtable_header, table_hdr,
>> +			     sizeof(struct acpi_table_pptt));

> Hmm. Not related to this patch but I have no idea why acpi_get_pptt()
> doesn't return a struct acpi_table_pptt as if it did this would be a simple
> + 1 and not require those who only sometimes deal with ACPI code to go
> check what that macro actually does!

Looks like that would have a knock on effect for the types in:
| topology_get_acpi_cpu_tag()
| acpi_find_processor_node()
| cache_setup_acpi_cpu()
| fetch_pptt_node()

... basically everywhere this file uses struct acpi_table_header...

It's simpler to cast it here if you're keen on the '+1' approach, but this pattern is how
this file does this. This is the same as acpi_pptt_leaf_node().


>> +	proc_sz = sizeof(struct acpi_pptt_processor);


> Maybe sizeof (*cpu_node) is more helpful to reader.

(again, same as acpi_pptt_leaf_node())


> Also shorter so you could do
> 	while ((unsigned long)entry + sizeof(*cpu_node) <= table_end)
> 
>> +	while ((unsigned long)entry + proc_sz <= table_end) {
>> +		cpu_node = (struct acpi_pptt_processor *)entry;
> 
> For me, assigning this before checking the type is inelegant.

I agree, but when in pptt.c ...


> but the nesting does get deep without it so I guess this is ok maybe, though
> I wonder if better reorganized to combine a different bunch of conditions.
> I think this is functionally identival.
> 
> 		if (entry->type == ACPI_PTT_TYPE_PROCESSOR) {
> 			struct acpi_pptt_processor *cpu_node = 
> 				(struct acpi_pptt_processor *)entry;
> 			if ((cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID) &&
> 			    (!acpi_pptt_leaf_node(table_hdr, cpu_node) &&
> 			    (cpu_node->acpi_processor_id == acpi_cpu_id)) {
> 				acpi_pptt_get_child_cpus(table_hdr, cpu_node, cpus);
> 				break;
> 		
> 			}
> 		}
> 		entry = ACPI_ADD_PTR(struct acpi_subtable_header, entry,
> 				     entry->length);

It is the same - I think this is better as it reduces the scope of cpu_node.


> More generally I wonder if it is worth adding a for_each_acpi_pptt_entry() macro.
> There is some precedence in drivers acpi such as for_each_nhlt_endpoint()

When this series had support for ACPI PPI-Partitions there were more PPTT helpers and I
had a function pointer based call_on_every_entry() kind of thing. Jeremy L though it
obscured the flow. A custom for_each_ is probably better.

I may float it as an RFC after these are done. I don't want to make this series any
bigger. It certainly makes the users easier on the eye.
| drivers/acpi/pptt.c | 43 +++++++++++++++----------------------------
| 1 file changed, 15 insertions(+), 28 deletions(-)

Stashed here for posterity:
https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git/commit/?h=pptt/for_each_pptt_entry/v0&id=353ceeba3d39c6b6a10eeb1a59c49649cdf719d8


> That's probably material for another day though unless you think it brings
> enough benefits to do it here.
> 
> 
>> +		if (entry->type == ACPI_PPTT_TYPE_PROCESSOR &&
>> +		    cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID) {
>> +			if (!acpi_pptt_leaf_node(table_hdr, cpu_node)) {
>> +				if (cpu_node->acpi_processor_id == acpi_cpu_id) {
>> +					acpi_pptt_get_child_cpus(table_hdr, cpu_node, cpus);
>> +					break;
>> +				}
>> +			}
>> +		}
>> +		entry = ACPI_ADD_PTR(struct acpi_subtable_header, entry,
>> +				     entry->length);
>> +	}
>> +}
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index 1c5bb1e887cd..f97a9ff678cc 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -1541,6 +1541,7 @@ int find_acpi_cpu_topology(unsigned int cpu, int level);
>>  int find_acpi_cpu_topology_cluster(unsigned int cpu);
>>  int find_acpi_cpu_topology_package(unsigned int cpu);
>>  int find_acpi_cpu_topology_hetero_id(unsigned int cpu);
>> +void acpi_pptt_get_cpus_from_container(u32 acpi_cpu_id, cpumask_t *cpus);
>>  #else
>>  static inline int acpi_pptt_cpu_is_thread(unsigned int cpu)
>>  {
>> @@ -1562,6 +1563,8 @@ static inline int find_acpi_cpu_topology_hetero_id(unsigned int cpu)
>>  {
>>  	return -EINVAL;
>>  }
>> +static inline void acpi_pptt_get_cpus_from_container(u32 acpi_cpu_id,
>> +						     cpumask_t *cpus) { }
>>  #endif
>>  
>>  void acpi_arch_init(void);
> 

Powered by blists - more mailing lists