[RFC PATCH v3 8/9] coresight: syscfg: Allow update of feature params from configfs

[Mike Leach]

Hi Mathieu,

On Thu, 26 Nov 2020 at 17:17, Mathieu Poirier
<mathieu.poirier at linaro.org> wrote:
>
> On Fri, Oct 30, 2020 at 05:56:54PM +0000, Mike Leach wrote:
> > Add in functionality to allow the user to update feature default parameter
> > values from the configfs interface.
> >
> > This updates all the device instances with the new values, removing the
> > need to set all devices individually via sysfs.
> >
> > Signed-off-by: Mike Leach <mike.leach at linaro.org>
> > ---
> >  .../hwtracing/coresight/coresight-config.c    | 36 ++++++++++++++++---
> >  .../hwtracing/coresight/coresight-config.h    |  1 +
> >  .../coresight/coresight-syscfg-configfs.c     |  3 ++
> >  .../hwtracing/coresight/coresight-syscfg.c    | 15 ++++++++
> >  .../hwtracing/coresight/coresight-syscfg.h    |  1 +
> >  5 files changed, 52 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-config.c b/drivers/hwtracing/coresight/coresight-config.c
> > index 04e7cb4ff769..7d30a415f2ff 100644
> > --- a/drivers/hwtracing/coresight/coresight-config.c
> > +++ b/drivers/hwtracing/coresight/coresight-config.c
> > @@ -96,6 +96,17 @@ static void cscfg_save_on_disable(struct cscfg_feature_csdev *feat, const bool f
> >       spin_unlock(feat->csdev_spinlock);
> >  }
> >
> > +/* load default values into params */
> > +static void cscfg_set_param_defaults(struct cscfg_feature_csdev *feat)
> > +{
> > +     int i;
> > +
> > +     spin_lock(&feat->desc->param_lock);
> > +     for (i = 0; i < feat->nr_params; i++)
> > +             feat->params[i].current_value = feat->desc->params[i].value;
> > +     spin_unlock(&feat->desc->param_lock);
> > +}
> > +
> >  /* default reset - restore default values, disable feature */
> >  static void cscfg_reset_feat(struct cscfg_feature_csdev *feat)
> >  {
> > @@ -111,10 +122,7 @@ static void cscfg_reset_feat(struct cscfg_feature_csdev *feat)
> >        * set the default values for all parameters and regs from the
> >        * relevant static descriptors.
> >        */
> > -     spin_lock(&feat->desc->param_lock);
> > -     for (i = 0; i < feat->nr_params; i++)
> > -             feat->params[i].current_value = feat->desc->params[i].value;
> > -     spin_unlock(&feat->desc->param_lock);
> > +     cscfg_set_param_defaults(feat);
> >
> >       for (i = 0; i < feat->nr_regs; i++) {
> >               reg_desc = &feat->desc->regs[i];
> > @@ -131,6 +139,26 @@ static void cscfg_reset_feat(struct cscfg_feature_csdev *feat)
> >       spin_unlock(feat->csdev_spinlock);
> >  }
> >
> > +/* update the parameters in a named feature from their defaults for the supplied device */
> > +void cscfg_csdev_set_param_defaults(struct coresight_device *csdev, const char *feat_name)
> > +{
> > +     struct cscfg_feature_csdev *feat;
> > +
> > +     spin_lock(&csdev->cfg_lock);
> > +     if (list_empty(&csdev->feat_list)) {
> > +             spin_unlock(&csdev->cfg_lock);
> > +             return;
> > +     }
> > +
> > +     list_for_each_entry(feat, &csdev->feat_list, node) {
> > +             if (!strcmp(feat_name, feat->desc->name)) {
> > +                     cscfg_set_param_defaults(feat);
> > +                     break;
> > +             }
> > +     }
> > +     spin_unlock(&csdev->cfg_lock);
> > +}
> > +
> >  void cscfg_set_def_ops(struct cscfg_feature_csdev *feat)
> >  {
> >       feat->ops.set_on_enable = cscfg_set_on_enable;
> > diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h
> > index 39fcac011aa0..1c6f0f903861 100644
> > --- a/drivers/hwtracing/coresight/coresight-config.h
> > +++ b/drivers/hwtracing/coresight/coresight-config.h
> > @@ -297,6 +297,7 @@ struct cscfg_csdev_feat_ops {
> >
> >  /* helper functions for feature manipulation */
> >  void cscfg_set_def_ops(struct cscfg_feature_csdev *feat);
> > +void cscfg_csdev_set_param_defaults(struct coresight_device *csdev, const char *feat_name);
> >
> >  /* enable / disable features or configs on a device */
> >  int cscfg_csdev_set_enabled_feats(struct coresight_device *csdev);
> > diff --git a/drivers/hwtracing/coresight/coresight-syscfg-configfs.c b/drivers/hwtracing/coresight/coresight-syscfg-configfs.c
> > index ff7ea678100a..1595c0c61db1 100644
> > --- a/drivers/hwtracing/coresight/coresight-syscfg-configfs.c
> > +++ b/drivers/hwtracing/coresight/coresight-syscfg-configfs.c
> > @@ -242,6 +242,9 @@ static ssize_t cscfg_param_value_store(struct config_item *item,
> >       param_item->param->value = val;
> >       spin_unlock(&param_item->desc->param_lock);
> >
> > +     /* push new value out to devices */
> > +     cscfg_update_named_feat_csdevs(param_item->desc->name);
> > +
> >       return size;
> >  }
> >  CONFIGFS_ATTR(cscfg_param_, value);
> > diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> > index 2cf67a038cc8..c42374342806 100644
> > --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> > +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> > @@ -315,6 +315,21 @@ const struct cscfg_feature_desc *cscfg_get_named_feat_desc(const char *name)
> >       return feat;
> >  }
> >
> > +/*
> > + * Set all parameter defaults for named feature.
> > + * Iterates through csdev list and updates param defaults on named feature.
> > + */
> > +void cscfg_update_named_feat_csdevs(const char *feat_name)
> > +{
> > +     struct cscfg_csdev_register *curr_item;
> > +
> > +     mutex_lock(&cscfg_mutex);
> > +     list_for_each_entry(curr_item, &cscfg_data.dev_list, item) {
> > +             cscfg_csdev_set_param_defaults(curr_item->csdev, feat_name);
> > +     }
> > +     mutex_unlock(&cscfg_mutex);
>
> This is what I was referring to when expressing my worries about the number of
> locks to manage.  Here we have a mutex holding a spinlock holding another
> spinlock.  It is a matter of time before we lockup the system.  I think RCUs
> will be our only option but that will seriously impede readability.
>
> I currently don't have anything better to suggest and as such will accept to
> move forward with the current situation.  We may think of something better as
> this set continue to mature.
>

yes  - I was concerned about this too. I have been working on a follow
up set to allow dynamic loading of configs - which necessitated
protecting loaded configs in use from unload.
This resulted in the use of a reference counting get / put paradigm
for config descriptors.

This method has been integrated into the next version of this
patchset, and all the locks / protection code re-configured - dropping
a couple of the locks along the way.

Regards

Mike



> > +}
> > +
> >  /*
> >   * External API function to load feature and config sets.
> >   * Take a 0 terminated array of feature descriptors and/or configuration
> > diff --git a/drivers/hwtracing/coresight/coresight-syscfg.h b/drivers/hwtracing/coresight/coresight-syscfg.h
> > index ce237a69677b..d07a0f11097f 100644
> > --- a/drivers/hwtracing/coresight/coresight-syscfg.h
> > +++ b/drivers/hwtracing/coresight/coresight-syscfg.h
> > @@ -48,6 +48,7 @@ int __init cscfg_init(void);
> >  void __exit cscfg_exit(void);
> >  int cscfg_preload(void);
> >  const struct cscfg_feature_desc *cscfg_get_named_feat_desc(const char *name);
> > +void cscfg_update_named_feat_csdevs(const char *feat_name);
> >
> >  /* syscfg external API */
> >  int cscfg_load_config_sets(struct cscfg_config_desc **cfg_descs,
> > --
> > 2.17.1
> >



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