The MPAM driver identifies caches by id for use with resctrl. It
needs to know the cache-id when probe-ing, but the value isn't set
in cacheinfo until device_initcall().

Expose the code that generates the cache-id. The parts of the MPAM
driver that run early can use this to set up the resctrl structures
before cacheinfo is ready in device_initcall().

Signed-off-by: James Morse <james.morse@arm.com>
---
Changes since v1:
 * Renamed cache_of_get_id() cache_of_calculate_id().
---
 drivers/base/cacheinfo.c  | 19 +++++++++++++------
 include/linux/cacheinfo.h |  1 +
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
index 613410705a47..f6289d142ba9 100644
--- a/drivers/base/cacheinfo.c
+++ b/drivers/base/cacheinfo.c
@@ -207,11 +207,10 @@ static bool match_cache_node(struct device_node *cpu,
 #define arch_compact_of_hwid(_x)	(_x)
 #endif
 
-static void cache_of_set_id(struct cacheinfo *this_leaf,
-			    struct device_node *cache_node)
+unsigned long cache_of_calculate_id(struct device_node *cache_node)
 {
 	struct device_node *cpu;
-	u32 min_id = ~0;
+	unsigned long min_id = ~0UL;
 
 	for_each_of_cpu_node(cpu) {
 		u64 id = of_get_cpu_hwid(cpu, 0);
@@ -219,15 +218,23 @@ static void cache_of_set_id(struct cacheinfo *this_leaf,
 		id = arch_compact_of_hwid(id);
 		if (FIELD_GET(GENMASK_ULL(63, 32), id)) {
 			of_node_put(cpu);
-			return;
+			return ~0UL;
 		}
 
 		if (match_cache_node(cpu, cache_node))
 			min_id = min(min_id, id);
 	}
 
-	if (min_id != ~0) {
-		this_leaf->id = min_id;
+	return min_id;
+}
+
+static void cache_of_set_id(struct cacheinfo *this_leaf,
+			    struct device_node *cache_node)
+{
+	unsigned long id = cache_of_calculate_id(cache_node);
+
+	if (id != ~0UL) {
+		this_leaf->id = id;
 		this_leaf->attributes |= CACHE_ID;
 	}
 }
diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
index c8f4f0a0b874..2dcbb69139e9 100644
--- a/include/linux/cacheinfo.h
+++ b/include/linux/cacheinfo.h
@@ -112,6 +112,7 @@ int acpi_get_cache_info(unsigned int cpu,
 #endif
 
 const struct attribute_group *cache_get_priv_group(struct cacheinfo *this_leaf);
+unsigned long cache_of_calculate_id(struct device_node *np);
 
 /*
  * Get the cacheinfo structure for the cache associated with @cpu at
-- 
2.20.1

Hi James,

On Fri, Aug 22, 2025 at 03:29:42PM +0000, James Morse wrote:
> The MPAM driver identifies caches by id for use with resctrl. It
> needs to know the cache-id when probe-ing, but the value isn't set
> in cacheinfo until device_initcall().
> 
> Expose the code that generates the cache-id. The parts of the MPAM
> driver that run early can use this to set up the resctrl structures
> before cacheinfo is ready in device_initcall().

Why can't the MPAM driver just consume the precomputed cache-id
information?

Possible reasons are that the MPAM driver probes too early, or that it
must parse the PPTT directly (which is true) and needs to label caches
consistently with the way the kernel does it.

> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
> Changes since v1:
>  * Renamed cache_of_get_id() cache_of_calculate_id().
> ---
>  drivers/base/cacheinfo.c  | 19 +++++++++++++------
>  include/linux/cacheinfo.h |  1 +
>  2 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
> index 613410705a47..f6289d142ba9 100644
> --- a/drivers/base/cacheinfo.c
> +++ b/drivers/base/cacheinfo.c
> @@ -207,11 +207,10 @@ static bool match_cache_node(struct device_node *cpu,
>  #define arch_compact_of_hwid(_x)	(_x)
>  #endif
>  
> -static void cache_of_set_id(struct cacheinfo *this_leaf,
> -			    struct device_node *cache_node)
> +unsigned long cache_of_calculate_id(struct device_node *cache_node)
>  {
>  	struct device_node *cpu;
> -	u32 min_id = ~0;
> +	unsigned long min_id = ~0UL;

Why the change of type here?

This does mean that 0xffffffff can now be generated as a valid cache-id,
but if that is necessary then this patch is also fixing a bug in the
code -- but the commit message doesn't say anything about that.

For a patch that is just exposing an internal result, it may be
better to keep the original type.  ~(u32)0 is already used as an
exceptional value.

[...]

Otherwise, this looks reasonable to me.

Cheers
---Dave

Hi Dave,

On 27/08/2025 11:46, Dave Martin wrote:
> On Fri, Aug 22, 2025 at 03:29:42PM +0000, James Morse wrote:
>> The MPAM driver identifies caches by id for use with resctrl. It
>> needs to know the cache-id when probe-ing, but the value isn't set
>> in cacheinfo until device_initcall().
>>
>> Expose the code that generates the cache-id. The parts of the MPAM
>> driver that run early can use this to set up the resctrl structures
>> before cacheinfo is ready in device_initcall().

> Why can't the MPAM driver just consume the precomputed cache-id
> information?

Because it would need to wait until cacheinfo was ready, and it would still
need a way of getting the cache-id for caches where all the CPUs are offline.

The resctrl glue code has a waitqueue to wait for device_initcall_sync(), but that is
asynchronous to driver probing, its triggered by the schedule_work() from the cpuhp
callbacks. This bit is about the driver's use, which just gets probed whenever the core
code feels like it.

I toyed with always using cacheinfo for everything, and just waiting - but the MPAM driver
already has to parse the PPTT to find the information it needs on ACPI platforms, so the
wait would only happen on DT.

It seemed simpler to grab what the value would be, instead of waiting (or probe defer) -
especially as this is also needed for caches where all the CPUs are offline.

(I'll add the offline-cpus angle to the commit message)


> Possible reasons are that the MPAM driver probes too early,

yup,

> or that it
> must parse the PPTT directly (which is true) and needs to label caches
> consistently with the way the kernel does it.

It needs to match what will be exposed to user-space from cacheinfo.
This isn't about the PPTT, its the value that is generated for DT systems.
The driver has to know if its ACPI or DT to call the appropriate thing to get cache-ids
before cacheinfo is ready.


>> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
>> index 613410705a47..f6289d142ba9 100644
>> --- a/drivers/base/cacheinfo.c
>> +++ b/drivers/base/cacheinfo.c
>> @@ -207,11 +207,10 @@ static bool match_cache_node(struct device_node *cpu,
>>  #define arch_compact_of_hwid(_x)	(_x)
>>  #endif
>>  
>> -static void cache_of_set_id(struct cacheinfo *this_leaf,
>> -			    struct device_node *cache_node)
>> +unsigned long cache_of_calculate_id(struct device_node *cache_node)
>>  {
>>  	struct device_node *cpu;
>> -	u32 min_id = ~0;
>> +	unsigned long min_id = ~0UL;

> Why the change of type here?

This is a hang over from Rob's approach of making the cache-id 64 bit.


> This does mean that 0xffffffff can now be generated as a valid cache-id,
> but if that is necessary then this patch is also fixing a bug in the
> code -- but the commit message doesn't say anything about that.
> 
> For a patch that is just exposing an internal result, it may be
> better to keep the original type.  ~(u32)0 is already used as an
> exceptional value.

Yup, I'll fix that.


Thanks!

James

Hi James,

On Wed, Aug 27, 2025 at 06:11:25PM +0100, James Morse wrote:
> Hi Dave,
> 
> On 27/08/2025 11:46, Dave Martin wrote:
> > On Fri, Aug 22, 2025 at 03:29:42PM +0000, James Morse wrote:
> >> The MPAM driver identifies caches by id for use with resctrl. It
> >> needs to know the cache-id when probe-ing, but the value isn't set
> >> in cacheinfo until device_initcall().
> >>
> >> Expose the code that generates the cache-id. The parts of the MPAM
> >> driver that run early can use this to set up the resctrl structures
> >> before cacheinfo is ready in device_initcall().
> 
> > Why can't the MPAM driver just consume the precomputed cache-id
> > information?
> 
> Because it would need to wait until cacheinfo was ready, and it would still
> need a way of getting the cache-id for caches where all the CPUs are offline.
> 
> The resctrl glue code has a waitqueue to wait for device_initcall_sync(), but that is
> asynchronous to driver probing, its triggered by the schedule_work() from the cpuhp
> callbacks. This bit is about the driver's use, which just gets probed whenever the core
> code feels like it.
> 
> I toyed with always using cacheinfo for everything, and just waiting - but the MPAM driver
> already has to parse the PPTT to find the information it needs on ACPI platforms, so the
> wait would only happen on DT.
> 
> It seemed simpler to grab what the value would be, instead of waiting (or probe defer) -
> especially as this is also needed for caches where all the CPUs are offline.
> 
> (I'll add the offline-cpus angle to the commit message)

Ack

> > Possible reasons are that the MPAM driver probes too early,
> 
> yup,
> 
> > or that it
> > must parse the PPTT directly (which is true) and needs to label caches
> > consistently with the way the kernel does it.
> 
> It needs to match what will be exposed to user-space from cacheinfo.
> This isn't about the PPTT, its the value that is generated for DT systems.

Right -- confused myself there.  From the point of view of this series,
the usage scenario isn't clear at this point.

> The driver has to know if its ACPI or DT to call the appropriate thing to get cache-ids
> before cacheinfo is ready.

I see.  This might be worth stating in the commit message.

> >> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
> >> index 613410705a47..f6289d142ba9 100644
> >> --- a/drivers/base/cacheinfo.c
> >> +++ b/drivers/base/cacheinfo.c
> >> @@ -207,11 +207,10 @@ static bool match_cache_node(struct device_node *cpu,
> >>  #define arch_compact_of_hwid(_x)	(_x)
> >>  #endif
> >>  
> >> -static void cache_of_set_id(struct cacheinfo *this_leaf,
> >> -			    struct device_node *cache_node)
> >> +unsigned long cache_of_calculate_id(struct device_node *cache_node)
> >>  {
> >>  	struct device_node *cpu;
> >> -	u32 min_id = ~0;
> >> +	unsigned long min_id = ~0UL;
> 
> > Why the change of type here?
> 
> This is a hang over from Rob's approach of making the cache-id 64 bit.

Ah, right.

(I have assumed that 0xffffffff is never going to clash with a valid
value.)

> > This does mean that 0xffffffff can now be generated as a valid cache-id,
> > but if that is necessary then this patch is also fixing a bug in the
> > code -- but the commit message doesn't say anything about that.
> > 
> > For a patch that is just exposing an internal result, it may be
> > better to keep the original type.  ~(u32)0 is already used as an
> > exceptional value.
> 
> Yup, I'll fix that.

OK -- it works either way, of course, but this should make the patch a
little less noisy.

Cheers
---Dave