[PATCH v3 00/16] arm_mpam: Add MPAM-Fb firmware support

[PATCH v3 00/16] arm_mpam: Add MPAM-Fb firmware support

[Andre Przywara]

Hi,

meet version 3 of the MPAM-Fb code, for firmware based MSC accesses.
Compared to last week's drop, this fixes many bugs that Ben's diligent
review revealed, many thanks for that!
No real major changes this time, though the revised mon_sel locking has
changed to a simpler scheme, and the MPAM-Fb protocol version is now checked
for compatibility at probe time. For the remaining smaller changes, find a
changelog below. Based on v7.2-rc1.

=======================
The Arm MPAM specification defines Memory System Components (MSCs),
which are devices that are programmed through an MMIO register frame. In
some occasions this turned out to be too limiting: the MSC might be
located behind a separate bus system (for instance inside an on-board
controller), it might be mapped secure-only, or in a different processor
socket without direct MMIO mapping. Also the MMIO access might be too slow
or it would need to be filtered or otherwise access controlled. Finally
there might be bugs in the MSC integration, which require a mediating
firmware to be accessible.

To accommodate all those different use cases, the MPAM-Fb specification
[1] describes an alternative way to access MSCs. Accesses to an MSC
would be wrapped in a message and communicated to the system using a
shared-memory/mailbox system mostly mimicking the Arm SCMI spec.
For ACPI systems, this would be abstracted through an ACPI PCC channel,
which provides the shared-memory region and the mailbox trigger. We can
lean on existing ACPI parsing code to register with these two
subsystems, but cannot rely on the existing SCMI code in the kernel.
This means we somewhat need to open code a very simplified SCMI handler,
which just provides enough functionality for the very basic subset of
SCMI that the MPAM-Fb spec requires.

The first 12 patches rework all MSC access wrappers to propagate error
information. Pure MMIO based MSC accesses would never fail, but the
MPAM-Fb access can go wrong in multiple ways. The patches have been split
up purely for reviewing reasons, if the number is a problem, we could as
well squash them.
Patch 13/16 solves a nasty problem: At the moment we protect stateful MSC
register accesses (mon_sel) through a spinlock. Unfortunately the mailbox
subsystem and the slow nature of the communication through this channel
forbid MPAM-Fb access in atomic context. So this patch uses a mutex when
using MPAM-Fb, and the locking fails when trying to grab that mutex in
atomic context, for instance when programming MSCs inside an interrupt
handler. We just deny the latter when using MPAM-Fb, ideally we wouldn't
need that (no need to IPI another core when the MSC access does not need
to be local to one particular core), or we simply deny that part of the
functionality (access through perf).
Patch 14/16 adds the code to redirect MSC accesses through the
PCC shmem/mailbox system.
Patch 15/16 avoids the error IRQ handler to do an MSC access when using
MPAM_Fb, since those accesses cannot run in atomic context.
The final patch 16/16 then adds the code to detect and store the PCC
channel information from the ACPI tables, and eventually enables
MPAM-Fb accesses.

This would enable systems where some MSCs are not accessible via MMIO to
use those components anyway.

Please have a look and test!

Cheers,
Andre

[1] https://developer.arm.com/documentation/den0144/latest

Changes in v3:
- drop inner/outer mon_sel lock patch, replace with simpler version
- harmonise code patterns in error propagation changes
- drop mon_sel lock before erroring out in mpam_ris_hw_probe_csu_nrdy()
- refactor mpam_msc_read_mbwu_l() to return an error
- drop special NRDY handling in __ris_msmon_read()
- add IRQ numbers in error interrupt handler to help pinpoint failure
- refactor MPAM-Fb message generation to accommodate more than read/write
- check MPAM-Fb protocol version at probe time
- drop mpam_fb.h header, merge into mpam_internal.h
- translate MPAM-Fb error code in Linux codes where needed
- clear IRQ flag bit in MPAM-Fb protocol header
- drop unneeded endianness conversions when crafting MPAM-Fb message
- use C struct to model MPAM-Fb message payload

Changes in v2:
- add patches to add error propagation to MSC access wrappers
- drop former patch 1/5 (not needed)
- drop lock in mpam_reprogram_msc(), to avoid double lock
- add support for multiple MSCs per PCC channel
- adjust SCMI protocol code to use a PCC subtype 3 channel
- let PCC code handle the PCC channel negotiation (due to subtype 3)
- drop SCMI names in shmem offsets, and use existing PCC type 3 struct
- adjust shmem field offsets to match MPAM-Fb spec, not pure SCMI
- prevent MPAM-Fb calls inside atomic smp_call_function_any() payload
- skip all MSC accesses inside the IRQ handler when using MPAM-Fb

Andre Przywara (16):
  arm_mpam: let low level MSC read accessors return an error
  arm_mpam: propagate MSC read errors for wrapper functions
  arm_mpam: propagate MSC read errors for hw_probe functions
  arm_mpam: propagate MSC read errors for mpam_msc_read_mbwu_l()
  arm_mpam: propagate MSC read errors for msmon helpers
  arm_mpam: propagate MSC read errors for __ris_msmon_read()
  arm_mpam: __ris_msmon_read(): get rid of nrdy special handling
  arm_mpam: propagate MSC read errors for state saving functions
  arm_mpam: let low level MSC write accessors return an error
  arm_mpam: propagate MSC write errors for ESR and part_sel wrappers
  arm_mpam: propagate MSC write errors for hardware probe functions
  arm_mpam: propagate MSC write errors for remaining MSC write users
  arm_mpam: prepare mon_sel locking for MPAM-Fb
  arm_mpam: add MPAM-Fb MSC firmware access support
  arm_mpam: prevent MPAM-Fb accesses inside IRQ handler
  arm_mpam: detect and enable MPAM-Fb PCC support

 drivers/acpi/arm64/mpam.c       |   2 +
 drivers/resctrl/Makefile        |   2 +-
 drivers/resctrl/mpam_devices.c  | 651 ++++++++++++++++++++++++--------
 drivers/resctrl/mpam_fb.c       | 208 ++++++++++
 drivers/resctrl/mpam_internal.h |  48 ++-
 include/linux/arm_mpam.h        |   2 +-
 6 files changed, 740 insertions(+), 173 deletions(-)
 create mode 100644 drivers/resctrl/mpam_fb.c

-- 
2.43.0

[PATCH v3 01/16] arm_mpam: let low level MSC read accessors return an error

[Andre Przywara]

The upcoming MPAM-Fb support does not use MMIO primitives to access an
MSC, but employs a shared-memory/doorbell based firmware protocol.
Its complexity means that is must be able to handle errors, whereas we
always assume an MSC access succeeds today.

Change the __mpam_read_reg() low level accessor function to return the
requested data through a pointer, and return an error code instead.
At the moment this is always 0, but this will change with alternative
MSC access methods.
Change all users of those MSC read wrappers to comply with the new
prototype, though at the moment without propagating any errors.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/resctrl/mpam_devices.c | 129 ++++++++++++++++++++-------------
 1 file changed, 78 insertions(+), 51 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index b69f99488111..df14b4513382 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -177,20 +177,23 @@ static void mpam_assert_partid_sizes_fixed(void)
 		WARN_ON_ONCE(!partid_max_published);
 }
 
-static u32 __mpam_read_reg(struct mpam_msc *msc, u16 reg)
+static int __mpam_read_reg(struct mpam_msc *msc, u16 reg, u32 *res)
 {
 	WARN_ON_ONCE(!cpumask_test_cpu(smp_processor_id(), &msc->accessibility));
 
-	return readl_relaxed(msc->mapped_hwpage + reg);
+	*res = readl_relaxed(msc->mapped_hwpage + reg);
+
+	return 0;
 }
 
-static inline u32 _mpam_read_partsel_reg(struct mpam_msc *msc, u16 reg)
+static inline int _mpam_read_partsel_reg(struct mpam_msc *msc, u16 reg,
+					 u32 *res)
 {
 	lockdep_assert_held_once(&msc->part_sel_lock);
-	return __mpam_read_reg(msc, reg);
+	return __mpam_read_reg(msc, reg, res);
 }
 
