Re: [PATCH v3 5/8] arm_mpam: Add device tree support for MSC probing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] of: error code leak from of_property_read_reg() in mpam_dt_parse_resources()
- [Low] of: unhandled error from get_cpumask_from_cache_id() leaves accessibility mask incomplete
--

commit df5cdd0f6660fd3028a1a26bcd32ad8c47e80043
Author: James Morse <james.morse@arm.com>
arm_mpam: Add device tree support for MSC probing

The MPAM driver currently discovers MSCs only via ACPI. Add a device tree
path so MSCs can be probed on DT-based platforms by parsing MSC nodes,
computing cache-id and affinity, and creating the RIS entries.

> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index c68135ee0ffcb..0640690eae2c0 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c

[ ... ]

> @@ -161,6 +164,171 @@ static void mpam_free_garbage(void)
>  	}
>  }
>  
> +/* Called recursively to walk the list of caches from a particular CPU */

[ ... ]

> +/*
> + * 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(u32 cache_id, u32 cache_level,
> +				   cpumask_t *affinity)
> +{
> +	int cpu;
> +
> +	if (!acpi_disabled)
> +		return acpi_pptt_get_cpumask_from_cache_id(cache_id, affinity);
> +
> +	for_each_possible_cpu(cpu) {
> +		struct device_node *cpu_node __free(device_node) = of_get_cpu_node(cpu, NULL);
> +		if (!cpu_node) {
> +			pr_err("Failed to find cpu%d device node\n", cpu);
> +			return -ENOENT;
> +		}

[Severity: Low]
Does this code leave the affinity mask partially populated on failure?

If of_get_cpu_node() fails for any CPU, the loop immediately aborts and returns
-ENOENT. Any CPUs that were successfully processed prior to the failure remain
populated in the affinity mask.

> +
> +		__mpam_get_cpumask_from_cache_id(cpu, cpu_node, cache_id,
> +						 cache_level, affinity);
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static int mpam_dt_parse_resources(struct mpam_msc *msc, void *ignored)
> +{
> +	u64 ris_idx = 0;
> +	int err, num_ris = 0;
> +	struct device_node *np;
> +
> +	np = msc->pdev->dev.of_node;
> +	for_each_available_child_of_node_scoped(np, iter) {
> +		err = of_property_read_reg(iter, 0, &ris_idx, NULL);
> +		if (!err) {
> +			/*
> +			 * ris_idx is read as u64 but indexes a 4-bit RIS selector
> +			 * (0..MPAM_MSC_MAX_NUM_RIS). Reject out-of-range values here,
> +			 * before it is narrowed to u8, so a large value cannot be
> +			 * truncated into a valid-looking index.
> +			 */
> +			if (ris_idx >= MPAM_MSC_MAX_NUM_RIS)
> +				return -EINVAL;
> +			num_ris++;
> +			err = mpam_dt_parse_resource(msc, iter, ris_idx);
> +			if (err)
> +				return err;
> +		}
> +	}
> +
> +	if (!num_ris)
> +		err = mpam_dt_parse_resource(msc, np, 0);
> +
> +	return err;
> +}

[Severity: Medium]
Can this function spuriously fail if a child node lacks a reg property?

In mpam_dt_parse_resources(), the loop variable err is overwritten by the
result of of_property_read_reg() for each child node. If a child node lacks
a reg property, of_property_read_reg() fails and sets err to a negative error
code.

If this failing child node is the last one processed in the loop, the loop
terminates with err holding that negative value. Provided at least one valid
RIS node was successfully processed earlier (num_ris > 0), the fallback block
is bypassed.

The function then returns the leaked err value, causing the MSC probe to
fail.

[ ... ]

> @@ -1987,15 +2145,34 @@ static int mpam_msc_setup_error_irq(struct mpam_msc *msc)
>   */
>  static void update_msc_accessibility(struct mpam_msc *msc)
>  {
> +	struct device *dev = &msc->pdev->dev;
> +	struct device_node *parent;
>  	u32 affinity_id;
>  	int err;
>  
> -	err = device_property_read_u32(&msc->pdev->dev, "cpu_affinity",
> -				       &affinity_id);
> -	if (err)
> +	if (!acpi_disabled) {
> +		err = device_property_read_u32(&msc->pdev->dev, "cpu_affinity",
> +					       &affinity_id);
> +		if (err)
> +			cpumask_copy(&msc->accessibility, cpu_possible_mask);
> +		else
> +			acpi_pptt_get_cpus_from_container(affinity_id,
> +							  &msc->accessibility);
> +
> +		return;
> +	}
> +
> +	/* Where an MSC can be accessed from depends on the path to of_node. */
> +	parent = of_get_parent(msc->pdev->dev.of_node);
> +	if (parent == of_root) {
>  		cpumask_copy(&msc->accessibility, cpu_possible_mask);
> -	else
> -		acpi_pptt_get_cpus_from_container(affinity_id, &msc->accessibility);
> +	} else {
> +		if (of_device_is_compatible(parent, "cache"))
> +			get_cpumask_from_cache(parent, &msc->accessibility);
> +		else
> +			dev_err_once(dev, "Cannot determine accessibility of MSC.\n");
> +	}
> +	of_node_put(parent);
>  }

[Severity: Low]
Is the error from get_cpumask_from_cache() intentionally ignored here?

The error code from mpam_get_cpumask_from_cache_id() propagates up through
get_cpumask_from_cache() to update_msc_accessibility(), but it is not checked.
This allows the driver probe to continue with a partially populated CPU
accessibility mask instead of failing cleanly.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-mpam-resctrl-dt-knp-support-v3-0-35196c2b43bf@oss.qualcomm.com?part=5