Re: [RFC PATCH v2 3/8] coresight: syscfg: Add registration and feature loading for cs devices

On Wed, Oct 07, 2020 at 12:15:52PM +0100, Mike Leach wrote:

Hi Mathieu,

On Tue, 6 Oct 2020 at 23:37, Mathieu Poirier mathieu.poirier@linaro.org wrote:

...

On Thu, Aug 27, 2020 at 03:34:06PM +0100, Mike Leach wrote:

...

API for individual devices to register with the cs_syscfg driver is added.

Devices register with matching information, and any features or configurations that match will be loaded into the device.

The feature and configuration loading is extended so that on load these are loaded into any currently registered devices. This allows configuration loading after devices have been registered.

Signed-off-by: Mike Leach mike.leach@linaro.org

.../hwtracing/coresight/coresight-config.h | 119 ++++++++ .../hwtracing/coresight/coresight-syscfg.c | 268 ++++++++++++++++++ .../hwtracing/coresight/coresight-syscfg.h | 22 ++ include/linux/coresight.h | 7 + 4 files changed, 416 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h index 413509f7d817..949a078c4692 100644 --- a/drivers/hwtracing/coresight/coresight-config.h +++ b/drivers/hwtracing/coresight/coresight-config.h @@ -151,4 +151,123 @@ struct cs_cfg_config_desc { const u64 *presets; /* nr_presets * nr_total_params */ };

+/**

• • config register instance - part of a loaded feature.

• •                        maps register values to driver structures
    

• • @value: value to use when setting feature on device / store for

• •       readback of volatile values.
    

• • @drv_store: pointer to internal driver element used to set the value

• •       in hardware.
    

• */

+struct cs_cfg_reg {

• union cs_cfg_regval value;
  

• void *drv_store;
  

+};

+/**

• • config parameter instance - part of a loaded feature.
• • @feat: parent feature
• • @reg: register value updated by this parameter.
• • @current_value: current value of parameter - may be set by user via

• •               sysfs, or modified during device operation.
    

• • @val64: true if 64 bit value
• */

+struct cs_cfg_parameter {

• struct cs_cfg_feature *feat;
  

• struct cs_cfg_reg *reg;
  

• u64 current_value;
  

• bool val64;
  

+};

+/**

• • cs_cfg_feat_ops - standard operations for loaded features.
• • CS config core provides basic defaults for these, which can be overridden by
• • device specific versions.
• • @set_on_enable: Set the feature on the driver before hw enable.
• • @save_on_disable: Save any volatile register values after hw disable.
• • @reset: Reset feature to default values.
• */

+struct cs_cfg_feat_ops {

• int (*set_on_enable)(struct cs_cfg_feature *feat, const bool force_set);
  

• void (*save_on_disable)(struct cs_cfg_feature *feat, const bool force_save);
  

• void (*reset)(struct cs_cfg_feature *feat);
  

+};

+/**

• • Feature instance loaded into a CoreSight device driver.
• • When a feature is loaded into a specific device, then this structure holds
• • the connections between the register / parameter values used and the
• • internal data structures that are written when the feature is enabled.
• • Since applying a feature modifies internal data structures in the device,
• • then we have a reference to the device spinlock to protect access to these
• • structures (@dev_spinlock).
• • @desc: pointer to the static descriptor for this feature.
• • @csdev: parent CoreSight device instance.
• • @node: list entry into feature list for this device.
• • @dev_spinlock: device spinlock..
• • @enabled: feature is enabled on this device.
• • @nr_params: number of parameters.
• • @params: current parameter values on this device
• • @param_group: sysfs group for feature parameters.
• • @ops: standard ops to enable and disable features on devices.
• */

+struct cs_cfg_feature {

• const struct cs_cfg_feature_desc *desc;
  

• struct coresight_device *csdev;
  

• struct list_head node;
  

• spinlock_t *dev_spinlock;
  

• bool enabled;
  

• int nr_params;
  

• struct cs_cfg_parameter *params;
  

• struct attribute_group *param_group;
  

• int nr_regs;
  

• struct cs_cfg_reg *regs;
  

• struct cs_cfg_feat_ops ops;
  

+};

