Hi James,

On 9/5/25 19:48, James Morse wrote:

Hi Ben,

On 27/08/2025 14:03, Ben Horgan wrote:

quoted

On 8/22/25 16:29, James Morse wrote:

quoted

Probing MPAM is convoluted. MSCs that are integrated with a CPU may
only be accessible from those CPUs, and they may not be online.
Touching the hardware early is pointless as MPAM can't be used until
the system-wide common values for num_partid and num_pmg have been
discovered.

Start with driver probe/remove and mapping the MSC.

quoted

quoted

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
new file mode 100644
index 000000000000..a0d9a699a6e7
--- /dev/null
+++ b/drivers/resctrl/mpam_devices.c

@@ -0,0 +1,336 @@

quoted

quoted

+static int mpam_dt_parse_resource(struct mpam_msc *msc, struct device_node *np,
+				  u32 ris_idx)
+{
+	int err = 0;
+	u32 level = 0;
+	unsigned long cache_id;
+	struct device_node *cache;
+
+	do {
+		if (of_device_is_compatible(np, "arm,mpam-cache")) {
+			cache = of_parse_phandle(np, "arm,mpam-device", 0);
+			if (!cache) {
+				pr_err("Failed to read phandle\n");
+				break;
+			}

This looks like this allows "arm,mpam-cache" and "arm,mpam-device" to be
used on an msc node when there are no ris children. This usage could be
reasonable but doesn't match the schema in the previous patch. Should
this usage be rejected or the schema extended?

The DT/ACPI stuff is only going to describe the things that make sense at a high level,
e.g. the controls for the L3. There may be other controls for stuff that doesn't make
sense in the hardware - these get discovered, grouped as 'unknown' and left alone.

Another angle on this is where there is an MSC that the OS will never make use of, but
needs to know about to find the system wide minimum value. (there is a comment about
this in the ACPI spec...)

I don't think its a problem if the magic dt-binding machinery is overly restrictive, that
is about validating DTB files...

I agree with your points. However, I was rather thinking that the code
allows more ways to describe the same thing than the schema does. In
that, you could write something like:


msc@80000 {
        compatible = "foo,a-standalone-msc";
        reg = <0x80000 0x1000>;

	...
        msc@10000 {
            compatible = "arm,mpam-msc arm,mpam-cache";
            arm,mpam-device = <&mem>;
            ...
         }
}

Although, now I've written this out, it doesn't seem sensible to worry
about this. Using ris compatibles on an msc, as in my example, is
clearly an error.

[snip]

quoted

quoted

+		} else if (msc->iface == MPAM_IFACE_PCC) {
+			msc->pcc_cl.dev = &pdev->dev;
+			msc->pcc_cl.rx_callback = mpam_pcc_rx_callback;
+			msc->pcc_cl.tx_block = false;
+			msc->pcc_cl.tx_tout = 1000; /* 1s */
+			msc->pcc_cl.knows_txdone = false;
+
+			msc->pcc_chan = pcc_mbox_request_channel(&msc->pcc_cl,
+								 msc->pcc_subspace_id);
+			if (IS_ERR(msc->pcc_chan)) {
+				pr_err("Failed to request MSC PCC channel\n");
+				err = PTR_ERR(msc->pcc_chan);
+				break;
+			}

I don't see pcc support added in this series. Should we fail the probe
if this interface is specified?

I've got patches from Andre P to support it on DT - but the platforms that need it keeping
popping in and out of existence. I'll pull these bits out - they were intended to check
the ACPI table wasn't totally rotten...

quoted

(If keeping, there is a missing pcc_mbox_free_channel() on the error path.)

When pcc_mbox_request_channel() fails? It already called mbox_free_channel() itself.

Apologies, this was relating to if the *_parse_resources() call below
failed.

quoted

quoted

+		}
+
+		list_add_rcu(&msc->glbl_list, &mpam_all_msc);
+		platform_set_drvdata(pdev, msc);
+	} while (0);
+	mutex_unlock(&mpam_list_lock);
+
+	if (!err) {
+		/* Create RIS entries described by firmware */
+		if (!acpi_disabled)
+			err = acpi_mpam_parse_resources(msc, plat_data);
+		else
+			err = mpam_dt_parse_resources(msc, plat_data);
+	}
+
+	if (!err && fw_num_msc == mpam_num_msc)
+		mpam_discovery_complete();
+
+	if (err && msc)
+		mpam_msc_drv_remove(pdev);
+
+	return err;
+}

[snip]>

Thanks,

James

Thanks,

Ben