Good afternoon Mike,

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

Creates a virtual coresight device on the bus to handle system configuration of the coresight infrastructure.

The device will manage system wide configuration, and allow complex programmed features to be added to individual device instances, and provide for system wide configuration selection on trace capture operations.

This patch updates the coresight core initialisation to create the single instance of the cs_syscfg coresight system device, on the coresight bus.

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

drivers/hwtracing/coresight/Makefile | 2 +- drivers/hwtracing/coresight/coresight-core.c | 29 +++- .../hwtracing/coresight/coresight-etm-perf.c | 2 +- .../hwtracing/coresight/coresight-etm-perf.h | 2 +- .../hwtracing/coresight/coresight-syscfg.c | 136 ++++++++++++++++++ .../hwtracing/coresight/coresight-syscfg.h | 39 +++++ include/linux/coresight.h | 1 + 7 files changed, 207 insertions(+), 4 deletions(-) create mode 100644 drivers/hwtracing/coresight/coresight-syscfg.c create mode 100644 drivers/hwtracing/coresight/coresight-syscfg.h

diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile index 1b35b55bd420..3605dd770664 100644 --- a/drivers/hwtracing/coresight/Makefile +++ b/drivers/hwtracing/coresight/Makefile @@ -4,7 +4,7 @@ # obj-$(CONFIG_CORESIGHT) += coresight.o coresight-y := coresight-core.o coresight-etm-perf.o coresight-platform.o \

• coresight-sysfs.o
  

• coresight-sysfs.o coresight-syscfg.o
  

obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o coresight-tmc-y := coresight-tmc-core.o coresight-tmc-etf.o \ coresight-tmc-etr.o diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index bf6edf468963..91f1fbd7d51d 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -21,6 +21,7 @@ #include "coresight-etm-perf.h" #include "coresight-priv.h" +#include "coresight-syscfg.h" static DEFINE_MUTEX(coresight_mutex); @@ -1669,8 +1670,17 @@ char *coresight_alloc_device_name(struct coresight_dev_list *dict, } EXPORT_SYMBOL_GPL(coresight_alloc_device_name); +/* match for virtual csdevs that have no underlying amba device */ +static int coresight_bus_match(struct device *dev, struct device_driver *drv) +{

• struct coresight_device *csdev = to_coresight_device(dev);
• return csdev->type == CORESIGHT_DEV_TYPE_SYSCFG;

+}

struct bus_type coresight_bustype = { .name = "coresight",

• .match = coresight_bus_match,

}; static int __init coresight_init(void) @@ -1683,13 +1693,29 @@ static int __init coresight_init(void) ret = etm_perf_init(); if (ret)

• bus_unregister(&coresight_bustype);
  

• goto exit_bus_unregister;
  

• /* initialise the coresight syscfg driver and device */
• ret = cscfg_driver_init();
• if (ret)

• goto exit_perf_clear;
  

• ret = cscfg_create_device();
• if (!ret)

• return 0;
  

• cscfg_driver_exit();

+exit_perf_clear:

• etm_perf_exit();

+exit_bus_unregister:

• bus_unregister(&coresight_bustype); return ret;

} static void __exit coresight_exit(void) {

• cscfg_clear_device();
• cscfg_driver_exit(); etm_perf_exit(); bus_unregister(&coresight_bustype);

} @@ -1700,3 +1726,4 @@ module_exit(coresight_exit); MODULE_AUTHOR("Pratik Patel pratikp@codeaurora.org"); MODULE_AUTHOR("Mathieu Poirier mathieu.poirier@linaro.org"); MODULE_DESCRIPTION("Arm CoreSight tracer driver"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 668b3ff11576..2d619b764738 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -619,7 +619,7 @@ int __init etm_perf_init(void) return ret; } -void __exit etm_perf_exit(void) +void etm_perf_exit(void) { perf_pmu_unregister(&etm_pmu); } diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h index 3e4f2ad5e193..29d90dfeba31 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.h +++ b/drivers/hwtracing/coresight/coresight-etm-perf.h @@ -83,6 +83,6 @@ static inline void *etm_perf_sink_config(struct perf_output_handle *handle) #endif /* CONFIG_CORESIGHT */ int __init etm_perf_init(void); -void __exit etm_perf_exit(void); +void etm_perf_exit(void); #endif diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c new file mode 100644 index 000000000000..badef8cfc7f8 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-syscfg.c @@ -0,0 +1,136 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (c) 2020 Linaro Limited, All rights reserved.
• • Author: Mike Leach mike.leach@linaro.org
• */

+#include <linux/platform_device.h>

+#include "coresight-syscfg.h"

+/*

• • cs_syscfg device is a virtual device managing the overall cs system.
• • It allows the loading of configurations and features, and loads these into
• • coresight devices as appropriate.
• */

+static DEFINE_MUTEX(cs_cfg_mutex);

+/* only one of these */ +static struct cs_cfg_device *cscfg_dev;

+static struct device_type cscfg_dev_type = {

• .name = "cs_syscfg",

+};

+#define to_device_cscfg() (&cscfg_dev->csdev.dev) +#define to_coresight_device_cscfg() (&cscfg_dev->csdev)

+/* cs_syscfg device instance handling */ +static void cscfg_device_release(struct device *dev) +{

• kfree(cscfg_dev->csdev.pdata);
• kfree(cscfg_dev);
• cscfg_dev = NULL;

+}

+int cscfg_create_device(void) +{

• struct device *dev;
• struct coresight_device *csdev;
• struct coresight_platform_data *pdata = NULL;
• int err = -ENOMEM;
• mutex_lock(&cs_cfg_mutex);
• if (cscfg_dev) {

• err = -EINVAL;
  

• goto create_dev_exit_unlock;
  

• }
• cscfg_dev = kzalloc(sizeof(*cscfg_dev), GFP_KERNEL);
• if (!cscfg_dev)

• goto create_dev_exit_unlock;
  

• pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
• if (!pdata) {

• kfree(cscfg_dev);
  

• cscfg_dev = NULL;
  

• goto create_dev_exit_unlock;
  

• }
• /* setup the device */
• dev = to_device_cscfg();
• dev_set_name(dev, "cs_syscfg");
• dev->bus = &coresight_bustype;
• dev->parent = &platform_bus;
• dev->type = &cscfg_dev_type;

As far as I can tell setting the device type doesn't do anything.

• /* setup a coresight device */
• csdev = to_coresight_device_cscfg();
• csdev->type = CORESIGHT_DEV_TYPE_SYSCFG;
• csdev->pdata = pdata;
• csdev->orphan = false;
• csdev->dev.release = cscfg_device_release;

I also fail to understand the requirement to use a coresight_device and to add it to the coresigh bus. To me it seems odd to have it there since it has no interaction with any of the other CS devices.

• err = device_register(dev);

+create_dev_exit_unlock:

• mutex_unlock(&cs_cfg_mutex);
• return err;

+}

+void cscfg_clear_device(void) +{

• mutex_lock(&cs_cfg_mutex);
• device_unregister(to_device_cscfg());
• mutex_unlock(&cs_cfg_mutex);

+}

+/* Finish allocating resources and load built-in configurations and features. */ +static int cscfg_probe(struct device *dev) +{

• int ret = 0;
• /* there can be only one... */
• if (dev != to_device_cscfg())

• return -EINVAL;
  

• /*

• * No actual hardware to handle, just allocate resources.
  

• * Probe is triggered when the core calls cscfg_create_device() to
  

• * create the device, so the mutex is already held.
  

• */
  

• INIT_LIST_HEAD(&cscfg_dev->dev_list);
• INIT_LIST_HEAD(&cscfg_dev->feat_list);
• INIT_LIST_HEAD(&cscfg_dev->config_list);
• dev_info(dev, "CSCFG initialised");
• return ret;

+}

