Re: [PATCH v4 06/10] arm_mpam: propagate MSC access errors for state saving function

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 24 Jul 2026 17:43:12 +0100
Ben Horgan <ben.horgan@arm.com> wrote:

> Hi Sudeep, Andre,
> 
> On 7/24/26 13:27, Sudeep Holla wrote:
> > On Fri, Jul 24, 2026 at 01:19:14PM +0200, Andre Przywara wrote:  
> >> Hi,
> >>
> >> On 7/24/26 12:07, Sudeep Holla wrote:  
> >>> On Thu, Jul 23, 2026 at 05:54:50PM +0200, Andre Przywara wrote:  
> >>>> Allow the mpam_save_mbwu_state() function to return an error, and
> >>>> propagate read and 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, 22 insertions(+), 7 deletions(-)
> >>>>
> >>>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> >>>> index bcff53477133..6329443c451f 100644
> >>>> --- a/drivers/resctrl/mpam_devices.c
> >>>> +++ b/drivers/resctrl/mpam_devices.c
> >>>> @@ -1833,22 +1833,37 @@ 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);
> >>>> -		mpam_write_monsel_reg(msc, CFG_MBWU_CTL, 0);
> >>>> +		ret = mpam_write_monsel_reg(msc, CFG_MON_SEL, mon_sel);
> >>>> +		if (ret)
> >>>> +			return ret;
> >>>> +		ret = mpam_read_monsel_reg(msc, CFG_MBWU_FLT, &cur_flt);
> >>>> +		if (ret)
> >>>> +			return ret;
> >>>> +		ret = mpam_read_monsel_reg(msc, CFG_MBWU_CTL, &cur_ctl);
> >>>> +		if (ret)
> >>>> +			return ret;  
> >>>
> >>> How does it work in general with PCC. Now that you can fail at any point,
> >>> what happens to the write that occurs before a failed read like above one.
> >>> Who will take care of erasing those new writes or it doesn't matter ?  
> 
> At least in this particular case it doesn't matter. The MON_SEL just configures which instance of
> the monitor we are reading or writing from and we'll just write MON_SEL again next time we want to
> interact with a monitor.
> 
> >>> Just checking as I don't have much knowledge on MPAM intrinsics.  
> >>
> >> TBH I don't know, but I think we consider MPAM botched at this point, and
> >> just stop the driver, similar to an error IRQ? But I am not sure this is
> >> properly implemented at this point. The focus of these first six patches was
> >> merely to lay the dirty groundwork for *being able* to handle errors, and do
> >> this now rather than in the future.
> >>  
> 
> I've just been discussing the error handling with James and trying to work out what we can do
> without limiting our options going forward. Ideally, if there are transient errors we'd like to
> retry in the kernel if it's not taking "too long" and otherwise report the error back to user space.

Agreed a retry makes sense.  Perhaps moan a fair bit via a ratelimited print though as
it is a pretty bad sign if retries are normally needed.

> For catastrophic errors, such as when the scp doesn't reply at all or that no MPAM commands are
> going to work again we should disable MPAM. As resctrl does not anticipate errors willy nilly from
> arch code it ends up reporting the info/last_cmd_status as "ok" even in situations when the MSC
> accesses have failed. I notice this in at least rdtgroup_schemata_write(). Before we can report
> proper failure information from resctrl and update last_cmd_status based on the error status from
> the architecture code it doesn't make sense to report the errors to user space. We would likely want
> resctrl to understand -ETIMEDOUT as well. As such, this leaves us the option of just nuking MPAM on
> any error reported from the MPAM firmware interface. For now, we needn't do any retries and just
> error out on the first error. It should be possible to just schedule mpam_broken_work from mpam_fb.c
> on error.

Hmm. Inelegant but I suppose that works... 

> 
> Sorry for prompting the writing of these error handling patches but they will likely come in useful
> in the future.
> 
> Thanks,
> 
> Ben
> 
> > 
> > Well, I agree to some extent, but PCC adds that failure case before which
> > it wasn't there. So, it is hard to claim that it was botched up before so
> > let it be. I will let James/Ben to decide if it was already botched up or
> > PCC addition makes it fragile in terms of error handling.  
> 
> 
>