Re: [PATCH v3 07/16] arm_mpam: __ris_msmon_read(): get rid of nrdy special handling

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

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

> Although so far MSC accesses couldn't fail, there is one special
> condition that would create an error: when the MBWU counter wouldn't be
> able to read a stable value, we were setting bit 63 to mark this value
> as unstable, and return this as an error later.
> Now since the functions can return a proper error value, we can get rid of
> this kludge and use the return value directly.
> 
> Remove the "nrdy" error flag variable, and assign -EBUSY to "ret" to handle
> this case.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Hi Andre

I'm still fussing about code flow and style :(

Obviously none of this is that important, but it does help make
the code more maintainable in the long run.

Jonathan

> ---
>  drivers/resctrl/mpam_devices.c | 38 +++++++++++++++-------------------
>  1 file changed, 17 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 84a8715464be..530ac0fe97b5 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -1306,7 +1306,6 @@ static void __ris_msmon_read(void *arg)
>  	u64 now;
>  	int ret;
>  	u32 now32;
> -	bool nrdy = false;
>  	bool config_mismatch;
>  	bool overflow = false;
>  	struct mon_read *m = arg;
> @@ -1371,14 +1370,18 @@ static void __ris_msmon_read(void *arg)
>  	switch (m->type) {
>  	case mpam_feat_msmon_csu:
>  		ret = mpam_read_monsel_reg(msc, CSU, &now32);
> +		if (!ret) {
> +			if ((now32 & MSMON___NRDY))
> +				ret = -EBUSY;
> +
> +			if (mpam_has_quirk(IGNORE_CSU_NRDY, msc) &&
> +			    m->waited_timeout)
> +				ret = 0;
Whilst it is from existing code, this pattern of set and error then clear it
is less than ideal.  Maybe

			if ((now32 & MSMON___NRDY) &&
			    !(mpam_has_quirk(IGNORE_CS_NRDY, MSC && m->waited_timeout))
				ret = -EBUSY;

is clearer as that odd intermediate state of ret never happens.

> +		}
>  		if (ret)
>  			goto out_unlock;
> -		nrdy = now32 & MSMON___NRDY;
> -		now = FIELD_GET(MSMON___VALUE, now32);
> -
> -		if (mpam_has_quirk(IGNORE_CSU_NRDY, msc) && m->waited_timeout)
> -			nrdy = false;
>  
> +		now = FIELD_GET(MSMON___VALUE, now32);
>  		break;
>  	case mpam_feat_msmon_mbwu_31counter:
>  	case mpam_feat_msmon_mbwu_44counter:
> @@ -1394,9 +1397,11 @@ static void __ris_msmon_read(void *arg)
>  				now = FIELD_GET(MSMON___L_VALUE, now);
>  		} else {
>  			ret = mpam_read_monsel_reg(msc, MBWU, &now32);
> +			if (!ret && (now32 & MSMON___NRDY))
> +				ret = -EBUSY;
>  			if (ret)
>  				goto out_unlock;
> -			nrdy = now32 & MSMON___NRDY;
> +
>  			now = FIELD_GET(MSMON___VALUE, now32);
>  		}
>  
> @@ -1404,9 +1409,6 @@ static void __ris_msmon_read(void *arg)
>  		    m->type != mpam_feat_msmon_mbwu_63counter)
>  			now *= 64;
>  
> -		if (nrdy)
> -			break;
> -
>  		mbwu_state = &ris->mbwu_state[ctx->mon];
>  
>  		if (overflow)
> @@ -1419,22 +1421,16 @@ static void __ris_msmon_read(void *arg)
>  		now += mbwu_state->correction;
>  		break;
>  	default:
> -		m->err = -EINVAL;
> +		ret = -EINVAL;
>  	}
> -	mpam_mon_sel_unlock(msc);
> -
> -	if (nrdy)
> -		m->err = -EBUSY;
> -
> -	if (!m->err)
> -		*m->val += now;
> -
> -	return;
>  
>  out_unlock:
>  	mpam_mon_sel_unlock(msc);
>  
> -	m->err = ret;
> +	if (ret)
> +		m->err = ret;
> +	else
> +		*m->val += now;

If you do the earlier suggestion of ACQUIRE() this all get simpler, but if you do keep
this, then burn a line or two of code to make it obvious what is error and what isn't.

	if (ret) {
		m->err = ret;
		return;
	}

	*m->val += now;
>  }