Re: [PATCH v3 07/16] arm_mpam: __ris_msmon_read(): get rid of nrdy special handling
From: Ben Horgan
Date: Thu Jul 23 2026 - 06:32:21 EST
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@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)
>>>>
>>>
>>
>