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

[Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>]

On Fri, 10 Jul 2026 16:45:10 +0200
Andre Przywara <andre.przywara@arm.com> 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 | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 041fca018e68..84a8715464be 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -1297,6 +1297,10 @@ 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;

Similar comments on ACQUIRE() making things easier here.
Note I'd introduce it as a trivial precursor patch before the rest of
this rework.


> @@ -1339,7 +1343,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;
> @@ -1364,7 +1370,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);
>  
> @@ -1385,7 +1393,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);
>  		}
> @@ -1748,6 +1758,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;
> @@ -1760,10 +1771,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;
return here.  It's consistent with some of the existing code and
general makes things more readable.

Obviously ignore any comments like this if they are about code
which will change later making the comment wrong!

> +				break;
> +			}
>  		}
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  /* Call with MSC cfg_lock held */