Re: [PATCH] arm_mpam: resctrl: Catch and propagate error from get_cpu_cacheinfo_id()

[Gavin Shan <gshan@redhat.com>]

On 9/16/26 11:34 PM, Andre Przywara wrote:
> On 9/16/26 11:49, Ben Horgan wrote:
>> On 07/09/2026 13:59, Ben Horgan wrote:
>>> On 02/09/2026 15:37, Andre Przywara wrote:
>>>> get_cpu_cacheinfo_id() can fail, in which case it returns a negative
>>>> error value.
>>>>
>>>> Check the returned value for this error condition, before passing the
>>>> value on to other code, which would hide the negative number in some high
>>>> value in the unsigned type.
>>>>
>>>> Fixes: 36528c7681b8 ("arm_mpam: resctrl: Add support for 'MB' resource")
>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>>
>>> This looks good to me. Out of interest what led you to find this?
>>>
>>> Reviewed-by: Ben Horgan <ben.horgan@arm.com>
>>
>> I seem to have been a bit hasty here.
>>
>> Sashiko points out at [1] that 0xFFFFFFFF is the only value we were previously considering invalid
>> and that the value coming from dt or acpi can provid other valid values that would after this patch
>> be considered invalid.
> 
> Fair, and I can easily change the check to only check explicitly for -1.
> 
> But this is somewhat broken already, right? I mean the return type for the existing get_cpu_cacheinfo_id() has always been "int". It looks like this comes from the x86 world, where the cache IDs never get that large? I can have a deeper look, but my gut feeling is that this is practically irrelevant, since we barely see Aff3 at all, not to mention high values of it.
> 

I guess we also can exploit CACHE_ID flag in struct cacheinfo::attributes.
struct cacheinfo::id is valid only when CACHE_ID flag is set. So we would
introduce our own helper as below, where an extra return value to indicate
the (returned) cached ID is valid or not.

In drivers/resctrl/mpam_internal.h:

/* NULL is returned if struct cacheinfo::attributes don't have CACHE_ID set */
static inline bool mpam_cpu_cache_id(int cpu, int level, unsigned int *id)
{
         struct cacheinfo *ci = get_cpu_cacheinfo_level(cpu, level);

         if (!ci)
                 return false;
         
         if (id)
                 *id = ci->id;
         
         return true;
}

Thanks,
Gavin