Re: [PATCH RFC v2 04/10] cacheinfo: Expose the code to generate a cache-id from a device_node

[Andre Przywara <andre.przywara@arm.com>]

Hi,

On 9/14/26 11:37, Yin Li wrote:
> From: James Morse <james.morse@arm.com>
> 
> 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(). Even after device_initcall(),
> the cache-id is only available if at least one CPU associated with
> the cache is online.
> 
> Instead of making the driver wait, 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>
> [ Yin Li: fix context conflicts in cacheinfo.c and cacheinfo.h; guard the
>    cache_of_calculate_id() declaration with CONFIG_OF to prevent build
>    errors when CONFIG_OF is not set ]

You can shorten that part in square brackets: doing adjustments due to 
rebasing is surely implied, and you can shorten the rest, like:
[ Yin Li: guard cache_of_calculate_id() prototype ]

Speaking of which ...

> Signed-off-by: Yin Li <yin.li@oss.qualcomm.com>
> ---
>   drivers/base/cacheinfo.c  | 17 ++++++++++++-----
>   include/linux/cacheinfo.h |  3 +++
>   2 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
> index 9f9c72727a05..f75e7f64038b 100644
> --- a/drivers/base/cacheinfo.c
> +++ b/drivers/base/cacheinfo.c
> @@ -226,8 +226,7 @@ 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)
> +u32 cache_of_calculate_id(struct device_node *cache_node)
>   {
>   	struct device_node *cpu;
>   	u32 min_id = ~0;
> @@ -238,15 +237,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 ~0;
>   		}
>   
>   		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)
> +{
> +	u32 id = cache_of_calculate_id(cache_node);
> +
> +	if (id != ~0) {
> +		this_leaf->id = id;
>   		this_leaf->attributes |= CACHE_ID;
>   	}
>   }
> diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
> index fc879ac4cc4f..c33bb3c8bd63 100644
> --- a/include/linux/cacheinfo.h
> +++ b/include/linux/cacheinfo.h
> @@ -113,6 +113,9 @@ int acpi_get_cache_info(unsigned int cpu,
>   #endif
>   
>   const struct attribute_group *cache_get_priv_group(struct cacheinfo *this_leaf);
> +#ifdef CONFIG_OF

Why is that, exactly? First IIUC it's quite uncommon to use #ifdef 
guards around prototypes (unless they are stubbed without the symbol 
defined). Using types protected by those symbols if certainly another 
reason, and it looks like this would be the case here, but I had no 
trouble building the kernel for x86, where CONFIG_OF is not defined.
So can you share a .config example (or give a hint) as to where this 
fails building?
And if it does, wouldn't it be better to always include <linux/of.h> in 
that file instead? I think I see a similar pattern elsewhere 
(rfkill-gpio.c, sound/ac97/bus.c).

Cheers,
Andre


> +u32 cache_of_calculate_id(struct device_node *np);
> +#endif
>   
>   /*
>    * Get the cacheinfo structure for the cache associated with @cpu at
>