Re: [PATCH v5 6/7] arm_mpam: Add support for memory controller MSC on DT platforms

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] arm_mpam: invalid compatible memory parent check in
  update_msc_accessibility()
- [High] arm_mpam: vMSC collision in mpam_vmsc_find() for DT memory MSCs
- [Medium] arm_mpam: mpam_dt_parse_resource() ignores memory phandle
  target node
- [Low] arm_mpam: undocumented arm,mpam-memory parsing in
  mpam_dt_parse_resource()
--

commit 76edb90c680c1ae30161829584d4c65a841ea202
Author: Shanker Donthineni <sdonthineni@nvidia.com>

arm_mpam: Add support for memory controller MSC on DT platforms

This commit adds Device Tree discovery support for memory-controller MSCs,
deriving the component ID from the NUMA node and marking MSCs under a memory
parent node as accessible from all CPUs.

> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 78caa8ae13490..e0ac6ef83d6e7 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -365,10 +365,12 @@ static int mpam_dt_parse_resource(struct mpam_msc *msc, struct device_node *np,
>  				  u8 ris_idx)
>  {
>  	int err = 0;
> -	u32 level = 0;
> -	u32 cache_id;
> +	u32 class_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;
> +	struct device_node *memory __free(device_node) = NULL;
>  	struct device_node *parent __free(device_node) = of_get_parent(np);
>  
>  	if (of_device_is_compatible(np, "arm,mpam-cache")) {
> @@ -377,27 +379,47 @@ static int mpam_dt_parse_resource(struct mpam_msc *msc, struct device_node *np,
>  			dev_err_once(dev, "Failed to read phandle\n");
>  			return -EINVAL;
>  		}
> +		type = MPAM_CLASS_CACHE;
> +
>  	} else if (of_device_is_compatible(parent, "cache")) {
>  		cache = of_node_get(parent);
> +		type = MPAM_CLASS_CACHE;
> +	} else if (of_device_is_compatible(np, "arm,mpam-memory")) {
> +		memory = of_parse_phandle(np, "arm,mpam-device", 0);
> +		if (!memory) {
> +			dev_err_once(dev, "Failed to read phandle\n");
> +			return -EINVAL;
> +		}
> +		type = MPAM_CLASS_MEMORY;

[Severity: Low]
Is this arm,mpam-memory branch dead code?
Documentation/devicetree/bindings/arm/arm,mpam-msc.yaml only documents the
arm,mpam-msc and arm,mpam-memory-controller-msc compatible strings and does
not define arm,mpam-memory or the arm,mpam-device phandle property, and there
are no in-tree Device Tree users of memory-controller MSCs in this series.

> +	} else if (of_device_is_compatible(np, "arm,mpam-memory-controller-msc")) {
> +		type = MPAM_CLASS_MEMORY;
>  	} else {
> -		/* For now, only caches are supported */
> -		cache = NULL;
> -		return err;
> -	}
> -
> -	err = of_property_read_u32(cache, "cache-level", &level);
> -	if (err) {
> -		dev_err_once(dev, "Failed to read cache-level\n");
> +		/*
> +		 * For now, only caches and memory controllers are
> +		 * supported.
> +		 */
>  		return err;
>  	}
>  
> -	cache_id = cache_of_calculate_id(cache);
> -	if (cache_id == ~0U) {
> -		dev_err_once(dev, "Failed to calculate cache-id\n");
> -		return -ENOENT;
> +	/* Determine the class and component ids, based on type. */
> +	if (type == MPAM_CLASS_CACHE) {
> +		err = of_property_read_u32(cache, "cache-level", &class_id);
> +		if (err) {
> +			dev_err_once(dev, "Failed to read cache-level\n");
> +			return err;
> +		}
> +		component_id = cache_of_calculate_id(cache);
> +		if (component_id == ~0U) {
> +			dev_err_once(dev, "Failed to calculate cache-id\n");
> +			return -ENOENT;
> +		}
> +	} else if (type == MPAM_CLASS_MEMORY) {
> +		err = of_node_to_nid(np);
> +		component_id = (err == NUMA_NO_NODE) ? 0 : err;
> +		class_id = MPAM_CLASS_ID_DEFAULT;
>  	}

[Severity: Medium]
When np is compatible with "arm,mpam-memory", does calling of_node_to_nid(np)
ignore the target memory node resolved from the "arm,mpam-device" phandle?

The phandle target is stored in memory, but memory is never read afterwards.
Because numa-node-id is on the referenced memory node rather than on np,
of_node_to_nid(np) assigns the wrong component_id (falling back to 0 or to the
parent MSC's node ID when multiple RIS child nodes point at different NUMA
memory nodes).  Passing memory ? memory : np to of_node_to_nid() would query
the referenced node when present.

>  
> -	return mpam_ris_create(msc, ris_idx, MPAM_CLASS_CACHE, level, cache_id);
> +	return mpam_ris_create(msc, ris_idx, type, class_id, component_id);
>  }

[Severity: High]
When a Device Tree platform has more than one memory-controller MSC on the
same NUMA node (or when NUMA is disabled), will mpam_ris_create() attach every
MSC's RIS to the first MSC's vMSC?

For all Device Tree platform devices, pdev->id is PLATFORM_DEVID_NONE (-1), so
do_mpam_msc_drv_probe() sets msc->id = -1 on every MSC.  Because all
MPAM_CLASS_MEMORY resources on the same node get class_id =
MPAM_CLASS_ID_DEFAULT and the same component_id, they share one
mpam_component, where mpam_vmsc_find() matches by msc->id:

drivers/resctrl/mpam_devices.c:mpam_vmsc_find() {
	list_for_each_entry(vmsc, &comp->vmsc, comp_list) {
		if (vmsc->msc->id == msc->id)
			return vmsc;
	}
}

Since vmsc->msc->id == msc->id evaluates (-1 == -1), every subsequent memory
MSC on that node reuses the first MSC's vMSC.  During mpam_msc_hw_probe(),
mpam_ris_hw_probe() then resolves msc = ris->vmsc->msc to the first MSC while
holding the second MSC's probe_lock and part_sel_lock, triggering lockdep
assertions and performing unserialized MMIO accesses on the first MSC while
leaving the remaining memory MSCs unprobed and unconfigured.  Matching
vmsc->msc == msc in mpam_vmsc_find() (or assigning each DT MSC a unique ID)
would keep each MSC in its own vMSC.

> @@ -2574,6 +2596,8 @@ static void update_msc_accessibility(struct mpam_msc *msc)
>  	} else {
>  		if (of_device_is_compatible(parent, "cache"))
>  			get_cpumask_from_cache(parent, &msc->accessibility);
> +		else if (of_device_is_compatible(parent, "memory"))
> +			cpumask_copy(&msc->accessibility, cpu_possible_mask);
>  		else
>  			dev_err_once(dev, "Cannot determine accessibility of MSC.\n");
>  	}

[Severity: High]
Can update_msc_accessibility() ever succeed for a non-root memory-controller
MSC with of_device_is_compatible(parent, "memory")?

Standard Device Tree memory nodes use device_type = "memory" rather than
compatible = "memory", and MMIO memory-controller MSC nodes live under bus
nodes (such as simple-bus) or memory-controller nodes rather than of_root.
For those nodes, parent != of_root and of_device_is_compatible(parent,
"memory") is false, leaving msc->accessibility empty and causing
do_mpam_msc_drv_probe() to fail with -EINVAL.

Conversely, if an MSC node were placed under a parent with compatible =
"memory", neither of_platform_default_populate() nor
mpam_dt_create_foundling_msc() (which only iterates over compatible = "cache"
nodes) would create a platform_device for it, even though mpam_dt_count_msc()
counts it in fw_num_msc, preventing MPAM from ever enabling.  In addition,
mpam_dt_parse_resource() only checks of_device_is_compatible(parent, "cache")
and does not handle a memory parent.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-mpam-resctrl-dt-knp-support-v5-0-150335a79381@oss.qualcomm.com?part=6