[PATCH v3 0/2] Fix lockdep issues seen in CoreSight configfs interface

[PATCH v3 0/2] Fix lockdep issues seen in CoreSight configfs interface

[Mike Leach]

Issues with lockdep possible deadlock scenarios have been reported when using the
coresight configfs interface handling complex configuration, when unloading modules.

These are due to holding the main configuration mutex during configfs register, but
taking it later when accessing configfs files.

The patches improve the clean up of configurations and update load and unload of
configurations and initialisation of configfs to fix the locking mechanisms.

Applies to coresight/next (5.19-rc2)
Tested on DB410c (with patch [0] also applied to fix separate console boot issue). 

Fixes: eb2ec49606c2 ("coresight: syscfg: Update load API for config loadable modules")
Reported-by: Suzuki Poulose <suzuki.poulose@arm.com>
Signed-off-by: Mike Leach <mike.leach@linaro.org>

[0] https://lkml.kernel.org/r/20220614124618.2830569-1-suzuki.poulose@arm.com

Changes since v2:
Added additional work to fix load and unload ops after issue recurred
due to file access.

Mike Leach (2):
  coresight: configfs: Fix unload of configurations on module exit
  coresight: syscfg: Update load and unload operations

 .../hwtracing/coresight/coresight-config.h    |   2 +
 .../hwtracing/coresight/coresight-syscfg.c    | 297 +++++++++++++++---
 .../hwtracing/coresight/coresight-syscfg.h    |  13 +
 3 files changed, 263 insertions(+), 49 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 1/2] coresight: configfs: Fix unload of configurations on module exit

[Mike Leach]

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@arm.com>
Signed-off-by: Mike Leach <mike.leach@linaro.org>
---
 .../hwtracing/coresight/coresight-syscfg.c    | 106 ++++++++++++++++--
 1 file changed, 95 insertions(+), 11 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
index 11850fd8c3b5..9cd7d3c91d8e 100644
--- a/drivers/hwtracing/coresight/coresight-syscfg.c
+++ b/drivers/hwtracing/coresight/coresight-syscfg.c
@@ -414,6 +414,27 @@ static void cscfg_remove_owned_csdev_features(struct coresight_device *csdev, vo
 	}
 }
 
+/*
+ * Unregister all configuration and features from configfs owned by load_owner.
+ * Although this is called without the list mutex being held, it is in the
+ * context of an unload operation which are strictly serialised,
+ * so the lists cannot change during this call.
+ */
+static void cscfg_fs_unregister_cfgs_feats(void *load_owner)
+{
+	struct cscfg_config_desc *config_desc;
+	struct cscfg_feature_desc *feat_desc;
+
+	list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) {
+		if (config_desc->load_owner == load_owner)
+			cscfg_configfs_del_config(config_desc);
+	}
+	list_for_each_entry(feat_desc, &cscfg_mgr->feat_desc_list, item) {
+		if (feat_desc->load_owner == load_owner)
+			cscfg_configfs_del_feature(feat_desc);
+	}
+}
+
 /*
  * removal is relatively easy - just remove from all lists, anything that
  * matches the owner. Memory for the descriptors will be managed by the owner,
@@ -1022,10 +1043,13 @@ struct device *cscfg_device(void)
 /* Must have a release function or the kernel will complain on module unload */
 static void cscfg_dev_release(struct device *dev)
 {
+	mutex_lock(&cscfg_mutex);
 	kfree(cscfg_mgr);
 	cscfg_mgr = NULL;
+	mutex_unlock(&cscfg_mutex);
 }
 
+
 /* a device is needed to "own" some kernel elements such as sysfs entries.  */
 static int cscfg_create_device(void)
 {
@@ -1042,6 +1066,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,17 +1087,74 @@ 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.
+ */
+static void cscfg_unload_cfgs_on_exit(void)
 {
-	struct cscfg_config_desc *cfg_desc;
+	struct cscfg_load_owner_info *owner_info = NULL;
 
+	/*
+	 * grab the mutex - even though we are exiting, some configfs files
+	 * may still be live till we dump them, so ensure list data is
+	 * protected from a race condition.
+	 */
 	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");
+			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 as the static descriptor
+			 * references we need to unload will have disappeared with the module.
+			 */
+			pr_err("cscfg: ERROR - a loadable module failed to "
+			       "unload configs on exit\n");
+			goto list_remove;
+		}
+
+		/* remove from configfs - outside the scope of the list mutex */
+		mutex_unlock(&cscfg_mutex);
+		cscfg_fs_unregister_cfgs_feats(owner_info);
+		mutex_lock(&cscfg_mutex);
+
+		/* Next unload from csdev lists. */
+		cscfg_unload_owned_cfgs_feats(owner_info);
+
+list_remove:
+		/* remove from load order list */
+		list_del(&owner_info->item);
 	}
+	mutex_unlock(&cscfg_mutex);
+}
+
+static void cscfg_clear_device(void)
+{
+	cscfg_unload_cfgs_on_exit();
 	cscfg_configfs_release(cscfg_mgr);
 	device_unregister(cscfg_device());
-	mutex_unlock(&cscfg_mutex);
 }
 
 /* Initialise system config management API device  */
@@ -1074,20 +1162,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)
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 2/2] coresight: syscfg: Update load and unload operations

[Mike Leach]

The configfs system is a source of access to the config information in the
configuration and feature lists.

This can result in additional LOCKDEP issues as a result of the mutex
ordering between the config list mutex (cscfg_mutex) and the configfs
system mutexes.

As such we need to adjust how load/unload operations work to ensure correct
operation.

1) Previously the cscfg_mutex was held throughout the load/unload
operation. This is now only held during configuration list manipulations,
resulting in a multi-stage load/unload process.

2) All operations that manipulate the configfs representation of the
configurations and features are now separated out and run without the
cscfg_mutex being held. This avoids circular lock_dep issue with the
built-in configfs mutexes and semaphores

3) As the load and unload is now multi-stage, some parts under the
cscfg_mutex and others not:
i) A flag indicating a load / unload operation in progress is used to
serialise load / unload operations.
ii) activating any configuration not possible when unload is in progress.
iii) Configurations have an "available" flag set only after the last load
stage for the configuration is complete. Activation of the configuration
not possible till flag is set.

4) Following load/unload rules remain:
i) Unload prevented while any configuration is active remains.
ii) Unload in strict reverse order of load.
iii) Existing configurations can be activated while a new load operation
is underway. (by definition there can be no dependencies between an
existing configuration and a new loading one due to ii) above.)

Signed-off-by: Mike Leach <mike.leach@linaro.org>
---
 .../hwtracing/coresight/coresight-config.h    |   2 +
 .../hwtracing/coresight/coresight-syscfg.c    | 191 ++++++++++++++----
 .../hwtracing/coresight/coresight-syscfg.h    |  13 ++
 3 files changed, 168 insertions(+), 38 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h
