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