+/* driver initialisation */ +static struct cs_cfg_driver cs_config_driver = {

This set uses cs_cfg_ and cscfg_ to declare structures and function, making it very difficult to read the code. Please pick one (I'd go with the latter but that's a personal opinion) and stick with it.

I also don't think the functionality provided in this file need to be a driver, it can simply be a set of functions handling configuration and features.

• .drv = {

• .name = "coresight-syscfg",
  

• .owner = THIS_MODULE,
  

• .suppress_bind_attrs = true,
  

• .probe = cscfg_probe,
  

• },

+};

+/* called from coresight core init */ +int __init cscfg_driver_init(void) +{

• cs_config_driver.drv.bus = &coresight_bustype;
• return driver_register(&cs_config_driver.drv);

+}

+void cscfg_driver_exit(void) +{

• driver_unregister(&cs_config_driver.drv);

+} diff --git a/drivers/hwtracing/coresight/coresight-syscfg.h b/drivers/hwtracing/coresight/coresight-syscfg.h new file mode 100644 index 000000000000..20ccca5e4151 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-syscfg.h @@ -0,0 +1,39 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/*

• • Coresight system configuration driver.
• */

+#ifndef CORESIGHT_SYSCFG_H +#define CORESIGHT_SYSCFG_H

+#include <linux/coresight.h> +#include <linux/device.h>

+/**

• • System configuration device.
• • @csdev: The coresight device instance.
• • @dev_list: List of other coresight devices registered with this device.
• • @feat_list: List of features to load into registered devices.
• • @config_list:List of system configurations to load into registered devices.
• */

+struct cs_cfg_device {

• struct coresight_device csdev;
• struct list_head dev_list;
• struct list_head feat_list;
• struct list_head config_list;
• int nr_csdev;

+};

+/* basic device driver */ +struct cs_cfg_driver {

• struct device_driver drv;

+};

+/* internal core operations for cscfg */ +int cscfg_create_device(void); +void cscfg_clear_device(void); +int __init cscfg_driver_init(void); +void cscfg_driver_exit(void);

+#endif /* CORESIGHT_SYSCFG_H */ diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 7d3c87e5b97c..ed0a9d938303 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -42,6 +42,7 @@ enum coresight_dev_type { CORESIGHT_DEV_TYPE_SOURCE, CORESIGHT_DEV_TYPE_HELPER, CORESIGHT_DEV_TYPE_ECT,

• CORESIGHT_DEV_TYPE_SYSCFG,

}; enum coresight_dev_subtype_sink { -- 2.17.1

On Tue, Oct 06, 2020 at 11:40:46AM +0100, Mike Leach wrote:

Hi Mathieu,

On Thu, 1 Oct 2020 at 21:07, Mathieu Poirier mathieu.poirier@linaro.org wrote:

...

Good afternoon Mike,

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

...

Creates a virtual coresight device on the bus to handle system configuration of the coresight infrastructure.

The device will manage system wide configuration, and allow complex programmed features to be added to individual device instances, and provide for system wide configuration selection on trace capture operations.

This patch updates the coresight core initialisation to create the single instance of the cs_syscfg coresight system device, on the coresight bus.

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

drivers/hwtracing/coresight/Makefile | 2 +- drivers/hwtracing/coresight/coresight-core.c | 29 +++- .../hwtracing/coresight/coresight-etm-perf.c | 2 +- .../hwtracing/coresight/coresight-etm-perf.h | 2 +- .../hwtracing/coresight/coresight-syscfg.c | 136 ++++++++++++++++++ .../hwtracing/coresight/coresight-syscfg.h | 39 +++++ include/linux/coresight.h | 1 + 7 files changed, 207 insertions(+), 4 deletions(-) create mode 100644 drivers/hwtracing/coresight/coresight-syscfg.c create mode 100644 drivers/hwtracing/coresight/coresight-syscfg.h

diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile index 1b35b55bd420..3605dd770664 100644 --- a/drivers/hwtracing/coresight/Makefile +++ b/drivers/hwtracing/coresight/Makefile @@ -4,7 +4,7 @@ # obj-$(CONFIG_CORESIGHT) += coresight.o coresight-y := coresight-core.o coresight-etm-perf.o coresight-platform.o \

•         coresight-sysfs.o
  

•         coresight-sysfs.o coresight-syscfg.o
  

obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o coresight-tmc-y := coresight-tmc-core.o coresight-tmc-etf.o \ coresight-tmc-etr.o diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index bf6edf468963..91f1fbd7d51d 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -21,6 +21,7 @@

#include "coresight-etm-perf.h" #include "coresight-priv.h" +#include "coresight-syscfg.h"

static DEFINE_MUTEX(coresight_mutex);

@@ -1669,8 +1670,17 @@ char *coresight_alloc_device_name(struct coresight_dev_list *dict, } EXPORT_SYMBOL_GPL(coresight_alloc_device_name);

+/* match for virtual csdevs that have no underlying amba device */ +static int coresight_bus_match(struct device *dev, struct device_driver *drv) +{

• struct coresight_device *csdev = to_coresight_device(dev);
  

• return csdev->type == CORESIGHT_DEV_TYPE_SYSCFG;
  

+}

struct bus_type coresight_bustype = { .name = "coresight",

• .match  = coresight_bus_match,
  

};

static int __init coresight_init(void) @@ -1683,13 +1693,29 @@ static int __init coresight_init(void)

  ret = etm_perf_init();
  if (ret)

•         bus_unregister(&coresight_bustype);
  

•         goto exit_bus_unregister;
  

• /* initialise the coresight syscfg driver and device */
  

• ret = cscfg_driver_init();
  

• if (ret)
  

•         goto exit_perf_clear;
  

• ret = cscfg_create_device();
  

• if (!ret)
  

•         return 0;
  
  

• cscfg_driver_exit();
  

+exit_perf_clear:

• etm_perf_exit();
  

+exit_bus_unregister:

• bus_unregister(&coresight_bustype);
  return ret;
  

}

static void __exit coresight_exit(void) {

• cscfg_clear_device();
  

• cscfg_driver_exit();
  etm_perf_exit();
  bus_unregister(&coresight_bustype);
  

} @@ -1700,3 +1726,4 @@ module_exit(coresight_exit); MODULE_AUTHOR("Pratik Patel pratikp@codeaurora.org"); MODULE_AUTHOR("Mathieu Poirier mathieu.poirier@linaro.org"); MODULE_DESCRIPTION("Arm CoreSight tracer driver"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 668b3ff11576..2d619b764738 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -619,7 +619,7 @@ int __init etm_perf_init(void) return ret; }

-void __exit etm_perf_exit(void) +void etm_perf_exit(void) { perf_pmu_unregister(&etm_pmu); } diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h index 3e4f2ad5e193..29d90dfeba31 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.h +++ b/drivers/hwtracing/coresight/coresight-etm-perf.h @@ -83,6 +83,6 @@ static inline void *etm_perf_sink_config(struct perf_output_handle *handle) #endif /* CONFIG_CORESIGHT */

int __init etm_perf_init(void); -void __exit etm_perf_exit(void); +void etm_perf_exit(void);

#endif diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c new file mode 100644 index 000000000000..badef8cfc7f8 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-syscfg.c @@ -0,0 +1,136 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (c) 2020 Linaro Limited, All rights reserved.
• • Author: Mike Leach mike.leach@linaro.org
• */

+#include <linux/platform_device.h>

+#include "coresight-syscfg.h"