index 2e1670523461..6ba013975741 100644
--- a/drivers/hwtracing/coresight/coresight-config.h
+++ b/drivers/hwtracing/coresight/coresight-config.h
@@ -134,6 +134,7 @@ struct cscfg_feature_desc {
  * @active_cnt:		ref count for activate on this configuration.
  * @load_owner:		handle to load owner for dynamic load and unload of configs.
  * @fs_group:		reference to configfs group for dynamic unload.
+ * @available:		config can be activated - multi-stage load sets true on completion.
  */
 struct cscfg_config_desc {
 	const char *name;
@@ -148,6 +149,7 @@ struct cscfg_config_desc {
 	atomic_t active_cnt;
 	void *load_owner;
 	struct config_group *fs_group;
+	bool available;
 };
 
 /**
diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
index 9cd7d3c91d8e..c2364098cabb 100644
--- a/drivers/hwtracing/coresight/coresight-syscfg.c
+++ b/drivers/hwtracing/coresight/coresight-syscfg.c
@@ -460,7 +460,6 @@ static void cscfg_unload_owned_cfgs_feats(void *load_owner)
 	/* remove from the config descriptor lists */
 	list_for_each_entry_safe(config_desc, cfg_tmp, &cscfg_mgr->config_desc_list, item) {
 		if (config_desc->load_owner == load_owner) {
-			cscfg_configfs_del_config(config_desc);
 			etm_perf_del_symlink_cscfg(config_desc);
 			list_del(&config_desc->item);
 		}
@@ -469,49 +468,28 @@ static void cscfg_unload_owned_cfgs_feats(void *load_owner)
 	/* remove from the feature descriptor lists */
 	list_for_each_entry_safe(feat_desc, feat_tmp, &cscfg_mgr->feat_desc_list, item) {
 		if (feat_desc->load_owner == load_owner) {
-			cscfg_configfs_del_feature(feat_desc);
 			list_del(&feat_desc->item);
 		}
 	}
 }
 
-/**
- * cscfg_load_config_sets - API function to load feature and config sets.
- *
- * Take a 0 terminated array of feature descriptors and/or configuration
- * descriptors and load into the system.
- * Features are loaded first to ensure configuration dependencies can be met.
- *
- * To facilitate dynamic loading and unloading, features and configurations
- * have a "load_owner", to allow later unload by the same owner. An owner may
- * be a loadable module or configuration dynamically created via configfs.
- * As later loaded configurations can use earlier loaded features, creating load
- * dependencies, a load order list is maintained. Unload is strictly in the
- * reverse order to load.
- *
- * @config_descs: 0 terminated array of configuration descriptors.
- * @feat_descs:   0 terminated array of feature descriptors.
- * @owner_info:	  Information on the owner of this set.
+/*
+ * load the features and configs to the lists - called with list mutex held
  */
-int cscfg_load_config_sets(struct cscfg_config_desc **config_descs,
-			   struct cscfg_feature_desc **feat_descs,
-			   struct cscfg_load_owner_info *owner_info)
+static int cscfg_load_owned_cfgs_feats(struct cscfg_config_desc **config_descs,
+				       struct cscfg_feature_desc **feat_descs,
+				       struct cscfg_load_owner_info *owner_info)
 {
-	int err = 0, i = 0;
-
-	mutex_lock(&cscfg_mutex);
+	int i = 0, err = 0;
 
 	/* load features first */
 	if (feat_descs) {
 		while (feat_descs[i]) {
 			err = cscfg_load_feat(feat_descs[i]);
-			if (!err)
-				err = cscfg_configfs_add_feature(feat_descs[i]);
 			if (err) {
 				pr_err("coresight-syscfg: Failed to load feature %s\n",
 				       feat_descs[i]->name);
-				cscfg_unload_owned_cfgs_feats(owner_info);
-				goto exit_unlock;
+				goto exit_err;
 			}
 			feat_descs[i]->load_owner = owner_info;
 			i++;
@@ -523,31 +501,143 @@ int cscfg_load_config_sets(struct cscfg_config_desc **config_descs,
 	if (config_descs) {
 		while (config_descs[i]) {
 			err = cscfg_load_config(config_descs[i]);
-			if (!err)
-				err = cscfg_configfs_add_config(config_descs[i]);
 			if (err) {
 				pr_err("coresight-syscfg: Failed to load configuration %s\n",
 				       config_descs[i]->name);
-				cscfg_unload_owned_cfgs_feats(owner_info);
-				goto exit_unlock;
+				goto exit_err;
 			}
 			config_descs[i]->load_owner = owner_info;
+			config_descs[i]->available = false;
+			i++;
+		}
+	}
+exit_err:
+	return err;
+}
+
+/* set configurations as available to activate at the end of the load process */
+static void cscfg_set_configs_available(struct cscfg_config_desc **config_descs)
+{
+	int i = 0;
+
+	if (config_descs) {
+		while (config_descs[i])	{
+			config_descs[i]->available = true;
 			i++;
 		}
 	}
+}
+
+/*
+ * Create and register each of the configurations and features with configfs.
+ * Called without mutex being held.
+ */
+static int cscfg_fs_register_cfgs_feats(struct cscfg_config_desc **config_descs,
+					struct cscfg_feature_desc **feat_descs)
+{
+	int i = 0, err = 0;
+
+	if (feat_descs) {
+		while (feat_descs[i]) {
+			err = cscfg_configfs_add_feature(feat_descs[i]);
+			if (err)
+				goto exit_err;
+			i++;
+		}
+	}
+	i = 0;
+	if (config_descs) {
+		while (config_descs[i]) {
+			err = cscfg_configfs_add_config(config_descs[i]);
+			if (err)
+				goto exit_err;
+			i++;
+		}
+	}
+exit_err:
+	return err;
+}
+
+/**
+ * cscfg_load_config_sets - API function to load feature and config sets.
+ *
+ * Take a 0 terminated array of feature descriptors and/or configuration
+ * descriptors and load into the system.
+ * Features are loaded first to ensure configuration dependencies can be met.
+ *
+ * To facilitate dynamic loading and unloading, features and configurations
+ * have a "load_owner", to allow later unload by the same owner. An owner may
+ * be a loadable module or configuration dynamically created via configfs.
+ * As later loaded configurations can use earlier loaded features, creating load
+ * dependencies, a load order list is maintained. Unload is strictly in the
+ * reverse order to load.
+ *
+ * @config_descs: 0 terminated array of configuration descriptors.
+ * @feat_descs:   0 terminated array of feature descriptors.
+ * @owner_info:	  Information on the owner of this set.
+ */
+int cscfg_load_config_sets(struct cscfg_config_desc **config_descs,
+			   struct cscfg_feature_desc **feat_descs,
+			   struct cscfg_load_owner_info *owner_info)
+{
+	int err = 0;
+
+	mutex_lock(&cscfg_mutex);
+	if (cscfg_mgr->load_op != CSCFG_LOAD_OP_NONE) {
+		mutex_unlock(&cscfg_mutex);
+		return -EBUSY;
+	}
+	cscfg_mgr->load_op = CSCFG_LOAD_OP_LOAD;
+
+	/* first load and add to the lists */
+	err = cscfg_load_owned_cfgs_feats(config_descs, feat_descs, owner_info);
+	if (err)
+		goto err_clean_load;
 
 	/* add the load owner to the load order list */
 	list_add_tail(&owner_info->item, &cscfg_mgr->load_order_list);
 	if (!list_is_singular(&cscfg_mgr->load_order_list)) {
 		/* lock previous item in load order list */
 		err = cscfg_owner_get(list_prev_entry(owner_info, item));
-		if (err) {
-			cscfg_unload_owned_cfgs_feats(owner_info);
-			list_del(&owner_info->item);
-		}
+		if (err)
+			goto err_clean_owner_list;
 	}
 
+	/*
+	 * make visible to configfs - configfs manipulation must occur outside
+	 * the list mutex lock to avoid circular lockdep issues with configfs
+	 * built in mutexes and semaphores. This is safe as it is not possible
+	 * to start a new load/unload operation till the current one is done.
+	 */
+	mutex_unlock(&cscfg_mutex);
+
+	/* create the configfs elements */
+	err = cscfg_fs_register_cfgs_feats(config_descs, feat_descs);
+	mutex_lock(&cscfg_mutex);
+
+	if (err)
+		goto err_clean_cfs;
+
+	/* mark any new configs as available for activation */
+	cscfg_set_configs_available(config_descs);
+	goto exit_unlock;
+
+
+err_clean_cfs:
+	/* cleanup after error registering with configfs */
+	cscfg_fs_unregister_cfgs_feats(owner_info);
+
+	if (!list_is_singular(&cscfg_mgr->load_order_list))
+		cscfg_owner_put(list_prev_entry(owner_info, item));
+
+err_clean_owner_list:
+	list_del(&owner_info->item);
+
+err_clean_load:
+	cscfg_unload_owned_cfgs_feats(owner_info);
+
 exit_unlock:
+	cscfg_mgr->load_op = CSCFG_LOAD_OP_NONE;
 	mutex_unlock(&cscfg_mutex);
 	return err;
 }
@@ -564,6 +654,9 @@ EXPORT_SYMBOL_GPL(cscfg_load_config_sets);
  * 1) no configurations are active.
  * 2) the set being unloaded was the last to be loaded to maintain dependencies.
  *
+ * Once the unload operation commences, we disallow any configuration being
+ * made active until it is complete.
+ *
  * @owner_info:	Information on owner for set being unloaded.
  */
 int cscfg_unload_config_sets(struct cscfg_load_owner_info *owner_info)
@@ -572,6 +665,13 @@ int cscfg_unload_config_sets(struct cscfg_load_owner_info *owner_info)
 	struct cscfg_load_owner_info *load_list_item = NULL;
 
 	mutex_lock(&cscfg_mutex);
+	if (cscfg_mgr->load_op != CSCFG_LOAD_OP_NONE) {
+		mutex_unlock(&cscfg_mutex);
+		return -EBUSY;
+	}
+
+	/* unload op in progress also prevents activation of any config */
+	cscfg_mgr->load_op = CSCFG_LOAD_OP_UNLOAD;
 
 	/* cannot unload if anything is active */
 	if (atomic_read(&cscfg_mgr->sys_active_cnt)) {
@@ -592,7 +692,12 @@ int cscfg_unload_config_sets(struct cscfg_load_owner_info *owner_info)
 		goto exit_unlock;
 	}
 
-	/* unload all belonging to load_owner */
+	/* remove from configfs - again outside the scope of the list mutex */
+	mutex_unlock(&cscfg_mutex);
+	cscfg_fs_unregister_cfgs_feats(owner_info);
+	mutex_lock(&cscfg_mutex);
+
+	/* unload everything from lists belonging to load_owner */
 	cscfg_unload_owned_cfgs_feats(owner_info);
 
 	/* remove from load order list */
@@ -603,7 +708,9 @@ int cscfg_unload_config_sets(struct cscfg_load_owner_info *owner_info)
 	list_del(&owner_info->item);
 
 exit_unlock:
+	cscfg_mgr->load_op = CSCFG_LOAD_OP_NONE;
 	mutex_unlock(&cscfg_mutex);
+
 	return err;
 }
 EXPORT_SYMBOL_GPL(cscfg_unload_config_sets);
@@ -780,8 +887,15 @@ static int _cscfg_activate_config(unsigned long cfg_hash)
 	struct cscfg_config_desc *config_desc;
 	int err = -EINVAL;
 
+	if (cscfg_mgr->load_op == CSCFG_LOAD_OP_UNLOAD)
+		return -EBUSY;
+
 	list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) {
 		if ((unsigned long)config_desc->event_ea->var == cfg_hash) {
+			/* if we happen upon a partly loaded config, can't use it */
+			if (config_desc->available == false)
+				return -EBUSY;
+
 			/* must ensure that config cannot be unloaded in use */
 			err = cscfg_owner_get(config_desc->load_owner);
 			if (err)
@@ -1072,6 +1186,7 @@ static int cscfg_create_device(void)
 	INIT_LIST_HEAD(&cscfg_mgr->config_desc_list);
 	INIT_LIST_HEAD(&cscfg_mgr->load_order_list);
 	atomic_set(&cscfg_mgr->sys_active_cnt, 0);
+	cscfg_mgr->load_op = CSCFG_LOAD_OP_NONE;
 
 	/* setup the device */
 	dev = cscfg_device();
diff --git a/drivers/hwtracing/coresight/coresight-syscfg.h b/drivers/hwtracing/coresight/coresight-syscfg.h
index 9106ffab4833..e8e812ce7a8b 100644
--- a/drivers/hwtracing/coresight/coresight-syscfg.h
+++ b/drivers/hwtracing/coresight/coresight-syscfg.h
@@ -12,6 +12,17 @@
 
 #include "coresight-config.h"
 
+/*
+ * Load operation types.
+ * When loading or unloading, another load operation cannot be run.
+ * When unloading configurations cannot be activated.
+ */
+enum cscfg_load_ops {
+	CSCFG_LOAD_OP_NONE,
+	CSCFG_LOAD_OP_LOAD,
+	CSCFG_LOAD_OP_UNLOAD
+};
+
 /**
  * System configuration manager device.
  *
@@ -30,6 +41,7 @@
  * @cfgfs_subsys:	configfs subsystem used to manage configurations.
  * @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_op:		A multi-stage load/unload operation is in progress.
  */
 struct cscfg_manager {
 	struct device dev;
@@ -41,6 +53,7 @@ struct cscfg_manager {
 	struct configfs_subsystem cfgfs_subsys;
 	u32 sysfs_active_config;
 	int sysfs_active_preset;
+	enum cscfg_load_ops load_op;
 };
 
 /* get reference to dev in cscfg_manager */
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/2] coresight: syscfg: Update load and unload operations

[Suzuki K Poulose]

Hi Mike

Thanks for the updated patch. Please find my comments inline.

On 17/06/2022 17:40, Mike Leach wrote:
> The configfs system is a source of access to the config information in the
> configuration and feature lists.
> 
> This can result in additional LOCKDEP issues as a result of the mutex
> ordering between the config list mutex (cscfg_mutex) and the configfs
> system mutexes.
> 
> As such we need to adjust how load/unload operations work to ensure correct
> operation.
> 
> 1) Previously the cscfg_mutex was held throughout the load/unload
> operation. This is now only held during configuration list manipulations,
> resulting in a multi-stage load/unload process.
> 
> 2) All operations that manipulate the configfs representation of the
> configurations and features are now separated out and run without the
> cscfg_mutex being held. This avoids circular lock_dep issue with the
> built-in configfs mutexes and semaphores
> 
> 3) As the load and unload is now multi-stage, some parts under the
> cscfg_mutex and others not:
> i) A flag indicating a load / unload operation in progress is used to
> serialise load / unload operations.
> ii) activating any configuration not possible when unload is in progress.
> iii) Configurations have an "available" flag set only after the last load
> stage for the configuration is complete. Activation of the configuration
> not possible till flag is set.
> 
> 4) Following load/unload rules remain:
> i) Unload prevented while any configuration is active remains.
> ii) Unload in strict reverse order of load.
> iii) Existing configurations can be activated while a new load operation
> is underway. (by definition there can be no dependencies between an
> existing configuration and a new loading one due to ii) above.)
> 
> Signed-off-by: Mike Leach <mike.leach@linaro.org>

