Re: [PATCH v3 08/16] arm_mpam: propagate MSC read errors for state saving functions

[Andre Przywara <andre.przywara@arm.com>]

Hi Jonathan,

On 7/10/26 21:00, Jonathan Cameron wrote:
> On Fri, 10 Jul 2026 16:45:12 +0200
> Andre Przywara <andre.przywara@arm.com> wrote:
> 
>> Allow the mpam_save_mbwu_state() function to return an error, and
>> propagate read errors from the lower level up.
> 
> I'm not following why propagating errors is related to the
> if (val != MSMON___L_NRDY)
> as nothing in this patch is changing val.

This MSMON__L_NRDY bit is some kind of software error condition, that 
gets set in mpam_msc_read_mbwu_l() to indicate an unstable value 
condition. In the lack of an error return value this was somewhat 
shoehorned into the return value, and now we can get rid of that kludge, 
since we gained a proper error return value. At least that was my hope, 
but maybe I missed something.

Cheers,
Andre

> 
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>   drivers/resctrl/mpam_devices.c | 30 +++++++++++++++++++-----------
>>   1 file changed, 19 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index 530ac0fe97b5..b2ecaba29fcc 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
>> @@ -1782,7 +1782,7 @@ static int mpam_save_mbwu_state(void *arg)
>>   {
>>   	int i;
>>   	u64 val;
>> -	int ret = 0;
>> +	int ret;
>>   	struct mon_cfg *cfg;
>>   	u32 cur_flt, cur_ctl, mon_sel;
>>   	struct mpam_msc_ris *ris = arg;
>> @@ -1799,8 +1799,12 @@ 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);
>> +		ret = mpam_read_monsel_reg(msc, CFG_MBWU_FLT, &cur_flt);
>> +		if (ret)
>> +			goto out_unlock;
>> +		ret = mpam_read_monsel_reg(msc, CFG_MBWU_CTL, &cur_ctl);
>> +		if (ret)
>> +			goto out_unlock;
>>   		mpam_write_monsel_reg(msc, CFG_MBWU_CTL, 0);
>>   
>>   		if (mpam_ris_has_mbwu_long_counter(ris)) {
>> @@ -1811,20 +1815,24 @@ static int mpam_save_mbwu_state(void *arg)
>>   		} else {
>>   			u32 val32;
>>   
>> -			mpam_read_monsel_reg(msc, MBWU, &val32);
>> +			ret = mpam_read_monsel_reg(msc, MBWU, &val32);
>> +			if (ret)
>> +				goto out_unlock;
>> +
>>   			val = val32;
>>   			mpam_write_monsel_reg(msc, MBWU, 0);
>>   		}
>>   
>> -		cfg->mon = i;
>> -		cfg->pmg = FIELD_GET(MSMON_CFG_x_FLT_PMG, cur_flt);
>> -		cfg->match_pmg = FIELD_GET(MSMON_CFG_x_CTL_MATCH_PMG, cur_ctl);
>> -		cfg->partid = FIELD_GET(MSMON_CFG_x_FLT_PARTID, cur_flt);
>> -		mbwu_state->correction += val;
>> -		mbwu_state->enabled = FIELD_GET(MSMON_CFG_x_CTL_EN, cur_ctl);
>> +		if (val != MSMON___L_NRDY) {
> 
> with ACQUIRE() you can just do
> 		if (val == MS_MON__L_NRDY)
> 			return;
> 
> and leave the rest where it was.
> However I don't actually see what this has to do wiht the main purpose of the
> patch to propagate errors. Perhaps a bit more detail in the commit message.
> 
> Thanks,
> 
> Jonathan
> 
> 
>> +			cfg->mon = i;
>> +			cfg->pmg = FIELD_GET(MSMON_CFG_x_FLT_PMG, cur_flt);
>> +			cfg->match_pmg = FIELD_GET(MSMON_CFG_x_CTL_MATCH_PMG, cur_ctl);
>> +			cfg->partid = FIELD_GET(MSMON_CFG_x_FLT_PARTID, cur_flt);
>> +			mbwu_state->correction += val;
>> +			mbwu_state->enabled = FIELD_GET(MSMON_CFG_x_CTL_EN, cur_ctl);
>> +		}
>>   		mpam_mon_sel_unlock(msc);
>>   	}
>> -
>>   	return 0;
>>   
>>   out_unlock:
>