+/*

• • cs_syscfg device is a virtual device managing the overall cs system.
• • It allows the loading of configurations and features, and loads these into
• • coresight devices as appropriate.
• */

+static DEFINE_MUTEX(cs_cfg_mutex);

+/* only one of these */ +static struct cs_cfg_device *cscfg_dev;

+static struct device_type cscfg_dev_type = {

• .name = "cs_syscfg",
  

+};

+#define to_device_cscfg() (&cscfg_dev->csdev.dev) +#define to_coresight_device_cscfg() (&cscfg_dev->csdev)

+/* cs_syscfg device instance handling */ +static void cscfg_device_release(struct device *dev) +{

• kfree(cscfg_dev->csdev.pdata);
  

• kfree(cscfg_dev);
  

• cscfg_dev = NULL;
  

+}

+int cscfg_create_device(void) +{

• struct device *dev;
  

• struct coresight_device *csdev;
  

• struct coresight_platform_data *pdata = NULL;
  

• int err = -ENOMEM;
  

• mutex_lock(&cs_cfg_mutex);
  

• if (cscfg_dev) {
  

•         err = -EINVAL;
  

•         goto create_dev_exit_unlock;
  

• }
  

• cscfg_dev = kzalloc(sizeof(*cscfg_dev), GFP_KERNEL);
  

• if (!cscfg_dev)
  

•         goto create_dev_exit_unlock;
  

• pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
  

• if (!pdata) {
  

•         kfree(cscfg_dev);
  

•         cscfg_dev = NULL;
  

•         goto create_dev_exit_unlock;
  

• }
  

• /* setup the device */
  

• dev = to_device_cscfg();
  

• dev_set_name(dev, "cs_syscfg");
  

• dev->bus = &coresight_bustype;
  

• dev->parent = &platform_bus;
  

• dev->type = &cscfg_dev_type;
  

As far as I can tell setting the device type doesn't do anything.

It provides name for the entry in /sys/devices/... - ie.

/sys/devices/platform/cs_syscfg

The device name is set in dev_set_name(), which is sufficient for proper labelling in sysfs.

...

...

• /* setup a coresight device */
  

• csdev = to_coresight_device_cscfg();
  

• csdev->type = CORESIGHT_DEV_TYPE_SYSCFG;
  

• csdev->pdata = pdata;
  

• csdev->orphan = false;
  

• csdev->dev.release = cscfg_device_release;
  

I also fail to understand the requirement to use a coresight_device and to add it to the coresigh bus. To me it seems odd to have it there since it has no interaction with any of the other CS devices.

Not at this early stage - this patch just introduces the device, without much functionality at present. But later on it becomes capable of loading features and configurations and enabling them across relevant devices on the bus.

The bus_type is very rich in feature and should provide everything you need. Using a csdev should be the very last option.

...

...

• err = device_register(dev);
  

+create_dev_exit_unlock:

• mutex_unlock(&cs_cfg_mutex);
  

• return err;
  

+}

+void cscfg_clear_device(void) +{

• mutex_lock(&cs_cfg_mutex);
  

• device_unregister(to_device_cscfg());
  

• mutex_unlock(&cs_cfg_mutex);
  

+}

+/* Finish allocating resources and load built-in configurations and features. */ +static int cscfg_probe(struct device *dev) +{

• int ret = 0;
  

• /* there can be only one... */
  

• if (dev != to_device_cscfg())
  

•         return -EINVAL;
  

• /*
  

•  * No actual hardware to handle, just allocate resources.
  

•  * Probe is triggered when the core calls cscfg_create_device() to
  

•  * create the device, so the mutex is already held.
  

•  */
  

• INIT_LIST_HEAD(&cscfg_dev->dev_list);
  

• INIT_LIST_HEAD(&cscfg_dev->feat_list);
  

• INIT_LIST_HEAD(&cscfg_dev->config_list);
  

• dev_info(dev, "CSCFG initialised");
  

• return ret;
  

+}

+/* driver initialisation */ +static struct cs_cfg_driver cs_config_driver = {

This set uses cs_cfg_ and cscfg_ to declare structures and function, making it very difficult to read the code. Please pick one (I'd go with the latter but that's a personal opinion) and stick with it.

Agreed - I started off using cs_cfg_.. for data structures, and cscfg_... for functions - but I'll use just the one as it is easier.

...

I also don't think the functionality provided in this file need to be a driver, it can simply be a set of functions handling configuration and features.

The thinking here was that:-

1. a virtual device is a good way of modelling functionality that

affects the entire coresight subsystem - i.e. treat it as a compound device. 2) A device object / kobject is required to support some kernel functionality such as sysfs and configfs when that is added.

That said, my work on configfs - which is not included in this set - would seem to indicate an explicit driver may not be needed, and a set of callback functions and data structures defined in the coresight module may be sufficient. So it may be possible to drop the driver abastaction for now and re-introduce if it does become necessary.

I think that makes sense and will help making this set smaller.

Thanks for the review

I will continue with patch 3 and 4 today.

Mike

...

...

• .drv = {
  

•         .name = "coresight-syscfg",
  

•         .owner = THIS_MODULE,
  

•         .suppress_bind_attrs = true,
  

•         .probe = cscfg_probe,
  

• },
  

+};

+/* called from coresight core init */ +int __init cscfg_driver_init(void) +{

• cs_config_driver.drv.bus = &coresight_bustype;
  

• return driver_register(&cs_config_driver.drv);
  

+}

+void cscfg_driver_exit(void) +{

• driver_unregister(&cs_config_driver.drv);
  

+} diff --git a/drivers/hwtracing/coresight/coresight-syscfg.h b/drivers/hwtracing/coresight/coresight-syscfg.h new file mode 100644 index 000000000000..20ccca5e4151 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-syscfg.h @@ -0,0 +1,39 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/*

• • Coresight system configuration driver.
• */

+#ifndef CORESIGHT_SYSCFG_H +#define CORESIGHT_SYSCFG_H

+#include <linux/coresight.h> +#include <linux/device.h>

+/**

• • System configuration device.
• • @csdev: The coresight device instance.
• • @dev_list: List of other coresight devices registered with this device.
• • @feat_list: List of features to load into registered devices.
• • @config_list:List of system configurations to load into registered devices.
• */

+struct cs_cfg_device {

• struct coresight_device csdev;
  

• struct list_head dev_list;
  

• struct list_head feat_list;
  

• struct list_head config_list;
  

• int nr_csdev;
  

+};

+/* basic device driver */ +struct cs_cfg_driver {

• struct device_driver    drv;
  

+};

+/* internal core operations for cscfg */ +int cscfg_create_device(void); +void cscfg_clear_device(void); +int __init cscfg_driver_init(void); +void cscfg_driver_exit(void);

+#endif /* CORESIGHT_SYSCFG_H */ diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 7d3c87e5b97c..ed0a9d938303 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -42,6 +42,7 @@ enum coresight_dev_type { CORESIGHT_DEV_TYPE_SOURCE, CORESIGHT_DEV_TYPE_HELPER, CORESIGHT_DEV_TYPE_ECT,

• CORESIGHT_DEV_TYPE_SYSCFG,
  

};

