Re: [PATCH RFC 06/15] arm_mpam: Fix cache ID sentinel from ~0UL to U32_MAX to match u32 return type

[Yin Li <yin.li@oss.qualcomm.com>]

On 9/2/2026 9:49 PM, Andre Przywara wrote:
> Hi,
> 
> On 8/11/26 15:30, Yin Li wrote:
>> cache_of_calculate_id() returns u32, but callers stored the result in
>> unsigned long variables and compared against ~0UL. On 64-bit systems,
>> U32_MAX (0xffffffff) assigned to unsigned long becomes 
>> 0x00000000ffffffff,
>> which does not equal ~0UL (0xffffffffffffffff), so invalid cache IDs are
>> silently accepted instead of being rejected.
> 
> Yes, I agree about this one, long is not right, it's u32 (acpi) or even 
> a plain int elsewhere (get_cpu_cacheinfo_id()).
> 
>>
>> Fix by changing all cache ID and component ID variables that receive the
>> return value of cache_of_calculate_id() to u32, and replace all ~0 and
>> ~0UL sentinel comparisons with U32_MAX.
> 
> ... but am not so sure about the U32_MAX change. I think ~0U is a common 
> idiom in the kernel to mean "mask of all 1's", and while U32_MAX is the 
> same, the _MAX part is slightly misleading here, I think.
> 
> So I think this patch should focus on dropping the long and L parts, but 
> keep the ~0U notation. Which means cacheinfo.c should not change, for 
> instance.
> 
> And also I wonder if that should be split up: one part to fix the 
> existing usage in v7.3-rc1, so basically the function prototype, and the 
> other part for the newly introduced DT code, which should then be squashed.
> 

Hi Andre,

Thanks for the review.

On the types: agreed, and the patch already changes them from unsigned
long to u32.

On the sentinel: the patch currently uses U32_MAX, but you make a fair
point that ~0U is the more common idiom for an all-ones mask and that
_MAX reads a bit misleading here. I'll switch it back to ~0U and leave
cacheinfo.c unchanged, so the patch focuses on just the long/UL type fix.
(Drew raised the opposite on patch 02/15, but as a question rather than 
a strong preference — Drew, please shout if you disagree.)

As discussed with Ben, I'll fold these type fixes back into the 
respective inherited patches.

Thx and BRs,
Yin

