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

[Andre Przywara <andre.przywara@arm.com>]

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?

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.

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)
>>>
>>
>