Re: [PATCH v3 03/16] arm_mpam: propagate MSC read errors for hw_probe functions

[Jonathan Cameron]

On Fri, 10 Jul 2026 16:45:07 +0200
Andre Przywara <andre.przywara@xxxxxxx> wrote:

> Allow the functions probing for MSC hardware and features to return an
> error, and propagate read errors from the lower level up.
> 
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
Hi Andre,

A few suggestions to use some newer kernel 'toys' to simplify this
code.

Thanks,

Jonathan

> ---
>  drivers/resctrl/mpam_devices.c | 70 ++++++++++++++++++++++++----------
>  1 file changed, 50 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 8fd2c38c821c..b1bd047da203 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -785,7 +785,7 @@ static void mpam_enable_quirks(struct mpam_msc *msc)
>  static bool mpam_ris_hw_probe_csu_nrdy(struct mpam_msc_ris *ris)
>  {
>  	u32 now, mon_sel, ctl_val;
> -	bool can_set, can_clear;
> +	bool can_set, can_clear, ret = false;

I know the surrounding code does this, but it is helpful to general readability to
not mix declarations with assignments from those that don't. I'd just
use an extra line for ret.

>  	struct mpam_msc *msc = ris->vmsc->msc;
>  
>  	if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
> @@ -804,20 +804,26 @@ static bool mpam_ris_hw_probe_csu_nrdy(struct mpam_msc_ris *ris)
>  	mpam_write_monsel_reg(msc, CFG_CSU_CTL, ctl_val);
>  
>  	_mpam_write_monsel_reg(msc, MSMON_CSU, MSMON___NRDY);
> -	_mpam_read_monsel_reg(msc, MSMON_CSU, &now);
> +	if (_mpam_read_monsel_reg(msc, MSMON_CSU, &now))
> +		goto out_unlock;
>  	can_set = now & MSMON___NRDY;
>  
>  	_mpam_write_monsel_reg(msc, MSMON_CSU, 0);
>  	/* Configuration change to try and coax hardware into setting nrdy */
>  	mpam_write_monsel_reg(msc, CFG_CSU_FLT, 0x1);
> -	_mpam_read_monsel_reg(msc, MSMON_CSU, &now);
> +	if (_mpam_read_monsel_reg(msc, MSMON_CSU, &now))
> +		goto out_unlock;
>  	can_clear = !(now & MSMON___NRDY);
> +
> +	ret = !can_set || !can_clear;

This is less readable than it could be. I'd be tempted to split good and bad paths.

Or better yet, now we have ACQUIRE() and ACQUIRE_ERR() can we use the cleanup.h
magic to do auto cleanup of mpam_mon_sel_unlock()?  Then the error paths can
become direct returns. I haven't read the rest of the mpam code for a few months, but
it might be much more generally useful given we now have a bunch more reasons
to return early.

> +
> +out_unlock:
>  	mpam_mon_sel_unlock(msc);
>  
> -	return (!can_set || !can_clear);
> +	return ret;
>  }
>  
>  
>  static int mpam_msc_hw_probe(struct mpam_msc *msc)
>  {
> +	int ret;
>  	u64 idr;
>  	u16 partid_max;
>  	u8 ris_idx, pmg_max;
> @@ -1016,10 +1040,12 @@ 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);
> -
> +	ret = mpam_msc_read_idr(msc, &idr);
> +	if (!ret)
> +		ret = mpam_read_partsel_reg(msc, IIDR, &msc->iidr);
>  	mutex_unlock(&msc->part_sel_lock);

This is a classic pattern where cleanup.h magic can give a more readable
result with all error conditions easy to spot as each can be fully
handled on its own.

	scoped_guard(mutex, &msc->part_sel_lock) {
		ret = mapm_msc_read_idr(msc, &idr);
		if (ret)
			return ret;

		ret = mpam_read_partsel_reg(msc, IIDR, &msc->iidr);
		if (ret)
			return ret;
	}

	mpam_enable_quirks(msc);
> +	if (ret)
> +		return ret;
>  
>  	mpam_enable_quirks(msc);
>