Re: [PATCH v5 03/10] arm_mpam: propagate MSC access errors for MBWU counters

[Ben Horgan]

Hi Lee,

On 7/30/26 02:33, Lee Trager wrote:
> On 7/29/26 6:41 AM, Andre Przywara wrote:
> 
>> @@ -1767,6 +1781,7 @@ static int mpam_save_mbwu_state(void *arg)
>>   {
>>       int i;
>>       u64 val;
>> +    int ret;
>>       struct mon_cfg *cfg;
>>       u32 cur_flt, cur_ctl, mon_sel;
>>       struct mpam_msc_ris *ris = arg;
>> @@ -1777,7 +1792,8 @@ static int mpam_save_mbwu_state(void *arg)
>>           mbwu_state = &ris->mbwu_state[i];
>>           cfg = &mbwu_state->cfg;
>>   -        if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
>> +        ACQUIRE(mon_sel_lock, guard)(msc);
>> +        if (ACQUIRE_ERR(mon_sel_lock, &guard))
>>               return -EIO;
>>             mon_sel = FIELD_PREP(MSMON_CFG_MON_SEL_MON_SEL, i) |
>> @@ -1788,7 +1804,9 @@ static int mpam_save_mbwu_state(void *arg)
>>           mpam_write_monsel_reg(msc, CFG_MBWU_CTL, 0);
>>             if (mpam_ris_has_mbwu_long_counter(ris)) {
>> -            val = mpam_msc_read_mbwu_l(msc);
>> +            ret = mpam_msc_read_mbwu_l(msc, &val);
>> +            if (ret)
>> +                return ret;
>>               mpam_msc_zero_mbwu_l(msc);
>>           } else {
>>               u32 val32;
> I think checking for (val & MSMON____L_NRDRY) is still required before updating the saved state.
> mpam_msc_read_mbw_l() can return 0 while setting val to MSMON___L_NRDY when it cannot obtain a
> stable value. The state saving path later adds val to mbwu_state->correction, so the sentinel would
> corrupt the saved correction. Additionally, as Ben noted during the v3 review, hardware may also set
> the NRDY bit.

This is an existing issue which I address in this patch:
https://lore.kernel.org/linux-arm-kernel/20260710115546.29644-7-ben.horgan@xxxxxxx/

Please can you check and see if that looks good to you.

Thanks,

Ben

>> @@ -1804,7 +1822,6 @@ static int mpam_save_mbwu_state(void *arg)
>>           cfg->partid = FIELD_GET(MSMON_CFG_x_FLT_PARTID, cur_flt);
>>           mbwu_state->correction += val;
>>           mbwu_state->enabled = FIELD_GET(MSMON_CFG_x_CTL_EN, cur_ctl);
>> -        mpam_mon_sel_unlock(msc);
>>       }
>>         return 0;