-#define mpam_read_partsel_reg(msc, reg) _mpam_read_partsel_reg(msc, MPAMF_##reg)
+#define mpam_read_partsel_reg(msc, reg, res) _mpam_read_partsel_reg(msc, MPAMF_##reg, res)
 
 static void __mpam_write_reg(struct mpam_msc *msc, u16 reg, u32 val)
 {
@@ -208,13 +211,14 @@ static inline void _mpam_write_partsel_reg(struct mpam_msc *msc, u16 reg, u32 va
 
 #define mpam_write_partsel_reg(msc, reg, val)  _mpam_write_partsel_reg(msc, MPAMCFG_##reg, val)
 
-static inline u32 _mpam_read_monsel_reg(struct mpam_msc *msc, u16 reg)
+static inline int _mpam_read_monsel_reg(struct mpam_msc *msc, u16 reg,
+					u32 *res)
 {
 	mpam_mon_sel_lock_held(msc);
-	return __mpam_read_reg(msc, reg);
+	return __mpam_read_reg(msc, reg, res);
 }
 
-#define mpam_read_monsel_reg(msc, reg) _mpam_read_monsel_reg(msc, MSMON_##reg)
+#define mpam_read_monsel_reg(msc, reg, res) _mpam_read_monsel_reg(msc, MSMON_##reg, res)
 
 static inline void _mpam_write_monsel_reg(struct mpam_msc *msc, u16 reg, u32 val)
 {
@@ -226,10 +230,11 @@ static inline void _mpam_write_monsel_reg(struct mpam_msc *msc, u16 reg, u32 val
 
 static bool mpam_msc_check_aidr(struct mpam_msc *msc)
 {
-	u32 aidr = __mpam_read_reg(msc, MPAMF_AIDR);
-	u32 major = FIELD_GET(MPAMF_AIDR_ARCH_MAJOR_REV, aidr);
-	u32 minor = FIELD_GET(MPAMF_AIDR_ARCH_MINOR_REV, aidr);
+	u32 aidr, major, minor;
 
+	__mpam_read_reg(msc, MPAMF_AIDR, &aidr);
+	major = FIELD_GET(MPAMF_AIDR_ARCH_MAJOR_REV, aidr);
+	minor = FIELD_GET(MPAMF_AIDR_ARCH_MINOR_REV, aidr);
 	/*
 	 * v0.0 and >v2.x aren't supported, but anything else should be backward
 	 * compatible to v0.1 or v1.0.
@@ -244,20 +249,22 @@ static bool mpam_msc_check_aidr(struct mpam_msc *msc)
 
 static u64 mpam_msc_read_idr(struct mpam_msc *msc)
 {
-	u64 idr_high = 0, idr_low;
+	u32 idr_high = 0, idr_low;
 
 	lockdep_assert_held(&msc->part_sel_lock);
 
-	idr_low = mpam_read_partsel_reg(msc, IDR);
+	mpam_read_partsel_reg(msc, IDR, &idr_low);
 	if (FIELD_GET(MPAMF_IDR_EXT, idr_low))
-		idr_high = mpam_read_partsel_reg(msc, IDR + 4);
+		mpam_read_partsel_reg(msc, IDR + 4, &idr_high);
 
-	return (idr_high << 32) | idr_low;
+	return ((u64)idr_high << 32) | idr_low;
 }
 
 static void mpam_msc_clear_esr(struct mpam_msc *msc)
 {
-	u64 esr_low = __mpam_read_reg(msc, MPAMF_ESR);
+	u32 esr_low;
+
+	__mpam_read_reg(msc, MPAMF_ESR, &esr_low);
 
 	if (!esr_low)
 		return;
@@ -275,13 +282,13 @@ static void mpam_msc_clear_esr(struct mpam_msc *msc)
 
 static u64 mpam_msc_read_esr(struct mpam_msc *msc)
 {
-	u64 esr_high = 0, esr_low;
+	u32 esr_high = 0, esr_low;
 
-	esr_low = __mpam_read_reg(msc, MPAMF_ESR);
+	__mpam_read_reg(msc, MPAMF_ESR, &esr_low);
 	if (msc->has_extd_esr)
-		esr_high = __mpam_read_reg(msc, MPAMF_ESR + 4);
+		__mpam_read_reg(msc, MPAMF_ESR + 4, &esr_high);
 
-	return (esr_high << 32) | esr_low;
+	return ((u64)esr_high << 32) | esr_low;
 }
 
 static void __mpam_part_sel_raw(u32 partsel, struct mpam_msc *msc)
@@ -774,13 +781,13 @@ static bool mpam_ris_hw_probe_csu_nrdy(struct mpam_msc_ris *ris)
 	mpam_write_monsel_reg(msc, CFG_CSU_CTL, ctl_val);
 
 	_mpam_write_monsel_reg(msc, MSMON_CSU, MSMON___NRDY);
-	now = _mpam_read_monsel_reg(msc, MSMON_CSU);
+	_mpam_read_monsel_reg(msc, MSMON_CSU, &now);
 	can_set = now & MSMON___NRDY;
 
 	_mpam_write_monsel_reg(msc, MSMON_CSU, 0);
 	/* Configuration change to try and coax hardware into setting nrdy */
 	mpam_write_monsel_reg(msc, CFG_CSU_FLT, 0x1);
-	now = _mpam_read_monsel_reg(msc, MSMON_CSU);
+	_mpam_read_monsel_reg(msc, MSMON_CSU, &now);
 	can_clear = !(now & MSMON___NRDY);
 	mpam_mon_sel_unlock(msc);
 
@@ -800,7 +807,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
 
 	/* Cache Capacity Partitioning */
 	if (FIELD_GET(MPAMF_IDR_HAS_CCAP_PART, ris->idr)) {
-		u32 ccap_features = mpam_read_partsel_reg(msc, CCAP_IDR);
+		u32 ccap_features;
+
+		mpam_read_partsel_reg(msc, CCAP_IDR, &ccap_features);
 
 		props->cmax_wd = FIELD_GET(MPAMF_CCAP_IDR_CMAX_WD, ccap_features);
 		if (props->cmax_wd &&
@@ -823,7 +832,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
 
 	/* Cache Portion partitioning */
 	if (FIELD_GET(MPAMF_IDR_HAS_CPOR_PART, ris->idr)) {
-		u32 cpor_features = mpam_read_partsel_reg(msc, CPOR_IDR);
+		u32 cpor_features;
+
+		mpam_read_partsel_reg(msc, CPOR_IDR, &cpor_features);
 
 		props->cpbm_wd = FIELD_GET(MPAMF_CPOR_IDR_CPBM_WD, cpor_features);
 		if (props->cpbm_wd)
@@ -832,7 +843,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
 
 	/* Memory bandwidth partitioning */
 	if (FIELD_GET(MPAMF_IDR_HAS_MBW_PART, ris->idr)) {
-		u32 mbw_features = mpam_read_partsel_reg(msc, MBW_IDR);
+		u32 mbw_features;
+
+		mpam_read_partsel_reg(msc, MBW_IDR, &mbw_features);
 
 		/* portion bitmap resolution */
 		props->mbw_pbm_bits = FIELD_GET(MPAMF_MBW_IDR_BWPBM_WD, mbw_features);
@@ -860,7 +873,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
 
 	/* Priority partitioning */
 	if (FIELD_GET(MPAMF_IDR_HAS_PRI_PART, ris->idr)) {
-		u32 pri_features = mpam_read_partsel_reg(msc, PRI_IDR);
+		u32 pri_features;
+
+		mpam_read_partsel_reg(msc, PRI_IDR, &pri_features);
 
 		props->intpri_wd = FIELD_GET(MPAMF_PRI_IDR_INTPRI_WD, pri_features);
 		if (props->intpri_wd && FIELD_GET(MPAMF_PRI_IDR_HAS_INTPRI, pri_features)) {
@@ -879,7 +894,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
 
 	/* Performance Monitoring */
 	if (FIELD_GET(MPAMF_IDR_HAS_MSMON, ris->idr)) {
-		u32 msmon_features = mpam_read_partsel_reg(msc, MSMON_IDR);
+		u32 msmon_features;
+
+		mpam_read_partsel_reg(msc, MSMON_IDR, &msmon_features);
 
 		/*
 		 * If the firmware max-nrdy-us property is missing, the
@@ -892,7 +909,8 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
 		if (FIELD_GET(MPAMF_MSMON_IDR_MSMON_CSU, msmon_features)) {
 			u32 csumonidr;
 
-			csumonidr = mpam_read_partsel_reg(msc, CSUMON_IDR);
+			mpam_read_partsel_reg(msc, CSUMON_IDR, &csumonidr);
+
 			props->num_csu_mon = FIELD_GET(MPAMF_CSUMON_IDR_NUM_MON, csumonidr);
 			if (props->num_csu_mon) {
 				bool hw_managed;
@@ -915,7 +933,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
 		}
 		if (FIELD_GET(MPAMF_MSMON_IDR_MSMON_MBWU, msmon_features)) {
 			bool has_long;
-			u32 mbwumon_idr = mpam_read_partsel_reg(msc, MBWUMON_IDR);
+			u32 mbwumon_idr;
+
+			mpam_read_partsel_reg(msc, MBWUMON_IDR, &mbwumon_idr);
 
 			props->num_mbwu_mon = FIELD_GET(MPAMF_MBWUMON_IDR_NUM_MON, mbwumon_idr);
 			if (props->num_mbwu_mon) {
@@ -945,8 +965,11 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
 	 */
 	if (FIELD_GET(MPAMF_IDR_HAS_PARTID_NRW, ris->idr) &&
 	    class->type != MPAM_CLASS_UNKNOWN) {
-		u32 nrwidr = mpam_read_partsel_reg(msc, PARTID_NRW_IDR);
-		u16 partid_max = FIELD_GET(MPAMF_PARTID_NRW_IDR_INTPARTID_MAX, nrwidr);
+		u16 partid_max;
+		u32 nrwidr;
+
+		mpam_read_partsel_reg(msc, PARTID_NRW_IDR, &nrwidr);
+		partid_max = FIELD_GET(MPAMF_PARTID_NRW_IDR_INTPARTID_MAX, nrwidr);
 
 		mpam_set_feature(mpam_feat_partid_nrw, props);
 		msc->partid_max = min(msc->partid_max, partid_max);
@@ -971,7 +994,8 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc)
 	/* Grab an IDR value to find out how many RIS there are */
 	mutex_lock(&msc->part_sel_lock);
 	idr = mpam_msc_read_idr(msc);
-	msc->iidr = mpam_read_partsel_reg(msc, IIDR);
+	mpam_read_partsel_reg(msc, IIDR, &msc->iidr);
+
 	mutex_unlock(&msc->part_sel_lock);
 
 	mpam_enable_quirks(msc);
@@ -1039,24 +1063,24 @@ static u64 mpam_msc_read_mbwu_l(struct mpam_msc *msc)
 {
 	int retry = 3;
 	u32 mbwu_l_low;
-	u64 mbwu_l_high1, mbwu_l_high2;
+	u32 mbwu_l_high1, mbwu_l_high2;
 
 	mpam_mon_sel_lock_held(msc);
 
 	WARN_ON_ONCE((MSMON_MBWU_L + sizeof(u64)) > msc->mapped_hwpage_sz);
 	WARN_ON_ONCE(!cpumask_test_cpu(smp_processor_id(), &msc->accessibility));
 
-	mbwu_l_high2 = __mpam_read_reg(msc, MSMON_MBWU_L + 4);
+	__mpam_read_reg(msc, MSMON_MBWU_L + 4, &mbwu_l_high2);
 	do {
 		mbwu_l_high1 = mbwu_l_high2;
-		mbwu_l_low = __mpam_read_reg(msc, MSMON_MBWU_L);
-		mbwu_l_high2 = __mpam_read_reg(msc, MSMON_MBWU_L + 4);
+		__mpam_read_reg(msc, MSMON_MBWU_L, &mbwu_l_low);
+		__mpam_read_reg(msc, MSMON_MBWU_L + 4, &mbwu_l_high2);
 
 		retry--;
 	} while (mbwu_l_high1 != mbwu_l_high2 && retry > 0);
 
 	if (mbwu_l_high1 == mbwu_l_high2)
-		return (mbwu_l_high1 << 32) | mbwu_l_low;
+		return ((u64)mbwu_l_high1 << 32) | mbwu_l_low;
 
 	pr_warn("Failed to read a stable value\n");
 	return MSMON___L_NRDY;
@@ -1120,14 +1144,14 @@ static void read_msmon_ctl_flt_vals(struct mon_read *m, u32 *ctl_val,
 
 	switch (m->type) {
 	case mpam_feat_msmon_csu:
-		*ctl_val = mpam_read_monsel_reg(msc, CFG_CSU_CTL);
-		*flt_val = mpam_read_monsel_reg(msc, CFG_CSU_FLT);
+		mpam_read_monsel_reg(msc, CFG_CSU_CTL, ctl_val);
+		mpam_read_monsel_reg(msc, CFG_CSU_FLT, flt_val);
 		break;
 	case mpam_feat_msmon_mbwu_31counter:
 	case mpam_feat_msmon_mbwu_44counter:
 	case mpam_feat_msmon_mbwu_63counter:
-		*ctl_val = mpam_read_monsel_reg(msc, CFG_MBWU_CTL);
-		*flt_val = mpam_read_monsel_reg(msc, CFG_MBWU_FLT);
+		mpam_read_monsel_reg(msc, CFG_MBWU_CTL, ctl_val);
+		mpam_read_monsel_reg(msc, CFG_MBWU_FLT, flt_val);
 		break;
 	default:
 		pr_warn("Unexpected monitor type %d\n", m->type);
@@ -1206,6 +1230,7 @@ static u64 mpam_msmon_overflow_val(enum mpam_device_features type,
 static void __ris_msmon_read(void *arg)
 {
 	u64 now;
+	u32 now32;
 	bool nrdy = false;
 	bool config_mismatch;
 	bool overflow = false;
@@ -1268,9 +1293,9 @@ static void __ris_msmon_read(void *arg)
 
 	switch (m->type) {
 	case mpam_feat_msmon_csu:
-		now = mpam_read_monsel_reg(msc, CSU);
-		nrdy = now & MSMON___NRDY;
-		now = FIELD_GET(MSMON___VALUE, now);
+		mpam_read_monsel_reg(msc, CSU, &now32);
+		nrdy = now32 & MSMON___NRDY;
+		now = FIELD_GET(MSMON___VALUE, now32);
 
 		if (mpam_has_quirk(IGNORE_CSU_NRDY, msc) && m->waited_timeout)
 			nrdy = false;
@@ -1288,9 +1313,9 @@ static void __ris_msmon_read(void *arg)
 			else
 				now = FIELD_GET(MSMON___L_VALUE, now);
 		} else {
-			now = mpam_read_monsel_reg(msc, MBWU);
-			nrdy = now & MSMON___NRDY;
-			now = FIELD_GET(MSMON___VALUE, now);
+			mpam_read_monsel_reg(msc, MBWU, &now32);
+			nrdy = now32 & MSMON___NRDY;
+			now = FIELD_GET(MSMON___VALUE, now32);
 		}
 
 		if (mpam_has_quirk(T241_MBW_COUNTER_SCALE_64, msc) &&
@@ -1685,16 +1710,18 @@ static int mpam_save_mbwu_state(void *arg)
 		mon_sel = FIELD_PREP(MSMON_CFG_MON_SEL_MON_SEL, i) |
 			  FIELD_PREP(MSMON_CFG_MON_SEL_RIS, ris->ris_idx);
 		mpam_write_monsel_reg(msc, CFG_MON_SEL, mon_sel);
-
-		cur_flt = mpam_read_monsel_reg(msc, CFG_MBWU_FLT);
-		cur_ctl = mpam_read_monsel_reg(msc, CFG_MBWU_CTL);
+		mpam_read_monsel_reg(msc, CFG_MBWU_FLT, &cur_flt);
+		mpam_read_monsel_reg(msc, CFG_MBWU_CTL, &cur_ctl);
 		mpam_write_monsel_reg(msc, CFG_MBWU_CTL, 0);
 
 		if (mpam_ris_has_mbwu_long_counter(ris)) {
 			val = mpam_msc_read_mbwu_l(msc);
 			mpam_msc_zero_mbwu_l(msc);
 		} else {
-			val = mpam_read_monsel_reg(msc, MBWU);
+			u32 val32;
+
+			mpam_read_monsel_reg(msc, MBWU, &val32);
+			val = val32;
 			mpam_write_monsel_reg(msc, MBWU, 0);
 		}
 
-- 
2.43.0

Re: [PATCH v3 01/16] arm_mpam: let low level MSC read accessors return an error

[Jonathan Cameron]

On Fri, 10 Jul 2026 16:45:05 +0200
Andre Przywara <andre.przywara@arm.com> wrote:

> The upcoming MPAM-Fb support does not use MMIO primitives to access an
> MSC, but employs a shared-memory/doorbell based firmware protocol.
> Its complexity means that is must be able to handle errors, whereas we
> always assume an MSC access succeeds today.
> 
> Change the __mpam_read_reg() low level accessor function to return the
> requested data through a pointer, and return an error code instead.
> At the moment this is always 0, but this will change with alternative
> MSC access methods.
> Change all users of those MSC read wrappers to comply with the new
> prototype, though at the moment without propagating any errors.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Why do it in this order? It seems like it is a path for some code churn 
from patch to path.  If you started at the outermost calls and
worked in adding error handling at each layer, you wouldn't end up with
the change here to add the parameter, then the same lines changed again
to stash the new return value. Those lines would change just once.
Maybe I'm wrong though and it ends up even messier.

Or as you suggest, maybe just squash the patches, which will also
remove the churn.

Jonathan

> @@ -226,10 +230,11 @@ static inline void _mpam_write_monsel_reg(struct mpam_msc *msc, u16 reg, u32 val
>  
>  static bool mpam_msc_check_aidr(struct mpam_msc *msc)
>  {
> -	u32 aidr = __mpam_read_reg(msc, MPAMF_AIDR);
> -	u32 major = FIELD_GET(MPAMF_AIDR_ARCH_MAJOR_REV, aidr);
> -	u32 minor = FIELD_GET(MPAMF_AIDR_ARCH_MINOR_REV, aidr);
> +	u32 aidr, major, minor;
>  
> +	__mpam_read_reg(msc, MPAMF_AIDR, &aidr);

This will need updating to handle the error.  If you were to instead do
the patches in the opposite order. So add return value to the outer
most calls that is always 0 then work your way in it should end up
as a fair bit less code churn.

That update is in a later patch.

> +	major = FIELD_GET(MPAMF_AIDR_ARCH_MAJOR_REV, aidr);
> +	minor = FIELD_GET(MPAMF_AIDR_ARCH_MINOR_REV, aidr);
>  	/*
>  	 * v0.0 and >v2.x aren't supported, but anything else should be backward
>  	 * compatible to v0.1 or v1.0.

Re: [PATCH v3 01/16] arm_mpam: let low level MSC read accessors return an error

[Andre Przywara]

Hi Jonathan,

many thanks for the time you spent on this - though it wasn't 
particularly pretty, I guess.

This is just a quick reply, unfortunately (well...) I will be on 
holidays next week, so cannot reply in detail now.

On 7/10/26 20:11, Jonathan Cameron wrote:
> On Fri, 10 Jul 2026 16:45:05 +0200
> Andre Przywara <andre.przywara@arm.com> wrote:
> 
>> The upcoming MPAM-Fb support does not use MMIO primitives to access an
>> MSC, but employs a shared-memory/doorbell based firmware protocol.
>> Its complexity means that is must be able to handle errors, whereas we
>> always assume an MSC access succeeds today.
>>
>> Change the __mpam_read_reg() low level accessor function to return the
>> requested data through a pointer, and return an error code instead.
>> At the moment this is always 0, but this will change with alternative
>> MSC access methods.
>> Change all users of those MSC read wrappers to comply with the new
>> prototype, though at the moment without propagating any errors.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> 
> Why do it in this order? It seems like it is a path for some code churn
> from patch to path.

Well, this is pain however you do this - as you figured. And I knew this 
before, that's why I avoided this in v1, but then found we have to bite 
the bullet at some point anyway, so we could as well just do it now.

So I did the change in one go, and the diff was completely unreadable 
and even more so unreviewable. Then I figured to start with the actual 
root cause: the low level accessors, to show the motivation, then split 
the rest up in reasonably small chunks. And with the unavoidable 
dependencies, this led to the structure you see, with the added churn of 
requested changes and refactors between v2 and v3.

> If you started at the outermost calls and
> worked in adding error handling at each layer, you wouldn't end up with
> the change here to add the parameter, then the same lines changed again
> to stash the new return value. Those lines would change just once.
> Maybe I'm wrong though and it ends up even messier.

Well, I guess there are a few ways to do this, but I doubt there is a 
really great one. And as you can imagine, there is little fun in trying 
a number of them, then comparing them. Doing this the first time was 
already painful. So I thought to just pick one approach and see what 
people say - I guess this was the wrong approach then :-(

> Or as you suggest, maybe just squash the patches, which will also
> remove the churn.
> 
> Jonathan
> 
>> @@ -226,10 +230,11 @@ static inline void _mpam_write_monsel_reg(struct mpam_msc *msc, u16 reg, u32 val
>>   
>>   static bool mpam_msc_check_aidr(struct mpam_msc *msc)
>>   {
>> -	u32 aidr = __mpam_read_reg(msc, MPAMF_AIDR);
>> -	u32 major = FIELD_GET(MPAMF_AIDR_ARCH_MAJOR_REV, aidr);
>> -	u32 minor = FIELD_GET(MPAMF_AIDR_ARCH_MINOR_REV, aidr);
>> +	u32 aidr, major, minor;
>>   
>> +	__mpam_read_reg(msc, MPAMF_AIDR, &aidr);
> 
> This will need updating to handle the error.  If you were to instead do
> the patches in the opposite order. So add return value to the outer
> most calls that is always 0 then work your way in it should end up
> as a fair bit less code churn.

But is it really better to review? You later say code lines are cheap, 
I'd say commits are cheap as well, but reviews are not. The idea was to 
allow separate, contained reviews, without forcing people to sit through 
all of this in one go - which you apparently did, so my condolences.
So yes, it's not the best to touch lines several times, but do we really 
care so much? Is it just about git blame?

Cheers,
Andre

> That update is in a later patch.
> 
>> +	major = FIELD_GET(MPAMF_AIDR_ARCH_MAJOR_REV, aidr);
>> +	minor = FIELD_GET(MPAMF_AIDR_ARCH_MINOR_REV, aidr);
>>   	/*
>>   	 * v0.0 and >v2.x aren't supported, but anything else should be backward
>>   	 * compatible to v0.1 or v1.0.

[PATCH v3 02/16] arm_mpam: propagate MSC read errors for wrapper functions

[Andre Przywara]

Allow the wrapper functions for IDR and ESR accesses to return an
error, and propagate read errors from the lower level up.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/resctrl/mpam_devices.c | 53 ++++++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 15 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index df14b4513382..8fd2c38c821c 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -247,27 +247,38 @@ static bool mpam_msc_check_aidr(struct mpam_msc *msc)
 	return true;
 }
 
-static u64 mpam_msc_read_idr(struct mpam_msc *msc)
+static int mpam_msc_read_idr(struct mpam_msc *msc, u64 *res)
 {
 	u32 idr_high = 0, idr_low;
+	int ret;
 
 	lockdep_assert_held(&msc->part_sel_lock);
 
-	mpam_read_partsel_reg(msc, IDR, &idr_low);
+	ret = mpam_read_partsel_reg(msc, IDR, &idr_low);
+	if (ret)
+		return ret;
+
 	if (FIELD_GET(MPAMF_IDR_EXT, idr_low))
-		mpam_read_partsel_reg(msc, IDR + 4, &idr_high);
+		ret = mpam_read_partsel_reg(msc, IDR + 4, &idr_high);
+	if (ret)
+		return ret;
+
+	*res = ((u64)idr_high << 32) | idr_low;
 
-	return ((u64)idr_high << 32) | idr_low;
+	return 0;
 }
 
-static void mpam_msc_clear_esr(struct mpam_msc *msc)
+static int mpam_msc_clear_esr(struct mpam_msc *msc)
 {
 	u32 esr_low;
+	int ret;
 
-	__mpam_read_reg(msc, MPAMF_ESR, &esr_low);
+	ret = __mpam_read_reg(msc, MPAMF_ESR, &esr_low);
+	if (ret)
+		return ret;
 
 	if (!esr_low)
-		return;
+		return 0;
 
 	/*
 	 * Clearing the high/low bits of MPAMF_ESR can not be atomic.
@@ -277,18 +288,30 @@ static void mpam_msc_clear_esr(struct mpam_msc *msc)
 	 */
 	if (msc->has_extd_esr)
 		__mpam_write_reg(msc, MPAMF_ESR + 4, 0);
+
 	__mpam_write_reg(msc, MPAMF_ESR, 0);
+
+	return 0;
 }
 
-static u64 mpam_msc_read_esr(struct mpam_msc *msc)
+static int mpam_msc_read_esr(struct mpam_msc *msc, u64 *res)
 {
 	u32 esr_high = 0, esr_low;
+	int ret;
 
-	__mpam_read_reg(msc, MPAMF_ESR, &esr_low);
-	if (msc->has_extd_esr)
-		__mpam_read_reg(msc, MPAMF_ESR + 4, &esr_high);
+	ret = __mpam_read_reg(msc, MPAMF_ESR, &esr_low);
+	if (ret)
+		return ret;
+
+	if (msc->has_extd_esr) {
+		ret = __mpam_read_reg(msc, MPAMF_ESR + 4, &esr_high);
+		if (ret)
+			return ret;
+	}
+
+	*res = ((u64)esr_high << 32) | esr_low;
 
-	return ((u64)esr_high << 32) | esr_low;
+	return 0;
 }
 
 static void __mpam_part_sel_raw(u32 partsel, struct mpam_msc *msc)
@@ -993,7 +1016,7 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc)
 
 	/* Grab an IDR value to find out how many RIS there are */
 	mutex_lock(&msc->part_sel_lock);
-	idr = mpam_msc_read_idr(msc);
+	mpam_msc_read_idr(msc, &idr);
 	mpam_read_partsel_reg(msc, IIDR, &msc->iidr);
 
 	mutex_unlock(&msc->part_sel_lock);
@@ -1009,7 +1032,7 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc)
 	for (ris_idx = 0; ris_idx <= msc->ris_max; ris_idx++) {
 		mutex_lock(&msc->part_sel_lock);
 		__mpam_part_sel(ris_idx, 0, msc);
-		idr = mpam_msc_read_idr(msc);
+		mpam_msc_read_idr(msc, &idr);
 		mutex_unlock(&msc->part_sel_lock);
 
 		partid_max = FIELD_GET(MPAMF_IDR_PARTID_MAX, idr);
@@ -2492,7 +2515,7 @@ static irqreturn_t __mpam_irq_handler(int irq, struct mpam_msc *msc)
 					   &msc->accessibility)))
 		return IRQ_NONE;
 
-	reg = mpam_msc_read_esr(msc);
+	mpam_msc_read_esr(msc, &reg);
 
 	errcode = FIELD_GET(MPAMF_ESR_ERRCODE, reg);
 	if (!errcode)
-- 
2.43.0

Re: [PATCH v3 02/16] arm_mpam: propagate MSC read errors for wrapper functions

[Jonathan Cameron]

On Fri, 10 Jul 2026 16:45:06 +0200
Andre Przywara <andre.przywara@arm.com> wrote:

> Allow the wrapper functions for IDR and ESR accesses to return an
> error, and propagate read errors from the lower level up.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Hi Andre,

Just a few superficial code style comments.

Thanks,

Jonathan

> ---
>  drivers/resctrl/mpam_devices.c | 53 ++++++++++++++++++++++++----------
>  1 file changed, 38 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index df14b4513382..8fd2c38c821c 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -247,27 +247,38 @@ static bool mpam_msc_check_aidr(struct mpam_msc *msc)
>  	return true;
>  }
>  
> -static u64 mpam_msc_read_idr(struct mpam_msc *msc)
> +static int mpam_msc_read_idr(struct mpam_msc *msc, u64 *res)
>  {
>  	u32 idr_high = 0, idr_low;
> +	int ret;
>  
>  	lockdep_assert_held(&msc->part_sel_lock);
>  
> -	mpam_read_partsel_reg(msc, IDR, &idr_low);
> +	ret = mpam_read_partsel_reg(msc, IDR, &idr_low);
> +	if (ret)
> +		return ret;
> +
>  	if (FIELD_GET(MPAMF_IDR_EXT, idr_low))
> -		mpam_read_partsel_reg(msc, IDR + 4, &idr_high);
> +		ret = mpam_read_partsel_reg(msc, IDR + 4, &idr_high);
> +	if (ret)
> +		return ret;

From a readability point of view, I'd indent the if (ret) as well
given that will then make it visually clear the check only applies
when the if is taken.

	if (FIELD_GET()) {
		ret = mpam_read_partsel_reg(msc, IDR + 4 &idr_high);
		if (ret)
			return ret;
	}

> +
> +	*res = ((u64)idr_high << 32) | idr_low;
>  
> -	return ((u64)idr_high << 32) | idr_low;
> +	return 0;
>  }
>  
> -static void mpam_msc_clear_esr(struct mpam_msc *msc)
> +static int mpam_msc_clear_esr(struct mpam_msc *msc)
>  {
>  	u32 esr_low;
> +	int ret;
>  
> -	__mpam_read_reg(msc, MPAMF_ESR, &esr_low);
> +	ret = __mpam_read_reg(msc, MPAMF_ESR, &esr_low);
> +	if (ret)
> +		return ret;
>  
>  	if (!esr_low)
> -		return;
> +		return 0;
>  
>  	/*
>  	 * Clearing the high/low bits of MPAMF_ESR can not be atomic.
> @@ -277,18 +288,30 @@ static void mpam_msc_clear_esr(struct mpam_msc *msc)
>  	 */
>  	if (msc->has_extd_esr)
>  		__mpam_write_reg(msc, MPAMF_ESR + 4, 0);
> +

Might be valid, but to me that smells like an unrelated cleanup
that shouldn't really be in a patch doing more meaningful work.  Perhaps
makes sense when you circle back to do writes.  BTW, I'm not sure
there is real benefit in separate patches doing reads from those doing writes!
Mind you I haven't read all the way through yet, so maybe I'm missing some
subtlety.

>  	__mpam_write_reg(msc, MPAMF_ESR, 0);
> +
> +	return 0;
>  }
>  
> -static u64 mpam_msc_read_esr(struct mpam_msc *msc)
> +static int mpam_msc_read_esr(struct mpam_msc *msc, u64 *res)
>  {
>  	u32 esr_high = 0, esr_low;
> +	int ret;
>  
> -	__mpam_read_reg(msc, MPAMF_ESR, &esr_low);
> -	if (msc->has_extd_esr)
> -		__mpam_read_reg(msc, MPAMF_ESR + 4, &esr_high);
> +	ret = __mpam_read_reg(msc, MPAMF_ESR, &esr_low);
> +	if (ret)
> +		return ret;
> +
> +	if (msc->has_extd_esr) {
> +		ret = __mpam_read_reg(msc, MPAMF_ESR + 4, &esr_high);
> +		if (ret)
> +			return ret;

So this is the style I suggest above. Good, but check for consistency.
It may feel like a really small thing (and it is :) but keeping code
very consistent helps a surprising amount when it comes to readability.

> +	}
> +
> +	*res = ((u64)esr_high << 32) | esr_low;
>  
> -	return ((u64)esr_high << 32) | esr_low;
> +	return 0;
>  }

[PATCH v3 03/16] arm_mpam: propagate MSC read errors for hw_probe functions

[Andre Przywara]

Allow the functions probing for MSC hardware and features to return an
error, and propagate read errors from the lower level up.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/resctrl/mpam_devices.c | 70 ++++++++++++++++++++++++----------
 1 file changed, 50 insertions(+), 20 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index 8fd2c38c821c..b1bd047da203 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -785,7 +785,7 @@ static void mpam_enable_quirks(struct mpam_msc *msc)
 static bool mpam_ris_hw_probe_csu_nrdy(struct mpam_msc_ris *ris)
 {
 	u32 now, mon_sel, ctl_val;
-	bool can_set, can_clear;
+	bool can_set, can_clear, ret = false;
 	struct mpam_msc *msc = ris->vmsc->msc;
 
 	if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
@@ -804,20 +804,26 @@ static bool mpam_ris_hw_probe_csu_nrdy(struct mpam_msc_ris *ris)
 	mpam_write_monsel_reg(msc, CFG_CSU_CTL, ctl_val);
 
 	_mpam_write_monsel_reg(msc, MSMON_CSU, MSMON___NRDY);
-	_mpam_read_monsel_reg(msc, MSMON_CSU, &now);
+	if (_mpam_read_monsel_reg(msc, MSMON_CSU, &now))
+		goto out_unlock;
 	can_set = now & MSMON___NRDY;
 
 	_mpam_write_monsel_reg(msc, MSMON_CSU, 0);
 	/* Configuration change to try and coax hardware into setting nrdy */
 	mpam_write_monsel_reg(msc, CFG_CSU_FLT, 0x1);
-	_mpam_read_monsel_reg(msc, MSMON_CSU, &now);
+	if (_mpam_read_monsel_reg(msc, MSMON_CSU, &now))
+		goto out_unlock;
 	can_clear = !(now & MSMON___NRDY);
+
+	ret = !can_set || !can_clear;
+
+out_unlock:
 	mpam_mon_sel_unlock(msc);
 
-	return (!can_set || !can_clear);
+	return ret;
 }
 
-static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
+static int mpam_ris_hw_probe(struct mpam_msc_ris *ris)
 {
 	int err;
 	struct mpam_msc *msc = ris->vmsc->msc;
@@ -832,7 +838,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
 	if (FIELD_GET(MPAMF_IDR_HAS_CCAP_PART, ris->idr)) {
 		u32 ccap_features;
 
-		mpam_read_partsel_reg(msc, CCAP_IDR, &ccap_features);
+		err = mpam_read_partsel_reg(msc, CCAP_IDR, &ccap_features);
+		if (err)
+			return err;
 
 		props->cmax_wd = FIELD_GET(MPAMF_CCAP_IDR_CMAX_WD, ccap_features);
 		if (props->cmax_wd &&
@@ -857,7 +865,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
 	if (FIELD_GET(MPAMF_IDR_HAS_CPOR_PART, ris->idr)) {
 		u32 cpor_features;
 
-		mpam_read_partsel_reg(msc, CPOR_IDR, &cpor_features);
+		err = mpam_read_partsel_reg(msc, CPOR_IDR, &cpor_features);
+		if (err)
+			return err;
 
 		props->cpbm_wd = FIELD_GET(MPAMF_CPOR_IDR_CPBM_WD, cpor_features);
 		if (props->cpbm_wd)
@@ -867,8 +877,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
 	/* Memory bandwidth partitioning */
 	if (FIELD_GET(MPAMF_IDR_HAS_MBW_PART, ris->idr)) {
 		u32 mbw_features;
-
-		mpam_read_partsel_reg(msc, MBW_IDR, &mbw_features);
+		err = mpam_read_partsel_reg(msc, MBW_IDR, &mbw_features);
+		if (err)
+			return err;
 
 		/* portion bitmap resolution */
 		props->mbw_pbm_bits = FIELD_GET(MPAMF_MBW_IDR_BWPBM_WD, mbw_features);
@@ -897,8 +908,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
 	/* Priority partitioning */
 	if (FIELD_GET(MPAMF_IDR_HAS_PRI_PART, ris->idr)) {
 		u32 pri_features;
-
-		mpam_read_partsel_reg(msc, PRI_IDR, &pri_features);
+		err = mpam_read_partsel_reg(msc, PRI_IDR, &pri_features);
+		if (err)
+			return err;
 
 		props->intpri_wd = FIELD_GET(MPAMF_PRI_IDR_INTPRI_WD, pri_features);
 		if (props->intpri_wd && FIELD_GET(MPAMF_PRI_IDR_HAS_INTPRI, pri_features)) {
@@ -919,7 +931,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
 	if (FIELD_GET(MPAMF_IDR_HAS_MSMON, ris->idr)) {
 		u32 msmon_features;
 
-		mpam_read_partsel_reg(msc, MSMON_IDR, &msmon_features);
+		err = mpam_read_partsel_reg(msc, MSMON_IDR, &msmon_features);
+		if (err)
+			return err;
 
 		/*
 		 * If the firmware max-nrdy-us property is missing, the
@@ -932,7 +946,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
 		if (FIELD_GET(MPAMF_MSMON_IDR_MSMON_CSU, msmon_features)) {
 			u32 csumonidr;
 
-			mpam_read_partsel_reg(msc, CSUMON_IDR, &csumonidr);
+			err = mpam_read_partsel_reg(msc, CSUMON_IDR, &csumonidr);
+			if (err)
+				return err;
 
 			props->num_csu_mon = FIELD_GET(MPAMF_CSUMON_IDR_NUM_MON, csumonidr);
 			if (props->num_csu_mon) {
@@ -958,7 +974,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
 			bool has_long;
 			u32 mbwumon_idr;
 
-			mpam_read_partsel_reg(msc, MBWUMON_IDR, &mbwumon_idr);
+			err = mpam_read_partsel_reg(msc, MBWUMON_IDR, &mbwumon_idr);
+			if (err)
+				return err;
 
 			props->num_mbwu_mon = FIELD_GET(MPAMF_MBWUMON_IDR_NUM_MON, mbwumon_idr);
 			if (props->num_mbwu_mon) {
@@ -991,16 +1009,22 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
 		u16 partid_max;
 		u32 nrwidr;
 
-		mpam_read_partsel_reg(msc, PARTID_NRW_IDR, &nrwidr);
+		err = mpam_read_partsel_reg(msc, PARTID_NRW_IDR, &nrwidr);
+		if (err)
+			return err;
+
 		partid_max = FIELD_GET(MPAMF_PARTID_NRW_IDR_INTPARTID_MAX, nrwidr);
 
 		mpam_set_feature(mpam_feat_partid_nrw, props);
 		msc->partid_max = min(msc->partid_max, partid_max);
 	}
+
+	return 0;
 }
 
 static int mpam_msc_hw_probe(struct mpam_msc *msc)
 {
+	int ret;
 	u64 idr;
 	u16 partid_max;
 	u8 ris_idx, pmg_max;
@@ -1016,10 +1040,12 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc)
 
 	/* Grab an IDR value to find out how many RIS there are */
 	mutex_lock(&msc->part_sel_lock);
-	mpam_msc_read_idr(msc, &idr);
-	mpam_read_partsel_reg(msc, IIDR, &msc->iidr);
-
+	ret = mpam_msc_read_idr(msc, &idr);
+	if (!ret)
+		ret = mpam_read_partsel_reg(msc, IIDR, &msc->iidr);
 	mutex_unlock(&msc->part_sel_lock);
+	if (ret)
+		return ret;
 
 	mpam_enable_quirks(msc);
 
@@ -1032,8 +1058,10 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc)
 	for (ris_idx = 0; ris_idx <= msc->ris_max; ris_idx++) {
 		mutex_lock(&msc->part_sel_lock);
 		__mpam_part_sel(ris_idx, 0, msc);
-		mpam_msc_read_idr(msc, &idr);
+		ret = mpam_msc_read_idr(msc, &idr);
 		mutex_unlock(&msc->part_sel_lock);
+		if (ret)
+			return ret;
 
 		partid_max = FIELD_GET(MPAMF_IDR_PARTID_MAX, idr);
 		pmg_max = FIELD_GET(MPAMF_IDR_PMG_MAX, idr);
@@ -1050,8 +1078,10 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc)
 
 		mutex_lock(&msc->part_sel_lock);
 		__mpam_part_sel(ris_idx, 0, msc);
-		mpam_ris_hw_probe(ris);
+		ret = mpam_ris_hw_probe(ris);
 		mutex_unlock(&msc->part_sel_lock);
+		if (ret)
+			return ret;
 	}
 
 	/* Clear any stale errors */
-- 
2.43.0

Re: [PATCH v3 03/16] arm_mpam: propagate MSC read errors for hw_probe functions

[Jonathan Cameron]

On Fri, 10 Jul 2026 16:45:07 +0200
Andre Przywara <andre.przywara@arm.com> wrote:

> Allow the functions probing for MSC hardware and features to return an
> error, and propagate read errors from the lower level up.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Hi Andre,

A few suggestions to use some newer kernel 'toys' to simplify this
code.

Thanks,

Jonathan

> ---
>  drivers/resctrl/mpam_devices.c | 70 ++++++++++++++++++++++++----------
>  1 file changed, 50 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 8fd2c38c821c..b1bd047da203 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -785,7 +785,7 @@ static void mpam_enable_quirks(struct mpam_msc *msc)
>  static bool mpam_ris_hw_probe_csu_nrdy(struct mpam_msc_ris *ris)
>  {
>  	u32 now, mon_sel, ctl_val;
> -	bool can_set, can_clear;
> +	bool can_set, can_clear, ret = false;

I know the surrounding code does this, but it is helpful to general readability to
not mix declarations with assignments from those that don't. I'd just
use an extra line for ret.

>  	struct mpam_msc *msc = ris->vmsc->msc;
>  
>  	if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
> @@ -804,20 +804,26 @@ static bool mpam_ris_hw_probe_csu_nrdy(struct mpam_msc_ris *ris)
>  	mpam_write_monsel_reg(msc, CFG_CSU_CTL, ctl_val);
>  
>  	_mpam_write_monsel_reg(msc, MSMON_CSU, MSMON___NRDY);
> -	_mpam_read_monsel_reg(msc, MSMON_CSU, &now);
> +	if (_mpam_read_monsel_reg(msc, MSMON_CSU, &now))
> +		goto out_unlock;
>  	can_set = now & MSMON___NRDY;
>  
>  	_mpam_write_monsel_reg(msc, MSMON_CSU, 0);
>  	/* Configuration change to try and coax hardware into setting nrdy */
>  	mpam_write_monsel_reg(msc, CFG_CSU_FLT, 0x1);
> -	_mpam_read_monsel_reg(msc, MSMON_CSU, &now);
> +	if (_mpam_read_monsel_reg(msc, MSMON_CSU, &now))
> +		goto out_unlock;
>  	can_clear = !(now & MSMON___NRDY);
> +
> +	ret = !can_set || !can_clear;

This is less readable than it could be. I'd be tempted to split good and bad paths.

Or better yet, now we have ACQUIRE() and ACQUIRE_ERR() can we use the cleanup.h
magic to do auto cleanup of mpam_mon_sel_unlock()?  Then the error paths can
become direct returns. I haven't read the rest of the mpam code for a few months, but
it might be much more generally useful given we now have a bunch more reasons
to return early.

> +
> +out_unlock:
>  	mpam_mon_sel_unlock(msc);
>  
> -	return (!can_set || !can_clear);
> +	return ret;
>  }
>  
>  
>  static int mpam_msc_hw_probe(struct mpam_msc *msc)
>  {
> +	int ret;
>  	u64 idr;
>  	u16 partid_max;
>  	u8 ris_idx, pmg_max;
> @@ -1016,10 +1040,12 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc)
>  
>  	/* Grab an IDR value to find out how many RIS there are */
>  	mutex_lock(&msc->part_sel_lock);
> -	mpam_msc_read_idr(msc, &idr);
> -	mpam_read_partsel_reg(msc, IIDR, &msc->iidr);
> -
> +	ret = mpam_msc_read_idr(msc, &idr);
> +	if (!ret)
> +		ret = mpam_read_partsel_reg(msc, IIDR, &msc->iidr);
>  	mutex_unlock(&msc->part_sel_lock);

This is a classic pattern where cleanup.h magic can give a more readable
result with all error conditions easy to spot as each can be fully
handled on its own.

	scoped_guard(mutex, &msc->part_sel_lock) {
		ret = mapm_msc_read_idr(msc, &idr);
		if (ret)
			return ret;

		ret = mpam_read_partsel_reg(msc, IIDR, &msc->iidr);
		if (ret)
			return ret;
	}

	mpam_enable_quirks(msc);
> +	if (ret)
> +		return ret;
>  
>  	mpam_enable_quirks(msc);
>  

[PATCH v3 04/16] arm_mpam: propagate MSC read errors for mpam_msc_read_mbwu_l()

[Andre Przywara]

Allow the mpam_msc_read_mbwu_l() function to return an error, and
propagate read errors from the lower level up.
So far we were using a special value to indicate the "unstable read"
condition, replace that with the actual Linux error code, since it's now
easy to do.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/resctrl/mpam_devices.c | 54 +++++++++++++++++++++++++---------
 1 file changed, 40 insertions(+), 14 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index b1bd047da203..3b6f9e552a9f 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -1112,8 +1112,9 @@ static bool mpam_ris_has_mbwu_long_counter(struct mpam_msc_ris *ris)
 		mpam_has_feature(mpam_feat_msmon_mbwu_44counter, &ris->props));
 }
 
-static u64 mpam_msc_read_mbwu_l(struct mpam_msc *msc)
+static int mpam_msc_read_mbwu_l(struct mpam_msc *msc, u64 *res)
 {
+	int ret;
 	int retry = 3;
 	u32 mbwu_l_low;
 	u32 mbwu_l_high1, mbwu_l_high2;
@@ -1123,20 +1124,30 @@ static u64 mpam_msc_read_mbwu_l(struct mpam_msc *msc)
 	WARN_ON_ONCE((MSMON_MBWU_L + sizeof(u64)) > msc->mapped_hwpage_sz);
 	WARN_ON_ONCE(!cpumask_test_cpu(smp_processor_id(), &msc->accessibility));
 
-	__mpam_read_reg(msc, MSMON_MBWU_L + 4, &mbwu_l_high2);
+	ret = __mpam_read_reg(msc, MSMON_MBWU_L + 4, &mbwu_l_high2);
+	if (ret)
+		return ret;
+
 	do {
 		mbwu_l_high1 = mbwu_l_high2;
-		__mpam_read_reg(msc, MSMON_MBWU_L, &mbwu_l_low);
-		__mpam_read_reg(msc, MSMON_MBWU_L + 4, &mbwu_l_high2);
+		ret = __mpam_read_reg(msc, MSMON_MBWU_L, &mbwu_l_low);
+		if (ret)
+			return ret;
+		ret = __mpam_read_reg(msc, MSMON_MBWU_L + 4, &mbwu_l_high2);
+		if (ret)
+			return ret;
 
 		retry--;
 	} while (mbwu_l_high1 != mbwu_l_high2 && retry > 0);
 
-	if (mbwu_l_high1 == mbwu_l_high2)
-		return ((u64)mbwu_l_high1 << 32) | mbwu_l_low;
+	if (mbwu_l_high1 == mbwu_l_high2) {
+		*res = ((u64)mbwu_l_high1 << 32) | mbwu_l_low;
+	} else {
+		pr_warn("Failed to read a stable value\n");
+		ret = -EBUSY;
+	}
 
-	pr_warn("Failed to read a stable value\n");
-	return MSMON___L_NRDY;
+	return ret;
 }
 
 static void mpam_msc_zero_mbwu_l(struct mpam_msc *msc)
@@ -1283,6 +1294,7 @@ static u64 mpam_msmon_overflow_val(enum mpam_device_features type,
 static void __ris_msmon_read(void *arg)
 {
 	u64 now;
+	int ret;
 	u32 now32;
 	bool nrdy = false;
 	bool config_mismatch;
@@ -1358,8 +1370,9 @@ static void __ris_msmon_read(void *arg)
 	case mpam_feat_msmon_mbwu_44counter:
 	case mpam_feat_msmon_mbwu_63counter:
 		if (m->type != mpam_feat_msmon_mbwu_31counter) {
-			now = mpam_msc_read_mbwu_l(msc);
-			nrdy = now & MSMON___L_NRDY;
+			ret = mpam_msc_read_mbwu_l(msc, &now);
+			if (ret)
+				goto out_unlock;
 
 			if (m->type == mpam_feat_msmon_mbwu_63counter)
 				now = FIELD_GET(MSMON___LWD_VALUE, now);
@@ -1397,10 +1410,15 @@ static void __ris_msmon_read(void *arg)
 	if (nrdy)
 		m->err = -EBUSY;
 
-	if (m->err)
-		return;
+	if (!m->err)
+		*m->val += now;
+
+	return;
+
+out_unlock:
+	mpam_mon_sel_unlock(msc);
 
-	*m->val += now;
+	m->err = ret;
 }
 
 static int _msmon_read(struct mpam_component *comp, struct mon_read *arg)
@@ -1747,6 +1765,7 @@ static int mpam_save_mbwu_state(void *arg)
 {
 	int i;
 	u64 val;
+	int ret = 0;
 	struct mon_cfg *cfg;
 	u32 cur_flt, cur_ctl, mon_sel;
 	struct mpam_msc_ris *ris = arg;
@@ -1768,7 +1787,9 @@ static int mpam_save_mbwu_state(void *arg)
 		mpam_write_monsel_reg(msc, CFG_MBWU_CTL, 0);
 
 		if (mpam_ris_has_mbwu_long_counter(ris)) {
-			val = mpam_msc_read_mbwu_l(msc);
+			ret = mpam_msc_read_mbwu_l(msc, &val);
+			if (ret)
+				goto out_unlock;
 			mpam_msc_zero_mbwu_l(msc);
 		} else {
 			u32 val32;
@@ -1788,6 +1809,11 @@ static int mpam_save_mbwu_state(void *arg)
 	}
 
 	return 0;
+
+out_unlock:
+	mpam_mon_sel_unlock(msc);
+
+	return ret;
 }
 
 /*
-- 
2.43.0

Re: [PATCH v3 04/16] arm_mpam: propagate MSC read errors for mpam_msc_read_mbwu_l()

[Jonathan Cameron]

On Fri, 10 Jul 2026 16:45:08 +0200
Andre Przywara <andre.przywara@arm.com> wrote:

> Allow the mpam_msc_read_mbwu_l() function to return an error, and
> propagate read errors from the lower level up.
> So far we were using a special value to indicate the "unstable read"
> condition, replace that with the actual Linux error code, since it's now
> easy to do.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/resctrl/mpam_devices.c | 54 +++++++++++++++++++++++++---------
>  1 file changed, 40 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index b1bd047da203..3b6f9e552a9f 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -1112,8 +1112,9 @@ static bool mpam_ris_has_mbwu_long_counter(struct mpam_msc_ris *ris)
>  		mpam_has_feature(mpam_feat_msmon_mbwu_44counter, &ris->props));
>  }
>  
> -static u64 mpam_msc_read_mbwu_l(struct mpam_msc *msc)
> +static int mpam_msc_read_mbwu_l(struct mpam_msc *msc, u64 *res)
>  {
> +	int ret;
>  	int retry = 3;
>  	u32 mbwu_l_low;
>  	u32 mbwu_l_high1, mbwu_l_high2;
> @@ -1123,20 +1124,30 @@ static u64 mpam_msc_read_mbwu_l(struct mpam_msc *msc)
>  	WARN_ON_ONCE((MSMON_MBWU_L + sizeof(u64)) > msc->mapped_hwpage_sz);
>  	WARN_ON_ONCE(!cpumask_test_cpu(smp_processor_id(), &msc->accessibility));
>  
> -	__mpam_read_reg(msc, MSMON_MBWU_L + 4, &mbwu_l_high2);
> +	ret = __mpam_read_reg(msc, MSMON_MBWU_L + 4, &mbwu_l_high2);
> +	if (ret)
> +		return ret;
> +
>  	do {
>  		mbwu_l_high1 = mbwu_l_high2;
> -		__mpam_read_reg(msc, MSMON_MBWU_L, &mbwu_l_low);
> -		__mpam_read_reg(msc, MSMON_MBWU_L + 4, &mbwu_l_high2);
> +		ret = __mpam_read_reg(msc, MSMON_MBWU_L, &mbwu_l_low);
> +		if (ret)
> +			return ret;
> +		ret = __mpam_read_reg(msc, MSMON_MBWU_L + 4, &mbwu_l_high2);
> +		if (ret)
> +			return ret;
>  
>  		retry--;
>  	} while (mbwu_l_high1 != mbwu_l_high2 && retry > 0);
>  
> -	if (mbwu_l_high1 == mbwu_l_high2)
> -		return ((u64)mbwu_l_high1 << 32) | mbwu_l_low;
> +	if (mbwu_l_high1 == mbwu_l_high2) {
> +		*res = ((u64)mbwu_l_high1 << 32) | mbwu_l_low;
> +	} else {
> +		pr_warn("Failed to read a stable value\n");
> +		ret = -EBUSY;

I guess this may get more complex in later patches, but for now why not just 
		return -EBUSY.

> +	}
>  
> -	pr_warn("Failed to read a stable value\n");
> -	return MSMON___L_NRDY;
> +	return ret;
>  }
>  
>  static void mpam_msc_zero_mbwu_l(struct mpam_msc *msc)
> @@ -1283,6 +1294,7 @@ static u64 mpam_msmon_overflow_val(enum mpam_device_features type,
>  static void __ris_msmon_read(void *arg)
>  {
>  	u64 now;
> +	int ret;
>  	u32 now32;
>  	bool nrdy = false;
>  	bool config_mismatch;
> @@ -1358,8 +1370,9 @@ static void __ris_msmon_read(void *arg)

Relevant bit of context isn't show, but as before looks like
ACQUIRE() and ACQUIRE_ERR() will make this simpler by just
letting you do direct returns on all error paths though
you will need to set m->err = ret in each one.  To me that
is simpler, but feel free to disagree.

>  	case mpam_feat_msmon_mbwu_44counter:
>  	case mpam_feat_msmon_mbwu_63counter:
>  		if (m->type != mpam_feat_msmon_mbwu_31counter) {
> -			now = mpam_msc_read_mbwu_l(msc);
> -			nrdy = now & MSMON___L_NRDY;
> +			ret = mpam_msc_read_mbwu_l(msc, &now);
> +			if (ret)
> +				goto out_unlock;
>  
>  			if (m->type == mpam_feat_msmon_mbwu_63counter)
>  				now = FIELD_GET(MSMON___LWD_VALUE, now);
> @@ -1397,10 +1410,15 @@ static void __ris_msmon_read(void *arg)
>  	if (nrdy)
>  		m->err = -EBUSY;
>  
> -	if (m->err)
> -		return;
> +	if (!m->err)
Above ACQUIRE() suggestion relies on it not being a problem to hold
the lock a little longer and incorporate these sets in m. I haven't
checked closely but I think that's fine.

> +		*m->val += now;
> +
> +	return;
> +
> +out_unlock:
> +	mpam_mon_sel_unlock(msc);
>  
> -	*m->val += now;
> +	m->err = ret;
>  }
>  
>  static int _msmon_read(struct mpam_component *comp, struct mon_read *arg)
> @@ -1747,6 +1765,7 @@ static int mpam_save_mbwu_state(void *arg)
>  {
>  	int i;
>  	u64 val;
> +	int ret = 0;
>  	struct mon_cfg *cfg;
>  	u32 cur_flt, cur_ctl, mon_sel;
>  	struct mpam_msc_ris *ris = arg;
> @@ -1768,7 +1787,9 @@ static int mpam_save_mbwu_state(void *arg)
>  		mpam_write_monsel_reg(msc, CFG_MBWU_CTL, 0);
>  
>  		if (mpam_ris_has_mbwu_long_counter(ris)) {
> -			val = mpam_msc_read_mbwu_l(msc);
> +			ret = mpam_msc_read_mbwu_l(msc, &val);
> +			if (ret)
> +				goto out_unlock;
>  			mpam_msc_zero_mbwu_l(msc);
>  		} else {
>  			u32 val32;
> @@ -1788,6 +1809,11 @@ static int mpam_save_mbwu_state(void *arg)
>  	}
>  
>  	return 0;
> +
> +out_unlock:
> +	mpam_mon_sel_unlock(msc);
ACQUIRE() would avoid need for this handling so I think this is another
good place to look at using it.

> +
> +	return ret;
>  }
>  
>  /*

[PATCH v3 05/16] arm_mpam: propagate MSC read errors for msmon helpers

[Andre Przywara]

Allow the helper functions for msmon accesses to return an error, and
propagate read errors from the lower level up.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/resctrl/mpam_devices.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index 3b6f9e552a9f..041fca018e68 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -1201,25 +1201,31 @@ static void gen_msmon_ctl_flt_vals(struct mon_read *m, u32 *ctl_val,
 	}
 }
 
-static void read_msmon_ctl_flt_vals(struct mon_read *m, u32 *ctl_val,
-				    u32 *flt_val)
+static int read_msmon_ctl_flt_vals(struct mon_read *m, u32 *ctl_val,
+				   u32 *flt_val)
 {
 	struct mpam_msc *msc = m->ris->vmsc->msc;
+	int ret;
 
 	switch (m->type) {
 	case mpam_feat_msmon_csu:
-		mpam_read_monsel_reg(msc, CFG_CSU_CTL, ctl_val);
-		mpam_read_monsel_reg(msc, CFG_CSU_FLT, flt_val);
+		ret = mpam_read_monsel_reg(msc, CFG_CSU_CTL, ctl_val);
+		if (!ret)
+			ret = mpam_read_monsel_reg(msc, CFG_CSU_FLT, flt_val);
 		break;
 	case mpam_feat_msmon_mbwu_31counter:
 	case mpam_feat_msmon_mbwu_44counter:
 	case mpam_feat_msmon_mbwu_63counter:
-		mpam_read_monsel_reg(msc, CFG_MBWU_CTL, ctl_val);
-		mpam_read_monsel_reg(msc, CFG_MBWU_FLT, flt_val);
+		ret = mpam_read_monsel_reg(msc, CFG_MBWU_CTL, ctl_val);
+		if (!ret)
+			ret = mpam_read_monsel_reg(msc, CFG_MBWU_FLT, flt_val);
 		break;
 	default:
 		pr_warn("Unexpected monitor type %d\n", m->type);
+		return -EINVAL;
 	}
+
+	return ret;
 }
 
 /* Remove values set by the hardware to prevent apparent mismatches. */
-- 
2.43.0

Re: [PATCH v3 05/16] arm_mpam: propagate MSC read errors for msmon helpers

[Jonathan Cameron]

On Fri, 10 Jul 2026 16:45:09 +0200
Andre Przywara <andre.przywara@arm.com> wrote:

> Allow the helper functions for msmon accesses to return an error, and
> propagate read errors from the lower level up.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Hi Andre,

Trivial style suggestions.  I'm fussy - I read a lot of code ;)

Jonathan

> ---
>  drivers/resctrl/mpam_devices.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 3b6f9e552a9f..041fca018e68 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -1201,25 +1201,31 @@ static void gen_msmon_ctl_flt_vals(struct mon_read *m, u32 *ctl_val,
>  	}
>  }
>  
> -static void read_msmon_ctl_flt_vals(struct mon_read *m, u32 *ctl_val,
> -				    u32 *flt_val)
> +static int read_msmon_ctl_flt_vals(struct mon_read *m, u32 *ctl_val,
> +				   u32 *flt_val)
>  {
>  	struct mpam_msc *msc = m->ris->vmsc->msc;
> +	int ret;
>  
>  	switch (m->type) {
>  	case mpam_feat_msmon_csu:
> -		mpam_read_monsel_reg(msc, CFG_CSU_CTL, ctl_val);
> -		mpam_read_monsel_reg(msc, CFG_CSU_FLT, flt_val);
> +		ret = mpam_read_monsel_reg(msc, CFG_CSU_CTL, ctl_val);
> +		if (!ret)
> +			ret = mpam_read_monsel_reg(msc, CFG_CSU_FLT, flt_val);
Burn a few lines to make this flow absolutely standard with errors as
the out of line sections.  Obviously it is a minor thing but the more
standard code is, the easier it is to review,

		ret = mpam_read_monsel_reg(msc, CFG_CSU_CTL, ctl_val);
		if (ret)
			return ret;

		return mpam_read_monsel_reg(msc, CFG_CS_FLT, flt_val);


>  		break;
>  	case mpam_feat_msmon_mbwu_31counter:
>  	case mpam_feat_msmon_mbwu_44counter:
>  	case mpam_feat_msmon_mbwu_63counter:
> -		mpam_read_monsel_reg(msc, CFG_MBWU_CTL, ctl_val);
> -		mpam_read_monsel_reg(msc, CFG_MBWU_FLT, flt_val);
> +		ret = mpam_read_monsel_reg(msc, CFG_MBWU_CTL, ctl_val);
> +		if (!ret)
> +			ret = mpam_read_monsel_reg(msc, CFG_MBWU_FLT, flt_val);

Same for this one.

>  		break;
>  	default:
>  		pr_warn("Unexpected monitor type %d\n", m->type);
> +		return -EINVAL;
>  	}
> +
> +	return ret;
With returns above, this can go.

>  }
>  
>  /* Remove values set by the hardware to prevent apparent mismatches. */

[PATCH v3 06/16] arm_mpam: propagate MSC read errors for __ris_msmon_read()

[Andre Przywara]

Allow the function for RIS accesses to return an error, and propagate
read errors from the lower level up.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/resctrl/mpam_devices.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index 041fca018e68..84a8715464be 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -1297,6 +1297,10 @@ static u64 mpam_msmon_overflow_val(enum mpam_device_features type,
 	return overflow_val;
 }
 
+/*
+ * This function might be called via smp_call_function_any(), so propagate
+ * errors inside the arg struct.
+ */
 static void __ris_msmon_read(void *arg)
 {
 	u64 now;
@@ -1339,7 +1343,9 @@ static void __ris_msmon_read(void *arg)
 	 * Read the existing configuration to avoid re-writing the same values.
 	 * This saves waiting for 'nrdy' on subsequent reads.
 	 */
-	read_msmon_ctl_flt_vals(m, &cur_ctl, &cur_flt);
+	ret = read_msmon_ctl_flt_vals(m, &cur_ctl, &cur_flt);
+	if (ret)
+		goto out_unlock;
 
 	if (mpam_feat_msmon_mbwu_31counter == m->type)
 		overflow = cur_ctl & MSMON_CFG_x_CTL_OFLOW_STATUS;
@@ -1364,7 +1370,9 @@ static void __ris_msmon_read(void *arg)
 
 	switch (m->type) {
 	case mpam_feat_msmon_csu:
-		mpam_read_monsel_reg(msc, CSU, &now32);
+		ret = mpam_read_monsel_reg(msc, CSU, &now32);
+		if (ret)
+			goto out_unlock;
 		nrdy = now32 & MSMON___NRDY;
 		now = FIELD_GET(MSMON___VALUE, now32);
 
@@ -1385,7 +1393,9 @@ static void __ris_msmon_read(void *arg)
 			else
 				now = FIELD_GET(MSMON___L_VALUE, now);
 		} else {
-			mpam_read_monsel_reg(msc, MBWU, &now32);
+			ret = mpam_read_monsel_reg(msc, MBWU, &now32);
+			if (ret)
+				goto out_unlock;
 			nrdy = now32 & MSMON___NRDY;
 			now = FIELD_GET(MSMON___VALUE, now32);
 		}
@@ -1748,6 +1758,7 @@ static int mpam_restore_mbwu_state(void *_ris)
 {
 	int i;
 	u64 val;
+	int ret = 0;
 	struct mon_read mwbu_arg;
 	struct mpam_msc_ris *ris = _ris;
 	struct mpam_class *class = ris->vmsc->comp->class;
@@ -1760,10 +1771,14 @@ static int mpam_restore_mbwu_state(void *_ris)
 			mwbu_arg.val = &val;
 
 			__ris_msmon_read(&mwbu_arg);
+			if (mwbu_arg.err) {
+				ret = mwbu_arg.err;
+				break;
+			}
 		}
 	}
 
-	return 0;
+	return ret;
 }
 
 /* Call with MSC cfg_lock held */
-- 
2.43.0

Re: [PATCH v3 06/16] arm_mpam: propagate MSC read errors for __ris_msmon_read()

[Jonathan Cameron]

On Fri, 10 Jul 2026 16:45:10 +0200
Andre Przywara <andre.przywara@arm.com> wrote:

> Allow the function for RIS accesses to return an error, and propagate
> read errors from the lower level up.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/resctrl/mpam_devices.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 041fca018e68..84a8715464be 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -1297,6 +1297,10 @@ static u64 mpam_msmon_overflow_val(enum mpam_device_features type,
>  	return overflow_val;
>  }
>  
> +/*
> + * This function might be called via smp_call_function_any(), so propagate
> + * errors inside the arg struct.
> + */
>  static void __ris_msmon_read(void *arg)
>  {
>  	u64 now;

Similar comments on ACQUIRE() making things easier here.
Note I'd introduce it as a trivial precursor patch before the rest of
this rework.


> @@ -1339,7 +1343,9 @@ static void __ris_msmon_read(void *arg)
>  	 * Read the existing configuration to avoid re-writing the same values.
>  	 * This saves waiting for 'nrdy' on subsequent reads.
>  	 */
> -	read_msmon_ctl_flt_vals(m, &cur_ctl, &cur_flt);
> +	ret = read_msmon_ctl_flt_vals(m, &cur_ctl, &cur_flt);
> +	if (ret)
> +		goto out_unlock;
>  
>  	if (mpam_feat_msmon_mbwu_31counter == m->type)
>  		overflow = cur_ctl & MSMON_CFG_x_CTL_OFLOW_STATUS;
> @@ -1364,7 +1370,9 @@ static void __ris_msmon_read(void *arg)
>  
>  	switch (m->type) {
>  	case mpam_feat_msmon_csu:
> -		mpam_read_monsel_reg(msc, CSU, &now32);
> +		ret = mpam_read_monsel_reg(msc, CSU, &now32);
> +		if (ret)
> +			goto out_unlock;
>  		nrdy = now32 & MSMON___NRDY;
>  		now = FIELD_GET(MSMON___VALUE, now32);
>  
> @@ -1385,7 +1393,9 @@ static void __ris_msmon_read(void *arg)
>  			else
>  				now = FIELD_GET(MSMON___L_VALUE, now);
>  		} else {
> -			mpam_read_monsel_reg(msc, MBWU, &now32);
> +			ret = mpam_read_monsel_reg(msc, MBWU, &now32);
> +			if (ret)
> +				goto out_unlock;
>  			nrdy = now32 & MSMON___NRDY;
>  			now = FIELD_GET(MSMON___VALUE, now32);
>  		}
> @@ -1748,6 +1758,7 @@ static int mpam_restore_mbwu_state(void *_ris)
>  {
>  	int i;
>  	u64 val;
> +	int ret = 0;
>  	struct mon_read mwbu_arg;
>  	struct mpam_msc_ris *ris = _ris;
>  	struct mpam_class *class = ris->vmsc->comp->class;
> @@ -1760,10 +1771,14 @@ static int mpam_restore_mbwu_state(void *_ris)
>  			mwbu_arg.val = &val;
>  
>  			__ris_msmon_read(&mwbu_arg);
> +			if (mwbu_arg.err) {
> +				ret = mwbu_arg.err;
return here.  It's consistent with some of the existing code and
general makes things more readable.

Obviously ignore any comments like this if they are about code
which will change later making the comment wrong!

> +				break;
> +			}
>  		}
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  /* Call with MSC cfg_lock held */

[PATCH v3 07/16] arm_mpam: __ris_msmon_read(): get rid of nrdy special handling

[Andre Przywara]

Although so far MSC accesses couldn't fail, there is one special
condition that would create an error: when the MBWU counter wouldn't be
able to read a stable value, we were setting bit 63 to mark this value
as unstable, and return this as an error later.
Now since the functions can return a proper error value, we can get rid of
this kludge and use the return value directly.

Remove the "nrdy" error flag variable, and assign -EBUSY to "ret" to handle
this case.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/resctrl/mpam_devices.c | 38 +++++++++++++++-------------------
 1 file changed, 17 insertions(+), 21 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index 84a8715464be..530ac0fe97b5 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -1306,7 +1306,6 @@ static void __ris_msmon_read(void *arg)
 	u64 now;
 	int ret;
 	u32 now32;
-	bool nrdy = false;
 	bool config_mismatch;
 	bool overflow = false;
 	struct mon_read *m = arg;
@@ -1371,14 +1370,18 @@ static void __ris_msmon_read(void *arg)
 	switch (m->type) {
 	case mpam_feat_msmon_csu:
 		ret = mpam_read_monsel_reg(msc, CSU, &now32);
+		if (!ret) {
+			if ((now32 & MSMON___NRDY))
+				ret = -EBUSY;
+
+			if (mpam_has_quirk(IGNORE_CSU_NRDY, msc) &&
+			    m->waited_timeout)
+				ret = 0;
+		}
 		if (ret)
 			goto out_unlock;
-		nrdy = now32 & MSMON___NRDY;
-		now = FIELD_GET(MSMON___VALUE, now32);
-
-		if (mpam_has_quirk(IGNORE_CSU_NRDY, msc) && m->waited_timeout)
-			nrdy = false;
 
+		now = FIELD_GET(MSMON___VALUE, now32);
 		break;
 	case mpam_feat_msmon_mbwu_31counter:
 	case mpam_feat_msmon_mbwu_44counter:
@@ -1394,9 +1397,11 @@ static void __ris_msmon_read(void *arg)
 				now = FIELD_GET(MSMON___L_VALUE, now);
 		} else {
 			ret = mpam_read_monsel_reg(msc, MBWU, &now32);
+			if (!ret && (now32 & MSMON___NRDY))
+				ret = -EBUSY;
 			if (ret)
 				goto out_unlock;
-			nrdy = now32 & MSMON___NRDY;
+
 			now = FIELD_GET(MSMON___VALUE, now32);
 		}
 
@@ -1404,9 +1409,6 @@ static void __ris_msmon_read(void *arg)
 		    m->type != mpam_feat_msmon_mbwu_63counter)
 			now *= 64;
 
-		if (nrdy)
-			break;
-
 		mbwu_state = &ris->mbwu_state[ctx->mon];
 
 		if (overflow)
@@ -1419,22 +1421,16 @@ static void __ris_msmon_read(void *arg)
 		now += mbwu_state->correction;
 		break;
 	default:
-		m->err = -EINVAL;
+		ret = -EINVAL;
 	}
-	mpam_mon_sel_unlock(msc);
-
-	if (nrdy)
-		m->err = -EBUSY;
-
-	if (!m->err)
-		*m->val += now;
-
-	return;
 
 out_unlock:
 	mpam_mon_sel_unlock(msc);
 
-	m->err = ret;
+	if (ret)
+		m->err = ret;
+	else
+		*m->val += now;
 }
 
 static int _msmon_read(struct mpam_component *comp, struct mon_read *arg)
-- 
2.43.0

Re: [PATCH v3 07/16] arm_mpam: __ris_msmon_read(): get rid of nrdy special handling

[Jonathan Cameron]

On Fri, 10 Jul 2026 16:45:11 +0200
Andre Przywara <andre.przywara@arm.com> wrote:

> Although so far MSC accesses couldn't fail, there is one special
> condition that would create an error: when the MBWU counter wouldn't be
> able to read a stable value, we were setting bit 63 to mark this value
> as unstable, and return this as an error later.
> Now since the functions can return a proper error value, we can get rid of
> this kludge and use the return value directly.
> 
> Remove the "nrdy" error flag variable, and assign -EBUSY to "ret" to handle
> this case.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Hi Andre

I'm still fussing about code flow and style :(

Obviously none of this is that important, but it does help make
the code more maintainable in the long run.

Jonathan

> ---
>  drivers/resctrl/mpam_devices.c | 38 +++++++++++++++-------------------
>  1 file changed, 17 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 84a8715464be..530ac0fe97b5 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -1306,7 +1306,6 @@ static void __ris_msmon_read(void *arg)
>  	u64 now;
>  	int ret;
>  	u32 now32;
> -	bool nrdy = false;
>  	bool config_mismatch;
>  	bool overflow = false;
>  	struct mon_read *m = arg;
> @@ -1371,14 +1370,18 @@ static void __ris_msmon_read(void *arg)
>  	switch (m->type) {
>  	case mpam_feat_msmon_csu:
>  		ret = mpam_read_monsel_reg(msc, CSU, &now32);
> +		if (!ret) {
> +			if ((now32 & MSMON___NRDY))
> +				ret = -EBUSY;
> +
> +			if (mpam_has_quirk(IGNORE_CSU_NRDY, msc) &&
> +			    m->waited_timeout)
> +				ret = 0;
Whilst it is from existing code, this pattern of set and error then clear it
is less than ideal.  Maybe

			if ((now32 & MSMON___NRDY) &&
			    !(mpam_has_quirk(IGNORE_CS_NRDY, MSC && m->waited_timeout))
				ret = -EBUSY;

is clearer as that odd intermediate state of ret never happens.

> +		}
>  		if (ret)
>  			goto out_unlock;
> -		nrdy = now32 & MSMON___NRDY;
> -		now = FIELD_GET(MSMON___VALUE, now32);
> -
> -		if (mpam_has_quirk(IGNORE_CSU_NRDY, msc) && m->waited_timeout)
> -			nrdy = false;
>  
> +		now = FIELD_GET(MSMON___VALUE, now32);
>  		break;
>  	case mpam_feat_msmon_mbwu_31counter:
>  	case mpam_feat_msmon_mbwu_44counter:
> @@ -1394,9 +1397,11 @@ static void __ris_msmon_read(void *arg)
>  				now = FIELD_GET(MSMON___L_VALUE, now);
>  		} else {
>  			ret = mpam_read_monsel_reg(msc, MBWU, &now32);
> +			if (!ret && (now32 & MSMON___NRDY))
> +				ret = -EBUSY;
>  			if (ret)
>  				goto out_unlock;
> -			nrdy = now32 & MSMON___NRDY;
> +
>  			now = FIELD_GET(MSMON___VALUE, now32);
>  		}
>  
> @@ -1404,9 +1409,6 @@ static void __ris_msmon_read(void *arg)
>  		    m->type != mpam_feat_msmon_mbwu_63counter)
>  			now *= 64;
>  
> -		if (nrdy)
> -			break;
> -
>  		mbwu_state = &ris->mbwu_state[ctx->mon];
>  
>  		if (overflow)
> @@ -1419,22 +1421,16 @@ static void __ris_msmon_read(void *arg)
>  		now += mbwu_state->correction;
>  		break;
>  	default:
> -		m->err = -EINVAL;
> +		ret = -EINVAL;
>  	}
> -	mpam_mon_sel_unlock(msc);
> -
> -	if (nrdy)
> -		m->err = -EBUSY;
> -
> -	if (!m->err)
> -		*m->val += now;
> -
> -	return;
>  
>  out_unlock:
>  	mpam_mon_sel_unlock(msc);
>  
> -	m->err = ret;
> +	if (ret)
> +		m->err = ret;
> +	else
> +		*m->val += now;

If you do the earlier suggestion of ACQUIRE() this all get simpler, but if you do keep
this, then burn a line or two of code to make it obvious what is error and what isn't.

	if (ret) {
		m->err = ret;
		return;
	}

	*m->val += now;
>  }

[PATCH v3 08/16] arm_mpam: propagate MSC read errors for state saving functions

[Andre Przywara]

Allow the mpam_save_mbwu_state() function to return an error, and
propagate read errors from the lower level up.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/resctrl/mpam_devices.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index 530ac0fe97b5..b2ecaba29fcc 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -1782,7 +1782,7 @@ static int mpam_save_mbwu_state(void *arg)
 {
 	int i;
 	u64 val;
-	int ret = 0;
+	int ret;
 	struct mon_cfg *cfg;
 	u32 cur_flt, cur_ctl, mon_sel;
 	struct mpam_msc_ris *ris = arg;
@@ -1799,8 +1799,12 @@ static int mpam_save_mbwu_state(void *arg)
 		mon_sel = FIELD_PREP(MSMON_CFG_MON_SEL_MON_SEL, i) |
 			  FIELD_PREP(MSMON_CFG_MON_SEL_RIS, ris->ris_idx);
 		mpam_write_monsel_reg(msc, CFG_MON_SEL, mon_sel);
-		mpam_read_monsel_reg(msc, CFG_MBWU_FLT, &cur_flt);
-		mpam_read_monsel_reg(msc, CFG_MBWU_CTL, &cur_ctl);
+		ret = mpam_read_monsel_reg(msc, CFG_MBWU_FLT, &cur_flt);
+		if (ret)
+			goto out_unlock;
+		ret = mpam_read_monsel_reg(msc, CFG_MBWU_CTL, &cur_ctl);
+		if (ret)
+			goto out_unlock;
 		mpam_write_monsel_reg(msc, CFG_MBWU_CTL, 0);
 
 		if (mpam_ris_has_mbwu_long_counter(ris)) {
@@ -1811,20 +1815,24 @@ static int mpam_save_mbwu_state(void *arg)
 		} else {
 			u32 val32;
 
-			mpam_read_monsel_reg(msc, MBWU, &val32);
+			ret = mpam_read_monsel_reg(msc, MBWU, &val32);
+			if (ret)
+				goto out_unlock;
+
 			val = val32;
 			mpam_write_monsel_reg(msc, MBWU, 0);
 		}
 
-		cfg->mon = i;
-		cfg->pmg = FIELD_GET(MSMON_CFG_x_FLT_PMG, cur_flt);
-		cfg->match_pmg = FIELD_GET(MSMON_CFG_x_CTL_MATCH_PMG, cur_ctl);
-		cfg->partid = FIELD_GET(MSMON_CFG_x_FLT_PARTID, cur_flt);
-		mbwu_state->correction += val;
-		mbwu_state->enabled = FIELD_GET(MSMON_CFG_x_CTL_EN, cur_ctl);
+		if (val != MSMON___L_NRDY) {
+			cfg->mon = i;
+			cfg->pmg = FIELD_GET(MSMON_CFG_x_FLT_PMG, cur_flt);
+			cfg->match_pmg = FIELD_GET(MSMON_CFG_x_CTL_MATCH_PMG, cur_ctl);
+			cfg->partid = FIELD_GET(MSMON_CFG_x_FLT_PARTID, cur_flt);
+			mbwu_state->correction += val;
+			mbwu_state->enabled = FIELD_GET(MSMON_CFG_x_CTL_EN, cur_ctl);
+		}
 		mpam_mon_sel_unlock(msc);
 	}
-
 	return 0;
 
 out_unlock:
-- 
2.43.0

Re: [PATCH v3 08/16] arm_mpam: propagate MSC read errors for state saving functions

[Jonathan Cameron]

On Fri, 10 Jul 2026 16:45:12 +0200
Andre Przywara <andre.przywara@arm.com> wrote:

> Allow the mpam_save_mbwu_state() function to return an error, and
> propagate read errors from the lower level up.

I'm not following why propagating errors is related to the
if (val != MSMON___L_NRDY)
as nothing in this patch is changing val.

> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/resctrl/mpam_devices.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 530ac0fe97b5..b2ecaba29fcc 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -1782,7 +1782,7 @@ static int mpam_save_mbwu_state(void *arg)
>  {
>  	int i;
>  	u64 val;
> -	int ret = 0;
> +	int ret;
>  	struct mon_cfg *cfg;
>  	u32 cur_flt, cur_ctl, mon_sel;
>  	struct mpam_msc_ris *ris = arg;
> @@ -1799,8 +1799,12 @@ static int mpam_save_mbwu_state(void *arg)
>  		mon_sel = FIELD_PREP(MSMON_CFG_MON_SEL_MON_SEL, i) |
>  			  FIELD_PREP(MSMON_CFG_MON_SEL_RIS, ris->ris_idx);
>  		mpam_write_monsel_reg(msc, CFG_MON_SEL, mon_sel);
> -		mpam_read_monsel_reg(msc, CFG_MBWU_FLT, &cur_flt);
> -		mpam_read_monsel_reg(msc, CFG_MBWU_CTL, &cur_ctl);
> +		ret = mpam_read_monsel_reg(msc, CFG_MBWU_FLT, &cur_flt);
> +		if (ret)
> +			goto out_unlock;
> +		ret = mpam_read_monsel_reg(msc, CFG_MBWU_CTL, &cur_ctl);
> +		if (ret)
> +			goto out_unlock;
>  		mpam_write_monsel_reg(msc, CFG_MBWU_CTL, 0);
>  
>  		if (mpam_ris_has_mbwu_long_counter(ris)) {
> @@ -1811,20 +1815,24 @@ static int mpam_save_mbwu_state(void *arg)
>  		} else {
>  			u32 val32;
>  
> -			mpam_read_monsel_reg(msc, MBWU, &val32);
> +			ret = mpam_read_monsel_reg(msc, MBWU, &val32);
> +			if (ret)
> +				goto out_unlock;
> +
>  			val = val32;
>  			mpam_write_monsel_reg(msc, MBWU, 0);
>  		}
>  
> -		cfg->mon = i;
> -		cfg->pmg = FIELD_GET(MSMON_CFG_x_FLT_PMG, cur_flt);
> -		cfg->match_pmg = FIELD_GET(MSMON_CFG_x_CTL_MATCH_PMG, cur_ctl);
> -		cfg->partid = FIELD_GET(MSMON_CFG_x_FLT_PARTID, cur_flt);
> -		mbwu_state->correction += val;
> -		mbwu_state->enabled = FIELD_GET(MSMON_CFG_x_CTL_EN, cur_ctl);
> +		if (val != MSMON___L_NRDY) {

with ACQUIRE() you can just do
		if (val == MS_MON__L_NRDY)
			return;

and leave the rest where it was.
However I don't actually see what this has to do wiht the main purpose of the
patch to propagate errors. Perhaps a bit more detail in the commit message.

Thanks,

Jonathan


> +			cfg->mon = i;
> +			cfg->pmg = FIELD_GET(MSMON_CFG_x_FLT_PMG, cur_flt);
> +			cfg->match_pmg = FIELD_GET(MSMON_CFG_x_CTL_MATCH_PMG, cur_ctl);
> +			cfg->partid = FIELD_GET(MSMON_CFG_x_FLT_PARTID, cur_flt);
> +			mbwu_state->correction += val;
> +			mbwu_state->enabled = FIELD_GET(MSMON_CFG_x_CTL_EN, cur_ctl);
> +		}
>  		mpam_mon_sel_unlock(msc);
>  	}
> -
>  	return 0;
>  
>  out_unlock:

[PATCH v3 09/16] arm_mpam: let low level MSC write accessors return an error

[Andre Przywara]

The upcoming MPAM-Fb support does not use MMIO primitives to access an
MSC, but employs a shared-memory/doorbell based firmware protocol.
Its complexity means that is must be able to handle errors, whereas we
always assume an MSC access succeeds today.

Change the __mpam_write_reg() low level accessor function to return an
error code. At the moment this is always 0, but this will change with
alternative MSC access methods.

Also change some low level wrappers to propagate the error.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/resctrl/mpam_devices.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index b2ecaba29fcc..f1e40ce24f5a 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -195,18 +195,20 @@ static inline int _mpam_read_partsel_reg(struct mpam_msc *msc, u16 reg,
 
 #define mpam_read_partsel_reg(msc, reg, res) _mpam_read_partsel_reg(msc, MPAMF_##reg, res)
 
-static void __mpam_write_reg(struct mpam_msc *msc, u16 reg, u32 val)
+static int __mpam_write_reg(struct mpam_msc *msc, u16 reg, u32 val)
 {
 	WARN_ON_ONCE(reg + sizeof(u32) > msc->mapped_hwpage_sz);
 	WARN_ON_ONCE(!cpumask_test_cpu(smp_processor_id(), &msc->accessibility));
 
 	writel_relaxed(val, msc->mapped_hwpage + reg);
+
+	return 0;
 }
 
-static inline void _mpam_write_partsel_reg(struct mpam_msc *msc, u16 reg, u32 val)
+static inline int _mpam_write_partsel_reg(struct mpam_msc *msc, u16 reg, u32 val)
 {
 	lockdep_assert_held_once(&msc->part_sel_lock);
-	__mpam_write_reg(msc, reg, val);
+	return __mpam_write_reg(msc, reg, val);
 }
 
 #define mpam_write_partsel_reg(msc, reg, val)  _mpam_write_partsel_reg(msc, MPAMCFG_##reg, val)
@@ -220,10 +222,10 @@ static inline int _mpam_read_monsel_reg(struct mpam_msc *msc, u16 reg,
 
 #define mpam_read_monsel_reg(msc, reg, res) _mpam_read_monsel_reg(msc, MSMON_##reg, res)
 
-static inline void _mpam_write_monsel_reg(struct mpam_msc *msc, u16 reg, u32 val)
+static inline int _mpam_write_monsel_reg(struct mpam_msc *msc, u16 reg, u32 val)
 {
 	mpam_mon_sel_lock_held(msc);
-	__mpam_write_reg(msc, reg, val);
+	return __mpam_write_reg(msc, reg, val);
 }
 
 #define mpam_write_monsel_reg(msc, reg, val)   _mpam_write_monsel_reg(msc, MSMON_##reg, val)
-- 
2.43.0

Re: [PATCH v3 09/16] arm_mpam: let low level MSC write accessors return an error

[Jonathan Cameron]

On Fri, 10 Jul 2026 16:45:13 +0200
Andre Przywara <andre.przywara@arm.com> wrote:

> The upcoming MPAM-Fb support does not use MMIO primitives to access an
> MSC, but employs a shared-memory/doorbell based firmware protocol.
> Its complexity means that is must be able to handle errors, whereas we
> always assume an MSC access succeeds today.
> 
> Change the __mpam_write_reg() low level accessor function to return an
> error code. At the moment this is always 0, but this will change with
> alternative MSC access methods.
> 
> Also change some low level wrappers to propagate the error.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
I mentioned earlier that to me a split of read vs write into different
series of patches is making things more complex than they might be.
They are same 'sort' of change so I'd do them together.

Then a reviewer can quickly see if all new sources of error are
covered.

Thanks (and sorry I didn't look at v2!)

Jonathan

> ---
>  drivers/resctrl/mpam_devices.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index b2ecaba29fcc..f1e40ce24f5a 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -195,18 +195,20 @@ static inline int _mpam_read_partsel_reg(struct mpam_msc *msc, u16 reg,
>  
>  #define mpam_read_partsel_reg(msc, reg, res) _mpam_read_partsel_reg(msc, MPAMF_##reg, res)
>  
> -static void __mpam_write_reg(struct mpam_msc *msc, u16 reg, u32 val)
> +static int __mpam_write_reg(struct mpam_msc *msc, u16 reg, u32 val)
>  {
>  	WARN_ON_ONCE(reg + sizeof(u32) > msc->mapped_hwpage_sz);
>  	WARN_ON_ONCE(!cpumask_test_cpu(smp_processor_id(), &msc->accessibility));
>  
>  	writel_relaxed(val, msc->mapped_hwpage + reg);
> +
> +	return 0;
>  }
>  
> -static inline void _mpam_write_partsel_reg(struct mpam_msc *msc, u16 reg, u32 val)
> +static inline int _mpam_write_partsel_reg(struct mpam_msc *msc, u16 reg, u32 val)
>  {
>  	lockdep_assert_held_once(&msc->part_sel_lock);
> -	__mpam_write_reg(msc, reg, val);
> +	return __mpam_write_reg(msc, reg, val);
>  }
>  
>  #define mpam_write_partsel_reg(msc, reg, val)  _mpam_write_partsel_reg(msc, MPAMCFG_##reg, val)
> @@ -220,10 +222,10 @@ static inline int _mpam_read_monsel_reg(struct mpam_msc *msc, u16 reg,
>  
>  #define mpam_read_monsel_reg(msc, reg, res) _mpam_read_monsel_reg(msc, MSMON_##reg, res)
>  
> -static inline void _mpam_write_monsel_reg(struct mpam_msc *msc, u16 reg, u32 val)
> +static inline int _mpam_write_monsel_reg(struct mpam_msc *msc, u16 reg, u32 val)
>  {
>  	mpam_mon_sel_lock_held(msc);
> -	__mpam_write_reg(msc, reg, val);
> +	return __mpam_write_reg(msc, reg, val);
>  }
>  
>  #define mpam_write_monsel_reg(msc, reg, val)   _mpam_write_monsel_reg(msc, MSMON_##reg, val)

[PATCH v3 10/16] arm_mpam: propagate MSC write errors for ESR and part_sel wrappers

[Andre Przywara]

Allow the wrapper functions for part_sel and ESR accesses to return an
error, and propagate write errors from the lower level up.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/resctrl/mpam_devices.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index f1e40ce24f5a..5a9760372666 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -288,12 +288,13 @@ static int mpam_msc_clear_esr(struct mpam_msc *msc)
 	 * lower half prevent hardware from updating either half of the
 	 * register.
 	 */
-	if (msc->has_extd_esr)
-		__mpam_write_reg(msc, MPAMF_ESR + 4, 0);
-
-	__mpam_write_reg(msc, MPAMF_ESR, 0);
+	if (msc->has_extd_esr) {
+		ret = __mpam_write_reg(msc, MPAMF_ESR + 4, 0);
+		if (ret)
+			return ret;
+	}
 
-	return 0;
+	return __mpam_write_reg(msc, MPAMF_ESR, 0);
 }
 
 static int mpam_msc_read_esr(struct mpam_msc *msc, u64 *res)
@@ -316,28 +317,28 @@ static int mpam_msc_read_esr(struct mpam_msc *msc, u64 *res)
 	return 0;
 }
 
-static void __mpam_part_sel_raw(u32 partsel, struct mpam_msc *msc)
+static int __mpam_part_sel_raw(u32 partsel, struct mpam_msc *msc)
 {
 	lockdep_assert_held(&msc->part_sel_lock);
 
-	mpam_write_partsel_reg(msc, PART_SEL, partsel);
+	return mpam_write_partsel_reg(msc, PART_SEL, partsel);
 }
 
-static void __mpam_part_sel(u8 ris_idx, u16 partid, struct mpam_msc *msc)
+static int __mpam_part_sel(u8 ris_idx, u16 partid, struct mpam_msc *msc)
 {
 	u32 partsel = FIELD_PREP(MPAMCFG_PART_SEL_RIS, ris_idx) |
 		      FIELD_PREP(MPAMCFG_PART_SEL_PARTID_SEL, partid);
 
-	__mpam_part_sel_raw(partsel, msc);
+	return __mpam_part_sel_raw(partsel, msc);
 }
 
-static void __mpam_intpart_sel(u8 ris_idx, u16 intpartid, struct mpam_msc *msc)
+static int __mpam_intpart_sel(u8 ris_idx, u16 intpartid, struct mpam_msc *msc)
 {
 	u32 partsel = FIELD_PREP(MPAMCFG_PART_SEL_RIS, ris_idx) |
 		      FIELD_PREP(MPAMCFG_PART_SEL_PARTID_SEL, intpartid) |
 		      MPAMCFG_PART_SEL_INTERNAL;
 
-	__mpam_part_sel_raw(partsel, msc);
+	return __mpam_part_sel_raw(partsel, msc);
 }
 
 int mpam_register_requestor(u16 partid_max, u8 pmg_max)
-- 
2.43.0

[PATCH v3 11/16] arm_mpam: propagate MSC write errors for hardware probe functions

[Andre Przywara]

Allow the MSC write accesses in the hardware probe functions to return an
error, and propagate write errors from the lower level up.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/resctrl/mpam_devices.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index 5a9760372666..222515a01b35 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -796,24 +796,30 @@ static bool mpam_ris_hw_probe_csu_nrdy(struct mpam_msc_ris *ris)
 
 	mon_sel = FIELD_PREP(MSMON_CFG_MON_SEL_MON_SEL, 0) |
 		  FIELD_PREP(MSMON_CFG_MON_SEL_RIS, ris->ris_idx);
-	mpam_write_monsel_reg(msc, CFG_MON_SEL, mon_sel);
+	if (mpam_write_monsel_reg(msc, CFG_MON_SEL, mon_sel))
+		goto out_unlock;
 
 	/* Hardware might ignore nrdy if it's not enabled */
 	ctl_val = MSMON_CFG_CSU_CTL_TYPE_CSU;
 	ctl_val |= MSMON_CFG_x_CTL_MATCH_PARTID;
 	ctl_val |= MSMON_CFG_x_CTL_MATCH_PMG;
 	ctl_val |= MSMON_CFG_x_CTL_EN;
-	mpam_write_monsel_reg(msc, CFG_CSU_FLT, 0);
-	mpam_write_monsel_reg(msc, CFG_CSU_CTL, ctl_val);
+	if (mpam_write_monsel_reg(msc, CFG_CSU_FLT, 0))
+		goto out_unlock;
+	if (mpam_write_monsel_reg(msc, CFG_CSU_CTL, ctl_val))
+		goto out_unlock;
 
-	_mpam_write_monsel_reg(msc, MSMON_CSU, MSMON___NRDY);
+	if (_mpam_write_monsel_reg(msc, MSMON_CSU, MSMON___NRDY))
+		goto out_unlock;
 	if (_mpam_read_monsel_reg(msc, MSMON_CSU, &now))
 		goto out_unlock;
 	can_set = now & MSMON___NRDY;
 
-	_mpam_write_monsel_reg(msc, MSMON_CSU, 0);
+	if (_mpam_write_monsel_reg(msc, MSMON_CSU, 0))
+		goto out_unlock;
 	/* Configuration change to try and coax hardware into setting nrdy */
-	mpam_write_monsel_reg(msc, CFG_CSU_FLT, 0x1);
+	if (mpam_write_monsel_reg(msc, CFG_CSU_FLT, 0x1))
+		goto out_unlock;
 	if (_mpam_read_monsel_reg(msc, MSMON_CSU, &now))
 		goto out_unlock;
 	can_clear = !(now & MSMON___NRDY);
@@ -1060,8 +1066,9 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc)
 
 	for (ris_idx = 0; ris_idx <= msc->ris_max; ris_idx++) {
 		mutex_lock(&msc->part_sel_lock);
-		__mpam_part_sel(ris_idx, 0, msc);
-		ret = mpam_msc_read_idr(msc, &idr);
+		ret = __mpam_part_sel(ris_idx, 0, msc);
+		if (!ret)
+			ret = mpam_msc_read_idr(msc, &idr);
 		mutex_unlock(&msc->part_sel_lock);
 		if (ret)
 			return ret;
@@ -1080,9 +1087,11 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc)
 		ris->idr = idr;
 
 		mutex_lock(&msc->part_sel_lock);
-		__mpam_part_sel(ris_idx, 0, msc);
-		ret = mpam_ris_hw_probe(ris);
+		ret = __mpam_part_sel(ris_idx, 0, msc);
+		if (!ret)
+			ret = mpam_ris_hw_probe(ris);
 		mutex_unlock(&msc->part_sel_lock);
+
 		if (ret)
 			return ret;
 	}
-- 
2.43.0

Re: [PATCH v3 11/16] arm_mpam: propagate MSC write errors for hardware probe functions

[Jonathan Cameron]

On Fri, 10 Jul 2026 16:45:15 +0200
Andre Przywara <andre.przywara@arm.com> wrote:

> Allow the MSC write accesses in the hardware probe functions to return an
> error, and propagate write errors from the lower level up.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> @@ -1080,9 +1087,11 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc)
>  		ris->idr = idr;
>  
>  		mutex_lock(&msc->part_sel_lock);
> -		__mpam_part_sel(ris_idx, 0, msc);
> -		ret = mpam_ris_hw_probe(ris);
> +		ret = __mpam_part_sel(ris_idx, 0, msc);
> +		if (!ret)
> +			ret = mpam_ris_hw_probe(ris);
>  		mutex_unlock(&msc->part_sel_lock);
> +
Similar to before, scoped_guard() and burn a few lines to get
simpler code flow.

Various other comments from earlier patches apply in this one
as well, I'm just assuming you'll make similar changes (if
you adopt them based on earlier comments)

Jonathan

>  		if (ret)
>  			return ret;
>  	}

[PATCH v3 12/16] arm_mpam: propagate MSC write errors for remaining MSC write users

[Andre Przywara]

Allow the remaining MSC device functions to return an error, and
propagate write errors from the lower level up.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>

propagate write errors up for mpam_save_mbwu_state()
---
 drivers/resctrl/mpam_devices.c | 82 +++++++++++++++++++++++-----------
 1 file changed, 56 insertions(+), 26 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index 222515a01b35..ca73029654b6 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -1162,15 +1162,20 @@ static int mpam_msc_read_mbwu_l(struct mpam_msc *msc, u64 *res)
 	return ret;
 }
 
-static void mpam_msc_zero_mbwu_l(struct mpam_msc *msc)
+static int mpam_msc_zero_mbwu_l(struct mpam_msc *msc)
 {
+	int ret;
+
 	mpam_mon_sel_lock_held(msc);
 
 	WARN_ON_ONCE((MSMON_MBWU_L + sizeof(u64)) > msc->mapped_hwpage_sz);
 	WARN_ON_ONCE(!cpumask_test_cpu(smp_processor_id(), &msc->accessibility));
 
-	__mpam_write_reg(msc, MSMON_MBWU_L, 0);
-	__mpam_write_reg(msc, MSMON_MBWU_L + 4, 0);
+	ret = __mpam_write_reg(msc, MSMON_MBWU_L, 0);
+	if (!ret)
+		ret = __mpam_write_reg(msc, MSMON_MBWU_L + 4, 0);
+
+	return ret;
 }
 
 static void gen_msmon_ctl_flt_vals(struct mon_read *m, u32 *ctl_val,
@@ -1249,10 +1254,11 @@ static inline void clean_msmon_ctl_val(u32 *cur_ctl)
 		*cur_ctl &= ~MSMON_CFG_MBWU_CTL_OFLOW_STATUS_L;
 }
 
-static void write_msmon_ctl_flt_vals(struct mon_read *m, u32 ctl_val,
-				     u32 flt_val)
+static int write_msmon_ctl_flt_vals(struct mon_read *m, u32 ctl_val,
+				    u32 flt_val)
 {
 	struct mpam_msc *msc = m->ris->vmsc->msc;
+	int ret;
 
 	/*
 	 * Write the ctl_val with the enable bit cleared, reset the counter,
@@ -1260,26 +1266,37 @@ static void write_msmon_ctl_flt_vals(struct mon_read *m, u32 ctl_val,
 	 */
 	switch (m->type) {
 	case mpam_feat_msmon_csu:
-		mpam_write_monsel_reg(msc, CFG_CSU_FLT, flt_val);
-		mpam_write_monsel_reg(msc, CFG_CSU_CTL, ctl_val);
-		mpam_write_monsel_reg(msc, CSU, 0);
-		mpam_write_monsel_reg(msc, CFG_CSU_CTL, ctl_val | MSMON_CFG_x_CTL_EN);
+		ret = mpam_write_monsel_reg(msc, CFG_CSU_FLT, flt_val);
+		if (!ret)
+			ret = mpam_write_monsel_reg(msc, CFG_CSU_CTL, ctl_val);
+		if (!ret)
+			ret = mpam_write_monsel_reg(msc, CSU, 0);
+		if (!ret)
+			ret = mpam_write_monsel_reg(msc, CFG_CSU_CTL, ctl_val | MSMON_CFG_x_CTL_EN);
 		break;
 	case mpam_feat_msmon_mbwu_31counter:
 	case mpam_feat_msmon_mbwu_44counter:
 	case mpam_feat_msmon_mbwu_63counter:
-		mpam_write_monsel_reg(msc, CFG_MBWU_FLT, flt_val);
-		mpam_write_monsel_reg(msc, CFG_MBWU_CTL, ctl_val);
-		mpam_write_monsel_reg(msc, CFG_MBWU_CTL, ctl_val | MSMON_CFG_x_CTL_EN);
+		ret = mpam_write_monsel_reg(msc, CFG_MBWU_FLT, flt_val);
+		if (!ret)
+			ret = mpam_write_monsel_reg(msc, CFG_MBWU_CTL, ctl_val);
+		if (!ret)
+			ret = mpam_write_monsel_reg(msc, CFG_MBWU_CTL,
+						    ctl_val | MSMON_CFG_x_CTL_EN);
 		/* Counting monitors require NRDY to be reset by software */
-		if (m->type == mpam_feat_msmon_mbwu_31counter)
-			mpam_write_monsel_reg(msc, MBWU, 0);
-		else
-			mpam_msc_zero_mbwu_l(m->ris->vmsc->msc);
+		if (!ret) {
+			if (m->type == mpam_feat_msmon_mbwu_31counter)
+				ret = mpam_write_monsel_reg(msc, MBWU, 0);
+			else
+				ret = mpam_msc_zero_mbwu_l(m->ris->vmsc->msc);
+		}
 		break;
 	default:
 		pr_warn("Unexpected monitor type %d\n", m->type);
+		return -EINVAL;
 	}
+
+	return ret;
 }
 
 static u64 __mpam_msmon_overflow_val(enum mpam_device_features type)
@@ -1334,7 +1351,9 @@ static void __ris_msmon_read(void *arg)
 	}
 	mon_sel = FIELD_PREP(MSMON_CFG_MON_SEL_MON_SEL, ctx->mon) |
 		  FIELD_PREP(MSMON_CFG_MON_SEL_RIS, ris->ris_idx);
-	mpam_write_monsel_reg(msc, CFG_MON_SEL, mon_sel);
+	ret = mpam_write_monsel_reg(msc, CFG_MON_SEL, mon_sel);
+	if (ret)
+		goto out_unlock;
 
 	switch (m->type) {
 	case mpam_feat_msmon_mbwu_31counter:
@@ -1370,14 +1389,16 @@ static void __ris_msmon_read(void *arg)
 			  cur_ctl != (ctl_val | MSMON_CFG_x_CTL_EN);
 
 	if (config_mismatch || reset_on_next_read) {
-		write_msmon_ctl_flt_vals(m, ctl_val, flt_val);
+		ret = write_msmon_ctl_flt_vals(m, ctl_val, flt_val);
 		overflow = false;
 	} else if (overflow) {
-		mpam_write_monsel_reg(msc, CFG_MBWU_CTL,
-				      cur_ctl &
-				      ~(MSMON_CFG_x_CTL_OFLOW_STATUS |
-					MSMON_CFG_MBWU_CTL_OFLOW_STATUS_L));
+		ret = mpam_write_monsel_reg(msc, CFG_MBWU_CTL,
+					    cur_ctl &
+					    ~(MSMON_CFG_x_CTL_OFLOW_STATUS |
+					    MSMON_CFG_MBWU_CTL_OFLOW_STATUS_L));
 	}
+	if (ret)
+		goto out_unlock;
 
 	switch (m->type) {
 	case mpam_feat_msmon_csu:
@@ -1810,20 +1831,27 @@ static int mpam_save_mbwu_state(void *arg)
 
 		mon_sel = FIELD_PREP(MSMON_CFG_MON_SEL_MON_SEL, i) |
 			  FIELD_PREP(MSMON_CFG_MON_SEL_RIS, ris->ris_idx);
-		mpam_write_monsel_reg(msc, CFG_MON_SEL, mon_sel);
+		ret = mpam_write_monsel_reg(msc, CFG_MON_SEL, mon_sel);
+		if (ret)
+			goto out_unlock;
 		ret = mpam_read_monsel_reg(msc, CFG_MBWU_FLT, &cur_flt);
 		if (ret)
 			goto out_unlock;
 		ret = mpam_read_monsel_reg(msc, CFG_MBWU_CTL, &cur_ctl);
 		if (ret)
 			goto out_unlock;
-		mpam_write_monsel_reg(msc, CFG_MBWU_CTL, 0);
+		ret = mpam_write_monsel_reg(msc, CFG_MBWU_CTL, 0);
+		if (ret)
+			goto out_unlock;
 
 		if (mpam_ris_has_mbwu_long_counter(ris)) {
 			ret = mpam_msc_read_mbwu_l(msc, &val);
 			if (ret)
 				goto out_unlock;
-			mpam_msc_zero_mbwu_l(msc);
+
+			ret = mpam_msc_zero_mbwu_l(msc);
+			if (ret)
+				goto out_unlock;
 		} else {
 			u32 val32;
 
@@ -1832,7 +1860,9 @@ static int mpam_save_mbwu_state(void *arg)
 				goto out_unlock;
 
 			val = val32;
-			mpam_write_monsel_reg(msc, MBWU, 0);
+			ret = mpam_write_monsel_reg(msc, MBWU, 0);
+			if (ret)
+				goto out_unlock;
 		}
 
 		if (val != MSMON___L_NRDY) {
-- 
2.43.0

Re: [PATCH v3 12/16] arm_mpam: propagate MSC write errors for remaining MSC write users

[Jonathan Cameron]

On Fri, 10 Jul 2026 16:45:16 +0200
Andre Przywara <andre.przywara@arm.com> wrote:

> Allow the remaining MSC device functions to return an error, and
> propagate write errors from the lower level up.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> 
> propagate write errors up for mpam_save_mbwu_state()
below the sign off?

> ---
>  drivers/resctrl/mpam_devices.c | 82 +++++++++++++++++++++++-----------
>  1 file changed, 56 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 222515a01b35..ca73029654b6 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -1162,15 +1162,20 @@ static int mpam_msc_read_mbwu_l(struct mpam_msc *msc, u64 *res)
>  	return ret;
>  }
>  
> -static void mpam_msc_zero_mbwu_l(struct mpam_msc *msc)
> +static int mpam_msc_zero_mbwu_l(struct mpam_msc *msc)
>  {
> +	int ret;
> +
>  	mpam_mon_sel_lock_held(msc);
>  
>  	WARN_ON_ONCE((MSMON_MBWU_L + sizeof(u64)) > msc->mapped_hwpage_sz);
>  	WARN_ON_ONCE(!cpumask_test_cpu(smp_processor_id(), &msc->accessibility));
>  
> -	__mpam_write_reg(msc, MSMON_MBWU_L, 0);
> -	__mpam_write_reg(msc, MSMON_MBWU_L + 4, 0);
> +	ret = __mpam_write_reg(msc, MSMON_MBWU_L, 0);
> +	if (!ret)
> +		ret = __mpam_write_reg(msc, MSMON_MBWU_L + 4, 0);

Code lines are cheap.  Burn them for simple flow.

	ret = __mpam_write_reg(msc, MSMON_MBWU_L, 0);
	if (ret)
		return ret;

	return __mpam_write_reg(msc, MSMON_MBWU_L + 4, 0);


> +
> +	return ret;
>  }
>  
>  static void gen_msmon_ctl_flt_vals(struct mon_read *m, u32 *ctl_val,
> +static int write_msmon_ctl_flt_vals(struct mon_read *m, u32 ctl_val,
> +				    u32 flt_val)
>  {
>  	struct mpam_msc *msc = m->ris->vmsc->msc;
> +	int ret;
>  
>  	/*
>  	 * Write the ctl_val with the enable bit cleared, reset the counter,
> @@ -1260,26 +1266,37 @@ static void write_msmon_ctl_flt_vals(struct mon_read *m, u32 ctl_val,
>  	 */
>  	switch (m->type) {
>  	case mpam_feat_msmon_csu:
> -		mpam_write_monsel_reg(msc, CFG_CSU_FLT, flt_val);
> -		mpam_write_monsel_reg(msc, CFG_CSU_CTL, ctl_val);
> -		mpam_write_monsel_reg(msc, CSU, 0);
> -		mpam_write_monsel_reg(msc, CFG_CSU_CTL, ctl_val | MSMON_CFG_x_CTL_EN);
> +		ret = mpam_write_monsel_reg(msc, CFG_CSU_FLT, flt_val);
> +		if (!ret)
> +			ret = mpam_write_monsel_reg(msc, CFG_CSU_CTL, ctl_val);
> +		if (!ret)
> +			ret = mpam_write_monsel_reg(msc, CSU, 0);
> +		if (!ret)
> +			ret = mpam_write_monsel_reg(msc, CFG_CSU_CTL, ctl_val | MSMON_CFG_x_CTL_EN);

By now you can probably guess my comments on this.

If you ever find yourself with a case where you can't do guard/ acquire()/early returns
etc, use a helper function that can just return on each err.

It is more code, but I'd much rather see that on error we 'stop' and return
than try and follow the path to see if anything else happens.

		ret = a();
		if (ret)
			return ret;

		ret = b();
		if (ret)
			return ret;

etc.

[PATCH v3 13/16] arm_mpam: prepare mon_sel locking for MPAM-Fb

[Andre Przywara]

The MSC MON_SEL register needs to be accessed from hardirq for the overflow
interrupt, and when taking an IPI to access these registers on platforms
where MSC are not accesible from every CPU. This makes an irqsave
spinlock the obvious lock to protect these registers. On systems with SCMI
mailboxes it must be able to sleep, meaning a mutex must be used. The
SCMI platforms can't support an overflow interrupt.
Clearly these two can't exist for one MSC at the same time.

Change the mon_sel locking wrapper function to only use a spinlock when
the MSC is accessed directly via MMIO. In case of MPAM-Fb, we use a
mutex, but only if we are in a sleepable context. If that's not the
case, we return an error. This should not happen, as MPAM-Fb by design
does not require an MSC access to happen from a specific CPU, so there
is no need for any IPIs or preemption disabling to satisfy CPU
constraints. And since overflow interrupts are not supported at the moment
anyway, we also wouldn't meet the other case.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/resctrl/mpam_internal.h | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/mpam_internal.h
index 04d1a59f02af..7b6e0df904f8 100644
--- a/drivers/resctrl/mpam_internal.h
+++ b/drivers/resctrl/mpam_internal.h
@@ -126,6 +126,7 @@ struct mpam_msc {
 	 */
 	raw_spinlock_t		_mon_sel_lock;
 	unsigned long		_mon_sel_flags;
+	struct mutex		mon_sel_mutex;
 
 	void __iomem		*mapped_hwpage;
 	size_t			mapped_hwpage_sz;
@@ -139,27 +140,44 @@ struct mpam_msc {
 /* Returning false here means accesses to mon_sel must fail and report an error. */
 static inline bool __must_check mpam_mon_sel_lock(struct mpam_msc *msc)
 {
-	/* Locking will require updating to support a firmware backed interface */
-	if (WARN_ON_ONCE(msc->iface != MPAM_IFACE_MMIO))
+	if (msc->iface == MPAM_IFACE_MMIO) {
+		raw_spin_lock_irqsave(&msc->_mon_sel_lock, msc->_mon_sel_flags);
+
+		return true;
+	}
+
+	if (!preemptible())
 		return false;
 
-	raw_spin_lock_irqsave(&msc->_mon_sel_lock, msc->_mon_sel_flags);
+	mutex_lock(&msc->mon_sel_mutex);
+
 	return true;
 }
 
 static inline void mpam_mon_sel_unlock(struct mpam_msc *msc)
 {
-	raw_spin_unlock_irqrestore(&msc->_mon_sel_lock, msc->_mon_sel_flags);
+	if (msc->iface == MPAM_IFACE_MMIO) {
+		raw_spin_unlock_irqrestore(&msc->_mon_sel_lock,
+					   msc->_mon_sel_flags);
+
+		return;
+	}
+
+	mutex_unlock(&msc->mon_sel_mutex);
 }
 
 static inline void mpam_mon_sel_lock_held(struct mpam_msc *msc)
 {
-	lockdep_assert_held_once(&msc->_mon_sel_lock);
+	if (msc->iface == MPAM_IFACE_MMIO)
+		lockdep_assert_held_once(&msc->_mon_sel_lock);
+	else
+		lockdep_assert_held_once(&msc->mon_sel_mutex);
 }
 
 static inline void mpam_mon_sel_lock_init(struct mpam_msc *msc)
 {
 	raw_spin_lock_init(&msc->_mon_sel_lock);
+	mutex_init(&msc->mon_sel_mutex);
 }
 
 /* Bits for mpam features bitmaps */
-- 
2.43.0

Re: [PATCH v3 13/16] arm_mpam: prepare mon_sel locking for MPAM-Fb

[Jonathan Cameron]

On Fri, 10 Jul 2026 16:45:17 +0200
Andre Przywara <andre.przywara@arm.com> wrote:

> The MSC MON_SEL register needs to be accessed from hardirq for the overflow
> interrupt, and when taking an IPI to access these registers on platforms
> where MSC are not accesible from every CPU. This makes an irqsave
> spinlock the obvious lock to protect these registers. On systems with SCMI
> mailboxes it must be able to sleep, meaning a mutex must be used. The
> SCMI platforms can't support an overflow interrupt.
> Clearly these two can't exist for one MSC at the same time.
> 
> Change the mon_sel locking wrapper function to only use a spinlock when
> the MSC is accessed directly via MMIO. In case of MPAM-Fb, we use a
> mutex, but only if we are in a sleepable context. If that's not the
> case, we return an error. This should not happen, as MPAM-Fb by design
> does not require an MSC access to happen from a specific CPU, so there
> is no need for any IPIs or preemption disabling to satisfy CPU
> constraints. And since overflow interrupts are not supported at the moment
> anyway, we also wouldn't meet the other case.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

I've not been paying attention to this topic, so I might come back
with some more feedback after I've had time to think about it a bit!
So trivial stuff only for now.

>  
>  static inline void mpam_mon_sel_lock_init(struct mpam_msc *msc)
>  {
>  	raw_spin_lock_init(&msc->_mon_sel_lock);
> +	mutex_init(&msc->mon_sel_mutex);

I'm not that fussed, but maybe add a call to mutex_destroy().
Most likely it'll never help catch anything in this code though..

>  }
>  
>  /* Bits for mpam features bitmaps */

[PATCH v3 14/16] arm_mpam: add MPAM-Fb MSC firmware access support

[Andre Przywara]

The Arm MPAM Firmware-backed (Fb) Profile document[1] describes an
alternative way of accessing the "Memory System Components" (MSC) in an
MPAM enabled system.
Normally the MSCs are MMIO mapped, but in some implementations this
might not be possible (MSC located outside of the local socket, MSC
mapped secure-only) or desirable (direct MMIO access too slow or needs
to be mediated through a control processor). MPAM-fb standardises a
protocol to abstract MSC accesses, building on the SCMI protocol.

Add functions that do an MSC read or write access by redirecting the
request through a firmware interface. For now this done via an ACPI
PCC shared memory and mailbox combination.

Since the protocol used is only a small subset of the full SCMI spec,
and the SCMI protocol has no full ACPI support anyway, open-code the
SCMI message generation and handshake, for just the fields we need.

[1] https://developer.arm.com/documentation/den0144/latest

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/resctrl/Makefile        |   2 +-
 drivers/resctrl/mpam_devices.c  |  26 +++-
 drivers/resctrl/mpam_fb.c       | 208 ++++++++++++++++++++++++++++++++
 drivers/resctrl/mpam_internal.h |  20 +++
 include/linux/arm_mpam.h        |   2 +-
 5 files changed, 251 insertions(+), 7 deletions(-)
 create mode 100644 drivers/resctrl/mpam_fb.c

diff --git a/drivers/resctrl/Makefile b/drivers/resctrl/Makefile
index 4f6d0e81f9b8..097c036724e9 100644
--- a/drivers/resctrl/Makefile
+++ b/drivers/resctrl/Makefile
@@ -1,5 +1,5 @@
 obj-$(CONFIG_ARM64_MPAM_DRIVER)			+= mpam.o
-mpam-y						+= mpam_devices.o
+mpam-y						+= mpam_devices.o mpam_fb.o
 mpam-$(CONFIG_ARM64_MPAM_RESCTRL_FS)		+= mpam_resctrl.o
 
 ccflags-$(CONFIG_ARM64_MPAM_DRIVER_DEBUG)	+= -DDEBUG
diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index ca73029654b6..4d3e642486d4 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -181,6 +181,9 @@ static int __mpam_read_reg(struct mpam_msc *msc, u16 reg, u32 *res)
 {
 	WARN_ON_ONCE(!cpumask_test_cpu(smp_processor_id(), &msc->accessibility));
 
+	if (msc->iface == MPAM_IFACE_PCC)
+		return mpam_fb_send_read_request(msc, reg, res);
+
 	*res = readl_relaxed(msc->mapped_hwpage + reg);
 
 	return 0;
@@ -197,9 +200,12 @@ static inline int _mpam_read_partsel_reg(struct mpam_msc *msc, u16 reg,
 
 static int __mpam_write_reg(struct mpam_msc *msc, u16 reg, u32 val)
 {
-	WARN_ON_ONCE(reg + sizeof(u32) > msc->mapped_hwpage_sz);
 	WARN_ON_ONCE(!cpumask_test_cpu(smp_processor_id(), &msc->accessibility));
 
+	if (msc->iface == MPAM_IFACE_PCC)
+		return mpam_fb_send_write_request(msc, reg, val);
+
+	WARN_ON_ONCE(reg + sizeof(u32) > msc->mapped_hwpage_sz);
 	writel_relaxed(val, msc->mapped_hwpage + reg);
 
 	return 0;
@@ -1133,7 +1139,8 @@ static int mpam_msc_read_mbwu_l(struct mpam_msc *msc, u64 *res)
 
 	mpam_mon_sel_lock_held(msc);
 
-	WARN_ON_ONCE((MSMON_MBWU_L + sizeof(u64)) > msc->mapped_hwpage_sz);
+	if (msc->iface == MPAM_IFACE_MMIO)
+		WARN_ON_ONCE((MSMON_MBWU_L + sizeof(u64)) > msc->mapped_hwpage_sz);
 	WARN_ON_ONCE(!cpumask_test_cpu(smp_processor_id(), &msc->accessibility));
 
 	ret = __mpam_read_reg(msc, MSMON_MBWU_L + 4, &mbwu_l_high2);
@@ -1481,9 +1488,15 @@ static int _msmon_read(struct mpam_component *comp, struct mon_read *arg)
 					 srcu_read_lock_held(&mpam_srcu)) {
 			arg->ris = ris;
 
-			err = smp_call_function_any(&msc->accessibility,
-						    __ris_msmon_read, arg,
-						    true);
+			if (msc->iface == MPAM_IFACE_MMIO) {
+				err = smp_call_function_any(&msc->accessibility,
+							    __ris_msmon_read,
+							    arg, true);
+			} else {
+				__ris_msmon_read(arg);
+				err = 0;
+			}
+
 			if (!err && arg->err)
 				err = arg->err;
 
@@ -1922,6 +1935,9 @@ static int mpam_get_msc_preferred_cpu(struct mpam_msc *msc)
 
 static int mpam_touch_msc(struct mpam_msc *msc, int (*fn)(void *a), void *arg)
 {
+	if (msc->iface != MPAM_IFACE_MMIO)
+		return fn(arg);
+
 	lockdep_assert_irqs_enabled();
 	lockdep_assert_cpus_held();
 	WARN_ON_ONCE(!srcu_read_lock_held((&mpam_srcu)));
diff --git a/drivers/resctrl/mpam_fb.c b/drivers/resctrl/mpam_fb.c
new file mode 100644
index 000000000000..7d7409910f28
--- /dev/null
+++ b/drivers/resctrl/mpam_fb.c
@@ -0,0 +1,208 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2024 Arm Ltd.
+
+#include <linux/arm_mpam.h>
+#include <linux/cleanup.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/gfp.h>
+#include <linux/list.h>
+#include <linux/mailbox_client.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/printk.h>
+#include <linux/processor.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+
+#include <acpi/pcc.h>
+
+#include <asm/mpam.h>
+
+#include "mpam_internal.h"
+
+#define MPAM_FB_PROTOCOL_ID	0x1a
+#define MPAM_PROTOCOL_VERSION	0x0
+#define MPAM_MSC_ATTRIBUTES_CMD	0x3
+#define MPAM_MSC_READ_CMD	0x4
+#define MPAM_MSC_WRITE_CMD	0x5
+
+#define MPAM_FB_ERR_SUCCESS		 0
+#define MPAM_FB_ERR_NOT_SUPPORTED	-1
+#define MPAM_FB_ERR_INVALID_PARAMETERS	-2
+#define MPAM_FB_ERR_DENIED		-3
+#define MPAM_FB_ERR_NOT_FOUND		-4
+#define MPAM_FB_ERR_OUT_OF_RANGE	-5
+#define MPAM_FB_ERR_BUSY		-6
+#define MPAM_FB_ERR_COMMS_ERROR		-7
+#define MPAM_FB_ERR_GENERIC_ERROR	-8
+#define MPAM_FB_ERR_HW_ERROR		-9
+#define MPAM_FB_ERR_PROTOCOL_ERROR	-10
+#define MPAM_FB_ERR_IN_USE		-11
+
+#define MPAM_MSC_PROT_ID_MASK	GENMASK(17, 10)
+#define MPAM_MSC_TOKEN_MASK	GENMASK(27, 18)
+
+struct mpam_fb_access_payload {
+	u32 msc_id;
+	u32 flags;
+	u32 reg_offset;
+	u32 value;
+} __packed;
+
+#define PCC_CHAN_FLAGS_IRQ	BIT(0)
+#define MPAM_VERSION_MSG_SIZE	(PCC_TYPE3_MSG_PAYLOAD_OFS)
+#define MPAM_READ_MSG_SIZE	(PCC_TYPE3_MSG_PAYLOAD_OFS + 3 * sizeof(u32))
+#define MPAM_WRITE_MSG_SIZE	(PCC_TYPE3_MSG_PAYLOAD_OFS + 4 * sizeof(u32))
+
+static atomic_t mpam_fb_token = ATOMIC_INIT(0);
+
+static int mpam_fb_build_version_message(unsigned int token,
+					 void __iomem *msg_buf)
+{
+	struct acpi_pcct_ext_pcc_shared_memory *pcc_shmem = msg_buf;
+
+	writel_relaxed(0, &pcc_shmem->flags);
+	writel_relaxed(MPAM_VERSION_MSG_SIZE, &pcc_shmem->length);
+	writel_relaxed(MPAM_PROTOCOL_VERSION |
+		       FIELD_PREP(MPAM_MSC_TOKEN_MASK, token) |
+		       FIELD_PREP(MPAM_MSC_PROT_ID_MASK, MPAM_FB_PROTOCOL_ID),
+		       &pcc_shmem->command);
+
+	return MPAM_VERSION_MSG_SIZE;
+}
+
+static int mpam_fb_build_read_message(int msc_id, int reg, unsigned int token,
+				      void __iomem *msg_buf)
+{
+	struct acpi_pcct_ext_pcc_shared_memory *pcc_shmem = msg_buf;
+	struct mpam_fb_access_payload *payload = msg_buf + sizeof(*pcc_shmem);
+
+	writel_relaxed(0, &pcc_shmem->flags);
+	writel_relaxed(MPAM_READ_MSG_SIZE, &pcc_shmem->length);
+	writel_relaxed(MPAM_MSC_READ_CMD |
+		       FIELD_PREP(MPAM_MSC_TOKEN_MASK, token) |
+		       FIELD_PREP(MPAM_MSC_PROT_ID_MASK, MPAM_FB_PROTOCOL_ID),
+		       &pcc_shmem->command);
+
+	writel_relaxed(msc_id, &payload->msc_id);
+	writel_relaxed(0, &payload->flags);
+	writel_relaxed(reg, &payload->reg_offset);
+
+	return MPAM_READ_MSG_SIZE;
+}
+
+static int mpam_fb_build_write_message(int msc_id, int reg, u32 val,
+				       unsigned int token,
+				       void __iomem *msg_buf)
+{
+	struct acpi_pcct_ext_pcc_shared_memory *pcc_shmem = msg_buf;
+	struct mpam_fb_access_payload *payload = msg_buf + sizeof(*pcc_shmem);
+
+	writel_relaxed(0, &pcc_shmem->flags);
+	writel_relaxed(MPAM_WRITE_MSG_SIZE, &pcc_shmem->length);
+	writel_relaxed(MPAM_MSC_WRITE_CMD |
+		       FIELD_PREP(MPAM_MSC_TOKEN_MASK, token) |
+		       FIELD_PREP(MPAM_MSC_PROT_ID_MASK, MPAM_FB_PROTOCOL_ID),
+		       &pcc_shmem->command);
+
+	writel_relaxed(msc_id, &payload->msc_id);
+	writel_relaxed(0, &payload->flags);
+	writel_relaxed(reg, &payload->reg_offset);
+	writel_relaxed(val, &payload->value);
+
+	return MPAM_WRITE_MSG_SIZE;
+}
+
+static int mpam_fb_send_request(struct mpam_pcc_chan *pcc_chan, u32 msc_id,
+				u16 reg, u32 *result, int mpam_fb_command)
+{
+	unsigned int token = atomic_inc_return(&mpam_fb_token);
+	struct acpi_pcct_ext_pcc_shared_memory *pcc_shmem;
+	struct pcc_mbox_chan *chan;
+	void __iomem *payload_ofs;
+	u32 status;
+	int ret;
+
+	if (!pcc_chan)
+		return -ENODEV;
+
+	chan = pcc_chan->pcc_chan;
+
+	guard(mutex)(&pcc_chan->pcc_chan_lock);
+
+	switch (mpam_fb_command) {
+	case MPAM_MSC_WRITE_CMD:
+		ret = mpam_fb_build_write_message(msc_id, reg, *result,
+						  token, chan->shmem);
+		break;
+	case MPAM_MSC_READ_CMD:
+		ret = mpam_fb_build_read_message(msc_id, reg,
+						 token, chan->shmem);
+		break;
+	case MPAM_PROTOCOL_VERSION:
+		ret = mpam_fb_build_version_message(token, chan->shmem);
+		break;
+	}
+	if (ret < 0)
+		return ret;
+
+	ret = mbox_send_message(chan->mchan, NULL);
+	if (ret < 0)
+		return ret;
+
+	pcc_shmem = chan->shmem;
+	payload_ofs = chan->shmem + sizeof(*pcc_shmem);
+	status = readl(&pcc_shmem->command);
+	if (FIELD_GET(MPAM_MSC_TOKEN_MASK, status) != token)
+		return -ETIMEDOUT;
+
+	ret = readl(payload_ofs + 0x0);
+	if (ret < 0) {
+		switch (ret) {
+		case MPAM_FB_ERR_NOT_SUPPORTED:
+			return -EOPNOTSUPP;
+		case MPAM_FB_ERR_INVALID_PARAMETERS:
+			return -EINVAL;
+		case MPAM_FB_ERR_NOT_FOUND:
+			return -ENOENT;
+		case MPAM_FB_ERR_OUT_OF_RANGE:
+			return -ERANGE;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	if (mpam_fb_command != MPAM_MSC_WRITE_CMD)
+		*result = readl(payload_ofs + 0x4);
+
+	return 0;
+}
+
+int mpam_fb_send_read_request(struct mpam_msc *msc, u16 reg, u32 *result)
+{
+	return mpam_fb_send_request(msc->pcc_chan, msc->mpam_fb_msc_id,
+				    reg, result, MPAM_MSC_READ_CMD);
+}
+
+int mpam_fb_send_write_request(struct mpam_msc *msc, u16 reg, u32 value)
+{
+	return mpam_fb_send_request(msc->pcc_chan, msc->mpam_fb_msc_id,
+				    reg, &value, MPAM_MSC_WRITE_CMD);
+}
+
+int mpam_fb_get_protocol_version(struct mpam_msc *msc)
+{
+	u32 version;
+	int ret;
+
+	ret = mpam_fb_send_request(msc->pcc_chan, 0,
+				   0, &version, MPAM_PROTOCOL_VERSION);
+	if (ret)
+		return ret;
+
+	return version;
+}
diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/mpam_internal.h
index 7b6e0df904f8..c3c4ddb4a561 100644
--- a/drivers/resctrl/mpam_internal.h
+++ b/drivers/resctrl/mpam_internal.h
@@ -11,6 +11,7 @@
 #include <linux/io.h>
 #include <linux/jump_label.h>
 #include <linux/llist.h>
+#include <linux/mailbox_client.h>
 #include <linux/mutex.h>
 #include <linux/resctrl.h>
 #include <linux/spinlock.h>
@@ -57,6 +58,15 @@ struct mpam_garbage {
 	struct platform_device	*pdev;
 };
 
+struct mpam_pcc_chan {
+	struct list_head	pcc_chans;
+	struct mbox_client	pcc_cl;
+	struct pcc_mbox_chan	*pcc_chan;
+	struct mutex		pcc_chan_lock; /* only one message at a time */
+	int			subspace_id;
+	int			refcount;
+};
+
 struct mpam_msc {
 	/* member of mpam_all_msc */
 	struct list_head	all_msc_list;
@@ -66,6 +76,8 @@ struct mpam_msc {
 
 	/* Not modified after mpam_is_enabled() becomes true */
 	enum mpam_msc_iface	iface;
+	struct mpam_pcc_chan	*pcc_chan;
+	int			mpam_fb_msc_id;	/* in its own name space */
 	u32			nrdy_usec;
 	cpumask_t		accessibility;
 	bool			has_extd_esr;
@@ -501,6 +513,14 @@ static inline void mpam_resctrl_offline_cpu(unsigned int cpu) { }
 static inline void mpam_resctrl_teardown_class(struct mpam_class *class) { }
 #endif /* CONFIG_RESCTRL_FS */
 
+/* MPAM-Fb Firmware-backed protocol wrappers */
+int mpam_fb_send_read_request(struct mpam_msc *msc, u16 reg, u32 *result);
+int mpam_fb_send_write_request(struct mpam_msc *msc, u16 reg, u32 value);
+int mpam_fb_get_protocol_version(struct mpam_msc *msc);
+
+#define PCC_TYPE3_MSG_PAYLOAD_OFS	0x10
+#define MPAM_FB_MAX_MSG_SIZE	(PCC_TYPE3_MSG_PAYLOAD_OFS + 4 * sizeof(u32))
+
 /*
  * MPAM MSCs have the following register layout. See:
  * Arm Memory System Resource Partitioning and Monitoring (MPAM) System
diff --git a/include/linux/arm_mpam.h b/include/linux/arm_mpam.h
index f92a36187a52..002f56e15362 100644
--- a/include/linux/arm_mpam.h
+++ b/include/linux/arm_mpam.h
@@ -12,7 +12,7 @@ struct mpam_msc;
 
 enum mpam_msc_iface {
 	MPAM_IFACE_MMIO,	/* a real MPAM MSC */
-	MPAM_IFACE_PCC,		/* a fake MPAM MSC */
+	MPAM_IFACE_PCC,		/* using the MPAM-Fb firmware redirection */
 };
 
 enum mpam_class_types {
-- 
2.43.0

Re: [PATCH v3 14/16] arm_mpam: add MPAM-Fb MSC firmware access support

[Jonathan Cameron]

On Fri, 10 Jul 2026 16:45:18 +0200
Andre Przywara <andre.przywara@arm.com> wrote:

> The Arm MPAM Firmware-backed (Fb) Profile document[1] describes an
> alternative way of accessing the "Memory System Components" (MSC) in an
> MPAM enabled system.

Blank line for consistency on paragraph breaks.

> Normally the MSCs are MMIO mapped, but in some implementations this

Hmm. Not sure what normal is in this case ;)  Maybe make it something like

In systems supported before this patch the MSCs are MMIO mapped,

> might not be possible (MSC located outside of the local socket, MSC
> mapped secure-only) or desirable (direct MMIO access too slow or needs
> to be mediated through a control processor). MPAM-fb standardises a
> protocol to abstract MSC accesses, building on the SCMI protocol.
> 
> Add functions that do an MSC read or write access by redirecting the
> request through a firmware interface. For now this done via an ACPI
> PCC shared memory and mailbox combination.
> 
> Since the protocol used is only a small subset of the full SCMI spec,
> and the SCMI protocol has no full ACPI support anyway, open-code the
> SCMI message generation and handshake, for just the fields we need.
> 
> [1] https://developer.arm.com/documentation/den0144/latest
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Hi Andre,

Various things inline.

Thanks,

Jonathan

...

> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index ca73029654b6..4d3e642486d4 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c

> @@ -1133,7 +1139,8 @@ static int mpam_msc_read_mbwu_l(struct mpam_msc *msc, u64 *res)
>  
>  	mpam_mon_sel_lock_held(msc);
>  
> -	WARN_ON_ONCE((MSMON_MBWU_L + sizeof(u64)) > msc->mapped_hwpage_sz);
> +	if (msc->iface == MPAM_IFACE_MMIO)
> +		WARN_ON_ONCE((MSMON_MBWU_L + sizeof(u64)) > msc->mapped_hwpage_sz);
>  	WARN_ON_ONCE(!cpumask_test_cpu(smp_processor_id(), &msc->accessibility));
>  
>  	ret = __mpam_read_reg(msc, MSMON_MBWU_L + 4, &mbwu_l_high2);
> @@ -1481,9 +1488,15 @@ static int _msmon_read(struct mpam_component *comp, struct mon_read *arg)
>  					 srcu_read_lock_held(&mpam_srcu)) {
>  			arg->ris = ris;
>  
> -			err = smp_call_function_any(&msc->accessibility,
> -						    __ris_msmon_read, arg,
> -						    true);
> +			if (msc->iface == MPAM_IFACE_MMIO) {
> +				err = smp_call_function_any(&msc->accessibility,
> +							    __ris_msmon_read,
> +							    arg, true);
> +			} else {
> +				__ris_msmon_read(arg);
> +				err = 0;
> +			}
> +
>  			if (!err && arg->err)
>  				err = arg->err;
FWIW
The arg->err check here seems pointless

			if (!err)
				err = arg->err;

is at most going to set a 0 to 0.

Maybe lift this into the if else as it should end up simpler in the else.

			if (msc->iface == MPAM_IFACE_MMIO) {
				err = smp_call_function_any(&msc->accessibility,
							    __ris_msmon_read,
							    arg, true);
				if (!err)
					err = arg->err;
			} else {
				__ris_msmon_read(arg);
				err = arg->err;
			}

			if (err)
				any_err = err;

 
...

> diff --git a/drivers/resctrl/mpam_fb.c b/drivers/resctrl/mpam_fb.c
> new file mode 100644
> index 000000000000..7d7409910f28
> --- /dev/null
> +++ b/drivers/resctrl/mpam_fb.c
> @@ -0,0 +1,208 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2024 Arm Ltd.

Really?  sat on this for 2 years? ;) 
Given it's changing during this posting I'd at least include this
year in the range.

> +
> +#include <linux/arm_mpam.h>
> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/gfp.h>
> +#include <linux/list.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>

Scrub these for things used in this patch.
Maybe others will be needed after later patches.
Not seeing any of_ stuff in here for instance.

> +#include <linux/platform_device.h>
> +#include <linux/printk.h>
> +#include <linux/processor.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/types.h>

> +struct mpam_fb_access_payload {
> +	u32 msc_id;
> +	u32 flags;
> +	u32 reg_offset;
> +	u32 value;
> +} __packed;
There are only a few of these in spec, and whilst they share the same layout
for fields they define (not all same length),

I'd define separate structures for each of the message types.
Added bonus being that...

> +
> +#define PCC_CHAN_FLAGS_IRQ	BIT(0)
> +#define MPAM_VERSION_MSG_SIZE	(PCC_TYPE3_MSG_PAYLOAD_OFS)
> +#define MPAM_READ_MSG_SIZE	(PCC_TYPE3_MSG_PAYLOAD_OFS + 3 * sizeof(u32))

These would then become  PCC_TYPE3_MSG_PAYLOAD_OFF + sizeof(mpam_fb_read_request_payload)
or something self describing along those lines.

> +#define MPAM_WRITE_MSG_SIZE	(PCC_TYPE3_MSG_PAYLOAD_OFS + 4 * sizeof(u32))
> +
> +static atomic_t mpam_fb_token = ATOMIC_INIT(0);
> +
> +static int mpam_fb_build_version_message(unsigned int token,
> +					 void __iomem *msg_buf)
> +{
> +	struct acpi_pcct_ext_pcc_shared_memory *pcc_shmem = msg_buf;
> +
> +	writel_relaxed(0, &pcc_shmem->flags);
> +	writel_relaxed(MPAM_VERSION_MSG_SIZE, &pcc_shmem->length);
> +	writel_relaxed(MPAM_PROTOCOL_VERSION |
> +		       FIELD_PREP(MPAM_MSC_TOKEN_MASK, token) |
> +		       FIELD_PREP(MPAM_MSC_PROT_ID_MASK, MPAM_FB_PROTOCOL_ID),
> +		       &pcc_shmem->command);
> +
> +	return MPAM_VERSION_MSG_SIZE;
> +}
> +
> +static int mpam_fb_build_read_message(int msc_id, int reg, unsigned int token,
> +				      void __iomem *msg_buf)
> +{
> +	struct acpi_pcct_ext_pcc_shared_memory *pcc_shmem = msg_buf;
> +	struct mpam_fb_access_payload *payload = msg_buf + sizeof(*pcc_shmem);
> +
> +	writel_relaxed(0, &pcc_shmem->flags);
> +	writel_relaxed(MPAM_READ_MSG_SIZE, &pcc_shmem->length);
> +	writel_relaxed(MPAM_MSC_READ_CMD |
> +		       FIELD_PREP(MPAM_MSC_TOKEN_MASK, token) |
> +		       FIELD_PREP(MPAM_MSC_PROT_ID_MASK, MPAM_FB_PROTOCOL_ID),
> +		       &pcc_shmem->command);
> +
> +	writel_relaxed(msc_id, &payload->msc_id);
> +	writel_relaxed(0, &payload->flags);
> +	writel_relaxed(reg, &payload->reg_offset);
> +
> +	return MPAM_READ_MSG_SIZE;
> +}

> +
> +static int mpam_fb_send_request(struct mpam_pcc_chan *pcc_chan, u32 msc_id,
> +				u16 reg, u32 *result, int mpam_fb_command)
> +{
> +	unsigned int token = atomic_inc_return(&mpam_fb_token);
> +	struct acpi_pcct_ext_pcc_shared_memory *pcc_shmem;
> +	struct pcc_mbox_chan *chan;
> +	void __iomem *payload_ofs;
> +	u32 status;
> +	int ret;
> +
> +	if (!pcc_chan)

Is this defense needed?  Feels like the sort of thing that is so fatal
if you get to call send_request() that you should really have failed a lot
earlier.  If it's protecting against a tear down race or similar add a comment

> +		return -ENODEV;

[PATCH v3 15/16] arm_mpam: prevent MPAM-Fb accesses inside IRQ handler

[Andre Przywara]

When an MPAM MSC gets into an error condition, it can trigger an error
IRQ. We cannot really do much about those errors, but we at least query
and log the error, then disable MPAM functionality.

This error report relies on reading the MSC's error status register
(ESR) in the IRQ handler, which is not possible for MPAM-Fb based
MSC accesses, since they involve mailbox routines that might sleep.
The same is true for clearing the interrupt at the source, which
requires MSC access.

For simplicity just skip the ESR read when the MSC is not using direct
MMIO accesses, and just ignore the pending interrupts. We will wrap up
MPAM functionality regardless, knowing the exact error value will not
change that.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/resctrl/mpam_devices.c | 38 ++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index 4d3e642486d4..220b4a06e739 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -2645,7 +2645,7 @@ static int mpam_disable_msc_ecr(void *_msc)
 
 static irqreturn_t __mpam_irq_handler(int irq, struct mpam_msc *msc)
 {
-	u64 reg;
+	u64 reg = 0;
 	u16 partid;
 	u8 errcode, pmg, ris;
 
@@ -2654,25 +2654,33 @@ static irqreturn_t __mpam_irq_handler(int irq, struct mpam_msc *msc)
 					   &msc->accessibility)))
 		return IRQ_NONE;
 
-	mpam_msc_read_esr(msc, &reg);
+	/* MPAM-Fb MSC accesses cannot be done in atomic context. */
+	if (msc->iface == MPAM_IFACE_MMIO) {
+		mpam_msc_read_esr(msc, &reg);
 
-	errcode = FIELD_GET(MPAMF_ESR_ERRCODE, reg);
-	if (!errcode)
-		return IRQ_NONE;
+		errcode = FIELD_GET(MPAMF_ESR_ERRCODE, reg);
+		if (!errcode)
+			return IRQ_NONE;
 
-	/* Clear level triggered irq */
-	mpam_msc_clear_esr(msc);
+		/* Clear level triggered irq */
+		mpam_msc_clear_esr(msc);
 
-	partid = FIELD_GET(MPAMF_ESR_PARTID_MON, reg);
-	pmg = FIELD_GET(MPAMF_ESR_PMG, reg);
-	ris = FIELD_GET(MPAMF_ESR_RIS, reg);
+		partid = FIELD_GET(MPAMF_ESR_PARTID_MON, reg);
+		pmg = FIELD_GET(MPAMF_ESR_PMG, reg);
+		ris = FIELD_GET(MPAMF_ESR_RIS, reg);
 
-	pr_err_ratelimited("error irq from msc:%u '%s', partid:%u, pmg: %u, ris: %u\n",
-			   msc->id, mpam_errcode_names[errcode], partid, pmg,
-			   ris);
+		pr_err_ratelimited("error irq from msc:%u '%s', partid:%u, pmg: %u, ris: %u\n",
+				   msc->id, mpam_errcode_names[errcode], partid,
+				   pmg, ris);
 
-	/* Disable this interrupt. */
-	mpam_disable_msc_ecr(msc);
+		/* Disable this interrupt. */
+		mpam_disable_msc_ecr(msc);
+	} else {
+		struct irq_data *d = irq_get_irq_data(irq);
+
+		pr_err_ratelimited("unknown error irq %d/%ld from msc:%u\n",
+				   irq, d ? irqd_to_hwirq(d) : -1, msc->id);
+	}
 
 	/* Are we racing with the thread disabling MPAM? */
 	if (!mpam_is_enabled())
-- 
2.43.0

[PATCH v3 16/16] arm_mpam: detect and enable MPAM-Fb PCC support

[Andre Przywara]

The Arm MPAM-Fb specification [1] describes a protocol to access MSC
registers through a firmware interface. This requires a shared memory
region to hold the message, and a mailbox to trigger the access.
For ACPI this is wrapped as a PCC channel, described using existing
ACPI abstractions.

Add code to parse those PCC table descriptions associated with an MSC,
and store the parsed information in the MSC struct.
There can be multiple PCC channels, and each channel can serve multiple
MSCs, so we need to keep track of the channel usage, using a list and
a refcount.
This will be used by the MPAM-Fb access wrapper code.

[1] https://developer.arm.com/documentation/den0144/latest

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/acpi/arm64/mpam.c      |   2 +
 drivers/resctrl/mpam_devices.c | 126 ++++++++++++++++++++++++++++++++-
 2 files changed, 126 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/arm64/mpam.c b/drivers/acpi/arm64/mpam.c
index 84963a20c3e7..64bc84bb2029 100644
--- a/drivers/acpi/arm64/mpam.c
+++ b/drivers/acpi/arm64/mpam.c
@@ -256,6 +256,8 @@ static struct platform_device * __init acpi_mpam_parse_msc(struct acpi_mpam_msc_
 	} else if (iface == MPAM_IFACE_PCC) {
 		props[next_prop++] = PROPERTY_ENTRY_U32("pcc-channel",
 							tbl_msc->base_address);
+		props[next_prop++] = PROPERTY_ENTRY_U32("msc-id",
+							tbl_msc->identifier);
 	}
 
 	acpi_mpam_parse_irqs(pdev, tbl_msc, res, &next_res);
diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index 220b4a06e739..7be98f13dd63 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -19,6 +19,7 @@
 #include <linux/irqdesc.h>
 #include <linux/list.h>
 #include <linux/lockdep.h>
+#include <linux/mailbox_client.h>
 #include <linux/mutex.h>
 #include <linux/platform_device.h>
 #include <linux/printk.h>
@@ -27,6 +28,9 @@
 #include <linux/types.h>
 #include <linux/workqueue.h>
 
+#include <acpi/pcc.h>
+#include <acpi/acpi_io.h>
+
 #include "mpam_internal.h"
 
 /* Values for the T241 errata workaround */
@@ -49,6 +53,88 @@ static LIST_HEAD(mpam_all_msc);
 
 struct srcu_struct mpam_srcu;
 
+/* PCC channels might be serving multiple MSCs, so keep a refcounted list. */
+static DEFINE_MUTEX(pcc_chan_list_lock);
+static LIST_HEAD(pcc_chan_list);
+
+static void mpam_pcc_rx_callback(struct mbox_client *cl, void *msg)
+{
+	/* TODO: wake up tasks blocked on this MSC's PCC channel */
+}
+
+static struct mpam_pcc_chan *mpam_pcc_chan_get(struct device *dev,
+					       int subspace_id)
+{
+	struct mpam_pcc_chan *cur;
+
+	mutex_lock(&pcc_chan_list_lock);
+
+	list_for_each_entry(cur, &pcc_chan_list, pcc_chans) {
+		if (cur->subspace_id == subspace_id) {
+			cur->refcount++;
+			mutex_unlock(&pcc_chan_list_lock);
+
+			return cur;
+		}
+	}
+
+	cur = kzalloc_obj(*cur);
+	if (!cur) {
+		mutex_unlock(&pcc_chan_list_lock);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	cur->pcc_cl.dev = dev;
+	cur->pcc_cl.rx_callback = mpam_pcc_rx_callback;
+	cur->pcc_cl.tx_block = true;
+	cur->pcc_cl.tx_tout = 1000; /* 1s */
+
+	cur->pcc_chan = pcc_mbox_request_channel(&cur->pcc_cl, subspace_id);
+	if (IS_ERR(cur->pcc_chan)) {
+		long err = PTR_ERR(cur->pcc_chan);
+
+		kfree(cur);
+		mutex_unlock(&pcc_chan_list_lock);
+		return ERR_PTR(err);
+	}
+
+	mutex_init(&cur->pcc_chan_lock);
+	cur->subspace_id = subspace_id;
+	cur->refcount = 1;
+
+	list_add_tail(&cur->pcc_chans, &pcc_chan_list);
+
+	mutex_unlock(&pcc_chan_list_lock);
+
+	return cur;
+}
+
+static int mpam_pcc_chan_put(struct mpam_pcc_chan *pcc_chan)
+{
+	struct mpam_pcc_chan *cur, *tmp;
+
+	if (!pcc_chan)
+		return 0;
+
+	mutex_lock(&pcc_chan_list_lock);
+
+	list_for_each_entry_safe(cur, tmp, &pcc_chan_list, pcc_chans) {
+		if (cur == pcc_chan) {
+			if (!--cur->refcount) {
+				pcc_mbox_free_channel(cur->pcc_chan);
+				list_del(&pcc_chan->pcc_chans);
+				kfree(cur);
+			}
+			mutex_unlock(&pcc_chan_list_lock);
+			return 0;
+		}
+	}
+
+	mutex_unlock(&pcc_chan_list_lock);
+
+	return -ENOENT;
+}
+
 /*
  * Number of MSCs that have been probed. Once all MSCs have been probed MPAM
  * can be enabled.
@@ -2208,6 +2294,8 @@ static void mpam_msc_drv_remove(struct platform_device *pdev)
 {
 	struct mpam_msc *msc = platform_get_drvdata(pdev);
 
+	mpam_pcc_chan_put(msc->pcc_chan);
+
 	mutex_lock(&mpam_list_lock);
 	mpam_msc_destroy(msc);
 	mutex_unlock(&mpam_list_lock);
@@ -2218,7 +2306,7 @@ static void mpam_msc_drv_remove(struct platform_device *pdev)
 static struct mpam_msc *do_mpam_msc_drv_probe(struct platform_device *pdev)
 {
 	int err;
-	u32 tmp;
+	u32 pcc_subspace_id;
 	struct mpam_msc *msc;
 	struct resource *msc_res;
 	struct device *dev = &pdev->dev;
@@ -2263,7 +2351,8 @@ static struct mpam_msc *do_mpam_msc_drv_probe(struct platform_device *pdev)
 	if (err)
 		return ERR_PTR(err);
 
-	if (device_property_read_u32(&pdev->dev, "pcc-channel", &tmp))
+	if (device_property_read_u32(&pdev->dev, "pcc-channel",
+				     &pcc_subspace_id))
 		msc->iface = MPAM_IFACE_MMIO;
 	else
 		msc->iface = MPAM_IFACE_PCC;
@@ -2279,6 +2368,39 @@ static struct mpam_msc *do_mpam_msc_drv_probe(struct platform_device *pdev)
 		}
 		msc->mapped_hwpage_sz = msc_res->end - msc_res->start;
 		msc->mapped_hwpage = io;
+	} else if (msc->iface == MPAM_IFACE_PCC) {
+		u32 msc_id;
+		int ret;
+
+		if (device_property_read_u32(&pdev->dev, "msc-id", &msc_id)) {
+			pr_err("missing MPAM-Fb MSC identifier\n");
+			return ERR_PTR(-EINVAL);
+		}
+		msc->mpam_fb_msc_id = msc_id;
+
+		msc->pcc_chan = mpam_pcc_chan_get(&pdev->dev, pcc_subspace_id);
+		if (IS_ERR(msc->pcc_chan)) {
+			pr_err("Failed to request MSC PCC channel\n");
+			return (void *)msc->pcc_chan;
+		}
+
+		if (msc->pcc_chan->pcc_chan->shmem_size < MPAM_FB_MAX_MSG_SIZE) {
+			pr_err("MPAM-Fb PCC channel size too small.\n");
+			mpam_pcc_chan_put(msc->pcc_chan);
+			return ERR_PTR(-ENOMEM);
+		}
+		ret = mpam_fb_get_protocol_version(msc);
+		if (ret < 0) {
+			pr_err("Cannot query MPAM-Fb protocol version.\n");
+			mpam_pcc_chan_put(msc->pcc_chan);
+			return ERR_PTR(-EIO);
+		}
+		if ((ret >> 16) != 1) {
+			pr_err("Incompatible MPAM-Fb protocol version %d.%d\n",
+			       ret >> 16, ret & 0xffff);
+			mpam_pcc_chan_put(msc->pcc_chan);
+			return ERR_PTR(-EINVAL);
+		}
 	} else {
 		return ERR_PTR(-EINVAL);
 	}
-- 
2.43.0

Re: [PATCH v3 16/16] arm_mpam: detect and enable MPAM-Fb PCC support

[Jonathan Cameron]

On Fri, 10 Jul 2026 16:45:20 +0200
Andre Przywara <andre.przywara@arm.com> wrote:

> The Arm MPAM-Fb specification [1] describes a protocol to access MSC
> registers through a firmware interface. This requires a shared memory
> region to hold the message, and a mailbox to trigger the access.
> For ACPI this is wrapped as a PCC channel, described using existing
> ACPI abstractions.
> 
> Add code to parse those PCC table descriptions associated with an MSC,
> and store the parsed information in the MSC struct.
> There can be multiple PCC channels, and each channel can serve multiple
> MSCs, so we need to keep track of the channel usage, using a list and
> a refcount.
> This will be used by the MPAM-Fb access wrapper code.
> 
> [1] https://developer.arm.com/documentation/den0144/latest
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Hi Andre,

A few things inline.

Thanks,

Jonathan

> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 220b4a06e739..7be98f13dd63 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -19,6 +19,7 @@
>  #include <linux/irqdesc.h>
>  #include <linux/list.h>
>  #include <linux/lockdep.h>
> +#include <linux/mailbox_client.h>
>  #include <linux/mutex.h>
>  #include <linux/platform_device.h>
>  #include <linux/printk.h>
> @@ -27,6 +28,9 @@
>  #include <linux/types.h>
>  #include <linux/workqueue.h>
>  
> +#include <acpi/pcc.h>
> +#include <acpi/acpi_io.h>
> +
>  #include "mpam_internal.h"
>  
>  /* Values for the T241 errata workaround */
> @@ -49,6 +53,88 @@ static LIST_HEAD(mpam_all_msc);
>  
>  struct srcu_struct mpam_srcu;
>  
> +/* PCC channels might be serving multiple MSCs, so keep a refcounted list. */
> +static DEFINE_MUTEX(pcc_chan_list_lock);
> +static LIST_HEAD(pcc_chan_list);
> +
> +static void mpam_pcc_rx_callback(struct mbox_client *cl, void *msg)
> +{
> +	/* TODO: wake up tasks blocked on this MSC's PCC channel */

That sounds like as significant todo. Does it matter for now?

> +}
> +
> +static struct mpam_pcc_chan *mpam_pcc_chan_get(struct device *dev,
> +					       int subspace_id)
> +{
> +	struct mpam_pcc_chan *cur;
> +
> +	mutex_lock(&pcc_chan_list_lock);

guard()

> +
> +	list_for_each_entry(cur, &pcc_chan_list, pcc_chans) {
> +		if (cur->subspace_id == subspace_id) {
> +			cur->refcount++;
> +			mutex_unlock(&pcc_chan_list_lock);
> +
> +			return cur;
> +		}
> +	}
> +
> +	cur = kzalloc_obj(*cur);
> +	if (!cur) {
> +		mutex_unlock(&pcc_chan_list_lock);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	cur->pcc_cl.dev = dev;
> +	cur->pcc_cl.rx_callback = mpam_pcc_rx_callback;
> +	cur->pcc_cl.tx_block = true;
> +	cur->pcc_cl.tx_tout = 1000; /* 1s */
> +
> +	cur->pcc_chan = pcc_mbox_request_channel(&cur->pcc_cl, subspace_id);
> +	if (IS_ERR(cur->pcc_chan)) {
> +		long err = PTR_ERR(cur->pcc_chan);
> +
> +		kfree(cur);
> +		mutex_unlock(&pcc_chan_list_lock);
> +		return ERR_PTR(err);
> +	}
> +
> +	mutex_init(&cur->pcc_chan_lock);
> +	cur->subspace_id = subspace_id;
> +	cur->refcount = 1;
> +
> +	list_add_tail(&cur->pcc_chans, &pcc_chan_list);
> +
> +	mutex_unlock(&pcc_chan_list_lock);
> +
> +	return cur;
> +}
> +
> +static int mpam_pcc_chan_put(struct mpam_pcc_chan *pcc_chan)
> +{
> +	struct mpam_pcc_chan *cur, *tmp;
> +
> +	if (!pcc_chan)
> +		return 0;
> +
> +	mutex_lock(&pcc_chan_list_lock);

guard()

> +
> +	list_for_each_entry_safe(cur, tmp, &pcc_chan_list, pcc_chans) {
> +		if (cur == pcc_chan) {
> +			if (!--cur->refcount) {

Maybe embed a kref_t? Then release. As much as anything acts as documentation
that this is reference counting.

> +				pcc_mbox_free_channel(cur->pcc_chan);
> +				list_del(&pcc_chan->pcc_chans);
> +				kfree(cur);
> +			}
> +			mutex_unlock(&pcc_chan_list_lock);
> +			return 0;
> +		}
> +	}
> +
> +	mutex_unlock(&pcc_chan_list_lock);
> +
> +	return -ENOENT;
> +}
> +
>  /*
>   * Number of MSCs that have been probed. Once all MSCs have been probed MPAM
>   * can be enabled.
> @@ -2208,6 +2294,8 @@ static void mpam_msc_drv_remove(struct platform_device *pdev)
>  {
>  	struct mpam_msc *msc = platform_get_drvdata(pdev);
>  
> +	mpam_pcc_chan_put(msc->pcc_chan);
> +
>  	mutex_lock(&mpam_list_lock);
>  	mpam_msc_destroy(msc);
>  	mutex_unlock(&mpam_list_lock);
> @@ -2218,7 +2306,7 @@ static void mpam_msc_drv_remove(struct platform_device *pdev)
>  static struct mpam_msc *do_mpam_msc_drv_probe(struct platform_device *pdev)
>  {
>  	int err;
> -	u32 tmp;
> +	u32 pcc_subspace_id;
>  	struct mpam_msc *msc;
>  	struct resource *msc_res;
>  	struct device *dev = &pdev->dev;
> @@ -2263,7 +2351,8 @@ static struct mpam_msc *do_mpam_msc_drv_probe(struct platform_device *pdev)
>  	if (err)
>  		return ERR_PTR(err);
>  
> -	if (device_property_read_u32(&pdev->dev, "pcc-channel", &tmp))
> +	if (device_property_read_u32(&pdev->dev, "pcc-channel",
> +				     &pcc_subspace_id))
>  		msc->iface = MPAM_IFACE_MMIO;
>  	else
>  		msc->iface = MPAM_IFACE_PCC;
> @@ -2279,6 +2368,39 @@ static struct mpam_msc *do_mpam_msc_drv_probe(struct platform_device *pdev)
>  		}
>  		msc->mapped_hwpage_sz = msc_res->end - msc_res->start;
>  		mcc->mapped_hwpage = io;
> +	} else if (msc->iface == MPAM_IFACE_PCC) {
> +		u32 msc_id;
> +		int ret;
> +
> +		if (device_property_read_u32(&pdev->dev, "msc-id", &msc_id)) {

earlier patch included of.h, should have been property.h given quite correctly
you are using the generic firmware accessors.

> +			pr_err("missing MPAM-Fb MSC identifier\n");
> +			return ERR_PTR(-EINVAL);
> +		}
> +		msc->mpam_fb_msc_id = msc_id;
> +
> +		msc->pcc_chan = mpam_pcc_chan_get(&pdev->dev, pcc_subspace_id);
> +		if (IS_ERR(msc->pcc_chan)) {
> +			pr_err("Failed to request MSC PCC channel\n");
> +			return (void *)msc->pcc_chan;

ERR_CAST()

> +		}
> +
> +		if (msc->pcc_chan->pcc_chan->shmem_size < MPAM_FB_MAX_MSG_SIZE) {
> +			pr_err("MPAM-Fb PCC channel size too small.\n");
> +			mpam_pcc_chan_put(msc->pcc_chan);
> +			return ERR_PTR(-ENOMEM);
> +		}

Blank line here as next bit is unrelated to the bit above.

> +		ret = mpam_fb_get_protocol_version(msc);
> +		if (ret < 0) {
> +			pr_err("Cannot query MPAM-Fb protocol version.\n");
> +			mpam_pcc_chan_put(msc->pcc_chan);
> +			return ERR_PTR(-EIO);
If there is a good reason to eat the error code, then add a comment. If not.
			return ERR_PTR(ret);

> +		}
> +		if ((ret >> 16) != 1) {
> +			pr_err("Incompatible MPAM-Fb protocol version %d.%d\n",
> +			       ret >> 16, ret & 0xffff);
> +			mpam_pcc_chan_put(msc->pcc_chan);
> +			return ERR_PTR(-EINVAL);
> +		}
>  	} else {
>  		return ERR_PTR(-EINVAL);
>  	}