enum coresight_dev_subtype_sink {

2.17.1

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

On Thu, Oct 15, 2020 at 11:44:30AM +0100, Mike Leach wrote:

Hi Mathieu,

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

...

On Tue, Oct 06, 2020 at 11:40:46AM +0100, Mike Leach wrote:

...

Hi Mathieu,

On Thu, 1 Oct 2020 at 21:07, Mathieu Poirier mathieu.poirier@linaro.org wrote:

...

Good afternoon Mike,

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

...

Creates a virtual coresight device on the bus to handle system configuration of the coresight infrastructure.

The device will manage system wide configuration, and allow complex programmed features to be added to individual device instances, and provide for system wide configuration selection on trace capture operations.

This patch updates the coresight core initialisation to create the single instance of the cs_syscfg coresight system device, on the coresight bus.

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

drivers/hwtracing/coresight/Makefile | 2 +- drivers/hwtracing/coresight/coresight-core.c | 29 +++- .../hwtracing/coresight/coresight-etm-perf.c | 2 +- .../hwtracing/coresight/coresight-etm-perf.h | 2 +- .../hwtracing/coresight/coresight-syscfg.c | 136 ++++++++++++++++++ .../hwtracing/coresight/coresight-syscfg.h | 39 +++++ include/linux/coresight.h | 1 + 7 files changed, 207 insertions(+), 4 deletions(-) create mode 100644 drivers/hwtracing/coresight/coresight-syscfg.c create mode 100644 drivers/hwtracing/coresight/coresight-syscfg.h

diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile index 1b35b55bd420..3605dd770664 100644 --- a/drivers/hwtracing/coresight/Makefile +++ b/drivers/hwtracing/coresight/Makefile @@ -4,7 +4,7 @@ # obj-$(CONFIG_CORESIGHT) += coresight.o coresight-y := coresight-core.o coresight-etm-perf.o coresight-platform.o \

•         coresight-sysfs.o
  

•         coresight-sysfs.o coresight-syscfg.o
  

obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o coresight-tmc-y := coresight-tmc-core.o coresight-tmc-etf.o \ coresight-tmc-etr.o diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index bf6edf468963..91f1fbd7d51d 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -21,6 +21,7 @@

#include "coresight-etm-perf.h" #include "coresight-priv.h" +#include "coresight-syscfg.h"

static DEFINE_MUTEX(coresight_mutex);

@@ -1669,8 +1670,17 @@ char *coresight_alloc_device_name(struct coresight_dev_list *dict, } EXPORT_SYMBOL_GPL(coresight_alloc_device_name);

+/* match for virtual csdevs that have no underlying amba device */ +static int coresight_bus_match(struct device *dev, struct device_driver *drv) +{

• struct coresight_device *csdev = to_coresight_device(dev);
  

• return csdev->type == CORESIGHT_DEV_TYPE_SYSCFG;
  

+}

struct bus_type coresight_bustype = { .name = "coresight",

• .match  = coresight_bus_match,
  

};

static int __init coresight_init(void) @@ -1683,13 +1693,29 @@ static int __init coresight_init(void)

  ret = etm_perf_init();
  if (ret)

•         bus_unregister(&coresight_bustype);
  

•         goto exit_bus_unregister;
  

• /* initialise the coresight syscfg driver and device */
  

• ret = cscfg_driver_init();
  

• if (ret)
  

•         goto exit_perf_clear;
  

• ret = cscfg_create_device();
  

• if (!ret)
  

•         return 0;
  
  

• cscfg_driver_exit();
  

+exit_perf_clear:

• etm_perf_exit();
  

+exit_bus_unregister:

• bus_unregister(&coresight_bustype);
  return ret;
  

}

static void __exit coresight_exit(void) {

• cscfg_clear_device();
  

• cscfg_driver_exit();
  etm_perf_exit();
  bus_unregister(&coresight_bustype);
  

} @@ -1700,3 +1726,4 @@ module_exit(coresight_exit); MODULE_AUTHOR("Pratik Patel pratikp@codeaurora.org"); MODULE_AUTHOR("Mathieu Poirier mathieu.poirier@linaro.org"); MODULE_DESCRIPTION("Arm CoreSight tracer driver"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 668b3ff11576..2d619b764738 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -619,7 +619,7 @@ int __init etm_perf_init(void) return ret; }

-void __exit etm_perf_exit(void) +void etm_perf_exit(void) { perf_pmu_unregister(&etm_pmu); } diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h index 3e4f2ad5e193..29d90dfeba31 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.h +++ b/drivers/hwtracing/coresight/coresight-etm-perf.h @@ -83,6 +83,6 @@ static inline void *etm_perf_sink_config(struct perf_output_handle *handle) #endif /* CONFIG_CORESIGHT */

int __init etm_perf_init(void); -void __exit etm_perf_exit(void); +void etm_perf_exit(void);

#endif diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c new file mode 100644 index 000000000000..badef8cfc7f8 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-syscfg.c @@ -0,0 +1,136 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (c) 2020 Linaro Limited, All rights reserved.
• • Author: Mike Leach mike.leach@linaro.org
• */

+#include <linux/platform_device.h>

+#include "coresight-syscfg.h"

+/*

• • cs_syscfg device is a virtual device managing the overall cs system.
• • It allows the loading of configurations and features, and loads these into
• • coresight devices as appropriate.
• */

+static DEFINE_MUTEX(cs_cfg_mutex);

+/* only one of these */ +static struct cs_cfg_device *cscfg_dev;

+static struct device_type cscfg_dev_type = {

• .name = "cs_syscfg",
  

+};

+#define to_device_cscfg() (&cscfg_dev->csdev.dev) +#define to_coresight_device_cscfg() (&cscfg_dev->csdev)

+/* cs_syscfg device instance handling */ +static void cscfg_device_release(struct device *dev) +{

• kfree(cscfg_dev->csdev.pdata);
  

• kfree(cscfg_dev);
  

• cscfg_dev = NULL;
  

+}

+int cscfg_create_device(void) +{

• struct device *dev;
  

• struct coresight_device *csdev;
  

• struct coresight_platform_data *pdata = NULL;
  

• int err = -ENOMEM;
  

• mutex_lock(&cs_cfg_mutex);
  

• if (cscfg_dev) {
  

•         err = -EINVAL;
  

•         goto create_dev_exit_unlock;
  

• }
  

• cscfg_dev = kzalloc(sizeof(*cscfg_dev), GFP_KERNEL);
  

• if (!cscfg_dev)
  

•         goto create_dev_exit_unlock;
  

• pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
  

• if (!pdata) {
  

•         kfree(cscfg_dev);
  

•         cscfg_dev = NULL;
  

•         goto create_dev_exit_unlock;
  

• }
  

• /* setup the device */
  

• dev = to_device_cscfg();
  

• dev_set_name(dev, "cs_syscfg");
  

• dev->bus = &coresight_bustype;
  

• dev->parent = &platform_bus;
  

• dev->type = &cscfg_dev_type;
  

As far as I can tell setting the device type doesn't do anything.

It provides name for the entry in /sys/devices/... - ie.

/sys/devices/platform/cs_syscfg

The device name is set in dev_set_name(), which is sufficient for proper labelling in sysfs.

...

...

...

• /* setup a coresight device */
  

• csdev = to_coresight_device_cscfg();
  

• csdev->type = CORESIGHT_DEV_TYPE_SYSCFG;
  

• csdev->pdata = pdata;
  

• csdev->orphan = false;
  

• csdev->dev.release = cscfg_device_release;
  

I also fail to understand the requirement to use a coresight_device and to add it to the coresigh bus. To me it seems odd to have it there since it has no interaction with any of the other CS devices.

Not at this early stage - this patch just introduces the device, without much functionality at present. But later on it becomes capable of loading features and configurations and enabling them across relevant devices on the bus.

The bus_type is very rich in feature and should provide everything you need. Using a csdev should be the very last option.

...

...

...

• err = device_register(dev);
  

+create_dev_exit_unlock:

• mutex_unlock(&cs_cfg_mutex);
  

• return err;
  

+}

