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 Yin,

thanks for the reply!

On 9/15/26 08:49, Yin Li wrote:
> 
> 
> On 9/14/2026 8:26 PM, Andre Przywara wrote:
>> 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).
>>
> 
> Hi Andre,
> 
> On the #ifdef CONFIG_OF: I also verified that removing the guard doesn't
> break the build. However, since the implementation in cacheinfo.c is
> itself guarded by #ifdef CONFIG_OF, exposing the prototype
> unconditionally could cause a link error on CONFIG_OF=n builds if called

How so? Just exposing a prototype wouldn't be a problem, as long as you 
don't try to call that function. And that would be caught by the linker, 
and then you have a different problem anyway (the caller).

The only reason to protect the prototype would be if a type used in the 
parameters is not defined. And on the face of it "struct device_node" is 
an OF specific type, declared in include/linux/of.h, but as mentioned, I 
can't produce a compiler error, and even if so, would prefer to include 
of.h instead.

> from outside the ARM64/MPAM path. A more idiomatic approach might be to
> use a stub to keep the header consistent with the implementation:
> 
>    #ifdef CONFIG_OF
>    u32 cache_of_calculate_id(struct device_node *np);
>    #else
>    static inline u32 cache_of_calculate_id(struct device_node *np)
>    {
>        return ~0U;
>    }
>    #endif
> 
> Would that work for you?

That's not necessary and doesn't solve the problem: the struct 
device_node would be present in both branches, so that doesn't help. And 
given there are no preprocessor protections for OF or ACPI in the whole 
of mpam_devices.c, that's a non-issue, I'd say.

> Otherwise I'm happy to just drop the guard in
> the next version.

So can you say whether you have a .config that does not build? Or was 
that issue just pointed out by some picky AI review tool?
Otherwise I would drop the guards, and wait for the kernel test robot or 
Arnd's infamous randconfig builds to show up the exact problem.

Cheers,
Andre

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