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 <andre.przywara at 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)
>