[PATCH v2] coresight: configfs: Fix unload of configurations on module exit

[Suzuki K Poulose]

Hi Mike,

Thanks for fixing this. Except for a minor nit, the patch looks good to me.

On 06/06/2022 16:26, Mike Leach wrote:
> Any loaded configurations must be correctly unloaded on coresight module
> exit, or issues can arise with nested locking in the configfs directory
> code if built with CONFIG_LOCKDEP.
> 
> Prior to this patch, the preloaded configuration configfs directory entries
> were being unloaded by the recursive code in
> configfs_unregister_subsystem().
> 
> However, when built with CONFIG_LOCKDEP, this caused a nested lock warning,
> which was not mitigated by the LOCKDEP dependent code in fs/configfs/dir.c
> designed to prevent this, due to the different directory levels for the
> root of the directory being removed.
> 
> As the preloaded (and all other) configurations are registered after
> configfs_register_subsystem(), we now explicitly unload them before the
> call to configfs_unregister_subsystem().
> 
> The new routine cscfg_unload_cfgs_on_exit() iterates through the load
> owner list to unload any remaining configurations that were not unloaded
> by the user before the module exits. This covers both the
> CSCFG_OWNER_PRELOAD and CSCFG_OWNER_MODULE owner types, and will be
> extended to cover future load owner types for CoreSight configurations.
> 
> Applies to coresight/next
> 
> Fixes: eb2ec49606c2 ("coresight: syscfg: Update load API for config loadable modules")
> Reported-by: Suzuki Poulose <suzuki.poulose at arm.com>
> Signed-off-by: Mike Leach <mike.leach at linaro.org>
> ---
> 
> Changes since v1:
> Altered ordering of init of cscfg_mgr to ensure lists valid for
> potential exit path on error.
> 
> ---
>   .../hwtracing/coresight/coresight-syscfg.c    | 72 ++++++++++++++++---
>   1 file changed, 61 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> index 11850fd8c3b5..050a32f7e439 100644
> --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> @@ -1042,6 +1042,13 @@ static int cscfg_create_device(void)
>   	if (!cscfg_mgr)
>   		goto create_dev_exit_unlock;
>   
> +	/* initialise the cscfg_mgr structure */
> +	INIT_LIST_HEAD(&cscfg_mgr->csdev_desc_list);
> +	INIT_LIST_HEAD(&cscfg_mgr->feat_desc_list);
> +	INIT_LIST_HEAD(&cscfg_mgr->config_desc_list);
> +	INIT_LIST_HEAD(&cscfg_mgr->load_order_list);
> +	atomic_set(&cscfg_mgr->sys_active_cnt, 0);
> +
>   	/* setup the device */
>   	dev = cscfg_device();
>   	dev->release = cscfg_dev_release;
> @@ -1056,14 +1063,61 @@ static int cscfg_create_device(void)
>   	return err;
>   }
>   
> -static void cscfg_clear_device(void)
> +/*
> + * Loading and unloading is generally on user discretion.
> + * If exiting due to coresight module unload, we need to unload any configurations that remain,
> + * before we unregister the configfs intrastructure.
> + *
> + * Do this by walking the load_owner list and taking appropriate action, depending on the load
> + * owner type.
> + *
> + * called with the cscfg_mutex held
> + */
> +
> +#define LOADABLE_MOD_ERR "cscfg: ERROR - a loadable module failed to unload configs on exit\n"

minor nit: Could we skip this ?

> +
> +static void cscfg_unload_cfgs_on_exit(void)
>   {
> -	struct cscfg_config_desc *cfg_desc;
> +	struct cscfg_load_owner_info *owner_info = NULL;
>   
> -	mutex_lock(&cscfg_mutex);
> -	list_for_each_entry(cfg_desc, &cscfg_mgr->config_desc_list, item) {
> -		etm_perf_del_symlink_cscfg(cfg_desc);
> +	while (!list_empty(&cscfg_mgr->load_order_list)) {
> +
> +		/* remove in reverse order of loading */
> +		owner_info = list_last_entry(&cscfg_mgr->load_order_list,
> +					     struct cscfg_load_owner_info, item);
> +
> +		/* action according to type */
> +		switch (owner_info->type) {
> +		case CSCFG_OWNER_PRELOAD:
> +			/*
> +			 * preloaded  descriptors are statically allocated in
> +			 * this module - just need to unload dynamic items from
> +			 * csdev lists, and remove from configfs directories.
> +			 */
> +			pr_info("cscfg: unloading preloaded configurations\n");
> +			cscfg_unload_owned_cfgs_feats(owner_info);
> +			break;
> +
> +		case  CSCFG_OWNER_MODULE:
> +			/*
> +			 * this is an error - the loadable module must have been unloaded prior
> +			 * to the coresight module unload. Therefore that module has not
> +			 * correctly unloaded configs in its own exit code.
> +			 * Nothing to do other than emit an error string.
> +			 */
> +			pr_err(LOADABLE_MOD_ERR);

Instead :
			pr_err("cscfg: ERROR - a loadable module failed"
				" to unload configs on exit\n");

Otherwise, I can confirm that the patch fixes the reported problem.

> +			break;
> +		}
> +
> +		/* remove from load order list */
> +		list_del(&owner_info->item);
>   	}
> +}
> +
> +static void cscfg_clear_device(void)
> +{
> +	mutex_lock(&cscfg_mutex);
> +	cscfg_unload_cfgs_on_exit();
>   	cscfg_configfs_release(cscfg_mgr);
>   	device_unregister(cscfg_device());
>   	mutex_unlock(&cscfg_mutex);
> @@ -1074,20 +1128,16 @@ int __init cscfg_init(void)
>   {
>   	int err = 0;
>   
> +	/* create the device and init cscfg_mgr */
>   	err = cscfg_create_device();
>   	if (err)
>   		return err;
>   
> +	/* initialise configfs subsystem */
>   	err = cscfg_configfs_init(cscfg_mgr);
>   	if (err)
>   		goto exit_err;
>   
> -	INIT_LIST_HEAD(&cscfg_mgr->csdev_desc_list);
> -	INIT_LIST_HEAD(&cscfg_mgr->feat_desc_list);
> -	INIT_LIST_HEAD(&cscfg_mgr->config_desc_list);
> -	INIT_LIST_HEAD(&cscfg_mgr->load_order_list);
> -	atomic_set(&cscfg_mgr->sys_active_cnt, 0);
> -
>   	/* preload built-in configurations */
>   	err = cscfg_preload(THIS_MODULE);
>   	if (err)