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

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

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.

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
>