+void cscfg_clear_device(void) +{

• mutex_lock(&cs_cfg_mutex);
  

• device_unregister(to_device_cscfg());
  

• mutex_unlock(&cs_cfg_mutex);
  

+}

+/* Finish allocating resources and load built-in configurations and features. */ +static int cscfg_probe(struct device *dev) +{

• int ret = 0;
  

• /* there can be only one... */
  

• if (dev != to_device_cscfg())
  

•         return -EINVAL;
  

• /*
  

•  * No actual hardware to handle, just allocate resources.
  

•  * Probe is triggered when the core calls cscfg_create_device() to
  

•  * create the device, so the mutex is already held.
  

•  */
  

• INIT_LIST_HEAD(&cscfg_dev->dev_list);
  

• INIT_LIST_HEAD(&cscfg_dev->feat_list);
  

• INIT_LIST_HEAD(&cscfg_dev->config_list);
  

• dev_info(dev, "CSCFG initialised");
  

• return ret;
  

+}

+/* driver initialisation */ +static struct cs_cfg_driver cs_config_driver = {

This set uses cs_cfg_ and cscfg_ to declare structures and function, making it very difficult to read the code. Please pick one (I'd go with the latter but that's a personal opinion) and stick with it.

Agreed - I started off using cs_cfg_.. for data structures, and cscfg_... for functions - but I'll use just the one as it is easier.

...

I also don't think the functionality provided in this file need to be a driver, it can simply be a set of functions handling configuration and features.

The thinking here was that:-

1. a virtual device is a good way of modelling functionality that

affects the entire coresight subsystem - i.e. treat it as a compound device. 2) A device object / kobject is required to support some kernel functionality such as sysfs and configfs when that is added.

That said, my work on configfs - which is not included in this set - would seem to indicate an explicit driver may not be needed, and a set of callback functions and data structures defined in the coresight module may be sufficient. So it may be possible to drop the driver abastaction for now and re-introduce if it does become necessary.

I think that makes sense and will help making this set smaller.

One requirement I didn't spot when dropping the syscfg device is the link into the perf cs_etm event.

I have been working on the perf events part of the set, and found that without an "owner" device we cannot set the configuration information under cs_etm. We cannot use individual csdevs from ETM etc as these can appear / disappear. The 'sinks" names are directly owned by the individual sinks - configurations are system wide so need a system owner.

This means I now have to re-introduce the syscfg coresight device.

You are correct - working with sysfs means that a device has to be involved. But it doesn't have to be a csdev and doesn't have to be added to the CS bus either. As far as I can see in this set the csdev itself isn't used but the device associated to the csdev is.

Regards

Mike

...

...

Thanks for the review

I will continue with patch 3 and 4 today.

...

Mike

...

...

• .drv = {
  

•         .name = "coresight-syscfg",
  

•         .owner = THIS_MODULE,
  

•         .suppress_bind_attrs = true,
  

•         .probe = cscfg_probe,
  

• },
  

+};

+/* called from coresight core init */ +int __init cscfg_driver_init(void) +{

• cs_config_driver.drv.bus = &coresight_bustype;
  

• return driver_register(&cs_config_driver.drv);
  

+}

+void cscfg_driver_exit(void) +{

• driver_unregister(&cs_config_driver.drv);
  

+} diff --git a/drivers/hwtracing/coresight/coresight-syscfg.h b/drivers/hwtracing/coresight/coresight-syscfg.h new file mode 100644 index 000000000000..20ccca5e4151 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-syscfg.h @@ -0,0 +1,39 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/*

• • Coresight system configuration driver.
• */

+#ifndef CORESIGHT_SYSCFG_H +#define CORESIGHT_SYSCFG_H

+#include <linux/coresight.h> +#include <linux/device.h>

+/**

• • System configuration device.
• • @csdev: The coresight device instance.
• • @dev_list: List of other coresight devices registered with this device.
• • @feat_list: List of features to load into registered devices.
• • @config_list:List of system configurations to load into registered devices.
• */

+struct cs_cfg_device {

• struct coresight_device csdev;
  

• struct list_head dev_list;
  

• struct list_head feat_list;
  

• struct list_head config_list;
  

• int nr_csdev;
  

+};

+/* basic device driver */ +struct cs_cfg_driver {

• struct device_driver    drv;
  

+};

+/* internal core operations for cscfg */ +int cscfg_create_device(void); +void cscfg_clear_device(void); +int __init cscfg_driver_init(void); +void cscfg_driver_exit(void);

+#endif /* CORESIGHT_SYSCFG_H */ diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 7d3c87e5b97c..ed0a9d938303 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -42,6 +42,7 @@ enum coresight_dev_type { CORESIGHT_DEV_TYPE_SOURCE, CORESIGHT_DEV_TYPE_HELPER, CORESIGHT_DEV_TYPE_ECT,

• CORESIGHT_DEV_TYPE_SYSCFG,
  

};

