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

[Gavin Shan]

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@xxxxxxx>
> > >
> > > This looks good to me. Out of interest what led you to find this?
> > >
> > > Reviewed-by: Ben Horgan <ben.horgan@xxxxxxx>
> >
> > 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