Re: [PATCH v4 07/15] coresight: syscfg: Use spinlock to protect active variables

[James Clark <james.clark@linaro.org>]

On 04/11/2025 3:21 pm, Leo Yan wrote:
> To support dynamically enabling and disabling the CoreSight path during
> CPU idle, cscfg_config_sysfs_get_active_cfg() may be called by the ETM
> driver in an atomic context. However, the function currently acquires a
> mutex, which is a sleepable lock, and this is not allowed in atomic
> context.
> 
> To make cscfg_config_sysfs_get_active_cfg() called in the idle flow,
> replace the mutex with a raw spinlock to protect the active
> variables.
> 
> Signed-off-by: Leo Yan <leo.yan@arm.com>

Reviewed-by: James Clark <james.clark@linaro.org>

> ---
>   drivers/hwtracing/coresight/coresight-syscfg.c | 22 +++++++++++++---------
>   drivers/hwtracing/coresight/coresight-syscfg.h |  2 ++
>   2 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> index 6836b05986e8091b4f8ad7d082d6a016f8cf7d74..53aa1a44f9ac8f65b4e3cce06a33255abfc17351 100644
> --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> @@ -957,15 +957,19 @@ int cscfg_config_sysfs_activate(struct cscfg_config_desc *config_desc, bool acti
>   
>   	cfg_hash = (unsigned long)config_desc->event_ea->var;
>   
> +	raw_spin_lock(&cscfg_mgr->spinlock);
> +
>   	if (activate) {
> +
>   		/* cannot be a current active value to activate this */
>   		if (cscfg_mgr->sysfs_active_config) {
>   			err = -EBUSY;
> -			goto exit_unlock;
> +		} else {
> +			err = _cscfg_activate_config(cfg_hash);
> +			if (!err)
> +				cscfg_mgr->sysfs_active_config = cfg_hash;
>   		}
> -		err = _cscfg_activate_config(cfg_hash);
> -		if (!err)
> -			cscfg_mgr->sysfs_active_config = cfg_hash;
> +
>   	} else {
>   		/* disable if matching current value */
>   		if (cscfg_mgr->sysfs_active_config == cfg_hash) {
> @@ -975,7 +979,8 @@ int cscfg_config_sysfs_activate(struct cscfg_config_desc *config_desc, bool acti
>   			err = -EINVAL;
>   	}
>   
> -exit_unlock:
> +	raw_spin_unlock(&cscfg_mgr->spinlock);
> +
>   	mutex_unlock(&cscfg_mutex);
>   	return err;
>   }
> @@ -983,9 +988,8 @@ int cscfg_config_sysfs_activate(struct cscfg_config_desc *config_desc, bool acti
>   /* set the sysfs preset value */
>   void cscfg_config_sysfs_set_preset(int preset)
>   {
> -	mutex_lock(&cscfg_mutex);
> +	guard(raw_spinlock)(&cscfg_mgr->spinlock);
>   	cscfg_mgr->sysfs_active_preset = preset;
> -	mutex_unlock(&cscfg_mutex);
>   }
>   
>   /*
> @@ -994,10 +998,9 @@ void cscfg_config_sysfs_set_preset(int preset)
>    */
>   void cscfg_config_sysfs_get_active_cfg(unsigned long *cfg_hash, int *preset)
>   {
> -	mutex_lock(&cscfg_mutex);
> +	guard(raw_spinlock)(&cscfg_mgr->spinlock);
>   	*preset = cscfg_mgr->sysfs_active_preset;
>   	*cfg_hash = cscfg_mgr->sysfs_active_config;
> -	mutex_unlock(&cscfg_mutex);
>   }
>   EXPORT_SYMBOL_GPL(cscfg_config_sysfs_get_active_cfg);
>   
> @@ -1201,6 +1204,7 @@ static int cscfg_create_device(void)
>   	INIT_LIST_HEAD(&cscfg_mgr->load_order_list);
>   	atomic_set(&cscfg_mgr->sys_active_cnt, 0);
>   	cscfg_mgr->load_state = CSCFG_NONE;
> +	raw_spin_lock_init(&cscfg_mgr->spinlock);
>   
>   	/* setup the device */
>   	dev = cscfg_device();
> diff --git a/drivers/hwtracing/coresight/coresight-syscfg.h b/drivers/hwtracing/coresight/coresight-syscfg.h
> index 66e2db890d8203853a0c3c907b48aa66dd8014e6..f52aba4608dadc1c7666e0e28c9b43dc1c33d1eb 100644
> --- a/drivers/hwtracing/coresight/coresight-syscfg.h
> +++ b/drivers/hwtracing/coresight/coresight-syscfg.h
> @@ -42,6 +42,7 @@ enum cscfg_load_ops {
>    * @sysfs_active_config:Active config hash used if CoreSight controlled from sysfs.
>    * @sysfs_active_preset:Active preset index used if CoreSight controlled from sysfs.
>    * @load_state:		A multi-stage load/unload operation is in progress.
> + * @spinlock:		Exclusive access sysfs_active_* variables.
>    */
>   struct cscfg_manager {
>   	struct device dev;
> @@ -54,6 +55,7 @@ struct cscfg_manager {
>   	u32 sysfs_active_config;
>   	int sysfs_active_preset;
>   	enum cscfg_load_ops load_state;
> +	raw_spinlock_t spinlock;
>   };
>   
>   /* get reference to dev in cscfg_manager */
>