+/**

• • Configuration instance when loaded into a device.
• • The instance contains references to loaded features on this device that are
• • used by the configuration.
• • @desc: reference to the descriptor for this configuration
• • @csdev: parent coresight device for this configuration instance.
• • @node: list entry within the coresight device
• • @id_hash: hash value of configuration name used for selection.
• • @nr_feat: Number of features on this device that are used in the

• •        configuration.
    

• • @feats: reference to the device features to enable.
• • @enabled: true if configuration is enabled on this device.
• */

+struct cs_cfg_config_dev {

Can we make cs_cfg_feature and cs_cfg_config_dev more similar? Something like cscfg_feature_csdev and cscfg_config_csdev. That way it is easy to remember and there is no misunderstanding as to what they do.

I'd already dropped the dev from this one, but I like your idea of csdev better so I'll add it to both.

...

...

• const struct cs_cfg_config_desc *desc;
  

• struct coresight_device *csdev;
  

• struct list_head node;
  

• unsigned long id_hash;
  

• int nr_feat;
  

• struct cs_cfg_feature **feats;
  

• bool enabled;
  

+};

+/**

• • Coresight device load ops.
• • Registered coresight devices provide these operations to create feature
• • instances compatible with the device hardware and drivers
• • @load_feat: Pass a feature descriptor into the device and create the

• •       create the loaded feature instance (struct cs_cfg_feature).
    

• */

+struct cs_cfg_dev_feat_ops {

• int (*load_feat)(struct coresight_device *csdev,
  

•                  const struct cs_cfg_feature_desc *feat_desc);
  

+};

#endif /* _CORESIGHT_CORESIGHT_CONFIG_H */ diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c index d49bcace76c8..479dc81993c2 100644 --- a/drivers/hwtracing/coresight/coresight-syscfg.c +++ b/drivers/hwtracing/coresight/coresight-syscfg.c @@ -31,6 +31,130 @@ static struct device_type cscfg_dev_type = {

/* load features and configuations into the lists */

+/* get name feature instance from a coresight device list of features */ +static struct cs_cfg_feature * +cscfg_get_feat_csdev(struct coresight_device *csdev, const char *name) +{

• struct cs_cfg_feature *feat = NULL;
  

• list_for_each_entry(feat, &csdev->feat_list, node) {
  

•         if (strcmp(feat->desc->name, name) == 0)
  

•                 return feat;
  

• }
  

• return NULL;
  

+}

+/* allocate the device config instance - with max number of used features */ +static struct cs_cfg_config_dev * +cscfg_alloc_dev_cfg(struct coresight_device *csdev, int nr_feats) +{

• struct cs_cfg_config_dev *dev_cfg = NULL;
  

• struct device *dev = csdev->dev.parent;
  

• /* this is being allocated using the devm for the coresight device */
  

• dev_cfg = devm_kzalloc(dev, sizeof(struct cs_cfg_config_dev), GFP_KERNEL);
  

    if (!dev_cfg)
            return NULL;

Will change.

...

...

• if (dev_cfg) {
  

•         dev_cfg->csdev = csdev;
  

•         dev_cfg->feats = devm_kcalloc(dev, nr_feats,
  

•                                       sizeof(struct cs_cfg_feature *),
  

•                                       GFP_KERNEL);
  

•         if (!dev_cfg->feats)
  

•                 dev_cfg = NULL;
  

• }
  

• return dev_cfg;
  

+}

+/* check match info between used feature from the config and a regisered device */ +static bool cscfg_match_feat_info(struct cs_cfg_match_info *used_cmp,

•                           struct cs_cfg_match_info *reg_dev)
  

+{

• /* if flags don't match then fail early */
  

• if (!(used_cmp->flags & reg_dev->flags))
  

•         return false;
  

• /* if input used_cmp has a name, then check this too */
  

• if (used_cmp->dev_name) {
  

•         if (strcmp(used_cmp->dev_name, reg_dev->dev_name) != 0)
  

•                 return false;
  

• }
  

• return true;
  

+}

+/* Load a config into a device if there are feature matches between config and device */ +static int cscfg_add_dev_cfg(struct coresight_device *csdev,

•                      struct cs_cfg_match_info *match_info,
  

•                      struct cs_cfg_config *cfg)
  

