[PATCH v2 07/15] arm_mpam: propagate MSC read errors for state saving functions

[Andre Przywara]

Hi,

On 7/1/26 22:19, Ben Horgan wrote:
> Hi Andre,
> 
> On 7/2/26 17:22, Andre Przywara wrote:
>> Allow the mpam_save_mbwu_state() function to return an error, and
>> propagate read errors from the lower level up.
>>
>> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
>> ---
>>   drivers/resctrl/mpam_devices.c | 47 +++++++++++++++++++++-------------
>>   1 file changed, 29 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index d18c7be86aaa..c50ca0e4f426 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
>> @@ -1773,6 +1773,7 @@ static int mpam_save_mbwu_state(void *arg)
>>   {
>>   	int i;
>>   	u64 val;
>> +	int ret;
>>   	struct mon_cfg *cfg;
>>   	u32 cur_flt, cur_ctl, mon_sel;
>>   	struct mpam_msc_ris *ris = arg;
>> @@ -1789,31 +1790,41 @@ 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_read_monsel_reg(msc, CFG_MBWU_FLT, &cur_flt);
>> +		if (!ret)
>> +			ret = mpam_read_monsel_reg(msc, CFG_MBWU_CTL, &cur_ctl);
>> +		if (!ret)
>> +			mpam_write_monsel_reg(msc, CFG_MBWU_CTL, 0);
>>   
>> -		if (mpam_ris_has_mbwu_long_counter(ris)) {
>> -			val = mpam_msc_read_mbwu_l(msc);
>> -			mpam_msc_zero_mbwu_l(msc);
>> -		} else {
>> -			u32 val32;
>> +		if (!ret) {
>> +			if (mpam_ris_has_mbwu_long_counter(ris)) {
>> +				val = mpam_msc_read_mbwu_l(msc);
>> +				mpam_msc_zero_mbwu_l(msc);
>> +			} else {
>> +				u32 val32;
>>   
>> -			mpam_read_monsel_reg(msc, MBWU, &val32);
>> -			val = val32;
>> -			mpam_write_monsel_reg(msc, MBWU, 0);
>> +				ret = mpam_read_monsel_reg(msc, MBWU, &val32);
>> +				if (!ret) {
>> +					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 (!ret && val != MSMON___L_NRDY) {
>> +			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);
>> +		if (ret)
>> +			break;
> 
> There is a lot of if(!ret) in this loop. Does it not end up cleaner to
> add an out_unlock label after the loop and jump to that in the failure
> cases?

Yeah, I changed it, I guess the diff looks better. Not entirely sure 
it's cleaner, there is now this little bitter taste of a global unlock 
where the current lock/unlock pair is *inside* the loop. Feel free to 
have a look and comment on an eventual v3 post.

Cheers,
Andre

> 
> Thanks,
> 
> Ben
> 
>>   	}
>>   
>> -	return 0;
>> +	return ret;
>>   }
>>   
>>   /*
>