[RFC PATCH 1/8] coresight: syscfg: Update API to allow dynamic load and unload
Mike Leach
mike.leach at linaro.org
Wed May 19 06:28:04 PDT 2021
Thanks - I'll sort both comments.
Regards
Mike
On Mon, 17 May 2021 at 18:27, Mathieu Poirier
<mathieu.poirier at linaro.org> wrote:
>
> On Mon, May 17, 2021 at 11:15:15AM -0600, Mathieu Poirier wrote:
> > On Wed, May 12, 2021 at 10:17:45PM +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 uses 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.
> >
> > There seems to be something wrong with the first sentence - I think the word
> > "uses" is extra. I found another one like that below.
> >
> > Otherwise this patch works. Despite the relative simplicity of what it does it
> > took me two tries to go through it, probably because of how spread out the
> > changes are. I don't think you could have done things differently though.
> >
> > Reviewed-by: Mathieu Poirier <mathieu.poirier at linaro.org>
> >
> > >
> > > 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>
> > > ---
> > > .../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 180202402eb5..ab5ec43a9dad 100644
> > > --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> > > +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> > > @@ -250,6 +250,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);
> > > @@ -267,6 +274,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);
> > > @@ -354,6 +368,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.
> > > *
> > > @@ -361,13 +441,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);
> > >
> > > @@ -380,8 +469,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++;
> > > }
> > > }
> > > @@ -396,18 +487,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 be owner.
>
> Shouldn't this be "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 */
> > > @@ -783,10 +930,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
> > >
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
More information about the linux-arm-kernel
mailing list