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

[Andre Przywara]

Hi,

On 7/10/26 20:56, Jonathan Cameron wrote:
> On Fri, 10 Jul 2026 16:45:11 +0200
> Andre Przywara <andre.przywara@xxxxxxx> 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@xxxxxxx>
> 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.

Is it? I see where you are coming from, and I actually had it like this 
before, but I found this combination of conditions harder to read. Also 
this is a quirk, so an exception, and I think the extra check makes this 
clearer that this is some unfortunate mishap we don't really want, but 
have to deal with.

But it's of course easy to change ...

> > +		}
> >   		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;
> >   }

So I started to put scoped_guard's and ACQUIRE() everywhere now, will 
see how this turns out.

Cheers,
Andre