+{

• struct cs_cfg_config_dev *dev_cfg = NULL;
  

• struct cs_cfg_uses_feat_desc *used_feat;
  

• struct cs_cfg_feature *feat;
  

• int checked, nr_uses = cfg->desc->nr_uses;
  

• /* look at each required feature and see if it matches any feature on the device */
  

• for (checked = 0; checked < nr_uses; checked++) {
  

•         used_feat = &cfg->desc->uses[checked];
  

•         if (cscfg_match_feat_info(&used_feat->match, match_info)) {
  

•                 /* device matched - get the feature */
  

•                 feat = cscfg_get_feat_csdev(csdev, used_feat->name);
  

•                 if (!feat)
  

•                         return -EINVAL;
  

•                 if (!dev_cfg) {
  

•                         dev_cfg = cscfg_alloc_dev_cfg(csdev, nr_uses);
  

•                         if (!dev_cfg)
  

•                                 return -ENOMEM;
  

•                         dev_cfg->desc = cfg->desc;
  

•                 }
  

•                 dev_cfg->feats[dev_cfg->nr_feat++] = feat;
  

•         }
  

• }
  

• /* if matched features, add config to device.*/
  

• if (dev_cfg)
  

•         list_add(&dev_cfg->node, &csdev->syscfg_list);
  

• return 0;
  

+}

+/*

• • Add the config to the set of registered devices - call with mutex locked.
• • Iterates through devices - any device that matches one or more of the
• • configuration features will load it, the others will ignore it.
• */

+static int cscfg_add_cfg_to_devs(struct cs_cfg_config *cfg) +{

cscfg_add_cfg_to_csdevs()

agreed - and for all dev => csdev

...

...

• struct cscfg_reg_csdev *curr_item;
  

• int err;
  

• list_for_each_entry(curr_item, &cscfg_dev->dev_list, item) {
  

•         err = cscfg_add_dev_cfg(curr_item->csdev, &curr_item->match_info, cfg);
  

cscfg_add_csdev_cfg()

...

•         if (err)
  

•                 return err;
  

• }
  

• return 0;
  

+}

+/*

• • Add feature to any matching devices - call with mutex locked.
• • Iterates through devices - any device that matches the feature will be
• • called to load it.
• */

+static int cscfg_add_feat_to_devs(struct cs_cfg_feature_desc *feat_desc) +{

cscfg_add_feat_do_csdev()

...

• struct cscfg_reg_csdev *curr_item;
  

• int err;
  

• list_for_each_entry(curr_item, &cscfg_dev->dev_list, item) {
  

•         if (curr_item->match_info.flags & feat_desc->match_flags) {
  

•                 if (curr_item->ops.load_feat) {
  

•                         err = curr_item->ops.load_feat(curr_item->csdev, feat_desc);
  

•                         if (err)
  

•                                 return err;
  

•                 } else
  

•                         return -EINVAL;
  

•         }
  

• }
  

• return 0;
  

+}

