Re: [PATCH v3 02/16] arm_mpam: propagate MSC read errors for wrapper functions

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

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

> Allow the wrapper functions for IDR and ESR accesses to return an
> error, and propagate read errors from the lower level up.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Hi Andre,

Just a few superficial code style comments.

Thanks,

Jonathan

> ---
>  drivers/resctrl/mpam_devices.c | 53 ++++++++++++++++++++++++----------
>  1 file changed, 38 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index df14b4513382..8fd2c38c821c 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -247,27 +247,38 @@ static bool mpam_msc_check_aidr(struct mpam_msc *msc)
>  	return true;
>  }
>  
> -static u64 mpam_msc_read_idr(struct mpam_msc *msc)
> +static int mpam_msc_read_idr(struct mpam_msc *msc, u64 *res)
>  {
>  	u32 idr_high = 0, idr_low;
> +	int ret;
>  
>  	lockdep_assert_held(&msc->part_sel_lock);
>  
> -	mpam_read_partsel_reg(msc, IDR, &idr_low);
> +	ret = mpam_read_partsel_reg(msc, IDR, &idr_low);
> +	if (ret)
> +		return ret;
> +
>  	if (FIELD_GET(MPAMF_IDR_EXT, idr_low))
> -		mpam_read_partsel_reg(msc, IDR + 4, &idr_high);
> +		ret = mpam_read_partsel_reg(msc, IDR + 4, &idr_high);
> +	if (ret)
> +		return ret;

From a readability point of view, I'd indent the if (ret) as well
given that will then make it visually clear the check only applies
when the if is taken.

	if (FIELD_GET()) {
		ret = mpam_read_partsel_reg(msc, IDR + 4 &idr_high);
		if (ret)
			return ret;
	}

> +
> +	*res = ((u64)idr_high << 32) | idr_low;
>  
> -	return ((u64)idr_high << 32) | idr_low;
> +	return 0;
>  }
>  
> -static void mpam_msc_clear_esr(struct mpam_msc *msc)
> +static int mpam_msc_clear_esr(struct mpam_msc *msc)
>  {
>  	u32 esr_low;
> +	int ret;
>  
> -	__mpam_read_reg(msc, MPAMF_ESR, &esr_low);
> +	ret = __mpam_read_reg(msc, MPAMF_ESR, &esr_low);
> +	if (ret)
> +		return ret;
>  
>  	if (!esr_low)
> -		return;
> +		return 0;
>  
>  	/*
>  	 * Clearing the high/low bits of MPAMF_ESR can not be atomic.
> @@ -277,18 +288,30 @@ static void mpam_msc_clear_esr(struct mpam_msc *msc)
>  	 */
>  	if (msc->has_extd_esr)
>  		__mpam_write_reg(msc, MPAMF_ESR + 4, 0);
> +

Might be valid, but to me that smells like an unrelated cleanup
that shouldn't really be in a patch doing more meaningful work.  Perhaps
makes sense when you circle back to do writes.  BTW, I'm not sure
there is real benefit in separate patches doing reads from those doing writes!
Mind you I haven't read all the way through yet, so maybe I'm missing some
subtlety.

>  	__mpam_write_reg(msc, MPAMF_ESR, 0);
> +
> +	return 0;
>  }
>  
> -static u64 mpam_msc_read_esr(struct mpam_msc *msc)
> +static int mpam_msc_read_esr(struct mpam_msc *msc, u64 *res)
>  {
>  	u32 esr_high = 0, esr_low;
> +	int ret;
>  
> -	__mpam_read_reg(msc, MPAMF_ESR, &esr_low);
> -	if (msc->has_extd_esr)
> -		__mpam_read_reg(msc, MPAMF_ESR + 4, &esr_high);
> +	ret = __mpam_read_reg(msc, MPAMF_ESR, &esr_low);
> +	if (ret)
> +		return ret;
> +
> +	if (msc->has_extd_esr) {
> +		ret = __mpam_read_reg(msc, MPAMF_ESR + 4, &esr_high);
> +		if (ret)
> +			return ret;

So this is the style I suggest above. Good, but check for consistency.
It may feel like a really small thing (and it is :) but keeping code
very consistent helps a surprising amount when it comes to readability.

> +	}
> +
> +	*res = ((u64)esr_high << 32) | esr_low;
>  
> -	return ((u64)esr_high << 32) | esr_low;
> +	return 0;
>  }