[PATCH v2 1/5] coresight: syscfg: Update API to allow dynamic load and unload
Mathieu Poirier
mathieu.poirier at linaro.org
Tue Nov 9 09:59:41 PST 2021
Good morning Mike,
On Tue, Oct 19, 2021 at 08:13:47PM +0100, Mike Leach wrote:
> Update the load API to permit the runtime loading and unloading of new
> configurations and features.
>
> On load, configurations and features are tagged with a "load owner" that
> is used to determine sets that were loaded together in a given API call.
>
> To unload the API uses the load owner to unload all elements previously
> loaded by that owner.
>
> The API also records the order in which different owners loaded
> their elements into the system. Later loading configurations can use
> previously loaded features, creating load dependencies. Therefore unload
> is enforced strictly in the reverse order to load.
>
> A load owner will be an additional loadable module, or a configuration
> created or loaded via configfs.
>
> Signed-off-by: Mike Leach <mike.leach at linaro.org>
> Reviewed-by: Mathieu Poirier <mathieu.poirier at linaro.org>
I couldn't remember the patchset so I started reviewing it again from scratch.
I didn't find any flaws in the code. On the flip side I think there is two
things happening here: 1) the addition of the owner concept and 2) the enhancement
of the API to add/remove configurations/features.
Please consider splitting in two different patches. More comments tomorrow...
Thanks,
Mathieu
> ---
> .../coresight/coresight-cfg-preload.c | 9 +-
> .../hwtracing/coresight/coresight-config.h | 9 +-
> .../coresight/coresight-syscfg-configfs.c | 20 +++
> .../coresight/coresight-syscfg-configfs.h | 2 +
> .../hwtracing/coresight/coresight-syscfg.c | 154 +++++++++++++++++-
> .../hwtracing/coresight/coresight-syscfg.h | 30 +++-
> 6 files changed, 216 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-cfg-preload.c b/drivers/hwtracing/coresight/coresight-cfg-preload.c
> index 751af3710d56..e237a4edfa09 100644
> --- a/drivers/hwtracing/coresight/coresight-cfg-preload.c
> +++ b/drivers/hwtracing/coresight/coresight-cfg-preload.c
> @@ -24,8 +24,13 @@ static struct cscfg_config_desc *preload_cfgs[] = {
> NULL
> };
>
> +static struct cscfg_load_owner_info preload_owner = {
> + .type = CSCFG_OWNER_PRELOAD,
> +};
> +
> /* preload called on initialisation */
> -int cscfg_preload(void)
> +int cscfg_preload(void *owner_handle)
> {
> - return cscfg_load_config_sets(preload_cfgs, preload_feats);
> + preload_owner.owner_handle = owner_handle;
> + return cscfg_load_config_sets(preload_cfgs, preload_feats, &preload_owner);
> }
> diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h
> index 25eb6c632692..9bd44b940add 100644
> --- a/drivers/hwtracing/coresight/coresight-config.h
> +++ b/drivers/hwtracing/coresight/coresight-config.h
> @@ -97,6 +97,8 @@ struct cscfg_regval_desc {
> * @params_desc: array of parameters used.
> * @nr_regs: number of registers used.
> * @regs_desc: array of registers used.
> + * @load_owner: handle to load owner for dynamic load and unload of features.
> + * @fs_group: reference to configfs group for dynamic unload.
> */
> struct cscfg_feature_desc {
> const char *name;
> @@ -107,6 +109,8 @@ struct cscfg_feature_desc {
> struct cscfg_parameter_desc *params_desc;
> int nr_regs;
> struct cscfg_regval_desc *regs_desc;
> + void *load_owner;
> + struct config_group *fs_group;
> };
>
> /**
> @@ -128,7 +132,8 @@ struct cscfg_feature_desc {
> * @presets: Array of preset values.
> * @event_ea: Extended attribute for perf event value
> * @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.
> */
> struct cscfg_config_desc {
> const char *name;
> @@ -141,6 +146,8 @@ struct cscfg_config_desc {
> const u64 *presets; /* nr_presets * nr_total_params */
> struct dev_ext_attribute *event_ea;
> atomic_t active_cnt;
> + void *load_owner;
> + struct config_group *fs_group;
> };
>
> /**
> diff --git a/drivers/hwtracing/coresight/coresight-syscfg-configfs.c b/drivers/hwtracing/coresight/coresight-syscfg-configfs.c
> index c547816b9000..345a62f1b728 100644
> --- a/drivers/hwtracing/coresight/coresight-syscfg-configfs.c
> +++ b/drivers/hwtracing/coresight/coresight-syscfg-configfs.c
> @@ -334,9 +334,19 @@ int cscfg_configfs_add_config(struct cscfg_config_desc *config_desc)
> if (IS_ERR(new_group))
> return PTR_ERR(new_group);
> err = configfs_register_group(&cscfg_configs_grp, new_group);
> + if (!err)
> + config_desc->fs_group = new_group;
> return err;
> }
>
> +void cscfg_configfs_del_config(struct cscfg_config_desc *config_desc)
> +{
> + if (config_desc->fs_group) {
> + configfs_unregister_group(config_desc->fs_group);
> + config_desc->fs_group = NULL;
> + }
> +}
> +
> static struct config_item_type cscfg_features_type = {
> .ct_owner = THIS_MODULE,
> };
> @@ -358,9 +368,19 @@ int cscfg_configfs_add_feature(struct cscfg_feature_desc *feat_desc)
> if (IS_ERR(new_group))
> return PTR_ERR(new_group);
> err = configfs_register_group(&cscfg_features_grp, new_group);
> + if (!err)
> + feat_desc->fs_group = new_group;
> return err;
> }
>
> +void cscfg_configfs_del_feature(struct cscfg_feature_desc *feat_desc)
> +{
> + if (feat_desc->fs_group) {
> + configfs_unregister_group(feat_desc->fs_group);
> + feat_desc->fs_group = NULL;
> + }
> +}
> +
> int cscfg_configfs_init(struct cscfg_manager *cscfg_mgr)
> {
> struct configfs_subsystem *subsys;
> diff --git a/drivers/hwtracing/coresight/coresight-syscfg-configfs.h b/drivers/hwtracing/coresight/coresight-syscfg-configfs.h
> index 7d6ffe35ca4c..ea1e54d29f7f 100644
> --- a/drivers/hwtracing/coresight/coresight-syscfg-configfs.h
> +++ b/drivers/hwtracing/coresight/coresight-syscfg-configfs.h
> @@ -41,5 +41,7 @@ int cscfg_configfs_init(struct cscfg_manager *cscfg_mgr);
> void cscfg_configfs_release(struct cscfg_manager *cscfg_mgr);
> int cscfg_configfs_add_config(struct cscfg_config_desc *config_desc);
> int cscfg_configfs_add_feature(struct cscfg_feature_desc *feat_desc);
> +void cscfg_configfs_del_config(struct cscfg_config_desc *config_desc);
> +void cscfg_configfs_del_feature(struct cscfg_feature_desc *feat_desc);
>
> #endif /* CORESIGHT_SYSCFG_CONFIGFS_H */
> diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> index fc0760f55c53..9bb0b0913a9a 100644
> --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> @@ -249,6 +249,13 @@ static int cscfg_check_feat_for_cfg(struct cscfg_config_desc *config_desc)
> static int cscfg_load_feat(struct cscfg_feature_desc *feat_desc)
> {
> int err;
> + struct cscfg_feature_desc *feat_desc_exist;
> +
> + /* new feature must have unique name */
> + list_for_each_entry(feat_desc_exist, &cscfg_mgr->feat_desc_list, item) {
> + if (!strcmp(feat_desc_exist->name, feat_desc->name))
> + return -EEXIST;
> + }
>
> /* add feature to any matching registered devices */
> err = cscfg_add_feat_to_csdevs(feat_desc);
> @@ -266,6 +273,13 @@ static int cscfg_load_feat(struct cscfg_feature_desc *feat_desc)
> static int cscfg_load_config(struct cscfg_config_desc *config_desc)
> {
> int err;
> + struct cscfg_config_desc *config_desc_exist;
> +
> + /* new configuration must have a unique name */
> + list_for_each_entry(config_desc_exist, &cscfg_mgr->config_desc_list, item) {
> + if (!strcmp(config_desc_exist->name, config_desc->name))
> + return -EEXIST;
> + }
>
> /* validate features are present */
> err = cscfg_check_feat_for_cfg(config_desc);
> @@ -353,6 +367,72 @@ int cscfg_update_feat_param_val(struct cscfg_feature_desc *feat_desc,
> return err;
> }
>
> +static void cscfg_remove_owned_csdev_configs(struct coresight_device *csdev, void *load_owner)
> +{
> + struct cscfg_config_csdev *config_csdev, *tmp;
> +
> + if (list_empty(&csdev->config_csdev_list))
> + return;
> +
> + list_for_each_entry_safe(config_csdev, tmp, &csdev->config_csdev_list, node) {
> + if (config_csdev->config_desc->load_owner == load_owner)
> + list_del(&config_csdev->node);
> + }
> +}
> +
> +static void cscfg_remove_owned_csdev_features(struct coresight_device *csdev, void *load_owner)
> +{
> + struct cscfg_feature_csdev *feat_csdev, *tmp;
> +
> + if (list_empty(&csdev->feature_csdev_list))
> + return;
> +
> + list_for_each_entry_safe(feat_csdev, tmp, &csdev->feature_csdev_list, node) {
> + if (feat_csdev->feat_desc->load_owner == load_owner)
> + list_del(&feat_csdev->node);
> + }
> +}
> +
> +/*
> + * removal is relatively easy - just remove from all lists, anything that
> + * matches the owner. Memory for the descriptors will be managed by the owner,
> + * memory for the csdev items is devm_ allocated with the individual csdev
> + * devices.
> + */
> +static void cscfg_unload_owned_cfgs_feats(void *load_owner)
> +{
> + struct cscfg_config_desc *config_desc, *cfg_tmp;
> + struct cscfg_feature_desc *feat_desc, *feat_tmp;
> + struct cscfg_registered_csdev *csdev_item;
> +
> + /* remove from each csdev instance feature and config lists */
> + list_for_each_entry(csdev_item, &cscfg_mgr->csdev_desc_list, item) {
> + /*
> + * for each csdev, check the loaded lists and remove if
> + * referenced descriptor is owned
> + */
> + cscfg_remove_owned_csdev_configs(csdev_item->csdev, load_owner);
> + cscfg_remove_owned_csdev_features(csdev_item->csdev, 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);
> + }
> + }
> +
> + /* 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.
> *
> @@ -360,13 +440,22 @@ int cscfg_update_feat_param_val(struct cscfg_feature_desc *feat_desc,
> * 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_feature_desc **feat_descs,
> + struct cscfg_load_owner_info *owner_info)
> {
> - int err, i = 0;
> + int err = 0, i = 0;
>
> mutex_lock(&cscfg_mutex);
>
> @@ -379,8 +468,10 @@ int cscfg_load_config_sets(struct cscfg_config_desc **config_descs,
> 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;
> }
> + feat_descs[i]->load_owner = owner_info;
> i++;
> }
> }
> @@ -395,18 +486,74 @@ int cscfg_load_config_sets(struct cscfg_config_desc **config_descs,
> 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;
> }
> + config_descs[i]->load_owner = owner_info;
> i++;
> }
> }
>
> + /* add the load owner to the load order list */
> + list_add_tail(&owner_info->item, &cscfg_mgr->load_order_list);
> +
> exit_unlock:
> mutex_unlock(&cscfg_mutex);
> return err;
> }
> EXPORT_SYMBOL_GPL(cscfg_load_config_sets);
>
> +/**
> + * cscfg_unload_config_sets - unload a set of configurations by owner.
> + *
> + * Dynamic unload of configuration and feature sets is done on the basis of
> + * the load owner of that set. Later loaded configurations can depend on
> + * features loaded earlier.
> + *
> + * Therefore, unload is only possible if:-
> + * 1) no configurations are active.
> + * 2) the set being unloaded was the last to be loaded to maintain dependencies.
> + *
> + * @owner_info: Information on owner for set being unloaded.
> + */
> +int cscfg_unload_config_sets(struct cscfg_load_owner_info *owner_info)
> +{
> + int err = 0;
> + struct cscfg_load_owner_info *load_list_item = NULL;
> +
> + mutex_lock(&cscfg_mutex);
> +
> + /* cannot unload if anything is active */
> + if (atomic_read(&cscfg_mgr->sys_active_cnt)) {
> + err = -EBUSY;
> + goto exit_unlock;
> + }
> +
> + /* cannot unload if not last loaded in load order */
> + if (!list_empty(&cscfg_mgr->load_order_list)) {
> + load_list_item = list_last_entry(&cscfg_mgr->load_order_list,
> + struct cscfg_load_owner_info, item);
> + if (load_list_item != owner_info)
> + load_list_item = NULL;
> + }
> +
> + if (!load_list_item) {
> + err = -EINVAL;
> + goto exit_unlock;
> + }
> +
> + /* unload all belonging to load_owner */
> + cscfg_unload_owned_cfgs_feats(owner_info);
> +
> + /* remove from load order list */
> + list_del(&load_list_item->item);
> +
> +exit_unlock:
> + mutex_unlock(&cscfg_mutex);
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(cscfg_unload_config_sets);
> +
> /* Handle coresight device registration and add configs and features to devices */
>
> /* iterate through config lists and load matching configs to device */
> @@ -826,10 +973,11 @@ int __init cscfg_init(void)
> 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();
> + err = cscfg_preload(THIS_MODULE);
> if (err)
> goto exit_err;
>
> diff --git a/drivers/hwtracing/coresight/coresight-syscfg.h b/drivers/hwtracing/coresight/coresight-syscfg.h
> index 8d018efd6ead..e2b2bdab31aa 100644
> --- a/drivers/hwtracing/coresight/coresight-syscfg.h
> +++ b/drivers/hwtracing/coresight/coresight-syscfg.h
> @@ -25,6 +25,7 @@
> * @csdev_desc_list: List of coresight devices registered with the configuration manager.
> * @feat_desc_list: List of feature descriptors to load into registered devices.
> * @config_desc_list: List of system configuration descriptors to load into registered devices.
> + * @load_order_list: Ordered list of owners for dynamically loaded configurations.
> * @sys_active_cnt: Total number of active config descriptor references.
> * @cfgfs_subsys: configfs subsystem used to manage configurations.
> */
> @@ -33,6 +34,7 @@ struct cscfg_manager {
> struct list_head csdev_desc_list;
> struct list_head feat_desc_list;
> struct list_head config_desc_list;
> + struct list_head load_order_list;
> atomic_t sys_active_cnt;
> struct configfs_subsystem cfgfs_subsys;
> };
> @@ -56,10 +58,32 @@ struct cscfg_registered_csdev {
> struct list_head item;
> };
>
> +/* owner types for loading and unloading of config and feature sets */
> +enum cscfg_load_owner_type {
> + CSCFG_OWNER_PRELOAD,
> +};
> +
> +/**
> + * Load item - item to add to the load order list allowing dynamic load and
> + * unload of configurations and features. Caller loading a config
> + * set provides a context handle for unload. API ensures that
> + * items unloaded strictly in reverse order from load to ensure
> + * dependencies are respected.
> + *
> + * @item: list entry for load order list.
> + * @type: type of owner - allows interpretation of owner_handle.
> + * @owner_handle: load context - handle for owner of loaded configs.
> + */
> +struct cscfg_load_owner_info {
> + struct list_head item;
> + int type;
> + void *owner_handle;
> +};
> +
> /* internal core operations for cscfg */
> int __init cscfg_init(void);
> void cscfg_exit(void);
> -int cscfg_preload(void);
> +int cscfg_preload(void *owner_handle);
> const struct cscfg_feature_desc *cscfg_get_named_feat_desc(const char *name);
> int cscfg_update_feat_param_val(struct cscfg_feature_desc *feat_desc,
> int param_idx, u64 value);
> @@ -67,7 +91,9 @@ int cscfg_update_feat_param_val(struct cscfg_feature_desc *feat_desc,
>
> /* syscfg manager external API */
> int cscfg_load_config_sets(struct cscfg_config_desc **cfg_descs,
> - struct cscfg_feature_desc **feat_descs);
> + struct cscfg_feature_desc **feat_descs,
> + struct cscfg_load_owner_info *owner_info);
> +int cscfg_unload_config_sets(struct cscfg_load_owner_info *owner_info);
> int cscfg_register_csdev(struct coresight_device *csdev, u32 match_flags,
> struct cscfg_csdev_feat_ops *ops);
> void cscfg_unregister_csdev(struct coresight_device *csdev);
> --
> 2.17.1
>
More information about the linux-arm-kernel
mailing list