Re: [PATCH] arm_mpam: Release the mon_sel lock on mpam_save_mbwu_state() errors

[Ben Horgan]

Hi Andre,

On 21/10/2026 10:26, Andre Przywara wrote:
> Hi,
> 
> On 10/20/26 13:48, Ping Li wrote:
>> mpam_save_mbwu_state() takes the mon_sel lock at the top of each loop
>> iteration, but the four error paths that return after failed
>> MPAMF_MON_SEL register accesses do so without releasing it.
>>
>> Take the lock with the conditional mon_sel_lock guard class instead,
>> so all exit paths release it.
> 
> many thanks for spotting this!
> 
> Similar lines were in the last patch version sent out, but as Ben mentioned, this somehow conflicted
> with the other MPAM changes, so it got accidentally removed.
> The original version always returned -EIO, but we now always use this error code inside mpam_fb.c

For consistency we should always return -EIO on the locking failure as that's what's done in all the
non-AQUIRE() cases. However, I do think this fix is ok as it is and that at some point down the line
we can update the conditional guard to enforce this.

DEFINE_CLASS(mon_sel_lock, struct mpam_msc *,
	     if (_T && !IS_ERR(_T)) mpam_mon_sel_unlock(_T),
	     mpam_mon_sel_lock(msc) ? msc : ERR_PTR(-EIO), struct mpam_msc *msc);
DEFINE_CLASS_IS_COND_GUARD(mon_sel_lock);

Thanks,

Ben



> already, so there is no difference, and this version here is more future-proof, should we lift the
> restricted range of errors handled in the future. So:
> 
> Reviewed-by: Andre Przywara <andre.przywara@xxxxxxx>
> 
> Thanks!
> Andre
>>
>> Fixes: a3a94c13c442 ("arm_mpam: propagate MSC access errors for state saving function")
>> Signed-off-by: Ping Li <leeonion.muyu@xxxxxxxxx>
>> ---
>>   drivers/resctrl/mpam_devices.c | 12 +++++-------
>>   1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index 48adc49b833c..c067b6d57d71 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
>> @@ -2037,8 +2037,10 @@ static int mpam_save_mbwu_state(void *arg)
>>       struct mpam_class *class = ris->vmsc->comp->class;
>>         for (i = 0; i < ris->props.num_mbwu_mon; i++) {
>> -        if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
>> -            return -EIO;
>> +        ACQUIRE(mon_sel_lock, guard)(msc);
>> +        ret = ACQUIRE_ERR(mon_sel_lock, &guard);
>> +        if (ret)
>> +            return ret;
>>             mbwu_state = &ris->mbwu_state[i];
>>           cfg = &mbwu_state->cfg;
>> @@ -2061,10 +2063,8 @@ static int mpam_save_mbwu_state(void *arg)
>>           cfg->partid = FIELD_GET(MSMON_CFG_x_FLT_PARTID, cur_flt);
>>           mbwu_state->enabled = FIELD_GET(MSMON_CFG_x_CTL_EN, cur_ctl);
>>   -        if (!mbwu_state->enabled) {
>> -            mpam_mon_sel_unlock(msc);
>> +        if (!mbwu_state->enabled)
>>               continue;
>> -        }
>>             val = 0;
>>           mbwu_arg = (struct mon_read) {
>> @@ -2088,8 +2088,6 @@ static int mpam_save_mbwu_state(void *arg)
>>           ret = mpam_write_monsel_reg(msc, CFG_MBWU_CTL, 0);
>>           if (ret)
>>               return ret;
>> -
>> -        mpam_mon_sel_unlock(msc);
>>       }
>>         return 0;
>>
>> base-commit: 8ea457f746d915d55f78b43d33b1cd618f3d93ad
>