Re: [PATCH v4 05/10] arm_mpam: propagate MSC access errors for __ris_msmon_read()

On Fri, Jul 24, 2026 at 01:17:06PM +0200, Andre Przywara wrote:
> Hi,
> 
> On 7/24/26 12:02, Sudeep Holla wrote:
> > On Thu, Jul 23, 2026 at 05:54:49PM +0200, Andre Przywara wrote:
> > > Allow the function for RIS accesses to return an error, and propagate
> > > read and write errors from the lower level up.
> > > This function is a bit special, since it can be called via an SMP
> > > helper, so the prototype must stay as void. So propagate any errors
> > > using the "err" field in the struct passed in as the sole argument to
> > > that function.
> > > 
> > 
> > Not sure if this was already discussed by build bisection regresses between
> > 4/10 and 5/10 as you change function prototype there and caller here or
> > am I missing to read it correctly ?
> 
> Does it? I build tested every patch multiple times yesterday, and just
> repeated it, and it comes back cleanly.
> 
> > > Signed-off-by: Andre Przywara <[email protected]>
> > > ---
> > >   drivers/resctrl/mpam_devices.c | 36 +++++++++++++++++++++++++---------
> > >   1 file changed, 27 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> > > index 9ee4eb4db03f..bcff53477133 100644
> > > --- a/drivers/resctrl/mpam_devices.c
> > > +++ b/drivers/resctrl/mpam_devices.c
> > > @@ -1328,6 +1328,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;
> > > @@ -1351,7 +1355,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);
> > > +	m->err = mpam_write_monsel_reg(msc, CFG_MON_SEL, mon_sel);
> > > +	if (m->err)
> > > +		return;
> > >   	switch (m->type) {
> > >   	case mpam_feat_msmon_mbwu_31counter:
> > > @@ -1371,7 +1377,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);
> > > +	m->err = read_msmon_ctl_flt_vals(m, &cur_ctl, &cur_flt);
> > > +	if (m->err)
> > > +		return;
> > > 
> > 
> > Atleast read_msmon_ctl_flt_vals() changed to return value in last patch.
> 
> Ah, but this is not a problem that breaks the build, as we formerly just
> ignored the (at this point bogus) error value, but now pass it up.

Rather than return I was thinking about added parameter to get the read value.

> But this is one occasion of "touching the same line twice", that was
> criticised before. Not nice, but the alternative would be to have one giant
> patch, which I figured to be unreadable.
> 

Ah then good. May be I was overthinking.

-- 
Regards,
Sudeep