enum coresight_dev_subtype_sink {

2.17.1

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

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

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

Add API call to load features and configurations into the system lists.

Defines generic structures and flags needed to describe features and configurations.

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

.../hwtracing/coresight/coresight-config.h | 154 ++++++++++++++++++ .../hwtracing/coresight/coresight-syscfg.c | 116 +++++++++++++ .../hwtracing/coresight/coresight-syscfg.h | 26 +++ 3 files changed, 296 insertions(+) create mode 100644 drivers/hwtracing/coresight/coresight-config.h

diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h new file mode 100644 index 000000000000..413509f7d817 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-config.h @@ -0,0 +1,154 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*

• • Copyright (c) 2020 Linaro Limited, All rights reserved.
• • Author: Mike Leach mike.leach@linaro.org
• */

+#ifndef _CORESIGHT_CORESIGHT_CONFIG_H +#define _CORESIGHT_CORESIGHT_CONFIG_H

+#include <linux/coresight.h> +#include <linux/types.h>

+/* CoreSight Configuration - complex component and system config handling */

+/* generic register flags */ +#define CS_CFG_REG_STD 0x80000000 /* reg is standard reg */ +#define CS_CFG_REG_RESOURCE 0x40000000 /* reg is a resource */ +#define CS_CFG_REG_VAL_PARAM 0x08000000 /* reg value uses param */ +#define CS_CFG_REG_VAL_MASK 0x04000000 /* reg value bit masked */ +#define CS_CFG_REG_VAL_64BIT 0x02000000 /* reg value 64 bit */ +#define CS_CFG_REG_VAL_SAVE 0x01000000 /* reg value save on disable */ +#define CS_CFG_REG_ID_MASK 0x00000FFF /* reg offset / id in device */ +#define CS_CFG_REG_DEV_MASK 0x00FFF000 /* device specific flags */

+/*

• • flags defining what device class a feature will match to when processing a
• • system configuration - used by config data and devices.
• */

+#define CS_CFG_MATCH_CLASS_SRC_ALL 0x0001 /* match any source */ +#define CS_CFG_MATCH_CLASS_SRC_ETM4 0x0002 /* match any ETMv4 device */ +#define CS_CFG_MATCH_CLASS_CTI_ALL 0x0004 /* match any CTI device */ +#define CS_CFG_MATCH_CLASS_CTI_CPU 0x0008 +#define CS_CFG_MATCH_CLASS_CTI_SYS 0x0010

+/* flags defining device instance matching - used in config data. */ +#define CS_CFG_MATCH_INST_ANY 0x80000000 /* any instance of a class */ +#define CS_CFG_MATCH_INST_AMBA_DEVICE 0x40000000 /* specific amba device */

+/* limit number of presets in a configuration */ +#define CS_CFG_CONFIG_PRESET_MAX 15

Any reason to set the maximum number of presets at 15? Could it be a list to be more flexible?

+/**

• • Parameter descriptor for a device feature.
• • @name: Name of parameter.
• • @value: Initial or default value.
• */

+struct cs_cfg_parameter_desc {

• const char *name;
• u64 value;

+};

+/**

• • Representation of register value.
• • Supports full 64 bit register value, or 32 bit value with optional mask
• • value.
• */

+union cs_cfg_regval {

• u64 val64;
• struct {

• u32 val32;
  

• u32 mask32;
  

• };

+};

+/**

• • Register descriptor for a device feature.
• • Flags define 32bit, 32bit+mask, 64bit.
• • @flags: flags defining the handling of the register.
• • @value: Initial value & optional mask.
• */

The initial values of registers could be sufficient to work with a feature - is my understanding correct?

+struct cs_cfg_reg_desc {

• u32 flags;
• union cs_cfg_regval value;

+};

Is this structure absolutely required? Can @flag be added to cs_cfg_regval to avoid the declaration of a new structure?

+/**

• • Device feature descriptor - combination of registers and parameters to
• • program a device to implement a specific complex function.
• • @name: feature name.
• • @brief: brief description of the feature.
• • @match_flags: matching information if loading into a device
• • @params: list of parameters used - 0 terminated.
• • @reg: list of registers used.
• */

+struct cs_cfg_feature_desc {

I suggest calling this cs_cfg_feature. That way we can rename cs_cfg_uses_feat_desc to cs_cfg_feature_desc and drop the "uses" part.

• const char *name;
• const char *brief;
• u32 match_flags;
• struct cs_cfg_parameter_desc *params;
• struct cs_cfg_reg_desc *regs;

+};

+/**

• • Device / feature matching information.
• • Used by loading configurations to define which class or specific devices
• • they want to match used features to, and registered devices to specify which
• • matching class and information they support.
• • The load process uses these matching pairs to load feature instances into
• • matching devices.
• */

+struct cs_cfg_match_info {

• u32 flags; /* match flags */

Please use "flags" or "match_flags" but not both.

• /* unique device name (e.g. amba bus name) for specific device match */
• const char *dev_name;

+};

+/**

• • Uses feature description.
• • @name: name of feature to use.
• • @nr_params: number of parameters the feature has.
• • @match: match info for the feature in use - may be all devices of a

• • class or a specific device in that class.
    

• */

+struct cs_cfg_uses_feat_desc {

• const char *name;

If I remember correctly @name needs to match with cs_cfg_feature_desc::name. If that is the case please add a comment to explicitly link them. That will make it easier for people to make the connection.

• int nr_params;

Shouldn't this be defined in cs_cfg_feature_desc?

• struct cs_cfg_match_info match;

+};

+/**

• • Configuration descriptor - describes selectable system configuration.
• • A configuration describes device features in use, and may provide preset
• • values for the parameters in those features.
• • A single set of presets is the sum of the parameters declared by
• • all the features in use - this value is @nr_total_params.
• • @name: name of the configuration - used for selection.
• • @brief: description of the purpose of the configuration.
• • @nr_uses: Number of features used in this configuration.
• • @used: feature use descriptors.
• • @nr_presets: Number of sets of presets supplied by this configuration.
• • @nr_total_params: Sum of all parameters declared by used features
• • @presets: Array of preset values.
• */

+struct cs_cfg_config_desc {

• const char *name;
• const char *brief;
• int nr_uses;

s/nr_uses/nr_feature_desc

• struct cs_cfg_uses_feat_desc *uses;

s/cs_cfg_uses_feat_desc *uses/cs_cfg_feature_desc *feature_desc

• int nr_presets;
• int nr_total_params;
• const u64 *presets; /* nr_presets * nr_total_params */

What is the difference between presets and the values held by cs_cfg_parameter_desc? Can we get rid of the latter?

+};

+#endif /* _CORESIGHT_CORESIGHT_CONFIG_H */ diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c index badef8cfc7f8..d49bcace76c8 100644 --- a/drivers/hwtracing/coresight/coresight-syscfg.c +++ b/drivers/hwtracing/coresight/coresight-syscfg.c @@ -6,6 +6,7 @@ #include <linux/platform_device.h> +#include "coresight-config.h" #include "coresight-syscfg.h" @@ -28,6 +29,121 @@ static struct device_type cscfg_dev_type = { #define to_device_cscfg() (&cscfg_dev->csdev.dev) #define to_coresight_device_cscfg() (&cscfg_dev->csdev) +/* load features and configuations into the lists */

+/* check feature list for a named feature - call with mutex locked. */ +static bool cscfg_match_list_feat(const char *name) +{

• struct cscfg_featlist_item *curr_item;
• list_for_each_entry(curr_item, &cscfg_dev->feat_list, item) {

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

• 	return true;
  

• }
• return false;

+}

+/* check all feat needed for cfg are in the list - call with mutex locked. */ +static int cscfg_check_feat_for_cfg(struct cs_cfg_config_desc *cfg_desc) +{

• int i;
• for (i = 0; i < cfg_desc->nr_uses; i++)

• if (!cscfg_match_list_feat(cfg_desc->uses[i].name))
  

• 	return -EINVAL;
  

• return 0;

+}

+/*

• • load feature - add to feature list.
• */

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

• struct cscfg_featlist_item *feat_item;
• if (!cscfg_dev)

• return -ENODEV;
  

• /* create a list item and add to the list */
• feat_item = devm_kzalloc(to_device_cscfg(),

• 		 sizeof(struct cscfg_featlist_item),
  

• 		 GFP_KERNEL);
  

• if (!feat_item)

• return -ENOMEM;
  

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

+}

+/*

• • load config into the system - validate used features exist then add to
• • config list.
• */

+static int cscfg_load_config(struct cs_cfg_config_desc *cfg_desc) +{

• int err = -ENODEV;
• struct cs_cfg_config *cfg;
• if (!cscfg_dev)

• return -ENODEV;
  

• /* validate features are present */
• err = cscfg_check_feat_for_cfg(cfg_desc);
• if (err)

• return err;
  

• /* add to list */
• cfg = devm_kzalloc(to_device_cscfg(),

• 	   sizeof(struct cs_cfg_config), GFP_KERNEL);
  

• if (!cfg)

• return -ENOMEM;
  

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

+}

+/*

• • External API function to load feature and config sets.
• • Take a 0 terminated array of feature descriptors and/or configuration
• • descriptors and load into the system.
• • Features are loaded first to ensure configuration dependencies can be met.
• */

+int cscfg_load_config_sets(struct cs_cfg_config_desc **cfg_descs,

• 	   struct cs_cfg_feature_desc **feat_descs)
  

+{

• int err, i = 0;
• mutex_lock(&cs_cfg_mutex);
• /* load features first */
• if (feat_descs) {

• while (feat_descs[i]) {
  

• 	err = cscfg_load_feat(feat_descs[i]);
  

• 	if (err)
  

• 		return err;
  

• 	i++;
  

• }
  

• }
• /* next any configurations to check feature dependencies */
• i = 0;
• if (cfg_descs) {

• while (cfg_descs[i]) {
  

• 	err = cscfg_load_config(cfg_descs[i]);
  

• 	if (err)
  

• 		return err;
  

• 	i++;
  

• }
  

• }

Features and configurations aren't unloaded when an error occurs. That is probably something that could go in csfg_device_release().

• mutex_unlock(&cs_cfg_mutex);
• return err;

+} +EXPORT_SYMBOL_GPL(cscfg_load_config_sets); /* 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 20ccca5e4151..b3d41096382d 100644 --- a/drivers/hwtracing/coresight/coresight-syscfg.h +++ b/drivers/hwtracing/coresight/coresight-syscfg.h @@ -30,10 +30,36 @@ struct cs_cfg_driver { struct device_driver drv; }; +/**

• • List entry for feature loaded into the system feature list.
• • @desc: Descriptor for the feature.
• • @item: List entry
• */