> Cheers,
> Andre
> 
>>
>> Also fix the sentinel values in cache_of_calculate_id() itself for
>> consistency.
>>
>> Signed-off-by: Yin Li <yin.li@oss.qualcomm.com>
>> ---
>>   drivers/base/cacheinfo.c        |  6 +++---
>>   drivers/resctrl/mpam_devices.c  | 16 ++++++++--------
>>   drivers/resctrl/mpam_internal.h |  2 +-
>>   3 files changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
>> index f75e7f64038b..a4e0d1d47e71 100644
>> --- a/drivers/base/cacheinfo.c
>> +++ b/drivers/base/cacheinfo.c
>> @@ -229,7 +229,7 @@ static bool match_cache_node(struct device_node *cpu,
>>   u32 cache_of_calculate_id(struct device_node *cache_node)
>>   {
>>       struct device_node *cpu;
>> -    u32 min_id = ~0;
>> +    u32 min_id = U32_MAX;
>>       for_each_of_cpu_node(cpu) {
>>           u64 id = of_get_cpu_hwid(cpu, 0);
>> @@ -237,7 +237,7 @@ u32 cache_of_calculate_id(struct device_node 
>> *cache_node)
>>           id = arch_compact_of_hwid(id);
>>           if (FIELD_GET(GENMASK_ULL(63, 32), id)) {
>>               of_node_put(cpu);
>> -            return ~0;
>> +            return U32_MAX;
>>           }
>>           if (match_cache_node(cpu, cache_node))
>> @@ -252,7 +252,7 @@ static void cache_of_set_id(struct cacheinfo 
>> *this_leaf,
>>   {
>>       u32 id = cache_of_calculate_id(cache_node);
>> -    if (id != ~0) {
>> +    if (id != U32_MAX) {
>>           this_leaf->id = id;
>>           this_leaf->attributes |= CACHE_ID;
>>       }
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/ 
>> mpam_devices.c
>> index 559fa09128b4..ddc15249ec1e 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
>> @@ -166,13 +166,13 @@ static void mpam_free_garbage(void)
>>   /* Called recursively to walk the list of caches from a particular 
>> CPU */
>>   static void __mpam_get_cpumask_from_cache_id(int cpu, struct 
>> device_node *cache_node,
>> -                         unsigned long cache_id,
>> +                         u32 cache_id,
>>                            u32 cache_level,
>>                            cpumask_t *affinity)
>>   {
>>       int err;
>>       u32 iter_level;
>> -    unsigned long iter_cache_id;
>> +    u32 iter_cache_id;
>>       struct device_node *iter_node __free(device_node) = 
>> of_find_next_cache_node(cache_node);
>>       if (!iter_node)
>> @@ -187,7 +187,7 @@ static void __mpam_get_cpumask_from_cache_id(int 
>> cpu, struct device_node *cache_
>>        * during device_initcall(). Use cache_of_calculate_id().
>>        */
>>       iter_cache_id = cache_of_calculate_id(iter_node);
>> -    if (iter_cache_id == ~0UL)
>> +    if (iter_cache_id == U32_MAX)
>>           return;
>>       if (iter_level == cache_level && iter_cache_id == cache_id)
>> @@ -202,7 +202,7 @@ static void __mpam_get_cpumask_from_cache_id(int 
>> cpu, struct device_node *cache_
>>    * The cacheinfo structures are only populated when CPUs are online.
>>    * This helper walks the device tree to include offline CPUs too.
>>    */
>> -int mpam_get_cpumask_from_cache_id(unsigned long cache_id, u32 
>> cache_level,
>> +int mpam_get_cpumask_from_cache_id(u32 cache_id, u32 cache_level,
>>                      cpumask_t *affinity)
>>   {
>>       int cpu;
>> @@ -229,7 +229,7 @@ static int get_cpumask_from_cache(struct 
>> device_node *cache,
>>   {
>>       int err;
>>       u32 cache_level;
>> -    unsigned long cache_id;
>> +    u32 cache_id;
>>       err = of_property_read_u32(cache, "cache-level", &cache_level);
>>       if (err) {
>> @@ -238,7 +238,7 @@ static int get_cpumask_from_cache(struct 
>> device_node *cache,
>>       }
>>       cache_id = cache_of_calculate_id(cache);
>> -    if (cache_id == ~0UL) {
>> +    if (cache_id == U32_MAX) {
>>           pr_err("Failed to calculate cache-id from cache node\n");
>>           return -ENOENT;
>>       }
>> @@ -264,7 +264,7 @@ static int mpam_dt_parse_resource(struct mpam_msc 
>> *msc, struct device_node *np,
>>   {
>>       int err = 0;
>>       u32 class_id = 0;
>> -    unsigned long component_id = 0;
>> +    u32 component_id = 0;
>>       struct device *dev = &msc->pdev->dev;
>>       enum mpam_class_types type = MPAM_CLASS_UNKNOWN;
>>       struct device_node *cache __free(device_node) = NULL;
>> @@ -308,7 +308,7 @@ static int mpam_dt_parse_resource(struct mpam_msc 
>> *msc, struct device_node *np,
>>               return err;
>>           }
>>           component_id = cache_of_calculate_id(cache);
>> -        if (component_id == ~0) {
>> +        if (component_id == U32_MAX) {
>>               dev_err_once(dev, "Failed to calculate cache-id\n");
>>               return -ENOENT;
>>           }
>> diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/ 
>> mpam_internal.h
>> index def0e3a65c23..aa45d00bcd07 100644
>> --- a/drivers/resctrl/mpam_internal.h
>> +++ b/drivers/resctrl/mpam_internal.h
>> @@ -470,7 +470,7 @@ int mpam_msmon_read(struct mpam_component *comp, 
>> struct mon_cfg *ctx,
>>               enum mpam_device_features, u64 *val);
>>   void mpam_msmon_reset_mbwu(struct mpam_component *comp, struct 
>> mon_cfg *ctx);
>> -int mpam_get_cpumask_from_cache_id(unsigned long cache_id, u32 
>> cache_level,
>> +int mpam_get_cpumask_from_cache_id(u32 cache_id, u32 cache_level,
>>                      cpumask_t *affinity);
>>   #ifdef CONFIG_RESCTRL_FS
>>
> 

-- 
Thx and BRs,
Yin