Re: [PATCH v7 07/11] arm_mpam: propagate MSC access errors for mpam_reprogram_ris_partid()

[Andre Przywara]

Hi Jonathan,

On 8/4/26 00:28, Jonathan Cameron wrote:
> On Fri, 31 Jul 2026 19:03:20 +0200
> Andre Przywara <andre.przywara@xxxxxxx> wrote:
>
> > Allow the mpam_reprogram_ris_partid() function check for and return
>
> function to check
>
> > errors, and propagate MSC read and write errors from the lower level up.
> > This also covers the callers of this function: mpam_reset_ris() and
> > apply_config().
> >
> > Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> One thing below to perhaps modify if you are respinning
>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>

Thanks!

> > ---
> >   drivers/resctrl/mpam_devices.c | 136 ++++++++++++++++++++++-----------
> >   1 file changed, 93 insertions(+), 43 deletions(-)
> >
> > diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> > index 38450c55e45e..32088ad1d67e 100644
> > --- a/drivers/resctrl/mpam_devices.c
> > +++ b/drivers/resctrl/mpam_devices.c
> > @@ -1601,7 +1601,7 @@ void mpam_msmon_reset_mbwu(struct mpam_component *comp, struct mon_cfg *ctx)
> >   	}
> >   }
> >   
> > -static void mpam_reset_msc_bitmap(struct mpam_msc *msc, u16 reg, u16 wd)
> > +static int mpam_reset_msc_bitmap(struct mpam_msc *msc, u16 reg, u16 wd)
> >   {
> >   	u32 num_words, msb;
> >   	u32 bm = ~0;
> > @@ -1610,15 +1610,19 @@ static void mpam_reset_msc_bitmap(struct mpam_msc *msc, u16 reg, u16 wd)
> >   	lockdep_assert_held(&msc->part_sel_lock);
> >   
> >   	if (wd == 0)
> > -		return;
> > +		return 0;
> >   
> >   	/*
> >   	 * Write all ~0 to all but the last 32bit-word, which may
> >   	 * have fewer bits...
> >   	 */
> >   	num_words = DIV_ROUND_UP(wd, 32);
> > -	for (i = 0; i < num_words - 1; i++, reg += sizeof(bm))
> > -		__mpam_write_reg(msc, reg, bm);
> > +	for (i = 0; i < num_words - 1; i++, reg += sizeof(bm)) {
>
> Not really related to what you are doing here, but if you like you could
> reduce the scope of i via
>
> 	for (int i = 0;...
>
> as it isn't used outside of this loop.

Yeah, true, but I'd rather keep it, otherwise I might get comments about 
unrelated changes ;-)

> > +		int ret = __mpam_write_reg(msc, reg, bm);
> > +
> > +		if (ret)
> > +			return ret;
> Personally I don't like this style but feel free to ignore as this is

I know what you mean, I am always in conflict about the on-the-spot 
initialisation and the empty line requirement, tearing apart the error 
check here. Changed to what you suggested.

Thanks,
Andre.


> very much a personal taste thing. I'd do
> 		int ret;
>
> 		ret = __mpam_write_reg(msc, reg, bm);
> 		if (ret)
> 			return ret;
> 	}
> just to keep the source of error and check for error next to each other.
>
> > +	}