Re: [PATCH v4 01/10] arm_mpam: let low level MSC accessors return an error

On Thu, 23 Jul 2026 17:54:45 +0200
Andre Przywara [off-list ref] 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

that it must

always assume an MMIO based 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.
Also change the __mpam_write_reg() accessor function to return an error
code. At the moment this is always 0, but this will change with alternative
MSC access methods.

Be consistent on paragraph formatting.  So blank line here.  Check whole
series for such tiny consistency problems.

Change all direct users of those MSC wrappers to comply with the new
prototypes, though the errors are not propagated all the way up yet.

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

Some comments inline.  A few are 'whilst you are here let us make this
more readable'.  I was in two minds about the temporary lack of handling
of uninitialised (looking) variables but in the interests of expediency
I suppose we can rely on the functions never actually failing at this
point in the series.

I will note that if you did adding the returns from outer calls to inner
you wouldn't get this problem.  However you might get other problems!

-static u64 mpam_msc_read_idr(struct mpam_msc *msc)
+static int mpam_msc_read_idr(struct mpam_msc *msc, u64 *res)
 {
-	u64 idr_high = 0, idr_low;
+	u32 idr_high = 0, idr_low;
+	int ret;
 
 	lockdep_assert_held(&msc->part_sel_lock);
 
-	idr_low = mpam_read_partsel_reg(msc, IDR);
-	if (FIELD_GET(MPAMF_IDR_EXT, idr_low))
-		idr_high = mpam_read_partsel_reg(msc, IDR + 4);
+	ret = mpam_read_partsel_reg(msc, IDR, &idr_low);
+	if (ret)
+		return ret;
+
+	if (FIELD_GET(MPAMF_IDR_EXT, idr_low)) {
+		ret = mpam_read_partsel_reg(msc, IDR + 4, &idr_high);
+		if (ret)
+			return ret;
+	} 

As a whilst you are here type of comment. I'd find this more obvious if it were

	} else {
		idr_high = 0;
	}

rather than assign a default at top of what is now a rather more complex function.

 
-	return (idr_high << 32) | idr_low;
+	*res = ((u64)idr_high << 32) | idr_low;
+
+	return 0;
 }

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

Same as the other example above.  } else { esr_high = 0; }
would be slightly clearer now this whole function has gotten more complex.

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

If an error were to occur and "now" not get assigned, would this being
accessing undefined data?  If checking return values doesn't make sense
in here it should still not result in something static analysis might
get annoyed by.  This gets cleaned up in next patch so maybe that
is just about ok.  Alternative would be to just initialize any such
variables in this patch and drop those initializations in next.
Doing it just above the code that is going to change anyway would be cleanest.

	now = 0;
	_mpam_read_monsel_reg(msc, MSMON_CSU, &now);

for example.

Hmm. I suppose this is a bit of a special case because we know the
calls don't return errors at this point in the set.

So whilst I don't like it I'll not insist that you fix all the
instances of this.

Reviewed-by: Jonathan Cameron <redacted>

 	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);