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/23/26 10:45, Andre Przywara wrote:
> Hi Ben,
> 
> On 7/20/26 18:09, Ben Horgan wrote:
>> 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.
> 
> I am not sure I see the problem, the idea of this patch was to use the opportunity of having now a
> proper return value, and to not hide that "nrdy" is actually an error flag. If I read the code
> correctly, then at the moment we flag the error early (using nrdy), but then continue with
> (potentially bogus?) "now" calculations, only to discard them towards the end of the function, to
> return an error when nrdy was set. So my idea was to just handle the error case early and return.
> Or do you mean I was just missing one case where NRDY was set?

I think the problem comes in patch 4 actually. Where you remove the checking for L_NRDY, which can
still be read from h/w. Both long and 31 bit counters would need to be considered for this kind of
cleanup.

> 
> In any case, to not jeopardise the whole series over this rather opportunistic patch, I will just
> drop any changes to nrdy handling. This makes the remaining patches easier to understand, I guess,
> since they are now more or less schematic "if (err) return err;" changes.
> 
> I think we can clean this up later if needed, in a follow up patch.

Sure.

Thanks,

Ben

> 
> Thanks,
> Andre
> 
>>>>>
>>>>> 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)
>>>>
>>>
>>
>