+struct cscfg_featlist_item {

• const struct cs_cfg_feature_desc *desc;
• struct list_head item;

+};

I would add the list_head to structure cs_cfg_feature_desc and get rid of cscfg_featlist_item.

+/**

• • List entry for configuration added to system config list.
• • @desc: Descriptor for the configuration.
• • @item: List entry.
• */

+struct cs_cfg_config {

• const struct cs_cfg_config_desc *desc;
• struct list_head item;

+};

Same as above.

/* internal core operations for cscfg */ int cscfg_create_device(void); void cscfg_clear_device(void); int __init cscfg_driver_init(void); 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);
  

#endif /* CORESIGHT_SYSCFG_H */

2.17.1

On Tue, Oct 06, 2020 at 12:18:51PM +0100, Mike Leach wrote:

Hi Mathieu,

On Thu, 1 Oct 2020 at 22:20, Mathieu Poirier mathieu.poirier@linaro.org wrote:

...

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

...

Add API call to load features and configurations into the system lists.

Defines generic structures and flags needed to describe features and configurations.

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

.../hwtracing/coresight/coresight-config.h | 154 ++++++++++++++++++ .../hwtracing/coresight/coresight-syscfg.c | 116 +++++++++++++ .../hwtracing/coresight/coresight-syscfg.h | 26 +++ 3 files changed, 296 insertions(+) create mode 100644 drivers/hwtracing/coresight/coresight-config.h

diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h new file mode 100644 index 000000000000..413509f7d817 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-config.h @@ -0,0 +1,154 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*

• • Copyright (c) 2020 Linaro Limited, All rights reserved.
• • Author: Mike Leach mike.leach@linaro.org
• */

+#ifndef _CORESIGHT_CORESIGHT_CONFIG_H +#define _CORESIGHT_CORESIGHT_CONFIG_H

+#include <linux/coresight.h> +#include <linux/types.h>

+/* CoreSight Configuration - complex component and system config handling */

+/* generic register flags */ +#define CS_CFG_REG_STD 0x80000000 /* reg is standard reg */ +#define CS_CFG_REG_RESOURCE 0x40000000 /* reg is a resource */ +#define CS_CFG_REG_VAL_PARAM 0x08000000 /* reg value uses param */ +#define CS_CFG_REG_VAL_MASK 0x04000000 /* reg value bit masked */ +#define CS_CFG_REG_VAL_64BIT 0x02000000 /* reg value 64 bit */ +#define CS_CFG_REG_VAL_SAVE 0x01000000 /* reg value save on disable */ +#define CS_CFG_REG_ID_MASK 0x00000FFF /* reg offset / id in device */ +#define CS_CFG_REG_DEV_MASK 0x00FFF000 /* device specific flags */

+/*

• • flags defining what device class a feature will match to when processing a
• • system configuration - used by config data and devices.
• */

+#define CS_CFG_MATCH_CLASS_SRC_ALL 0x0001 /* match any source */ +#define CS_CFG_MATCH_CLASS_SRC_ETM4 0x0002 /* match any ETMv4 device */ +#define CS_CFG_MATCH_CLASS_CTI_ALL 0x0004 /* match any CTI device */ +#define CS_CFG_MATCH_CLASS_CTI_CPU 0x0008 +#define CS_CFG_MATCH_CLASS_CTI_SYS 0x0010

+/* flags defining device instance matching - used in config data. */ +#define CS_CFG_MATCH_INST_ANY 0x80000000 /* any instance of a class */ +#define CS_CFG_MATCH_INST_AMBA_DEVICE 0x40000000 /* specific amba device */

+/* limit number of presets in a configuration */ +#define CS_CFG_CONFIG_PRESET_MAX 15

Any reason to set the maximum number of presets at 15? Could it be a list to be more flexible?

This is linked to the number of bits (4) used to select presets in the perf config2 value, introduced later in the perf updates. We can certainly increase the number of bits ./ max entries - but this seems like a reasonable starting point.

Oh boy - the relation with the 4 bits in config2 was completely lost on me. Adding a comment to clearly identify the link between the two would go a long way.

That being said I remember frowning a little when I looked at how config2 was used and something I will pay close attention to in this second pass.

...

...

+/**

• • Parameter descriptor for a device feature.
• • @name: Name of parameter.
• • @value: Initial or default value.
• */

+struct cs_cfg_parameter_desc {

• const char *name;
  

• u64 value;
  

+};

+/**

• • Representation of register value.
• • Supports full 64 bit register value, or 32 bit value with optional mask
• • value.
• */

+union cs_cfg_regval {

• u64 val64;
  

• struct {
  

•         u32 val32;
  

•         u32 mask32;
  

• };
  

+};

+/**

• • Register descriptor for a device feature.
• • Flags define 32bit, 32bit+mask, 64bit.
• • @flags: flags defining the handling of the register.
• • @value: Initial value & optional mask.
• */

The initial values of registers could be sufficient to work with a feature - is my understanding correct?

Correct. Values in the description of the feature form the defaults for programming the device. These can be absolute values - which are used directly, resource values - where the loading process will select an appropriate resource (e.g. etm counter), or parameter value, which has a default value that can be updated at runtime via sysfs / configfs.

...

...

+struct cs_cfg_reg_desc {

• u32 flags;
  

• union cs_cfg_regval value;
  

+};

Is this structure absolutely required? Can @flag be added to cs_cfg_regval to avoid the declaration of a new structure?

The structures are split into 'descriptors' - which describe a feature / configuration and 'instances' which represent the feature loaded into a specific device. So in a system with 4 ETMv4 , a single feature descriptor for ETMv4, will result in 4 instances loaded into the respective ETM devices. The reg_val appears in both.

That said, for simplification purposes, we could move the flags into the value and drop this structure at the cost of a very marginal memory increase.

Thanks

...

...

+/**

• • Device feature descriptor - combination of registers and parameters to
• • program a device to implement a specific complex function.
• • @name: feature name.
• • @brief: brief description of the feature.
• • @match_flags: matching information if loading into a device
• • @params: list of parameters used - 0 terminated.
• • @reg: list of registers used.
• */

+struct cs_cfg_feature_desc {

I suggest calling this cs_cfg_feature. That way we can rename cs_cfg_uses_feat_desc to cs_cfg_feature_desc and drop the "uses" part.

cs_cfg_feature exists as the structure used for the instance of a feature loaded into a device, introduced in a later patch.

My goal is to find a different label than "uses". It took me a while wrap my head around it and even after that I kept tripping on it.

Maybe keep it as it is for now and we can revisit as this set matures.

...

...

• const char *name;
  

• const char *brief;
  

• u32 match_flags;
  

• struct cs_cfg_parameter_desc *params;
  

• struct cs_cfg_reg_desc *regs;
  

+};

+/**

• • Device / feature matching information.
• • Used by loading configurations to define which class or specific devices
• • they want to match used features to, and registered devices to specify which
• • matching class and information they support.
• • The load process uses these matching pairs to load feature instances into
• • matching devices.
• */

+struct cs_cfg_match_info {

• u32 flags;      /* match flags */
  

Please use "flags" or "match_flags" but not both.

Will do.

...

...

• /* unique device name (e.g. amba bus name) for specific device match */
  

• const char *dev_name;
  

+};

