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

Hi Ben,

thanks for having a look!

On 7/15/26 15:39, Ben Horgan wrote:
> 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.

Mmh, not sure I understand your concern: to me it looks like nrdy is 
some kind of error flag, that we used in absence of a proper error 
return value. Now we have "int ret;", so can use that directly? But to 
me it looks like nothing really changes, or did I miss something?

I have no really strong opinion of this patch, it was more an pportunity 
to consolidate the crude error handling in this function. I am happy to 
drop it, if you like, maybe we can revisit this later.

Cheers,
Andre

> 
> Thanks,
> 
> Ben
> 
>>
>> Signed-off-by: Andre Przywara <[email protected]>
>> ---
>>   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)
>