With this patch, the issue is resolved. Thus, I would expect this one to
have the Fixes tag too, so that it gets picked up in the stable tree.
Some minor comments below.


> ---
>   .../hwtracing/coresight/coresight-config.h    |   2 +
>   .../hwtracing/coresight/coresight-syscfg.c    | 191 ++++++++++++++----
>   .../hwtracing/coresight/coresight-syscfg.h    |  13 ++
>   3 files changed, 168 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h
> index 2e1670523461..6ba013975741 100644
> --- a/drivers/hwtracing/coresight/coresight-config.h
> +++ b/drivers/hwtracing/coresight/coresight-config.h
> @@ -134,6 +134,7 @@ struct cscfg_feature_desc {
>    * @active_cnt:		ref count for activate on this configuration.
>    * @load_owner:		handle to load owner for dynamic load and unload of configs.
>    * @fs_group:		reference to configfs group for dynamic unload.
> + * @available:		config can be activated - multi-stage load sets true on completion.
>    */
>   struct cscfg_config_desc {
>   	const char *name;
> @@ -148,6 +149,7 @@ struct cscfg_config_desc {
>   	atomic_t active_cnt;
>   	void *load_owner;
>   	struct config_group *fs_group;
> +	bool available;
>   };
>   
>   /**
> diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> index 9cd7d3c91d8e..c2364098cabb 100644
> --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> @@ -460,7 +460,6 @@ static void cscfg_unload_owned_cfgs_feats(void *load_owner)
>   	/* remove from the config descriptor lists */
>   	list_for_each_entry_safe(config_desc, cfg_tmp, &cscfg_mgr->config_desc_list, item) {
>   		if (config_desc->load_owner == load_owner) {
> -			cscfg_configfs_del_config(config_desc);
>   			etm_perf_del_symlink_cscfg(config_desc);
>   			list_del(&config_desc->item);
>   		}
> @@ -469,49 +468,28 @@ static void cscfg_unload_owned_cfgs_feats(void *load_owner)
>   	/* remove from the feature descriptor lists */
>   	list_for_each_entry_safe(feat_desc, feat_tmp, &cscfg_mgr->feat_desc_list, item) {
>   		if (feat_desc->load_owner == load_owner) {
> -			cscfg_configfs_del_feature(feat_desc);
>   			list_del(&feat_desc->item);
>   		}
>   	}
>   }
>   
> -/**
> - * cscfg_load_config_sets - API function to load feature and config sets.
> - *
> - * Take a 0 terminated array of feature descriptors and/or configuration
> - * descriptors and load into the system.
> - * Features are loaded first to ensure configuration dependencies can be met.
> - *
> - * To facilitate dynamic loading and unloading, features and configurations
> - * have a "load_owner", to allow later unload by the same owner. An owner may
> - * be a loadable module or configuration dynamically created via configfs.
> - * As later loaded configurations can use earlier loaded features, creating load
> - * dependencies, a load order list is maintained. Unload is strictly in the
> - * reverse order to load.
> - *
> - * @config_descs: 0 terminated array of configuration descriptors.
> - * @feat_descs:   0 terminated array of feature descriptors.
> - * @owner_info:	  Information on the owner of this set.
> +/*
> + * load the features and configs to the lists - called with list mutex held
>    */

Minor nit: We could enforce this at runtime with the lockdep.

	lockdep_assert_held(mutex_ptr)

> -int cscfg_load_config_sets(struct cscfg_config_desc **config_descs,
> -			   struct cscfg_feature_desc **feat_descs,
> -			   struct cscfg_load_owner_info *owner_info)
> +static int cscfg_load_owned_cfgs_feats(struct cscfg_config_desc **config_descs,
> +				       struct cscfg_feature_desc **feat_descs,
> +				       struct cscfg_load_owner_info *owner_info)
>   {
> -	int err = 0, i = 0;
> -
> -	mutex_lock(&cscfg_mutex);
> +	int i = 0, err = 0;
>   
>   	/* load features first */
>   	if (feat_descs) {
>   		while (feat_descs[i]) {
>   			err = cscfg_load_feat(feat_descs[i]);
> -			if (!err)
> -				err = cscfg_configfs_add_feature(feat_descs[i]);
>   			if (err) {
>   				pr_err("coresight-syscfg: Failed to load feature %s\n",
>   				       feat_descs[i]->name);
> -				cscfg_unload_owned_cfgs_feats(owner_info);
> -				goto exit_unlock;
> +				goto exit_err;

				return err; ?

>   			}
>   			feat_descs[i]->load_owner = owner_info;
>   			i++;
> @@ -523,31 +501,143 @@ int cscfg_load_config_sets(struct cscfg_config_desc **config_descs,
>   	if (config_descs) {
>   		while (config_descs[i]) {
>   			err = cscfg_load_config(config_descs[i]);
> -			if (!err)
> -				err = cscfg_configfs_add_config(config_descs[i]);
>   			if (err) {
>   				pr_err("coresight-syscfg: Failed to load configuration %s\n",
>   				       config_descs[i]->name);
> -				cscfg_unload_owned_cfgs_feats(owner_info);
> -				goto exit_unlock;
> +				goto exit_err;

				return err; ?

>   			}
>   			config_descs[i]->load_owner = owner_info;
> +			config_descs[i]->available = false;
> +			i++;
> +		}
> +	}
> +exit_err:

minor nit: s/exit_err/done ? Since this is also the normal exit
path. Or we should simply exit from the failure points as we don't
have any cleanup.

> +	return err;
> +}
> +
> +/* set configurations as available to activate at the end of the load process */
> +static void cscfg_set_configs_available(struct cscfg_config_desc **config_descs)
> +{
> +	int i = 0;
> +
> +	if (config_descs) {
> +		while (config_descs[i])	{
> +			config_descs[i]->available = true;
>   			i++;
>   		}
>   	}
> +}
> +
> +/*
> + * Create and register each of the configurations and features with configfs.
> + * Called without mutex being held.
> + */
> +static int cscfg_fs_register_cfgs_feats(struct cscfg_config_desc **config_descs,
> +					struct cscfg_feature_desc **feat_descs)
> +{
> +	int i = 0, err = 0;
> +
> +	if (feat_descs) {
> +		while (feat_descs[i]) {
> +			err = cscfg_configfs_add_feature(feat_descs[i]);
> +			if (err)
> +				goto exit_err;
> +			i++;
> +		}
> +	}

minor nit: The above could be :

		for (i = 0; feat_descs[i]; i++) {
			err = cscfg_configfs_add_feature(feat_descs[i]);
			if (err)
				return err;
		}

			
> +	i = 0;

You may skip this, if we switch to for() below.

> +	if (config_descs) {
> +		while (config_descs[i]) {
> +			err = cscfg_configfs_add_config(config_descs[i]);
> +			if (err)
> +				goto exit_err;
> +			i++;
> +		}

Same as above.

> +	}
> +exit_err:
> +	return err;
> +}
> +
> +/**
> + * cscfg_load_config_sets - API function to load feature and config sets.
> + *
> + * Take a 0 terminated array of feature descriptors and/or configuration
> + * descriptors and load into the system.
> + * Features are loaded first to ensure configuration dependencies can be met.
> + *
> + * To facilitate dynamic loading and unloading, features and configurations
> + * have a "load_owner", to allow later unload by the same owner. An owner may
> + * be a loadable module or configuration dynamically created via configfs.
> + * As later loaded configurations can use earlier loaded features, creating load
> + * dependencies, a load order list is maintained. Unload is strictly in the
> + * reverse order to load.
> + *
> + * @config_descs: 0 terminated array of configuration descriptors.
> + * @feat_descs:   0 terminated array of feature descriptors.
> + * @owner_info:	  Information on the owner of this set.
> + */
> +int cscfg_load_config_sets(struct cscfg_config_desc **config_descs,
> +			   struct cscfg_feature_desc **feat_descs,
> +			   struct cscfg_load_owner_info *owner_info)
> +{
> +	int err = 0;
> +
> +	mutex_lock(&cscfg_mutex);
> +	if (cscfg_mgr->load_op != CSCFG_LOAD_OP_NONE) {
> +		mutex_unlock(&cscfg_mutex);
> +		return -EBUSY;
> +	}
> +	cscfg_mgr->load_op = CSCFG_LOAD_OP_LOAD;
> +
> +	/* first load and add to the lists */
> +	err = cscfg_load_owned_cfgs_feats(config_descs, feat_descs, owner_info);
> +	if (err)
> +		goto err_clean_load;
>   
>   	/* add the load owner to the load order list */
>   	list_add_tail(&owner_info->item, &cscfg_mgr->load_order_list);
>   	if (!list_is_singular(&cscfg_mgr->load_order_list)) {
>   		/* lock previous item in load order list */
>   		err = cscfg_owner_get(list_prev_entry(owner_info, item));
> -		if (err) {
> -			cscfg_unload_owned_cfgs_feats(owner_info);
> -			list_del(&owner_info->item);
> -		}
> +		if (err)
> +			goto err_clean_owner_list;
>   	}
>   
> +	/*
> +	 * make visible to configfs - configfs manipulation must occur outside
> +	 * the list mutex lock to avoid circular lockdep issues with configfs
> +	 * built in mutexes and semaphores. This is safe as it is not possible
> +	 * to start a new load/unload operation till the current one is done.
> +	 */
> +	mutex_unlock(&cscfg_mutex);
> +
> +	/* create the configfs elements */
> +	err = cscfg_fs_register_cfgs_feats(config_descs, feat_descs);
> +	mutex_lock(&cscfg_mutex);
> +
> +	if (err)
> +		goto err_clean_cfs;
> +
> +	/* mark any new configs as available for activation */
> +	cscfg_set_configs_available(config_descs);
> +	goto exit_unlock;
> +
> +
> +err_clean_cfs:
> +	/* cleanup after error registering with configfs */
> +	cscfg_fs_unregister_cfgs_feats(owner_info);
> +
> +	if (!list_is_singular(&cscfg_mgr->load_order_list))
> +		cscfg_owner_put(list_prev_entry(owner_info, item));
> +
> +err_clean_owner_list:
> +	list_del(&owner_info->item);
> +
> +err_clean_load:
> +	cscfg_unload_owned_cfgs_feats(owner_info);
> +
>   exit_unlock:
> +	cscfg_mgr->load_op = CSCFG_LOAD_OP_NONE;
>   	mutex_unlock(&cscfg_mutex);
>   	return err;
>   }
> @@ -564,6 +654,9 @@ EXPORT_SYMBOL_GPL(cscfg_load_config_sets);
>    * 1) no configurations are active.
>    * 2) the set being unloaded was the last to be loaded to maintain dependencies.
>    *
> + * Once the unload operation commences, we disallow any configuration being
> + * made active until it is complete.
> + *
>    * @owner_info:	Information on owner for set being unloaded.
>    */
>   int cscfg_unload_config_sets(struct cscfg_load_owner_info *owner_info)
> @@ -572,6 +665,13 @@ int cscfg_unload_config_sets(struct cscfg_load_owner_info *owner_info)
>   	struct cscfg_load_owner_info *load_list_item = NULL;
>   
>   	mutex_lock(&cscfg_mutex);
> +	if (cscfg_mgr->load_op != CSCFG_LOAD_OP_NONE) {
> +		mutex_unlock(&cscfg_mutex);
> +		return -EBUSY;
> +	}
> +
> +	/* unload op in progress also prevents activation of any config */
> +	cscfg_mgr->load_op = CSCFG_LOAD_OP_UNLOAD;
>   
>   	/* cannot unload if anything is active */
>   	if (atomic_read(&cscfg_mgr->sys_active_cnt)) {
> @@ -592,7 +692,12 @@ int cscfg_unload_config_sets(struct cscfg_load_owner_info *owner_info)
>   		goto exit_unlock;
>   	}
>   
> -	/* unload all belonging to load_owner */
> +	/* remove from configfs - again outside the scope of the list mutex */
> +	mutex_unlock(&cscfg_mutex);
> +	cscfg_fs_unregister_cfgs_feats(owner_info);
> +	mutex_lock(&cscfg_mutex);
> +
> +	/* unload everything from lists belonging to load_owner */
>   	cscfg_unload_owned_cfgs_feats(owner_info);
>   
>   	/* remove from load order list */
> @@ -603,7 +708,9 @@ int cscfg_unload_config_sets(struct cscfg_load_owner_info *owner_info)
>   	list_del(&owner_info->item);
>   
>   exit_unlock:
> +	cscfg_mgr->load_op = CSCFG_LOAD_OP_NONE;
>   	mutex_unlock(&cscfg_mutex);
> +
>   	return err;
>   }
>   EXPORT_SYMBOL_GPL(cscfg_unload_config_sets);
> @@ -780,8 +887,15 @@ static int _cscfg_activate_config(unsigned long cfg_hash)
>   	struct cscfg_config_desc *config_desc;
>   	int err = -EINVAL;
>   
> +	if (cscfg_mgr->load_op == CSCFG_LOAD_OP_UNLOAD)
> +		return -EBUSY;
> +
>   	list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) {
>   		if ((unsigned long)config_desc->event_ea->var == cfg_hash) {
> +			/* if we happen upon a partly loaded config, can't use it */
> +			if (config_desc->available == false)
> +				return -EBUSY;
> +
>   			/* must ensure that config cannot be unloaded in use */
>   			err = cscfg_owner_get(config_desc->load_owner);
>   			if (err)
> @@ -1072,6 +1186,7 @@ static int cscfg_create_device(void)
>   	INIT_LIST_HEAD(&cscfg_mgr->config_desc_list);
>   	INIT_LIST_HEAD(&cscfg_mgr->load_order_list);
>   	atomic_set(&cscfg_mgr->sys_active_cnt, 0);
> +	cscfg_mgr->load_op = CSCFG_LOAD_OP_NONE;
>   
>   	/* setup the device */
>   	dev = cscfg_device();
> diff --git a/drivers/hwtracing/coresight/coresight-syscfg.h b/drivers/hwtracing/coresight/coresight-syscfg.h
> index 9106ffab4833..e8e812ce7a8b 100644
> --- a/drivers/hwtracing/coresight/coresight-syscfg.h
> +++ b/drivers/hwtracing/coresight/coresight-syscfg.h
> @@ -12,6 +12,17 @@
>   
>   #include "coresight-config.h"
>   
> +/*
> + * Load operation types.
> + * When loading or unloading, another load operation cannot be run.
> + * When unloading configurations cannot be activated.
> + */
> +enum cscfg_load_ops {
> +	CSCFG_LOAD_OP_NONE,
> +	CSCFG_LOAD_OP_LOAD,
> +	CSCFG_LOAD_OP_UNLOAD
> +};
> +
>   /**
>    * System configuration manager device.
>    *
> @@ -30,6 +41,7 @@
>    * @cfgfs_subsys:	configfs subsystem used to manage configurations.
>    * @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_op:		A multi-stage load/unload operation is in progress.

Super minor nit: Could we rename this from load_op to state ?

e.g.
	CSFG_LOAD
	CSFG_NONE or even CSFG_IDLE
	CSFG_UNLOAD

Essentially this indicate the operation/state of the csfg_manager.

Suzuki

>    */
>   struct cscfg_manager {
>   	struct device dev;
> @@ -41,6 +53,7 @@ struct cscfg_manager {
>   	struct configfs_subsystem cfgfs_subsys;
>   	u32 sysfs_active_config;
>   	int sysfs_active_preset;
> +	enum cscfg_load_ops load_op;
>   };
>   
>   /* get reference to dev in cscfg_manager */


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/2] coresight: syscfg: Update load and unload operations

[Mike Leach]

Hi Suzuki,

Thanks for the review.

I've address the issues per your requests and will spin out the next
version shortly

Regards

Mike

On Tue, 21 Jun 2022 at 11:21, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>
> Hi Mike
>
> Thanks for the updated patch. Please find my comments inline.
>
> On 17/06/2022 17:40, Mike Leach wrote:
> > The configfs system is a source of access to the config information in the
> > configuration and feature lists.
> >
> > This can result in additional LOCKDEP issues as a result of the mutex
> > ordering between the config list mutex (cscfg_mutex) and the configfs
> > system mutexes.
> >
> > As such we need to adjust how load/unload operations work to ensure correct
> > operation.
> >
> > 1) Previously the cscfg_mutex was held throughout the load/unload
> > operation. This is now only held during configuration list manipulations,
> > resulting in a multi-stage load/unload process.
> >
> > 2) All operations that manipulate the configfs representation of the
> > configurations and features are now separated out and run without the
> > cscfg_mutex being held. This avoids circular lock_dep issue with the
> > built-in configfs mutexes and semaphores
> >
> > 3) As the load and unload is now multi-stage, some parts under the
> > cscfg_mutex and others not:
> > i) A flag indicating a load / unload operation in progress is used to
> > serialise load / unload operations.
> > ii) activating any configuration not possible when unload is in progress.
> > iii) Configurations have an "available" flag set only after the last load
> > stage for the configuration is complete. Activation of the configuration
> > not possible till flag is set.
> >
> > 4) Following load/unload rules remain:
> > i) Unload prevented while any configuration is active remains.
> > ii) Unload in strict reverse order of load.
> > iii) Existing configurations can be activated while a new load operation
> > is underway. (by definition there can be no dependencies between an
> > existing configuration and a new loading one due to ii) above.)
> >
> > Signed-off-by: Mike Leach <mike.leach@linaro.org>
>
> With this patch, the issue is resolved. Thus, I would expect this one to
> have the Fixes tag too, so that it gets picked up in the stable tree.
> Some minor comments below.
>
>
> > ---
> >   .../hwtracing/coresight/coresight-config.h    |   2 +
> >   .../hwtracing/coresight/coresight-syscfg.c    | 191 ++++++++++++++----
> >   .../hwtracing/coresight/coresight-syscfg.h    |  13 ++
> >   3 files changed, 168 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h
> > index 2e1670523461..6ba013975741 100644
> > --- a/drivers/hwtracing/coresight/coresight-config.h
> > +++ b/drivers/hwtracing/coresight/coresight-config.h
> > @@ -134,6 +134,7 @@ struct cscfg_feature_desc {
> >    * @active_cnt:             ref count for activate on this configuration.
> >    * @load_owner:             handle to load owner for dynamic load and unload of configs.
> >    * @fs_group:               reference to configfs group for dynamic unload.
> > + * @available:               config can be activated - multi-stage load sets true on completion.
> >    */
> >   struct cscfg_config_desc {
> >       const char *name;
> > @@ -148,6 +149,7 @@ struct cscfg_config_desc {
> >       atomic_t active_cnt;
> >       void *load_owner;
> >       struct config_group *fs_group;
> > +     bool available;
> >   };
> >
> >   /**
> > diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> > index 9cd7d3c91d8e..c2364098cabb 100644
> > --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> > +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> > @@ -460,7 +460,6 @@ static void cscfg_unload_owned_cfgs_feats(void *load_owner)
> >       /* remove from the config descriptor lists */
> >       list_for_each_entry_safe(config_desc, cfg_tmp, &cscfg_mgr->config_desc_list, item) {
> >               if (config_desc->load_owner == load_owner) {
> > -                     cscfg_configfs_del_config(config_desc);
> >                       etm_perf_del_symlink_cscfg(config_desc);
> >                       list_del(&config_desc->item);
> >               }
> > @@ -469,49 +468,28 @@ static void cscfg_unload_owned_cfgs_feats(void *load_owner)
> >       /* remove from the feature descriptor lists */
> >       list_for_each_entry_safe(feat_desc, feat_tmp, &cscfg_mgr->feat_desc_list, item) {
> >               if (feat_desc->load_owner == load_owner) {
> > -                     cscfg_configfs_del_feature(feat_desc);
> >                       list_del(&feat_desc->item);
> >               }
> >       }
> >   }
> >
> > -/**
> > - * cscfg_load_config_sets - API function to load feature and config sets.
> > - *
> > - * Take a 0 terminated array of feature descriptors and/or configuration
> > - * descriptors and load into the system.
> > - * Features are loaded first to ensure configuration dependencies can be met.
> > - *
> > - * To facilitate dynamic loading and unloading, features and configurations
> > - * have a "load_owner", to allow later unload by the same owner. An owner may
> > - * be a loadable module or configuration dynamically created via configfs.
> > - * As later loaded configurations can use earlier loaded features, creating load
> > - * dependencies, a load order list is maintained. Unload is strictly in the
> > - * reverse order to load.
> > - *
> > - * @config_descs: 0 terminated array of configuration descriptors.
> > - * @feat_descs:   0 terminated array of feature descriptors.
> > - * @owner_info:        Information on the owner of this set.
> > +/*
> > + * load the features and configs to the lists - called with list mutex held
> >    */
>
> Minor nit: We could enforce this at runtime with the lockdep.
>
>         lockdep_assert_held(mutex_ptr)
>
> > -int cscfg_load_config_sets(struct cscfg_config_desc **config_descs,
> > -                        struct cscfg_feature_desc **feat_descs,
> > -                        struct cscfg_load_owner_info *owner_info)
> > +static int cscfg_load_owned_cfgs_feats(struct cscfg_config_desc **config_descs,
> > +                                    struct cscfg_feature_desc **feat_descs,
> > +                                    struct cscfg_load_owner_info *owner_info)
> >   {
> > -     int err = 0, i = 0;
> > -
> > -     mutex_lock(&cscfg_mutex);
> > +     int i = 0, err = 0;
> >
> >       /* load features first */
> >       if (feat_descs) {
> >               while (feat_descs[i]) {
> >                       err = cscfg_load_feat(feat_descs[i]);
> > -                     if (!err)
> > -                             err = cscfg_configfs_add_feature(feat_descs[i]);
> >                       if (err) {
> >                               pr_err("coresight-syscfg: Failed to load feature %s\n",
> >                                      feat_descs[i]->name);
> > -                             cscfg_unload_owned_cfgs_feats(owner_info);
> > -                             goto exit_unlock;
> > +                             goto exit_err;
>
>                                 return err; ?
>
> >                       }
> >                       feat_descs[i]->load_owner = owner_info;
> >                       i++;
> > @@ -523,31 +501,143 @@ int cscfg_load_config_sets(struct cscfg_config_desc **config_descs,
> >       if (config_descs) {
> >               while (config_descs[i]) {
> >                       err = cscfg_load_config(config_descs[i]);
> > -                     if (!err)
> > -                             err = cscfg_configfs_add_config(config_descs[i]);
> >                       if (err) {
> >                               pr_err("coresight-syscfg: Failed to load configuration %s\n",
> >                                      config_descs[i]->name);
> > -                             cscfg_unload_owned_cfgs_feats(owner_info);
> > -                             goto exit_unlock;
> > +                             goto exit_err;
>
>                                 return err; ?
>
> >                       }
> >                       config_descs[i]->load_owner = owner_info;
> > +                     config_descs[i]->available = false;
> > +                     i++;
> > +             }
> > +     }
> > +exit_err:
>
> minor nit: s/exit_err/done ? Since this is also the normal exit
> path. Or we should simply exit from the failure points as we don't
> have any cleanup.
>
> > +     return err;
> > +}
> > +
> > +/* set configurations as available to activate at the end of the load process */
> > +static void cscfg_set_configs_available(struct cscfg_config_desc **config_descs)
> > +{
> > +     int i = 0;
> > +
> > +     if (config_descs) {
> > +             while (config_descs[i]) {
> > +                     config_descs[i]->available = true;
> >                       i++;
> >               }
> >       }
> > +}
> > +
> > +/*
> > + * Create and register each of the configurations and features with configfs.
> > + * Called without mutex being held.
> > + */
> > +static int cscfg_fs_register_cfgs_feats(struct cscfg_config_desc **config_descs,
> > +                                     struct cscfg_feature_desc **feat_descs)
> > +{
> > +     int i = 0, err = 0;
> > +
> > +     if (feat_descs) {
> > +             while (feat_descs[i]) {
> > +                     err = cscfg_configfs_add_feature(feat_descs[i]);
> > +                     if (err)
> > +                             goto exit_err;
> > +                     i++;
> > +             }
> > +     }
>
> minor nit: The above could be :
>
>                 for (i = 0; feat_descs[i]; i++) {
>                         err = cscfg_configfs_add_feature(feat_descs[i]);
>                         if (err)
>                                 return err;
>                 }
>
>
> > +     i = 0;
>
> You may skip this, if we switch to for() below.
>
> > +     if (config_descs) {
> > +             while (config_descs[i]) {
> > +                     err = cscfg_configfs_add_config(config_descs[i]);
> > +                     if (err)
> > +                             goto exit_err;
> > +                     i++;
> > +             }
>
> Same as above.
>
> > +     }
> > +exit_err:
> > +     return err;
> > +}
> > +
> > +/**
> > + * cscfg_load_config_sets - API function to load feature and config sets.
> > + *
> > + * Take a 0 terminated array of feature descriptors and/or configuration
> > + * descriptors and load into the system.
> > + * Features are loaded first to ensure configuration dependencies can be met.
> > + *
> > + * To facilitate dynamic loading and unloading, features and configurations
> > + * have a "load_owner", to allow later unload by the same owner. An owner may
> > + * be a loadable module or configuration dynamically created via configfs.
> > + * As later loaded configurations can use earlier loaded features, creating load
> > + * dependencies, a load order list is maintained. Unload is strictly in the
> > + * reverse order to load.
> > + *
> > + * @config_descs: 0 terminated array of configuration descriptors.
> > + * @feat_descs:   0 terminated array of feature descriptors.
> > + * @owner_info:        Information on the owner of this set.
> > + */
> > +int cscfg_load_config_sets(struct cscfg_config_desc **config_descs,
> > +                        struct cscfg_feature_desc **feat_descs,
> > +                        struct cscfg_load_owner_info *owner_info)
> > +{
> > +     int err = 0;
> > +
> > +     mutex_lock(&cscfg_mutex);
> > +     if (cscfg_mgr->load_op != CSCFG_LOAD_OP_NONE) {
> > +             mutex_unlock(&cscfg_mutex);
> > +             return -EBUSY;
> > +     }
> > +     cscfg_mgr->load_op = CSCFG_LOAD_OP_LOAD;
> > +
> > +     /* first load and add to the lists */
> > +     err = cscfg_load_owned_cfgs_feats(config_descs, feat_descs, owner_info);
> > +     if (err)
> > +             goto err_clean_load;
> >
> >       /* add the load owner to the load order list */
> >       list_add_tail(&owner_info->item, &cscfg_mgr->load_order_list);
> >       if (!list_is_singular(&cscfg_mgr->load_order_list)) {
> >               /* lock previous item in load order list */
> >               err = cscfg_owner_get(list_prev_entry(owner_info, item));
> > -             if (err) {
> > -                     cscfg_unload_owned_cfgs_feats(owner_info);
> > -                     list_del(&owner_info->item);
> > -             }
> > +             if (err)
> > +                     goto err_clean_owner_list;
> >       }
> >
> > +     /*
> > +      * make visible to configfs - configfs manipulation must occur outside
> > +      * the list mutex lock to avoid circular lockdep issues with configfs
> > +      * built in mutexes and semaphores. This is safe as it is not possible
> > +      * to start a new load/unload operation till the current one is done.
> > +      */
> > +     mutex_unlock(&cscfg_mutex);
> > +
> > +     /* create the configfs elements */
> > +     err = cscfg_fs_register_cfgs_feats(config_descs, feat_descs);
> > +     mutex_lock(&cscfg_mutex);
> > +
> > +     if (err)
> > +             goto err_clean_cfs;
> > +
> > +     /* mark any new configs as available for activation */
> > +     cscfg_set_configs_available(config_descs);
> > +     goto exit_unlock;
> > +
> > +
> > +err_clean_cfs:
> > +     /* cleanup after error registering with configfs */
> > +     cscfg_fs_unregister_cfgs_feats(owner_info);
> > +
> > +     if (!list_is_singular(&cscfg_mgr->load_order_list))
> > +             cscfg_owner_put(list_prev_entry(owner_info, item));
> > +
> > +err_clean_owner_list:
> > +     list_del(&owner_info->item);
> > +
> > +err_clean_load:
> > +     cscfg_unload_owned_cfgs_feats(owner_info);
> > +
> >   exit_unlock:
> > +     cscfg_mgr->load_op = CSCFG_LOAD_OP_NONE;
> >       mutex_unlock(&cscfg_mutex);
> >       return err;
> >   }
> > @@ -564,6 +654,9 @@ EXPORT_SYMBOL_GPL(cscfg_load_config_sets);
> >    * 1) no configurations are active.
> >    * 2) the set being unloaded was the last to be loaded to maintain dependencies.
> >    *
> > + * Once the unload operation commences, we disallow any configuration being
> > + * made active until it is complete.
> > + *
> >    * @owner_info:     Information on owner for set being unloaded.
> >    */
> >   int cscfg_unload_config_sets(struct cscfg_load_owner_info *owner_info)
> > @@ -572,6 +665,13 @@ int cscfg_unload_config_sets(struct cscfg_load_owner_info *owner_info)
> >       struct cscfg_load_owner_info *load_list_item = NULL;
> >
> >       mutex_lock(&cscfg_mutex);
> > +     if (cscfg_mgr->load_op != CSCFG_LOAD_OP_NONE) {
> > +             mutex_unlock(&cscfg_mutex);
> > +             return -EBUSY;
> > +     }
> > +
> > +     /* unload op in progress also prevents activation of any config */
> > +     cscfg_mgr->load_op = CSCFG_LOAD_OP_UNLOAD;
> >
> >       /* cannot unload if anything is active */
> >       if (atomic_read(&cscfg_mgr->sys_active_cnt)) {
> > @@ -592,7 +692,12 @@ int cscfg_unload_config_sets(struct cscfg_load_owner_info *owner_info)
> >               goto exit_unlock;
> >       }
> >
> > -     /* unload all belonging to load_owner */
> > +     /* remove from configfs - again outside the scope of the list mutex */
> > +     mutex_unlock(&cscfg_mutex);
> > +     cscfg_fs_unregister_cfgs_feats(owner_info);
> > +     mutex_lock(&cscfg_mutex);
> > +
> > +     /* unload everything from lists belonging to load_owner */
> >       cscfg_unload_owned_cfgs_feats(owner_info);
> >
> >       /* remove from load order list */
> > @@ -603,7 +708,9 @@ int cscfg_unload_config_sets(struct cscfg_load_owner_info *owner_info)
> >       list_del(&owner_info->item);
> >
> >   exit_unlock:
> > +     cscfg_mgr->load_op = CSCFG_LOAD_OP_NONE;
> >       mutex_unlock(&cscfg_mutex);
> > +
> >       return err;
> >   }
> >   EXPORT_SYMBOL_GPL(cscfg_unload_config_sets);
> > @@ -780,8 +887,15 @@ static int _cscfg_activate_config(unsigned long cfg_hash)
> >       struct cscfg_config_desc *config_desc;
> >       int err = -EINVAL;
> >
> > +     if (cscfg_mgr->load_op == CSCFG_LOAD_OP_UNLOAD)
> > +             return -EBUSY;
> > +
> >       list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) {
> >               if ((unsigned long)config_desc->event_ea->var == cfg_hash) {
> > +                     /* if we happen upon a partly loaded config, can't use it */
> > +                     if (config_desc->available == false)
> > +                             return -EBUSY;
> > +
> >                       /* must ensure that config cannot be unloaded in use */
> >                       err = cscfg_owner_get(config_desc->load_owner);
> >                       if (err)
> > @@ -1072,6 +1186,7 @@ static int cscfg_create_device(void)
> >       INIT_LIST_HEAD(&cscfg_mgr->config_desc_list);
> >       INIT_LIST_HEAD(&cscfg_mgr->load_order_list);
> >       atomic_set(&cscfg_mgr->sys_active_cnt, 0);
> > +     cscfg_mgr->load_op = CSCFG_LOAD_OP_NONE;
> >
> >       /* setup the device */
> >       dev = cscfg_device();
> > diff --git a/drivers/hwtracing/coresight/coresight-syscfg.h b/drivers/hwtracing/coresight/coresight-syscfg.h
> > index 9106ffab4833..e8e812ce7a8b 100644
> > --- a/drivers/hwtracing/coresight/coresight-syscfg.h
> > +++ b/drivers/hwtracing/coresight/coresight-syscfg.h
> > @@ -12,6 +12,17 @@
> >
> >   #include "coresight-config.h"
> >
> > +/*
> > + * Load operation types.
> > + * When loading or unloading, another load operation cannot be run.
> > + * When unloading configurations cannot be activated.
> > + */
> > +enum cscfg_load_ops {
> > +     CSCFG_LOAD_OP_NONE,
> > +     CSCFG_LOAD_OP_LOAD,
> > +     CSCFG_LOAD_OP_UNLOAD
> > +};
> > +
> >   /**
> >    * System configuration manager device.
> >    *
> > @@ -30,6 +41,7 @@
> >    * @cfgfs_subsys:   configfs subsystem used to manage configurations.
> >    * @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_op:         A multi-stage load/unload operation is in progress.
>
> Super minor nit: Could we rename this from load_op to state ?
>
> e.g.
>         CSFG_LOAD
>         CSFG_NONE or even CSFG_IDLE
>         CSFG_UNLOAD
>
> Essentially this indicate the operation/state of the csfg_manager.
>
> Suzuki
>
> >    */
> >   struct cscfg_manager {
> >       struct device dev;
> > @@ -41,6 +53,7 @@ struct cscfg_manager {
> >       struct configfs_subsystem cfgfs_subsys;
> >       u32 sysfs_active_config;
> >       int sysfs_active_preset;
> > +     enum cscfg_load_ops load_op;
> >   };
> >
> >   /* get reference to dev in cscfg_manager */
>


-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel