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

[Ben Horgan]

Hi Andre,

On 7/20/26 16:58, Andre Przywara wrote:
> 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.

What I was trying to say is that mpam_msc_read_mbwu_l() could previously return a value with bit 63,
MSMON__L_NRDY set in two cases, one set by s/w and one set by h/w. Either when it reads that
directly from the hardware or when it is set in the function to indicate an unstable value. The h/w
case is the same for 31 bit counters too except in that case the h/w sets bit 31, MSMON_NRDY. Using
'ret' to directly return -EBUSY for the s/w case where a stable value is not reached for 44 or 63
bit counters doesn't mean that the h/w case won't happen.


Thanks,

Ben

> 
> Cheers,
> Andre
> 
>>
>> Thanks,
>>
>> Ben
>>
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>>> ---
>>>   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)
>>
>