Re: [PATCH v4 01/10] arm_mpam: let low level MSC accessors return an error
From: Andre Przywara
Date: Tue Jul 28 2026 - 11:53:26 EST
Hi Jonathan,
On 7/28/26 00:12, Jonathan Cameron wrote:
On Thu, 23 Jul 2026 17:54:45 +0200
Andre Przywara <andre.przywara@xxxxxxx> 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@xxxxxxx>
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 probably should have made this explicit in at least the cover letter, but that was the intention: for most of the patches, the lower level functions only ever return 0, so no error path will be triggered at this point. This allows to code to grow more slowly, and remaining issues are then fixed in later patches (7/10-9/10), before only the last patch actually activates MPAM-Fb, and introduces the possibility of an error return. So there is no real functional change, it's all refactoring.
Interestingly Sashiko missed that, many complaints are about things that cannot happen before patch 10/10.
I will add a comment to that effect to the commit message and the cover letter.
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!
Thanks for the suggestion, I might try it the next time I encounter a similar problem! I briefly looked at applying it here, but already struggled to identify those outer functions. So I think this "lower level first" is still more comprehensible.
---
drivers/resctrl/mpam_devices.c | 204 +++++++++++++++++++++------------
1 file changed, 130 insertions(+), 74 deletions(-)
diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index b69f99488111..912c21ce7f9f 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
-static u64 mpam_msc_read_idr(struct mpam_msc *msc)As a whilst you are here type of comment. I'd find this more obvious if it were
+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;
+ }
} else {
idr_high = 0;
}
rather than assign a default at top of what is now a rather more complex function.
I see what you mean, makes sense.
- 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;
}
int mpam_register_requestor(u16 partid_max, u8 pmg_max)
@@ -774,13 +810,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);
If an error were to occur and "now" not get assigned, would this being
accessing undefined data?
But there will be no error at this point, as __mpam_read_reg() unconditionally still returns 0. And this missing error code check will be fixed in a later patch, before proper error returns get enabled.
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 <jonathan.cameron@xxxxxxxxxxxxxxxx>
Thanks!
Cheers,
Andre
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);