Re: [PATCH] arm_mpam: Restore the cached configuration when a write to an MSC fails

[Andre Przywara <andre.przywara@arm.com>]

Hi,

On 11/4/26 11:15, Jinjie Ruan wrote:
> mpam_apply_config() caches the new configuration before writing it to
> the RISes. When mpam_touch_msc() fails, the cache keeps the new values,
> so a retry with the same schemata is skipped as a no-op and never
> reaches the hardware.

Do we really need this? Has this been observed to solve a problem?
If any of the MSC accesses fail (for MPAM-Fb only), we already tear down 
the whole MPAM driver immediately, so any subsequent accesses wouldn't 
work. So I think there is little point in trying to fix or restore any 
configuration?

Propagating errors up the call chain is another issue, but I don't think 
we need to care about any hardware settings in case of a failure.

Cheers,
Andre

> 
> Restore the cached configuration on failure so the retry writes it to
> the RISes again.
> 
> Only arm64 MPAM systems are affected; x86 does not support MPAM.
> 
> Fixes: 60e33a1864fd ("arm_mpam: propagate MSC access errors for mpam_reprogram_ris_partid()")
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> Cc: <stable@vger.kernel.org> # 7.4.x
> ---
>   drivers/resctrl/mpam_devices.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index c067b6d57d71..bb667a416660 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -3412,6 +3412,7 @@ static bool mpam_update_config(struct mpam_config *cfg,
>   int mpam_apply_config(struct mpam_component *comp, u16 partid,
>   		      struct mpam_config *cfg)
>   {
> +	struct mpam_config old_cfg = comp->cfg[partid];
>   	struct mpam_write_config_arg arg;
>   	struct mpam_msc_ris *ris;
>   	struct mpam_vmsc *vmsc;
> @@ -3439,8 +3440,10 @@ int mpam_apply_config(struct mpam_component *comp, u16 partid,
>   					 srcu_read_lock_held(&mpam_srcu)) {
>   			arg.ris = ris;
>   			ret = mpam_touch_msc(msc, __write_config, &arg);
> -			if (ret)
> +			if (ret) {
> +				comp->cfg[partid] = old_cfg;
>   				return ret;
> +			}
>   			ris->in_reset_state = false;
>   		}
>   	}