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

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

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.

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