+/**

• • Uses feature description.
• • @name: name of feature to use.
• • @nr_params: number of parameters the feature has.
• • @match: match info for the feature in use - may be all devices of a

• •       class or a specific device in that class.
    

• */

+struct cs_cfg_uses_feat_desc {

• const char *name;
  

If I remember correctly @name needs to match with cs_cfg_feature_desc::name. If that is the case please add a comment to explicitly link them. That will make it easier for people to make the connection.

will do.

...

...

• int nr_params;
  

Shouldn't this be defined in cs_cfg_feature_desc?

yes - that would be better.

...

...

• struct cs_cfg_match_info match;
  

+};

+/**

• • Configuration descriptor - describes selectable system configuration.
• • A configuration describes device features in use, and may provide preset
• • values for the parameters in those features.
• • A single set of presets is the sum of the parameters declared by
• • all the features in use - this value is @nr_total_params.
• • @name: name of the configuration - used for selection.
• • @brief: description of the purpose of the configuration.
• • @nr_uses: Number of features used in this configuration.
• • @used: feature use descriptors.
• • @nr_presets: Number of sets of presets supplied by this configuration.
• • @nr_total_params: Sum of all parameters declared by used features
• • @presets: Array of preset values.
• */

+struct cs_cfg_config_desc {

• const char *name;
  

• const char *brief;
  

• int nr_uses;
  

s/nr_uses/nr_feature_desc

...

• struct cs_cfg_uses_feat_desc *uses;
  

s/cs_cfg_uses_feat_desc *uses/cs_cfg_feature_desc *feature_desc

...

• int nr_presets;
  

• int nr_total_params;
  

• const u64 *presets; /* nr_presets * nr_total_params */
  

What is the difference between presets and the values held by cs_cfg_parameter_desc? Can we get rid of the latter?

The cs_cfg_parameter_desc represent the default parameter values for the feature - which can be loaded / used independently of a configuration that provides presets.

...

...

+};

+#endif /* _CORESIGHT_CORESIGHT_CONFIG_H */ diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c index badef8cfc7f8..d49bcace76c8 100644 --- a/drivers/hwtracing/coresight/coresight-syscfg.c +++ b/drivers/hwtracing/coresight/coresight-syscfg.c @@ -6,6 +6,7 @@

#include <linux/platform_device.h>

+#include "coresight-config.h" #include "coresight-syscfg.h"

@@ -28,6 +29,121 @@ static struct device_type cscfg_dev_type = { #define to_device_cscfg() (&cscfg_dev->csdev.dev) #define to_coresight_device_cscfg() (&cscfg_dev->csdev)

+/* load features and configuations into the lists */

+/* check feature list for a named feature - call with mutex locked. */ +static bool cscfg_match_list_feat(const char *name) +{

• struct cscfg_featlist_item *curr_item;
  

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

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

•                 return true;
  

• }
  

• return false;
  

+}

+/* check all feat needed for cfg are in the list - call with mutex locked. */ +static int cscfg_check_feat_for_cfg(struct cs_cfg_config_desc *cfg_desc) +{

• int i;
  

• for (i = 0; i < cfg_desc->nr_uses; i++)
  

•         if (!cscfg_match_list_feat(cfg_desc->uses[i].name))
  

•                 return -EINVAL;
  

• return 0;
  

+}

+/*

• • load feature - add to feature list.
• */

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

• struct cscfg_featlist_item *feat_item;
  

• if (!cscfg_dev)
  

•         return -ENODEV;
  

• /* create a list item and add to the list */
  

• feat_item = devm_kzalloc(to_device_cscfg(),
  

•                          sizeof(struct cscfg_featlist_item),
  

•                          GFP_KERNEL);
  

• if (!feat_item)
  

•         return -ENOMEM;
  

• feat_item->desc = feat_desc;
  

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

• return 0;
  

+}

+/*

• • load config into the system - validate used features exist then add to
• • config list.
• */

+static int cscfg_load_config(struct cs_cfg_config_desc *cfg_desc) +{

• int err = -ENODEV;
  

• struct cs_cfg_config *cfg;
  

• if (!cscfg_dev)
  

•         return -ENODEV;
  

• /* validate features are present */
  

• err = cscfg_check_feat_for_cfg(cfg_desc);
  

• if (err)
  

•         return err;
  

• /* add to list */
  

• cfg = devm_kzalloc(to_device_cscfg(),
  

•                    sizeof(struct cs_cfg_config), GFP_KERNEL);
  

• if (!cfg)
  

•         return -ENOMEM;
  

• cfg->desc = cfg_desc;
  

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

• return 0;
  

+}

+/*

• • External API function to load feature and config sets.
• • Take a 0 terminated array of feature descriptors and/or configuration
• • descriptors and load into the system.
• • Features are loaded first to ensure configuration dependencies can be met.
• */

+int cscfg_load_config_sets(struct cs_cfg_config_desc **cfg_descs,

•                    struct cs_cfg_feature_desc **feat_descs)
  

+{

• int err, i = 0;
  

• mutex_lock(&cs_cfg_mutex);
  

• /* load features first */
  

• if (feat_descs) {
  

•         while (feat_descs[i]) {
  

•                 err = cscfg_load_feat(feat_descs[i]);
  

•                 if (err)
  

•                         return err;
  

•                 i++;
  

•         }
  

• }
  

• /* next any configurations to check feature dependencies */
  

• i = 0;
  

• if (cfg_descs) {
  

•         while (cfg_descs[i]) {
  

•                 err = cscfg_load_config(cfg_descs[i]);
  

•                 if (err)
  

•                         return err;
  

•                 i++;
  

•         }
  

• }
  

Features and configurations aren't unloaded when an error occurs. That is probably something that could go in csfg_device_release().

At present, configurations & features can only be loaded via the built-in block of code. This really shouldn't have errors - but we should probably have a least a message here.. The error handling is broken as has been pointed out in Yabin's mail.

Ok

Once dynamic loading is permitted - either by loadable module or though the configfs - the handing of unload will be added. For a given set, failing to load a configuration does not necessarily invalidate the previous features, which can be used directly or in other configurations. At present the memory associated with loading configurations and features into the device is allocate using the devm_ allocators associated with the virtual syscfg device - and thus is correctly cleaned up.

...

...

• mutex_unlock(&cs_cfg_mutex);
  

• return err;
  

+} +EXPORT_SYMBOL_GPL(cscfg_load_config_sets);

/* 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 20ccca5e4151..b3d41096382d 100644 --- a/drivers/hwtracing/coresight/coresight-syscfg.h +++ b/drivers/hwtracing/coresight/coresight-syscfg.h @@ -30,10 +30,36 @@ struct cs_cfg_driver { struct device_driver drv; };

+/**

• • List entry for feature loaded into the system feature list.
• • @desc: Descriptor for the feature.
• • @item: List entry
• */

+struct cscfg_featlist_item {

• const struct cs_cfg_feature_desc *desc;
  

• struct list_head item;
  

+};

I would add the list_head to structure cs_cfg_feature_desc and get rid of cscfg_featlist_item.

agreed.

...

...

+/**

• • List entry for configuration added to system config list.
• • @desc: Descriptor for the configuration.
• • @item: List entry.
• */

+struct cs_cfg_config {

• const struct cs_cfg_config_desc *desc;
  

• struct list_head item;
  

+};

Same as above.

agreed.

...

...

/* internal core operations for cscfg */ int cscfg_create_device(void); void cscfg_clear_device(void); int __init cscfg_driver_init(void); 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);
  

#endif /* CORESIGHT_SYSCFG_H */

2.17.1

Thanks for the review

Regards

Mike

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

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.

• 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;

• 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()

• 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().

• 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().

• 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

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.