Re: [PATCH 10/33] arm_mpam: Add probe/remove for mpam msc driver and kbuild boiler plate

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Sat, 8 Nov 2025 19:28:43 +1000
Gavin Shan <gshan@redhat.com> wrote:

> Hi Ben,
> 
> On 11/7/25 10:34 PM, Ben Horgan wrote:
> > From: James Morse <james.morse@arm.com>
> > 
> > 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.
> >   
> 
> I'm not sure if below commit log is more clearer as I'm not a English
> native speaker:

I'm normally very flexible on English usage as long as meaning is
conveyed but I'm not keen on changing authors Engish in a fashion that
doesn't necessarily improve it. So a few thoughts from me as a native
speaker. Note don't mind changing text for clarity if the English usage
is too obscure.

> 
> MPAM probing is convoluted. MSCs that are integrated to a set of CPUs
> may only be accessible from those CPUs, ...

To me both are equally valid.

Probing MPAM -> implies MPAM is a thing which is probed (MPAM as noun)
MPAM probing -> implies that MPAM is a special form of probing (MPAM as kind
of adjective giving the flavor of probing that is happening)

So slight preference from me for current text.

> 
> > Start with driver probe/remove and mapping the MSC.
> > 
> > CC: Carl Worth <carl@os.amperecomputing.com>
> > Tested-by: Fenghua Yu <fenghuay@nvidia.com>
> > Tested-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> > Tested-by: Peter Newman <peternewman@google.com>
> > Signed-off-by: James Morse <james.morse@arm.com>
> > Signed-off-by: Ben Horgan <ben.horgan@arm.com>

> > diff --git a/drivers/resctrl/Kconfig b/drivers/resctrl/Kconfig
> > new file mode 100644
> > index 000000000000..ef2f3adf64a9
> > --- /dev/null
> > +++ b/drivers/resctrl/Kconfig
> > @@ -0,0 +1,15 @@
> > +menuconfig ARM64_MPAM_DRIVER
> > +	bool "MPAM driver"
> > +	depends on ARM64 && ARM64_MPAM && EXPERT
> > +	help
> > +	  Memory System Resource Partitioning and Monitoring (MPAM) driver for
> > +	  System IP, e,g. caches and memory controllers.
> > +
> > +if ARM64_MPAM_DRIVER
> > +
> > +config ARM64_MPAM_DRIVER_DEBUG
> > +	bool "Enable debug messages from the MPAM driver"
> > +	help
> > +	  Say yes here to enable debug messages from the MPAM driver.
> > +
> > +endif  
> 
> I am asking myself why "depends on ARM64_MPAM_DRIVER" can't be used here? :-)

Fairly sure that came up in an earlier review. IIRC other stuff going to be added
later in this if/endif block.

> 
> > diff --git a/drivers/resctrl/Makefile b/drivers/resctrl/Makefile
> > new file mode 100644
> > index 000000000000..898199dcf80d
> > --- /dev/null
> > +++ b/drivers/resctrl/Makefile
> > @@ -0,0 +1,4 @@
> > +obj-$(CONFIG_ARM64_MPAM_DRIVER)			+= mpam.o
> > +mpam-y						+= mpam_devices.o
> > +
> > +ccflags-$(CONFIG_ARM64_MPAM_DRIVER_DEBUG)	+= -DDEBUG
> > diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> > new file mode 100644
> > index 000000000000..6c6be133d73a
> > --- /dev/null
> > +++ b/drivers/resctrl/mpam_devices.c
> > @@ -0,0 +1,194 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (C) 2025 Arm Ltd.
> > +
> > +#define pr_fmt(fmt) "%s:%s: " fmt, KBUILD_MODNAME, __func__
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/arm_mpam.h>
> > +#include <linux/cacheinfo.h>
> > +#include <linux/cpumask.h>
> > +#include <linux/device.h>
> > +#include <linux/errno.h>
> > +#include <linux/gfp.h>
> > +#include <linux/list.h>
> > +#include <linux/lockdep.h>
> > +#include <linux/mutex.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/printk.h>
> > +#include <linux/srcu.h>
> > +#include <linux/types.h>
> > +
> > +#include "mpam_internal.h"
> > +
> > +/*
> > + * mpam_list_lock protects the SRCU lists when writing. Once the
> > + * mpam_enabled key is enabled these lists are read-only,
> > + * unless the error interrupt disables the driver.
> > + */  
> 
> s/when writing/for writing
To me not a clear improvement.  The when writing is a bit passive in that
it is talking about something that 'is the case' rather than something 'we
make' the case. 

> s/are read-only/become read-only
Nope to this one.  Become implies it happens at a later stage, are
implies that it happens before mpam_enabled_key is enabled which is more
correct (subtle though!)

> 
> > +static DEFINE_MUTEX(mpam_list_lock);
> > +static LIST_HEAD(mpam_all_msc);
> > +
> > +struct srcu_struct mpam_srcu;
> > +
> > +/*
> > + * Number of MSCs that have been probed. Once all MSC have been probed MPAM
> > + * can be enabled.
> > + */  
> 
> s/all MSC/all MSCs  (?)
yes.
> 
> > +static atomic_t mpam_num_msc;
> > +
> > +/*
> > + * An MSC can control traffic from a set of CPUs, but may only be accessible
> > + * from a (hopefully wider) set of CPUs. The common reason for this is power
> > + * management. If all the CPUs in a cluster are in PSCI:CPU_SUSPEND, the
> > + * corresponding cache may also be powered off. By making accesses from
> > + * one of those CPUs, we ensure this isn't the case.
> > + */  
> 
> s/An MSC/A MSC (?)

No on this one.  I would pronounce MSC and Em-ess-sea
so rules are you therefore use An.  This is a truely weird quirk of
English!


> s/from a/from the
Another subtle case.  "the" implies that we all know exactly which wider
set of CPUs.  "a" implies that there is at least one that can use. So
in my mind, a is slightly more correct, but others may disagree.

> s/isn't the case/is the case (?)

I think original is correct.  The thing that isn't the case is the
".. may also be powered off." and that's what we want to avoid.
I'm not 100% sure on intention of that comment though - so good
one to query!

Thanks,

Jonathan