Message-ID: <19ca6019-de69-4c5d-bf06-2e04a254f286@arm.com>
Date: Mon, 8 Sep 2025 11:54:27 +0100
From: Ben Horgan <ben.horgan@....com>
To: James Morse <james.morse@....com>, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-acpi@...r.kernel.org,
 devicetree@...r.kernel.org
Cc: shameerali.kolothum.thodi@...wei.com,
 D Scott Phillips OS <scott@...amperecomputing.com>,
 carl@...amperecomputing.com, lcherian@...vell.com,
 bobo.shaobowang@...wei.com, tan.shaopeng@...itsu.com,
 baolin.wang@...ux.alibaba.com, Jamie Iles <quic_jiles@...cinc.com>,
 Xin Hao <xhao@...ux.alibaba.com>, peternewman@...gle.com,
 dfustini@...libre.com, amitsinght@...vell.com,
 David Hildenbrand <david@...hat.com>, Rex Nie <rex.nie@...uarmicro.com>,
 Dave Martin <dave.martin@....com>, Koba Ko <kobak@...dia.com>,
 Shanker Donthineni <sdonthineni@...dia.com>, fenghuay@...dia.com,
 baisheng.gao@...soc.com, Jonathan Cameron <jonathan.cameron@...wei.com>,
 Rob Herring <robh@...nel.org>, Rohit Mathew <rohit.mathew@....com>,
 Rafael Wysocki <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
 Lorenzo Pieralisi <lpieralisi@...nel.org>, Hanjun Guo
 <guohanjun@...wei.com>, Sudeep Holla <sudeep.holla@....com>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Catalin Marinas <catalin.marinas@....com>,
 Will Deacon <will@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Danilo Krummrich <dakr@...nel.org>
Subject: Re: [PATCH 10/33] arm_mpam: Add probe/remove for mpam msc driver and
 kbuild boiler plate

Hi James,

On 9/5/25 19:48, James Morse wrote:
> Hi Ben,
> 
> On 27/08/2025 14:03, Ben Horgan wrote:
>> On 8/22/25 16:29, James Morse wrote:
>>> 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.
> 
>>> 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 @@
> 
>>> +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@...00 {
        compatible = "foo,a-standalone-msc";
        reg = <0x80000 0x1000>;

	...
        msc@...00 {
            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]
>>> +		} 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...
> 
> 
>> (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.

> 
> 
>>> +		}
>>> +
>>> +		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

Powered by blists - more mailing lists