Re: [RFC PATCH 1/8] coresight: syscfg: Update API to allow dynamic load and unload

[Mike Leach <mike.leach@linaro.org>]

Thanks - I'll sort both comments.

Regards

Mike

On Mon, 17 May 2021 at 18:27, Mathieu Poirier
<mathieu.poirier@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@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@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

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