Re: [PATCH v4] arm_mpam: Use an atomic counter to schedule mpam_enable_once()

[Ben Horgan <ben.horgan@arm.com>]

Hi Ping,

On 15/09/2026 12:56, Ping Li wrote:
> mpam_discovery_cpu_online() scheduled mpam_enable() after each MSC
> probe, and mpam_enable() walked the whole MSC list taking each probe
> lock in turn to check if all the MSCs had been probed.
> 
> Count the MSCs that have had their hardware probed with
> mpam_num_msc_probed, and schedule mpam_enable_once() directly when the
> count reaches fw_num_msc, the number of MSCs reported by firmware.
> MSCs that were already probed no longer cause work to be scheduled.
> 
> Suggested-by: Ben Horgan <ben.horgan@arm.com>
> Signed-off-by: Ping Li <leeonion.muyu@gmail.com>
> ---
> Changes in v4:
> - Use guard(mutex) for the probe lock and continue/break to flatten
>   the nesting, per review feedback.
> - Compare the probed count against fw_num_msc instead of
>   mpam_num_msc, which is more robust against unbind/bind incrementing
>   mpam_num_msc past fw_num_msc.
> - Remove the stale comment for the removed mpam_enable() declaration.
> 
>  drivers/resctrl/mpam_devices.c  | 57 ++++++++++-----------------------
>  drivers/resctrl/mpam_internal.h |  2 --
>  2 files changed, 17 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 2f09f4b78bd3..adfb734755e6 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -49,12 +49,20 @@ static LIST_HEAD(mpam_all_msc);
>  
>  struct srcu_struct mpam_srcu;
>  
> +/* Number of MSCs reported by firmware */
> +static int fw_num_msc;
> +
>  /*
>   * Number of MSCs that have been probed. Once all MSCs have been probed MPAM
>   * can be enabled.
>   */
>  static atomic_t mpam_num_msc;
>  
> +/* Number of MSCs that have had their hardware probed */
> +static atomic_t mpam_num_msc_probed;
> +
> +static void mpam_enable_once(struct work_struct *work);
> +
>  static int mpam_cpuhp_state;
>  static DEFINE_MUTEX(mpam_cpuhp_state_lock);
>  
> @@ -72,7 +80,7 @@ static DEFINE_SPINLOCK(partid_max_lock);
>   * scheduled via this work_struct. If access to an MSC depends on a CPU that
>   * was not brought online at boot, this can happen surprisingly late.
>   */
> -static DECLARE_WORK(mpam_enable_work, &mpam_enable);
> +static DECLARE_WORK(mpam_enable_work, &mpam_enable_once);
>  
>  /*
>   * All mpam error interrupts indicate a software bug. On receipt, disable the
> @@ -1854,7 +1862,6 @@ static int mpam_discovery_cpu_online(unsigned int cpu)
>  {
>  	int err = 0;
>  	struct mpam_msc *msc;
> -	bool new_device_probed = false;
>  
>  	if (mpam_is_enabled())
>  		return 0;
> @@ -1865,18 +1872,18 @@ static int mpam_discovery_cpu_online(unsigned int cpu)
>  		if (!cpumask_test_cpu(cpu, &msc->accessibility))
>  			continue;
>  
> -		mutex_lock(&msc->probe_lock);
> -		if (!msc->probed)
> -			err = mpam_msc_hw_probe(msc);
> -		mutex_unlock(&msc->probe_lock);
> +		guard(mutex)(&msc->probe_lock);
> +		if (msc->probed)
> +			continue;
>  
> +		err = mpam_msc_hw_probe(msc);
>  		if (err)
>  			break;

Can we schedule the mpam_broken_work here to avoid having the same if condition twice?

Thanks,

Ben

> -		new_device_probed = true;
> +
> +		if (atomic_add_return(1, &mpam_num_msc_probed) == fw_num_msc)
> +			schedule_work(&mpam_enable_work);
>  	}
>  
> -	if (new_device_probed && !err)
> -		schedule_work(&mpam_enable_work);
>  	if (err) {
>  		mpam_disable_reason = "error during probing";
>  		schedule_work(&mpam_broken_work);
> @@ -2106,8 +2113,6 @@ static struct mpam_msc *do_mpam_msc_drv_probe(struct platform_device *pdev)
>  	return msc;
>  }
>  
> -static int fw_num_msc;
> -
>  static int mpam_msc_drv_probe(struct platform_device *pdev)
>  {
>  	int err;
> @@ -2713,7 +2718,7 @@ static int mpam_allocate_config(void)
>  	return 0;
>  }
>  
> -static void mpam_enable_once(void)
> +static void mpam_enable_once(struct work_struct *work)
>  {
>  	int err;
>  
> @@ -2867,34 +2872,6 @@ void mpam_disable(struct work_struct *ignored)
>  	pr_err_once("MPAM disabled due to %s\n", mpam_disable_reason);
>  }
>  
> -/*
> - * Enable mpam once all devices have been probed.
> - * Scheduled by mpam_discovery_cpu_online() once all devices have been created.
> - * Also scheduled when new devices are probed when new CPUs come online.
> - */
> -void mpam_enable(struct work_struct *work)
> -{
> -	static atomic_t once;
> -	struct mpam_msc *msc;
> -	bool all_devices_probed = true;
> -
> -	/* Have we probed all the hw devices? */
> -	guard(srcu)(&mpam_srcu);
> -	list_for_each_entry_srcu(msc, &mpam_all_msc, all_msc_list,
> -				 srcu_read_lock_held(&mpam_srcu)) {
> -		mutex_lock(&msc->probe_lock);
> -		if (!msc->probed)
> -			all_devices_probed = false;
> -		mutex_unlock(&msc->probe_lock);
> -
> -		if (!all_devices_probed)
> -			break;
> -	}
> -
> -	if (all_devices_probed && !atomic_fetch_inc(&once))
> -		mpam_enable_once();
> -}
> -
>  #define maybe_update_config(cfg, feature, newcfg, member, changes) do { \
>  	if (mpam_has_feature(feature, newcfg) &&			\
>  	    (newcfg)->member != (cfg)->member) {			\
> diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/mpam_internal.h
> index 04d1a59f02af..085f086e2b2d 100644
> --- a/drivers/resctrl/mpam_internal.h
> +++ b/drivers/resctrl/mpam_internal.h
> @@ -452,8 +452,6 @@ extern struct list_head mpam_classes;
>  extern u16 mpam_partid_max;
>  extern u8 mpam_pmg_max;
>  
> -/* Scheduled work callback to enable mpam once all MSC have been probed */
> -void mpam_enable(struct work_struct *work);
>  void mpam_disable(struct work_struct *work);
>  
>  /* Reset all the RIS in a class under cpus_read_lock() */