/* check feature list for a named feature - call with mutex locked. */ static bool cscfg_match_list_feat(const char *name) { @@ -60,6 +184,7 @@ static int cscfg_check_feat_for_cfg(struct cs_cfg_config_desc *cfg_desc) static int cscfg_load_feat(struct cs_cfg_feature_desc *feat_desc) { struct cscfg_featlist_item *feat_item;

• int err;
  
  if (!cscfg_dev)
          return -ENODEV;
  

@@ -71,6 +196,11 @@ static int cscfg_load_feat(struct cs_cfg_feature_desc *feat_desc) if (!feat_item) return -ENOMEM;

• /* add feature to any matching registered devices */
  

• err = cscfg_add_feat_to_devs(feat_desc);
  

• if (err)
  

•         return err;
  

• feat_item->desc = feat_desc;
  list_add(&feat_item->item, &cscfg_dev->feat_list);
  
  

@@ -101,6 +231,11 @@ static int cscfg_load_config(struct cs_cfg_config_desc *cfg_desc) return -ENOMEM; cfg->desc = cfg_desc;

• /* add config to any matching registered device */
  

• err = cscfg_add_cfg_to_devs(cfg);
  

• if (err)
  

•         return err;
  

• list_add(&cfg->item, &cscfg_dev->config_list);
  
  return 0;
  

@@ -145,6 +280,139 @@ int cscfg_load_config_sets(struct cs_cfg_config_desc **cfg_descs, } EXPORT_SYMBOL_GPL(cscfg_load_config_sets);

+/* Handle coresight device registration and add configs and features to devices */

+/* iterate through config lists and load matching configs to device */ +static int cscfg_add_cfg_csdev(struct coresight_device *csdev,

•                        struct cs_cfg_match_info *info)
  

+{

• struct cs_cfg_config *curr_item;
  

• int err = 0;
  

• list_for_each_entry(curr_item, &cscfg_dev->config_list, item) {
  

•         err = cscfg_add_dev_cfg(csdev, info, curr_item);
  

•         if (err)
  

•                 break;
  

• }
  

• return err;
  

+}

+/* iterate through feature lists and load matching features to device */ +static int cscfg_add_feat_csdev(struct coresight_device *csdev,

•                         struct cs_cfg_match_info *info,
  

•                         struct cs_cfg_dev_feat_ops *ops)
  

+{

• struct cscfg_featlist_item *curr_item;
  

• int err = 0;
  

• if (!ops->load_feat)
  

•         return -EINVAL;
  

• list_for_each_entry(curr_item, &cscfg_dev->feat_list, item) {
  

•         if (curr_item->desc->match_flags & info->flags) {
  

•                 /* pass the feature descriptor to the device load op */
  

•                 err = ops->load_feat(csdev, curr_item->desc);
  

•                 if (err)
  

•                         break;
  

•         }
  

• }
  

• return err;
  

+}

+/* Add coresight device to list and copy its matching info */ +static int cscfg_list_add_entry(struct coresight_device *csdev,

•                         struct cs_cfg_match_info *info,
  

•                         struct cs_cfg_dev_feat_ops *ops)
  

+{

• struct device *dev = to_device_cscfg();
  

• struct cscfg_reg_csdev *list_entry;
  

• const char *name = NULL;
  

• list_entry = devm_kzalloc(dev, sizeof(struct cscfg_reg_csdev), GFP_KERNEL);
  

• if (!list_entry)
  

•         return -ENOMEM;
  

• if (info->dev_name) {
  

•         name = devm_kstrdup(dev, info->dev_name, GFP_KERNEL);
  

•         if (!name)
  

•                 return -ENOMEM;
  

• }
  

• list_entry->csdev = csdev;
  

We need to make sure @csdev doesn't go away while we hold a reference to it. To do that I suggest to use coresight_get_ref() after making it a public function. You will also need to move pm_runtime_get_sync() out of there and back in coresight_grab_device(). Releasing the reference count to @csdev using coresight_put_ref() also has to be done in cscfg_list_remove_entry().

This is guaranteed as csdevs are added to the list on cscfg_register_csdev(), - protected by the cscfg mutex, and removed on cscfg_unregister_csdev() - again protected by the mutex. These are called at the during the coresight_register / coresight_unregister phases for the driver - so the reference cannot disappear without it being unregistered, and the mutex prevents simultaneous register and unregister for a single device.

Yes, you are correct.

...

...

• list_entry->match_info.flags = info->flags;
  

• list_entry->match_info.dev_name = name;
  

• list_entry->ops.load_feat = ops->load_feat;
  

• list_add(&list_entry->item, &cscfg_dev->dev_list);
  

• INIT_LIST_HEAD(&csdev->feat_list);
  

• INIT_LIST_HEAD(&csdev->syscfg_list);
  

• spin_lock_init(&csdev->cfg_lock);
  

• return 0;
  

+}

+/* remove entry from registered device list */ +static bool cscfg_list_remove_entry(struct coresight_device *csdev) +{

• struct cscfg_reg_csdev *curr_item, *tmp;
  

• list_for_each_entry_safe(curr_item, tmp, &cscfg_dev->dev_list, item) {
  

•         if (curr_item->csdev == csdev) {
  

•                 list_del(&curr_item->item);
  

•                 return true;
  

•         }
  

• }
  

• return false;
  

+}

+/* register a coresight device with the cs_syscfg device */ +int cscfg_register_csdev(struct coresight_device *csdev,

•                  struct cs_cfg_match_info *info,
  

•                  struct cs_cfg_dev_feat_ops *ops)
  

+{

• int ret = 0;
  

• mutex_lock(&cs_cfg_mutex);
  

• if (!cscfg_dev) {
  

•         ret = -ENODEV;
  

•         goto reg_csdev_unlock;
  

• }
  

• /* add device to list of registered devices  */
  

• ret = cscfg_list_add_entry(csdev, info, ops);
  

• if (ret)
  

•         goto reg_csdev_unlock;
  

• /* now load any registered features and configs matching the device. */
  

• ret = cscfg_add_feat_csdev(csdev, info, ops);
  

• if (ret)
  

•         goto reg_csdev_unlock;
  

• ret = cscfg_add_cfg_csdev(csdev, info);
  

• if (ret)
  

•         goto reg_csdev_unlock;
  

• cscfg_dev->nr_csdev++;
  

• dev_info(&csdev->dev, "CSCFG registered %s", dev_name(&csdev->dev));
  

+reg_csdev_unlock:

• mutex_unlock(&cs_cfg_mutex);
  

• return ret;
  

+} +EXPORT_SYMBOL_GPL(cscfg_register_csdev);

+/* remove coresight device and update counts - syscfg device at 0 */ +void cscfg_unregister_csdev(struct coresight_device *csdev) +{

• mutex_lock(&cs_cfg_mutex);
  

• if (cscfg_dev) {
  

•         if (cscfg_list_remove_entry(csdev))
  

•                 cscfg_dev->nr_csdev--;
  

• }
  

• mutex_unlock(&cs_cfg_mutex);
  

+} +EXPORT_SYMBOL_GPL(cscfg_unregister_csdev);

/* cs_syscfg device instance handling */ static void cscfg_device_release(struct device *dev) { diff --git a/drivers/hwtracing/coresight/coresight-syscfg.h b/drivers/hwtracing/coresight/coresight-syscfg.h index b3d41096382d..8ba800cc2e42 100644 --- a/drivers/hwtracing/coresight/coresight-syscfg.h +++ b/drivers/hwtracing/coresight/coresight-syscfg.h @@ -9,6 +9,8 @@ #include <linux/coresight.h> #include <linux/device.h>

+#include "coresight-config.h"

/**

• System configuration device.

@@ -52,6 +54,22 @@ struct cs_cfg_config { struct list_head item; };

+/**

• • List entry for Coresight devices that are registered as supporting complex
• • config operations.
• • @csdev: The registered device.
• • @match_info: The matching type information.
• • @ops: Operations supported by the registered device.
• • @item: list entry.
• */

+struct cscfg_reg_csdev {

Please call this cscfg_registered_csdev. I agree it is long but there is no doubt about what its purpose is when reading the code.

...

• struct coresight_device *csdev;
  

• struct cs_cfg_match_info match_info;
  

• struct cs_cfg_dev_feat_ops ops;
  

Are you planning any more operations because right now there is only a single one, i.e load_feat().

reset and enable - will be used if controlling trace / features from sysfs / configfs.

...

...

• struct list_head item;
  

+};

/* internal core operations for cscfg */ int cscfg_create_device(void); void cscfg_clear_device(void); @@ -61,5 +79,9 @@ void cscfg_driver_exit(void); /* syscfg external API */ int cscfg_load_config_sets(struct cs_cfg_config_desc **cfg_descs, struct cs_cfg_feature_desc **feat_descs); +int cscfg_register_csdev(struct coresight_device *csdev,

•                  struct cs_cfg_match_info *info,
  

•                  struct cs_cfg_dev_feat_ops *ops);
  

+void cscfg_unregister_csdev(struct coresight_device *csdev);

#endif /* CORESIGHT_SYSCFG_H */ diff --git a/include/linux/coresight.h b/include/linux/coresight.h index ed0a9d938303..120eb6af4a80 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -190,6 +190,9 @@ struct coresight_sysfs_link {

• @nr_links: number of sysfs links created to other components from this

•       device. These will appear in the "connections" group.
  

• @has_conns_grp: Have added a "connections" group for sysfs links.

• • @feat_list: List of complex feature programming added to the device.
• • @syscfg_list: List of system configurations added to the device.
• • @cfg_lock: spinlock to protect feature and config lists for this device.
  */

struct coresight_device { struct coresight_platform_data *pdata; @@ -210,6 +213,10 @@ struct coresight_device { int nr_links; bool has_conns_grp; bool ect_enabled; /* true only if associated ect device is enabled */

• /* complex config and feature lists */
  

• struct list_head feat_list;
  

• struct list_head syscfg_list;
  

• spinlock_t cfg_lock;
  

};

/*

2.17.1

Thanks for looking at this.

Mike

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