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

[Ben Horgan <ben.horgan@arm.com>]

Hi Andre,

On 7/10/26 15:45, Andre Przywara 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.

I don't think we want this patch. The h/w can still return (as much as it ever could) and so we
still need to handle it even if we are no longer augmenting its meaning in software to also indicate
an unstable 64 bit value.

Thanks,

Ben

> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  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;
> +		}
>  		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;
>  }
>  
>  static int _msmon_read(struct mpam_component *comp, struct mon_read *arg)