Re: [PATCH v4 02/10] arm_mpam: propagate MSC access errors for hw_probe functions

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 23 Jul 2026 17:54:46 +0200
Andre Przywara <andre.przywara@arm.com> wrote:

> Allow the functions probing for MSC hardware and features to return an
> error, and propagate read and write errors from the lower level up.
> This uses some "scoped cleanup" functions like scoped_guard() and
> ACQUIRE() to avoid the complexity of error handling when a lock has been
> taken. Since the mon_sel_lock is a bit special (even more so in an
> upcoming patch), we define a new GUARD type for it.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Hi Andre

A couple of whites space comments inline.

Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

> ---
>  drivers/resctrl/mpam_devices.c  | 127 +++++++++++++++++++++-----------
>  drivers/resctrl/mpam_internal.h |   8 ++
>  2 files changed, 91 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 912c21ce7f9f..ecd45a585942 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c

>  
>  static int mpam_msc_hw_probe(struct mpam_msc *msc)
>  {
> +	int ret;
>  	u64 idr;
>  	u16 partid_max;
>  	u8 ris_idx, pmg_max;
> @@ -1021,11 +1047,14 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc)
>  	}
>  
>  	/* Grab an IDR value to find out how many RIS there are */
> -	mutex_lock(&msc->part_sel_lock);
> -	mpam_msc_read_idr(msc, &idr);
> -	mpam_read_partsel_reg(msc, IIDR, &msc->iidr);
> -
> -	mutex_unlock(&msc->part_sel_lock);
> +	scoped_guard(mutex, &msc->part_sel_lock) {
> +		ret = mpam_msc_read_idr(msc, &idr);
> +		if (ret)
> +			return ret;

Aim for formatting consistency.  I'd put a blank line here and
in similar locations to keep each read / error check block separate...

> +		ret = mpam_read_partsel_reg(msc, IIDR, &msc->iidr);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	mpam_enable_quirks(msc);
>  
> @@ -1036,10 +1065,15 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc)
>  	msc->pmg_max = FIELD_GET(MPAMF_IDR_PMG_MAX, idr);
>  
>  	for (ris_idx = 0; ris_idx <= msc->ris_max; ris_idx++) {
> -		mutex_lock(&msc->part_sel_lock);
> -		__mpam_part_sel(ris_idx, 0, msc);
> -		mpam_msc_read_idr(msc, &idr);
> -		mutex_unlock(&msc->part_sel_lock);
> +		scoped_guard(mutex, &msc->part_sel_lock) {
> +			ret = __mpam_part_sel(ris_idx, 0, msc);
> +			if (ret)
> +				return ret;
> +
Just like you have done here.
> +			ret = mpam_msc_read_idr(msc, &idr);
> +			if (ret)
> +				return ret;
> +		}
>  
> diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/mpam_internal.h
> index 04d1a59f02af..0c3f6a040b20 100644
> --- a/drivers/resctrl/mpam_internal.h
> +++ b/drivers/resctrl/mpam_internal.h
> @@ -162,6 +162,14 @@ static inline void mpam_mon_sel_lock_init(struct mpam_msc *msc)
>  	raw_spin_lock_init(&msc->_mon_sel_lock);
>  }
>  
> +DEFINE_GUARD(mon_sel,
> +	     struct mpam_msc *,
> +	     mpam_mon_sel_lock(_T),
> +	     mpam_mon_sel_unlock(_T));
Why wrap so much.  Something like:
DEFINE_GUARD(mon_sel, struct mpam_msc *,
	     mpam_mon_sel_lock(_T), mpam_mon_sel_unlock(_T));

or all on one line seems fine to me.



> +DEFINE_GUARD_COND(mon_sel, _lock,
> +		  mpam_mon_sel_lock(_T),
> +		  _RET);
> +

Similar.

>  /* Bits for mpam features bitmaps */
>  enum mpam_device_features {
>  	mpam_feat_cpor_part,