Re: [PATCH v7 06/11] arm_mpam: propagate MSC access errors for state saving function

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

On Fri, 31 Jul 2026 19:03:19 +0200
Andre Przywara <andre.przywara@arm.com> wrote:

> Allow the mpam_save_mbwu_state() function to return an error, and
> propagate read and write errors from the lower level up.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
> Reviewed-by: Ben Horgan <ben.horgan@arm.com>

If you happen to be respinning i think the code grouping vs white
space in here could be slightly improved.

> ---
>  drivers/resctrl/mpam_devices.c | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index fa8ed20a6740..38450c55e45e 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -1838,22 +1838,37 @@ 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_write_monsel_reg(msc, CFG_MON_SEL, mon_sel);
> +		if (ret)
> +			return ret;
> +		ret = mpam_read_monsel_reg(msc, CFG_MBWU_FLT, &cur_flt);
> +		if (ret)
> +			return ret;
> +		ret = mpam_read_monsel_reg(msc, CFG_MBWU_CTL, &cur_ctl);
> +		if (ret)
> +			return ret;
> +		ret = mpam_write_monsel_reg(msc, CFG_MBWU_CTL, 0);
> +		if (ret)
> +			return ret;
>  
>  		if (mpam_ris_has_mbwu_long_counter(ris)) {
>  			ret = mpam_msc_read_mbwu_l(msc, &val);
>  			if (ret)
>  				return ret;
> -			mpam_msc_zero_mbwu_l(msc);
> +			ret = mpam_msc_zero_mbwu_l(msc);
> +			if (ret)
> +				return ret;
>  		} else {
>  			u32 val32;
>  
> -			mpam_read_monsel_reg(msc, MBWU, &val32);
> +			ret = mpam_read_monsel_reg(msc, MBWU, &val32);
> +			if (ret)
> +				return ret;
> +
>  			val = val32;
> -			mpam_write_monsel_reg(msc, MBWU, 0);
> +			ret = mpam_write_monsel_reg(msc, MBWU, 0);
> +			if (ret)
> +				return ret;
The grouping via white space usage here seems less than ideal.
You have:
			ret = mpam_read_monsel_reg(msc, MBWU, &val32);
			if (ret)
				return ret;

  			val = val32; // this looks to be part of block below not above.
			ret = mpam_write_monsel_reg(msc, MBWU, 0);
			if (ret)
				return ret;

Probably:
			ret = mpam_read_monsel_reg(msc, MBWU, &val32);
			if (ret)
				return ret;
  			val = val32; // Now it is in groups with related code.

			ret = mpam_write_monsel_reg(msc, MBWU, 0);
			if (ret)
				return ret;

Or don't have a blank line in there at all.

>  		}
>  
>  		cfg->mon = i;