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

Hi,

On 7/27/26 23:51, Jonathan Cameron wrote:
> On Fri, 24 Jul 2026 17:43:12 +0100
> Ben Horgan <[email protected]> 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 <[email protected]>
>>>>>> ---
>>>>>>   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.

Just to roll back on that a little bit. We will need the error handling patches even if we are
scheduling mpam_broken. This is so that we can also return the error code up to resctrl. As calling
mpam_broken will mean last_cmd_status is no longer present there is no file to read incorrect status
from. (Andre, this is just to summarise what we discussed off list.)

Thanks,

Ben

>>
>> 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.  
>>
>>
>>
>