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

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

[Andre Przywara]

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 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@arm.com>

Thanks!
Andre
> 
> Fixes: a3a94c13c442 ("arm_mpam: propagate MSC access errors for state saving function")
> Signed-off-by: Ping Li <leeonion.muyu@gmail.com>
> ---
>   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

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@arm.com>
> 
> Thanks!
> Andre
>>
>> Fixes: a3a94c13c442 ("arm_mpam: propagate MSC access errors for state saving function")
>> Signed-off-by: Ping Li <leeonion.muyu@gmail.com>
>> ---
>>   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
> 

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

[Ben Horgan]

Hi Ping,

On 20/10/2026 12: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.
> 
> Fixes: a3a94c13c442 ("arm_mpam: propagate MSC access errors for state saving function")
> Signed-off-by: Ping Li <leeonion.muyu@gmail.com>

Thanks for this. Looks like this was a result of the conflict between the MPAM-Fb series and my
minor fixes series.

Reviewed-by: Ben Horgan <ben.horgan@arm.com>

Cheers,

Ben

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

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

[Ping Li]

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.

Fixes: a3a94c13c442 ("arm_mpam: propagate MSC access errors for state saving function")
Signed-off-by: Ping Li <leeonion.muyu@gmail.com>
---
 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
-- 
2.34.1