[PATCH v3 2/2] coresight: syscfg: Update load and unload operations
Suzuki K Poulose
suzuki.poulose at arm.com
Tue Jun 21 03:21:38 PDT 2022
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 at 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 */
More information about the linux-arm-kernel
mailing list