Re: [PATCH v2 06/15] arm_mpam: propagate MSC read errors for __ris_msmon_read()

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

Hi Ben,

On 7/1/26 22:14, Ben Horgan wrote:
> Hi Andre,
> 
> On 7/2/26 17:22, Andre Przywara wrote:
>> Allow the function for RIS accesses to return an error, and propagate
>> read errors from the lower level up.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>   drivers/resctrl/mpam_devices.c | 35 +++++++++++++++++++++++++++-------
>>   1 file changed, 28 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index b259fe20a614..d18c7be86aaa 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
>> @@ -1285,9 +1285,14 @@ static u64 mpam_msmon_overflow_val(enum mpam_device_features type,
>>   	return overflow_val;
>>   }
>>   
>> +/*
>> + * This function might be called via smp_call_function_any(), so propagate
>> + * errors inside the arg struct.
>> + */
>>   static void __ris_msmon_read(void *arg)
>>   {
>>   	u64 now;
>> +	int ret;
>>   	u32 now32;
>>   	bool nrdy = false;
>>   	bool config_mismatch;
>> @@ -1326,7 +1331,9 @@ static void __ris_msmon_read(void *arg)
>>   	 * Read the existing configuration to avoid re-writing the same values.
>>   	 * This saves waiting for 'nrdy' on subsequent reads.
>>   	 */
>> -	read_msmon_ctl_flt_vals(m, &cur_ctl, &cur_flt);
>> +	ret = read_msmon_ctl_flt_vals(m, &cur_ctl, &cur_flt);
>> +	if (ret)
>> +		goto out_unlock;
>>   
>>   	if (mpam_feat_msmon_mbwu_31counter == m->type)
>>   		overflow = cur_ctl & MSMON_CFG_x_CTL_OFLOW_STATUS;
>> @@ -1351,7 +1358,9 @@ static void __ris_msmon_read(void *arg)
>>   
>>   	switch (m->type) {
>>   	case mpam_feat_msmon_csu:
>> -		mpam_read_monsel_reg(msc, CSU, &now32);
>> +		ret = mpam_read_monsel_reg(msc, CSU, &now32);
>> +		if (ret)
>> +			goto out_unlock;
>>   		nrdy = now32 & MSMON___NRDY;
>>   		now = FIELD_GET(MSMON___VALUE, now32);
>>   
>> @@ -1371,7 +1380,9 @@ static void __ris_msmon_read(void *arg)
>>   			else
>>   				now = FIELD_GET(MSMON___L_VALUE, now);
>>   		} else {
>> -			mpam_read_monsel_reg(msc, MBWU, &now32);
>> +			ret = mpam_read_monsel_reg(msc, MBWU, &now32);
>> +			if (ret)
>> +				goto out_unlock;
>>   			nrdy = now32 & MSMON___NRDY;
>>   			now = FIELD_GET(MSMON___VALUE, now32);
>>   		}
>> @@ -1402,10 +1413,15 @@ static void __ris_msmon_read(void *arg)
>>   	if (nrdy)
>>   		m->err = -EBUSY;
>>   
>> -	if (m->err)
>> -		return;
>> +	if (!m->err)
>> +		*m->val += now;
> 
> Why does this change? Can't you keep this the 'if' statement as before
> and more the 'return' after the '*m->val += now;'?

Because I need to explicitly return now anyway, since the error path 
below would just unlock again otherwise. So it would look like:

	if (m->err)
		return;

	*m->val += now;

	return;
out_unlock:
	mpam_mon_sel_unlock(msc);
...

which seemed like one return would be enough, and just conditionally do 
the assignment instead. I'd prefer to keep it like in this patch, but 
let me know if you disagree and I would change it to the one sketched above.

Cheers,
Andre

> 
> Thanks,
> 
> Ben
> 
>> +
>> +	return;
>>   
>> -	*m->val += now;
>> +out_unlock:
>> +	mpam_mon_sel_unlock(msc);
>> +
>> +	m->err = ret;
>>   }
>>   
>>   static int _msmon_read(struct mpam_component *comp, struct mon_read *arg)
>> @@ -1729,6 +1745,7 @@ static int mpam_restore_mbwu_state(void *_ris)
>>   {
>>   	int i;
>>   	u64 val;
>> +	int ret = 0;
>>   	struct mon_read mwbu_arg;
>>   	struct mpam_msc_ris *ris = _ris;
>>   	struct mpam_class *class = ris->vmsc->comp->class;
>> @@ -1741,10 +1758,14 @@ static int mpam_restore_mbwu_state(void *_ris)
>>   			mwbu_arg.val = &val;
>>   
>>   			__ris_msmon_read(&mwbu_arg);
>> +			if (mwbu_arg.err) {
>> +				ret = mwbu_arg.err;
>> +				break;
>> +			}
>>   		}
>>   	}
>>   
>> -	return 0;
>> +	return ret;
>>   }
>>   
>>   /* Call with MSC cfg_lock held */
>