[PATCH 0/7] perf: Communicate sink via event::attr:config2

[PATCH 0/7] perf: Communicate sink via event::attr:config2

[Mathieu Poirier]

From: "Mathieu Poirier]" <mathieu.poirier@linaro.org>

This set is a refurbished version of this one [1].  I dropped the version
count and changed the name because a new approach is taken.

The end result is the same though, that is to allow multiple sources to
select the same sink for a session which is a prerequisite for the CoreSight
support of CPU-wide trace scenarios.

Here the sink ID is communicated to the kernel by way of the event's
configuration attribure (event::attr::config2).  The ID itself is an
hash of the sink's name, something that is readily available and doesn't
require the addition of a new information field in the coresight_device
structure.

With this new strategy the mechanic used to communicate sink selection to the
kernel via sysfs is no longer needed and removed from the code base.

Everthing has been applied and tested on 5.0-rc2.

Regards,
Mathieu

PS: Greg, I'm drawing your attention to patch 02 where I seem to be the first
to use function sysfs_add_file_to_group() in this way.

[1]. https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1818488.html

Mathieu Poirier (6):
  perf/aux: Make perf_event accessible to setup_aux()
  coresight: perf: Add "sinks" group to PMU directory
  coresight: Use event attributes for sink selection
  perf tools: Use event attributes to send sink information to kernel
  perf tools: Removing CoreSight set_drv_config() API
  perf tools: Remove PMU::set_drv_config API

linaro (1):
  perf pmu: Moving EVENT_SOURCE_DEVICE_PATH to PMU header file

 arch/s390/kernel/perf_cpum_sf.c               |  6 +-
 arch/x86/events/intel/bts.c                   |  4 +-
 arch/x86/events/intel/pt.c                    |  5 +-
 .../hwtracing/coresight/coresight-etm-perf.c  | 65 ++++++++++---
 .../hwtracing/coresight/coresight-etm-perf.h  |  1 +
 drivers/hwtracing/coresight/coresight-priv.h  |  1 +
 drivers/hwtracing/coresight/coresight.c       | 56 +++++++++++
 drivers/perf/arm_spe_pmu.c                    |  6 +-
 include/linux/perf_event.h                    |  2 +-
 kernel/events/ring_buffer.c                   |  2 +-
 tools/perf/arch/arm/util/cs-etm.c             | 94 ++++++++-----------
 tools/perf/arch/arm/util/cs-etm.h             |  3 -
 tools/perf/arch/arm/util/pmu.c                |  2 -
 tools/perf/builtin-record.c                   | 10 --
 tools/perf/builtin-stat.c                     |  9 --
 tools/perf/builtin-top.c                      | 13 ---
 tools/perf/util/Build                         |  1 -
 tools/perf/util/drv_configs.c                 | 78 ---------------
 tools/perf/util/drv_configs.h                 | 26 -----
 tools/perf/util/pmu.c                         |  2 -
 tools/perf/util/pmu.h                         |  2 +-
 21 files changed, 163 insertions(+), 225 deletions(-)
 delete mode 100644 tools/perf/util/drv_configs.c
 delete mode 100644 tools/perf/util/drv_configs.h

-- 
2.17.1

[PATCH 1/7] perf/aux: Make perf_event accessible to setup_aux()

[Mathieu Poirier]

When pmu::setup_aux() is called the coresight PMU needs to know which
sink to use for the session by looking up the information in the
event's attr::config2 field.

As such simply replace the cpu information by the complete perf_event
structure and change all affected customers.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 arch/s390/kernel/perf_cpum_sf.c                  | 6 +++---
 arch/x86/events/intel/bts.c                      | 4 +++-
 arch/x86/events/intel/pt.c                       | 5 +++--
 drivers/hwtracing/coresight/coresight-etm-perf.c | 6 +++---
 drivers/perf/arm_spe_pmu.c                       | 6 +++---
 include/linux/perf_event.h                       | 2 +-
 kernel/events/ring_buffer.c                      | 2 +-
 7 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/arch/s390/kernel/perf_cpum_sf.c b/arch/s390/kernel/perf_cpum_sf.c
index bfabeb1889cc..1266194afb02 100644
--- a/arch/s390/kernel/perf_cpum_sf.c
+++ b/arch/s390/kernel/perf_cpum_sf.c
@@ -1600,7 +1600,7 @@ static void aux_sdb_init(unsigned long sdb)
 
 /*
  * aux_buffer_setup() - Setup AUX buffer for diagnostic mode sampling
- * @cpu:	On which to allocate, -1 means current
+ * @event:	Event the buffer is setup for, event->cpu == -1 means current
  * @pages:	Array of pointers to buffer pages passed from perf core
  * @nr_pages:	Total pages
  * @snapshot:	Flag for snapshot mode
@@ -1612,8 +1612,8 @@ static void aux_sdb_init(unsigned long sdb)
  *
  * Return the private AUX buffer structure if success or NULL if fails.
  */
-static void *aux_buffer_setup(int cpu, void **pages, int nr_pages,
-			      bool snapshot)
+static void *aux_buffer_setup(struct perf_event *event, void **pages,
+			      int nr_pages, bool snapshot)
 {
 	struct sf_buffer *sfb;
 	struct aux_buffer *aux;
diff --git a/arch/x86/events/intel/bts.c b/arch/x86/events/intel/bts.c
index a01ef1b0f883..7cdd7b13bbda 100644
--- a/arch/x86/events/intel/bts.c
+++ b/arch/x86/events/intel/bts.c
@@ -77,10 +77,12 @@ static size_t buf_size(struct page *page)
 }
 
 static void *
-bts_buffer_setup_aux(int cpu, void **pages, int nr_pages, bool overwrite)
+bts_buffer_setup_aux(struct perf_event *event, void **pages,
+		     int nr_pages, bool overwrite)
 {
 	struct bts_buffer *buf;
 	struct page *page;
+	int cpu = event->cpu;
 	int node = (cpu == -1) ? cpu : cpu_to_node(cpu);
 	unsigned long offset;
 	size_t size = nr_pages << PAGE_SHIFT;
diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index 9494ca68fd9d..c0e86ff21f81 100644
--- a/arch/x86/events/intel/pt.c
+++ b/arch/x86/events/intel/pt.c
@@ -1114,10 +1114,11 @@ static int pt_buffer_init_topa(struct pt_buffer *buf, unsigned long nr_pages,
  * Return:	Our private PT buffer structure.
  */
 static void *
-pt_buffer_setup_aux(int cpu, void **pages, int nr_pages, bool snapshot)
+pt_buffer_setup_aux(struct perf_event *event, void **pages,
+		    int nr_pages, bool snapshot)
 {
 	struct pt_buffer *buf;
-	int node, ret;
+	int node, ret, cpu = event->cpu;
 
 	if (!nr_pages)
 		return NULL;
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index abe8249b893b..f21eb28b6782 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -177,15 +177,15 @@ static void etm_free_aux(void *data)
 	schedule_work(&event_data->work);
 }
 
-static void *etm_setup_aux(int event_cpu, void **pages,
+static void *etm_setup_aux(struct perf_event *event, void **pages,
 			   int nr_pages, bool overwrite)
 {
-	int cpu;
+	int cpu = event->cpu;
 	cpumask_t *mask;
 	struct coresight_device *sink;
 	struct etm_event_data *event_data = NULL;
 
-	event_data = alloc_event_data(event_cpu);
+	event_data = alloc_event_data(cpu);
 	if (!event_data)
 		return NULL;
 	INIT_WORK(&event_data->work, free_event_data);
diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
index 8e46a9dad2fa..7cb766dafe85 100644
--- a/drivers/perf/arm_spe_pmu.c
+++ b/drivers/perf/arm_spe_pmu.c
@@ -824,10 +824,10 @@ static void arm_spe_pmu_read(struct perf_event *event)
 {
 }
 
-static void *arm_spe_pmu_setup_aux(int cpu, void **pages, int nr_pages,
-				   bool snapshot)
+static void *arm_spe_pmu_setup_aux(struct perf_event *event, void **pages,
+				   int nr_pages, bool snapshot)
 {
-	int i;
+	int i, cpu = event->cpu;
 	struct page **pglist;
 	struct arm_spe_pmu_buf *buf;
 
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 1d5c551a5add..3e49b2144808 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -409,7 +409,7 @@ struct pmu {
 	/*
 	 * Set up pmu-private data structures for an AUX area
 	 */
-	void *(*setup_aux)		(int cpu, void **pages,
+	void *(*setup_aux)		(struct perf_event *event, void **pages,
 					 int nr_pages, bool overwrite);
 					/* optional */
 
diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index 4a9937076331..857308295f63 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -658,7 +658,7 @@ int rb_alloc_aux(struct ring_buffer *rb, struct perf_event *event,
 			goto out;
 	}
 
-	rb->aux_priv = event->pmu->setup_aux(event->cpu, rb->aux_pages, nr_pages,
+	rb->aux_priv = event->pmu->setup_aux(event, rb->aux_pages, nr_pages,
 					     overwrite);
 	if (!rb->aux_priv)
 		goto out;
-- 
2.17.1

Re: [PATCH 1/7] perf/aux: Make perf_event accessible to setup_aux()

[Suzuki K Poulose]

On 15/01/2019 23:07, Mathieu Poirier wrote:
> When pmu::setup_aux() is called the coresight PMU needs to know which
> sink to use for the session by looking up the information in the
> event's attr::config2 field.
> 
> As such simply replace the cpu information by the complete perf_event
> structure and change all affected customers.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---

Looks fine to me. FWIW:

Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>

[PATCH 2/7] coresight: perf: Add "sinks" group to PMU directory

[Mathieu Poirier]

Add a "sinks" directory entry so that users can see all the sinks
available in the system in a single place.  Individual sink are added
as they are registered with the coresight bus.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 .../hwtracing/coresight/coresight-etm-perf.c  | 43 +++++++++++++++++++
 .../hwtracing/coresight/coresight-etm-perf.h  |  1 +
 drivers/hwtracing/coresight/coresight.c       | 17 ++++++++
 3 files changed, 61 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index f21eb28b6782..292bd409a68c 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -14,6 +14,7 @@
 #include <linux/perf_event.h>
 #include <linux/percpu-defs.h>
 #include <linux/slab.h>
+#include <linux/stringhash.h>
 #include <linux/types.h>
 #include <linux/workqueue.h>
 
@@ -43,8 +44,18 @@ static const struct attribute_group etm_pmu_format_group = {
 	.attrs  = etm_config_formats_attr,
 };
 
+static struct attribute *etm_config_sinks_attr[] = {
+	NULL,
+};
+
+static const struct attribute_group etm_pmu_sinks_group = {
+	.name   = "sinks",
+	.attrs  = etm_config_sinks_attr,
+};
+
 static const struct attribute_group *etm_pmu_attr_groups[] = {
 	&etm_pmu_format_group,
+	&etm_pmu_sinks_group,
 	NULL,
 };
 
@@ -479,6 +490,38 @@ int etm_perf_symlink(struct coresight_device *csdev, bool link)
 	return 0;
 }
 
+static ssize_t etm_perf_sink_name_show(struct device *dev,
+				       struct device_attribute *dattr,
+				       char *buf)
+{
+	/* See function coresight_sink_by_id() to know where this is used */
+	u32 hash = hashlen_hash(hashlen_string(NULL, dattr->attr.name));
+
+	return scnprintf(buf, PAGE_SIZE, "%x\n", hash);
+}
+
+int etm_perf_symlink_sink(struct coresight_device *csdev)
+{
+	struct device *pmu_dev = etm_pmu.dev;
+	struct device *pdev = csdev->dev.parent;
+	struct device_attribute *dev_attr;
+
+	if (csdev->type != CORESIGHT_DEV_TYPE_SINK &&
+	    csdev->type != CORESIGHT_DEV_TYPE_LINKSINK)
+		return -EINVAL;
+
+	if (!etm_perf_up)
+		return -EPROBE_DEFER;
+
+	dev_attr = kzalloc(sizeof(*dev_attr), GFP_KERNEL);
+	dev_attr->attr.name = kstrdup(dev_name(pdev), GFP_KERNEL);
+	dev_attr->attr.mode = 0444;
+	dev_attr->show = etm_perf_sink_name_show;
+
+	return sysfs_add_file_to_group(&pmu_dev->kobj,
+				       &dev_attr->attr, "sinks");
+}
+
 static int __init etm_perf_init(void)
 {
 	int ret;
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h
index da7d9336a15c..529a47285c0f 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.h
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.h
@@ -59,6 +59,7 @@ struct etm_event_data {
 
 #ifdef CONFIG_CORESIGHT
 int etm_perf_symlink(struct coresight_device *csdev, bool link);
+int etm_perf_symlink_sink(struct coresight_device *csdev);
 static inline void *etm_perf_sink_config(struct perf_output_handle *handle)
 {
 	struct etm_event_data *data = perf_get_aux(handle);
diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index 2b0df1a0a8df..526f122a38ee 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -18,6 +18,7 @@
 #include <linux/delay.h>
 #include <linux/pm_runtime.h>
 
+#include "coresight-etm-perf.h"
 #include "coresight-priv.h"
 
 static DEFINE_MUTEX(coresight_mutex);
@@ -1167,6 +1168,22 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
 		goto err_out;
 	}
 
+	if (csdev->type == CORESIGHT_DEV_TYPE_SINK ||
+	    csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) {
+		ret = etm_perf_symlink_sink(csdev);
+
+		if (ret) {
+			device_unregister(&csdev->dev);
+			/*
+			 * As with the above, all resources are free'd
+			 * explicitly via coresight_device_release() triggered
+			 * from put_device(), which is in turn called from
+			 * function device_unregister().
+			 */
+			goto err_out;
+		}
+	}
+
 	mutex_lock(&coresight_mutex);
 
 	coresight_fixup_device_conns(csdev);
-- 
2.17.1

Re: [PATCH 2/7] coresight: perf: Add "sinks" group to PMU directory

[Suzuki K Poulose]

On 15/01/2019 23:07, Mathieu Poirier wrote:
> Add a "sinks" directory entry so that users can see all the sinks
> available in the system in a single place.  Individual sink are added
> as they are registered with the coresight bus.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>   .../hwtracing/coresight/coresight-etm-perf.c  | 43 +++++++++++++++++++
>   .../hwtracing/coresight/coresight-etm-perf.h  |  1 +
>   drivers/hwtracing/coresight/coresight.c       | 17 ++++++++
>   3 files changed, 61 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index f21eb28b6782..292bd409a68c 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -14,6 +14,7 @@
>   #include <linux/perf_event.h>
>   #include <linux/percpu-defs.h>
>   #include <linux/slab.h>
> +#include <linux/stringhash.h>
>   #include <linux/types.h>
>   #include <linux/workqueue.h>
>   
> @@ -43,8 +44,18 @@ static const struct attribute_group etm_pmu_format_group = {
>   	.attrs  = etm_config_formats_attr,
>   };
>   
> +static struct attribute *etm_config_sinks_attr[] = {
> +	NULL,
> +};
> +
> +static const struct attribute_group etm_pmu_sinks_group = {
> +	.name   = "sinks",
> +	.attrs  = etm_config_sinks_attr,
> +};
> +
>   static const struct attribute_group *etm_pmu_attr_groups[] = {
>   	&etm_pmu_format_group,
> +	&etm_pmu_sinks_group,
>   	NULL,
>   };
>   
> @@ -479,6 +490,38 @@ int etm_perf_symlink(struct coresight_device *csdev, bool link)
>   	return 0;
>   }
>   
> +static ssize_t etm_perf_sink_name_show(struct device *dev,
> +				       struct device_attribute *dattr,
> +				       char *buf)
> +{
> +	/* See function coresight_sink_by_id() to know where this is used */
> +	u32 hash = hashlen_hash(hashlen_string(NULL, dattr->attr.name));
> +
> +	return scnprintf(buf, PAGE_SIZE, "%x\n", hash);
> +}
> +
> +int etm_perf_symlink_sink(struct coresight_device *csdev)
> +{
> +	struct device *pmu_dev = etm_pmu.dev;
> +	struct device *pdev = csdev->dev.parent;
> +	struct device_attribute *dev_attr;
> +
> +	if (csdev->type != CORESIGHT_DEV_TYPE_SINK &&
> +	    csdev->type != CORESIGHT_DEV_TYPE_LINKSINK)
> +		return -EINVAL;
> +
> +	if (!etm_perf_up)
> +		return -EPROBE_DEFER;
> +
> +	dev_attr = kzalloc(sizeof(*dev_attr), GFP_KERNEL);
> +	dev_attr->attr.name = kstrdup(dev_name(pdev), GFP_KERNEL);
> +	dev_attr->attr.mode = 0444;
> +	dev_attr->show = etm_perf_sink_name_show;

I would have  added the attribute to the csdev (say, sink_attr),
and add that to the group, so that it is easier to remove the
attribute when the sink device is removed from the system (when
we get there). It would be good to have something in place to remove the
attribute.

Otherwise, the overall approach looks fine to me.

Suzuki

Re: [PATCH 2/7] coresight: perf: Add "sinks" group to PMU directory

[Mathieu Poirier]

Good evening Suzuki,

On Wed, 16 Jan 2019 at 03:29, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>
>
>
> On 15/01/2019 23:07, Mathieu Poirier wrote:
> > Add a "sinks" directory entry so that users can see all the sinks
> > available in the system in a single place.  Individual sink are added
> > as they are registered with the coresight bus.
> >
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >   .../hwtracing/coresight/coresight-etm-perf.c  | 43 +++++++++++++++++++
> >   .../hwtracing/coresight/coresight-etm-perf.h  |  1 +
> >   drivers/hwtracing/coresight/coresight.c       | 17 ++++++++
> >   3 files changed, 61 insertions(+)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > index f21eb28b6782..292bd409a68c 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > @@ -14,6 +14,7 @@
> >   #include <linux/perf_event.h>
> >   #include <linux/percpu-defs.h>
> >   #include <linux/slab.h>
> > +#include <linux/stringhash.h>
> >   #include <linux/types.h>
> >   #include <linux/workqueue.h>
> >
> > @@ -43,8 +44,18 @@ static const struct attribute_group etm_pmu_format_group = {
> >       .attrs  = etm_config_formats_attr,
> >   };
> >
> > +static struct attribute *etm_config_sinks_attr[] = {
> > +     NULL,
> > +};
> > +
> > +static const struct attribute_group etm_pmu_sinks_group = {
> > +     .name   = "sinks",
> > +     .attrs  = etm_config_sinks_attr,
> > +};
> > +
> >   static const struct attribute_group *etm_pmu_attr_groups[] = {
> >       &etm_pmu_format_group,
> > +     &etm_pmu_sinks_group,
> >       NULL,
> >   };
> >
> > @@ -479,6 +490,38 @@ int etm_perf_symlink(struct coresight_device *csdev, bool link)
> >       return 0;
> >   }
> >
> > +static ssize_t etm_perf_sink_name_show(struct device *dev,
> > +                                    struct device_attribute *dattr,
> > +                                    char *buf)
> > +{
> > +     /* See function coresight_sink_by_id() to know where this is used */
> > +     u32 hash = hashlen_hash(hashlen_string(NULL, dattr->attr.name));
> > +
> > +     return scnprintf(buf, PAGE_SIZE, "%x\n", hash);
> > +}
> > +
> > +int etm_perf_symlink_sink(struct coresight_device *csdev)
> > +{
> > +     struct device *pmu_dev = etm_pmu.dev;
> > +     struct device *pdev = csdev->dev.parent;
> > +     struct device_attribute *dev_attr;
> > +
> > +     if (csdev->type != CORESIGHT_DEV_TYPE_SINK &&
> > +         csdev->type != CORESIGHT_DEV_TYPE_LINKSINK)
> > +             return -EINVAL;
> > +
> > +     if (!etm_perf_up)
> > +             return -EPROBE_DEFER;
> > +
> > +     dev_attr = kzalloc(sizeof(*dev_attr), GFP_KERNEL);
> > +     dev_attr->attr.name = kstrdup(dev_name(pdev), GFP_KERNEL);
> > +     dev_attr->attr.mode = 0444;
> > +     dev_attr->show = etm_perf_sink_name_show;
>
> I would have  added the attribute to the csdev (say, sink_attr),
> and add that to the group, so that it is easier to remove the
> attribute when the sink device is removed from the system (when
> we get there). It would be good to have something in place to remove the
> attribute.

My hope was to avoid introducing a new field in the already bloated
coresight_device structure, and on top of things a component specific
field.  I think it would be worth it if we'd envision making the
coresight drivers removable in a not so distant future.  But doing
something like that is quite tricky (as Kim quickly found out) and
skirts the bottom of the list of priorities, if on it at all.

I'll change it if you're really keen on it but it would be code that
is never used.

Mathieu

>
> Otherwise, the overall approach looks fine to me.
>
> Suzuki

Re: [PATCH 2/7] coresight: perf: Add "sinks" group to PMU directory

[Suzuki K Poulose]

Hi Mathieu,

On 16/01/2019 23:43, Mathieu Poirier wrote:
> Good evening Suzuki,
> 
> On Wed, 16 Jan 2019 at 03:29, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>>
>>
>>
>> On 15/01/2019 23:07, Mathieu Poirier wrote:
>>> Add a "sinks" directory entry so that users can see all the sinks
>>> available in the system in a single place.  Individual sink are added
>>> as they are registered with the coresight bus.
>>>
>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>

...

>>> +static ssize_t etm_perf_sink_name_show(struct device *dev,
>>> +                                    struct device_attribute *dattr,
>>> +                                    char *buf)
>>> +{
>>> +     /* See function coresight_sink_by_id() to know where this is used */
>>> +     u32 hash = hashlen_hash(hashlen_string(NULL, dattr->attr.name));
>>> +
>>> +     return scnprintf(buf, PAGE_SIZE, "%x\n", hash);
>>> +}
>>> +
>>> +int etm_perf_symlink_sink(struct coresight_device *csdev)
>>> +{
>>> +     struct device *pmu_dev = etm_pmu.dev;
>>> +     struct device *pdev = csdev->dev.parent;
>>> +     struct device_attribute *dev_attr;
>>> +
>>> +     if (csdev->type != CORESIGHT_DEV_TYPE_SINK &&
>>> +         csdev->type != CORESIGHT_DEV_TYPE_LINKSINK)
>>> +             return -EINVAL;
>>> +
>>> +     if (!etm_perf_up)
>>> +             return -EPROBE_DEFER;
>>> +
>>> +     dev_attr = kzalloc(sizeof(*dev_attr), GFP_KERNEL);
>>> +     dev_attr->attr.name = kstrdup(dev_name(pdev), GFP_KERNEL);
>>> +     dev_attr->attr.mode = 0444;
>>> +     dev_attr->show = etm_perf_sink_name_show;
>>
>> I would have  added the attribute to the csdev (say, sink_attr),
>> and add that to the group, so that it is easier to remove the
>> attribute when the sink device is removed from the system (when
>> we get there). It would be good to have something in place to remove the
>> attribute.
> 
> My hope was to avoid introducing a new field in the already bloated
> coresight_device structure, and on top of things a component specific
> field.  I think it would be worth it if we'd envision making the

I agree. May be we could add a union for the fields specific to the "Type" of
the device.

> coresight drivers removable in a not so distant future.  But doing
> something like that is quite tricky (as Kim quickly found out) and
> skirts the bottom of the list of priorities, if on it at all.

> 
> I'll change it if you're really keen on it but it would be code that
> is never used.

Yes, I understand. But we would want to get there sometime in the future,
in order to allow using the Coresight out of the box on Enterprise systems.
So it would be good to prepare towards that as we go.


Also, it makes the failure handling easier.

Cheers
Suzuki

Re: [PATCH 2/7] coresight: perf: Add "sinks" group to PMU directory

[Greg KH]

On Tue, Jan 15, 2019 at 04:07:37PM -0700, Mathieu Poirier wrote:
> Add a "sinks" directory entry so that users can see all the sinks
> available in the system in a single place.  Individual sink are added
> as they are registered with the coresight bus.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  .../hwtracing/coresight/coresight-etm-perf.c  | 43 +++++++++++++++++++
>  .../hwtracing/coresight/coresight-etm-perf.h  |  1 +
>  drivers/hwtracing/coresight/coresight.c       | 17 ++++++++
>  3 files changed, 61 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index f21eb28b6782..292bd409a68c 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -14,6 +14,7 @@
>  #include <linux/perf_event.h>
>  #include <linux/percpu-defs.h>
>  #include <linux/slab.h>
> +#include <linux/stringhash.h>
>  #include <linux/types.h>
>  #include <linux/workqueue.h>
>  
> @@ -43,8 +44,18 @@ static const struct attribute_group etm_pmu_format_group = {
>  	.attrs  = etm_config_formats_attr,
>  };
>  
> +static struct attribute *etm_config_sinks_attr[] = {
> +	NULL,
> +};
> +
> +static const struct attribute_group etm_pmu_sinks_group = {
> +	.name   = "sinks",
> +	.attrs  = etm_config_sinks_attr,
> +};
> +
>  static const struct attribute_group *etm_pmu_attr_groups[] = {
>  	&etm_pmu_format_group,
> +	&etm_pmu_sinks_group,
>  	NULL,
>  };
>  
> @@ -479,6 +490,38 @@ int etm_perf_symlink(struct coresight_device *csdev, bool link)
>  	return 0;
>  }
>  
> +static ssize_t etm_perf_sink_name_show(struct device *dev,
> +				       struct device_attribute *dattr,
> +				       char *buf)
> +{
> +	/* See function coresight_sink_by_id() to know where this is used */
> +	u32 hash = hashlen_hash(hashlen_string(NULL, dattr->attr.name));
> +
> +	return scnprintf(buf, PAGE_SIZE, "%x\n", hash);
> +}
> +
> +int etm_perf_symlink_sink(struct coresight_device *csdev)
> +{
> +	struct device *pmu_dev = etm_pmu.dev;
> +	struct device *pdev = csdev->dev.parent;
> +	struct device_attribute *dev_attr;
> +
> +	if (csdev->type != CORESIGHT_DEV_TYPE_SINK &&
> +	    csdev->type != CORESIGHT_DEV_TYPE_LINKSINK)
> +		return -EINVAL;
> +
> +	if (!etm_perf_up)
> +		return -EPROBE_DEFER;
> +
> +	dev_attr = kzalloc(sizeof(*dev_attr), GFP_KERNEL);
> +	dev_attr->attr.name = kstrdup(dev_name(pdev), GFP_KERNEL);
> +	dev_attr->attr.mode = 0444;
> +	dev_attr->show = etm_perf_sink_name_show;
> +
> +	return sysfs_add_file_to_group(&pmu_dev->kobj,
> +				       &dev_attr->attr, "sinks");

What is so odd about this call that you needed me to review this?

And what happens if this call fails, do you leak memory?

thanks,

greg k-h

Re: [PATCH 2/7] coresight: perf: Add "sinks" group to PMU directory

[Mathieu Poirier]

On Wed, 16 Jan 2019 at 08:39, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Jan 15, 2019 at 04:07:37PM -0700, Mathieu Poirier wrote:
> > Add a "sinks" directory entry so that users can see all the sinks
> > available in the system in a single place.  Individual sink are added
> > as they are registered with the coresight bus.
> >
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >  .../hwtracing/coresight/coresight-etm-perf.c  | 43 +++++++++++++++++++
> >  .../hwtracing/coresight/coresight-etm-perf.h  |  1 +
> >  drivers/hwtracing/coresight/coresight.c       | 17 ++++++++
> >  3 files changed, 61 insertions(+)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > index f21eb28b6782..292bd409a68c 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/perf_event.h>
> >  #include <linux/percpu-defs.h>
> >  #include <linux/slab.h>
> > +#include <linux/stringhash.h>
> >  #include <linux/types.h>
> >  #include <linux/workqueue.h>
> >
> > @@ -43,8 +44,18 @@ static const struct attribute_group etm_pmu_format_group = {
> >       .attrs  = etm_config_formats_attr,
> >  };
> >
> > +static struct attribute *etm_config_sinks_attr[] = {
> > +     NULL,
> > +};
> > +
> > +static const struct attribute_group etm_pmu_sinks_group = {
> > +     .name   = "sinks",
> > +     .attrs  = etm_config_sinks_attr,
> > +};
> > +
> >  static const struct attribute_group *etm_pmu_attr_groups[] = {
> >       &etm_pmu_format_group,
> > +     &etm_pmu_sinks_group,
> >       NULL,
> >  };
> >
> > @@ -479,6 +490,38 @@ int etm_perf_symlink(struct coresight_device *csdev, bool link)
> >       return 0;
> >  }
> >
> > +static ssize_t etm_perf_sink_name_show(struct device *dev,
> > +                                    struct device_attribute *dattr,
> > +                                    char *buf)
> > +{
> > +     /* See function coresight_sink_by_id() to know where this is used */
> > +     u32 hash = hashlen_hash(hashlen_string(NULL, dattr->attr.name));
> > +
> > +     return scnprintf(buf, PAGE_SIZE, "%x\n", hash);
> > +}
> > +
> > +int etm_perf_symlink_sink(struct coresight_device *csdev)
> > +{
> > +     struct device *pmu_dev = etm_pmu.dev;
> > +     struct device *pdev = csdev->dev.parent;
> > +     struct device_attribute *dev_attr;
> > +
> > +     if (csdev->type != CORESIGHT_DEV_TYPE_SINK &&
> > +         csdev->type != CORESIGHT_DEV_TYPE_LINKSINK)
> > +             return -EINVAL;
> > +
> > +     if (!etm_perf_up)
> > +             return -EPROBE_DEFER;
> > +
> > +     dev_attr = kzalloc(sizeof(*dev_attr), GFP_KERNEL);
> > +     dev_attr->attr.name = kstrdup(dev_name(pdev), GFP_KERNEL);
> > +     dev_attr->attr.mode = 0444;
> > +     dev_attr->show = etm_perf_sink_name_show;
> > +
> > +     return sysfs_add_file_to_group(&pmu_dev->kobj,
> > +                                    &dev_attr->attr, "sinks");
>
> What is so odd about this call that you needed me to review this?

As far as I can tell nobody is feeding a dynamic struct attribute to
the function and I wasn't sure if it was because they were told not to
or simply because it wasn't needed, hence asking for a second opinion.

>
> And what happens if this call fails, do you leak memory?
>
> thanks,
>
> greg k-h

Re: [PATCH 2/7] coresight: perf: Add "sinks" group to PMU directory

[Greg KH]

On Wed, Jan 16, 2019 at 09:14:33AM -0700, Mathieu Poirier wrote:
> On Wed, 16 Jan 2019 at 08:39, Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Jan 15, 2019 at 04:07:37PM -0700, Mathieu Poirier wrote:
> > > Add a "sinks" directory entry so that users can see all the sinks
> > > available in the system in a single place.  Individual sink are added
> > > as they are registered with the coresight bus.
> > >
> > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > > ---
> > >  .../hwtracing/coresight/coresight-etm-perf.c  | 43 +++++++++++++++++++
> > >  .../hwtracing/coresight/coresight-etm-perf.h  |  1 +
> > >  drivers/hwtracing/coresight/coresight.c       | 17 ++++++++
> > >  3 files changed, 61 insertions(+)
> > >
> > > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > > index f21eb28b6782..292bd409a68c 100644
> > > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> > > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > > @@ -14,6 +14,7 @@
> > >  #include <linux/perf_event.h>
> > >  #include <linux/percpu-defs.h>
> > >  #include <linux/slab.h>
> > > +#include <linux/stringhash.h>
> > >  #include <linux/types.h>
> > >  #include <linux/workqueue.h>
> > >
> > > @@ -43,8 +44,18 @@ static const struct attribute_group etm_pmu_format_group = {
> > >       .attrs  = etm_config_formats_attr,
> > >  };
> > >
> > > +static struct attribute *etm_config_sinks_attr[] = {
> > > +     NULL,
> > > +};
> > > +
> > > +static const struct attribute_group etm_pmu_sinks_group = {
> > > +     .name   = "sinks",
> > > +     .attrs  = etm_config_sinks_attr,
> > > +};
> > > +
> > >  static const struct attribute_group *etm_pmu_attr_groups[] = {
> > >       &etm_pmu_format_group,
> > > +     &etm_pmu_sinks_group,
> > >       NULL,
> > >  };
> > >
> > > @@ -479,6 +490,38 @@ int etm_perf_symlink(struct coresight_device *csdev, bool link)
> > >       return 0;
> > >  }
> > >
> > > +static ssize_t etm_perf_sink_name_show(struct device *dev,
> > > +                                    struct device_attribute *dattr,
> > > +                                    char *buf)
> > > +{
> > > +     /* See function coresight_sink_by_id() to know where this is used */
> > > +     u32 hash = hashlen_hash(hashlen_string(NULL, dattr->attr.name));
> > > +
> > > +     return scnprintf(buf, PAGE_SIZE, "%x\n", hash);
> > > +}
> > > +
> > > +int etm_perf_symlink_sink(struct coresight_device *csdev)
> > > +{
> > > +     struct device *pmu_dev = etm_pmu.dev;
> > > +     struct device *pdev = csdev->dev.parent;
> > > +     struct device_attribute *dev_attr;
> > > +
> > > +     if (csdev->type != CORESIGHT_DEV_TYPE_SINK &&
> > > +         csdev->type != CORESIGHT_DEV_TYPE_LINKSINK)
> > > +             return -EINVAL;
> > > +
> > > +     if (!etm_perf_up)
> > > +             return -EPROBE_DEFER;
> > > +
> > > +     dev_attr = kzalloc(sizeof(*dev_attr), GFP_KERNEL);
> > > +     dev_attr->attr.name = kstrdup(dev_name(pdev), GFP_KERNEL);
> > > +     dev_attr->attr.mode = 0444;
> > > +     dev_attr->show = etm_perf_sink_name_show;
> > > +
> > > +     return sysfs_add_file_to_group(&pmu_dev->kobj,
> > > +                                    &dev_attr->attr, "sinks");
> >
> > What is so odd about this call that you needed me to review this?
> 
> As far as I can tell nobody is feeding a dynamic struct attribute to
> the function and I wasn't sure if it was because they were told not to
> or simply because it wasn't needed, hence asking for a second opinion.

Ah.  Well, again, this is a good question to answer:

> > And what happens if this call fails, do you leak memory?

And also, what happens when you unload the device, who frees the
attribute's memory?

thanks,

greg k-h

Re: [PATCH 2/7] coresight: perf: Add "sinks" group to PMU directory

[Mathieu Poirier]

On Wed, 16 Jan 2019 at 09:33, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jan 16, 2019 at 09:14:33AM -0700, Mathieu Poirier wrote:
> > On Wed, 16 Jan 2019 at 08:39, Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Tue, Jan 15, 2019 at 04:07:37PM -0700, Mathieu Poirier wrote:
> > > > Add a "sinks" directory entry so that users can see all the sinks
> > > > available in the system in a single place.  Individual sink are added
> > > > as they are registered with the coresight bus.
> > > >
> > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > > > ---
> > > >  .../hwtracing/coresight/coresight-etm-perf.c  | 43 +++++++++++++++++++
> > > >  .../hwtracing/coresight/coresight-etm-perf.h  |  1 +
> > > >  drivers/hwtracing/coresight/coresight.c       | 17 ++++++++
> > > >  3 files changed, 61 insertions(+)
> > > >
> > > > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > > > index f21eb28b6782..292bd409a68c 100644
> > > > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> > > > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > > > @@ -14,6 +14,7 @@
> > > >  #include <linux/perf_event.h>
> > > >  #include <linux/percpu-defs.h>
> > > >  #include <linux/slab.h>
> > > > +#include <linux/stringhash.h>
> > > >  #include <linux/types.h>
> > > >  #include <linux/workqueue.h>
> > > >
> > > > @@ -43,8 +44,18 @@ static const struct attribute_group etm_pmu_format_group = {
> > > >       .attrs  = etm_config_formats_attr,
> > > >  };
> > > >
> > > > +static struct attribute *etm_config_sinks_attr[] = {
> > > > +     NULL,
> > > > +};
> > > > +
> > > > +static const struct attribute_group etm_pmu_sinks_group = {
> > > > +     .name   = "sinks",
> > > > +     .attrs  = etm_config_sinks_attr,
> > > > +};
> > > > +
> > > >  static const struct attribute_group *etm_pmu_attr_groups[] = {
> > > >       &etm_pmu_format_group,
> > > > +     &etm_pmu_sinks_group,
> > > >       NULL,
> > > >  };
> > > >
> > > > @@ -479,6 +490,38 @@ int etm_perf_symlink(struct coresight_device *csdev, bool link)
> > > >       return 0;
> > > >  }
> > > >
> > > > +static ssize_t etm_perf_sink_name_show(struct device *dev,
> > > > +                                    struct device_attribute *dattr,
> > > > +                                    char *buf)
> > > > +{
> > > > +     /* See function coresight_sink_by_id() to know where this is used */
> > > > +     u32 hash = hashlen_hash(hashlen_string(NULL, dattr->attr.name));
> > > > +
> > > > +     return scnprintf(buf, PAGE_SIZE, "%x\n", hash);
> > > > +}
> > > > +
> > > > +int etm_perf_symlink_sink(struct coresight_device *csdev)
> > > > +{
> > > > +     struct device *pmu_dev = etm_pmu.dev;
> > > > +     struct device *pdev = csdev->dev.parent;
> > > > +     struct device_attribute *dev_attr;
> > > > +
> > > > +     if (csdev->type != CORESIGHT_DEV_TYPE_SINK &&
> > > > +         csdev->type != CORESIGHT_DEV_TYPE_LINKSINK)
> > > > +             return -EINVAL;
> > > > +
> > > > +     if (!etm_perf_up)
> > > > +             return -EPROBE_DEFER;
> > > > +
> > > > +     dev_attr = kzalloc(sizeof(*dev_attr), GFP_KERNEL);
> > > > +     dev_attr->attr.name = kstrdup(dev_name(pdev), GFP_KERNEL);
> > > > +     dev_attr->attr.mode = 0444;
> > > > +     dev_attr->show = etm_perf_sink_name_show;
> > > > +
> > > > +     return sysfs_add_file_to_group(&pmu_dev->kobj,
> > > > +                                    &dev_attr->attr, "sinks");
> > >
> > > What is so odd about this call that you needed me to review this?
> >
> > As far as I can tell nobody is feeding a dynamic struct attribute to
> > the function and I wasn't sure if it was because they were told not to
> > or simply because it wasn't needed, hence asking for a second opinion.
>
> Ah.  Well, again, this is a good question to answer:
>
> > > And what happens if this call fails, do you leak memory?

That's something I will fix in the next revision.

>
> And also, what happens when you unload the device, who frees the
> attribute's memory?

If configured, coresight devices aren't removable.

Thanks for the review,
Mathieu

>
> thanks,
>
> greg k-h

Re: [PATCH 2/7] coresight: perf: Add "sinks" group to PMU directory

[Greg KH]

On Wed, Jan 16, 2019 at 09:38:09AM -0700, Mathieu Poirier wrote:
> On Wed, 16 Jan 2019 at 09:33, Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Jan 16, 2019 at 09:14:33AM -0700, Mathieu Poirier wrote:
> > > On Wed, 16 Jan 2019 at 08:39, Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Tue, Jan 15, 2019 at 04:07:37PM -0700, Mathieu Poirier wrote:
> > > > > Add a "sinks" directory entry so that users can see all the sinks
> > > > > available in the system in a single place.  Individual sink are added
> > > > > as they are registered with the coresight bus.
> > > > >
> > > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > > > > ---
> > > > >  .../hwtracing/coresight/coresight-etm-perf.c  | 43 +++++++++++++++++++
> > > > >  .../hwtracing/coresight/coresight-etm-perf.h  |  1 +
> > > > >  drivers/hwtracing/coresight/coresight.c       | 17 ++++++++
> > > > >  3 files changed, 61 insertions(+)
> > > > >
> > > > > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > > > > index f21eb28b6782..292bd409a68c 100644
> > > > > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> > > > > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > > > > @@ -14,6 +14,7 @@
> > > > >  #include <linux/perf_event.h>
> > > > >  #include <linux/percpu-defs.h>
> > > > >  #include <linux/slab.h>
> > > > > +#include <linux/stringhash.h>
> > > > >  #include <linux/types.h>
> > > > >  #include <linux/workqueue.h>
> > > > >
> > > > > @@ -43,8 +44,18 @@ static const struct attribute_group etm_pmu_format_group = {
> > > > >       .attrs  = etm_config_formats_attr,
> > > > >  };
> > > > >
> > > > > +static struct attribute *etm_config_sinks_attr[] = {
> > > > > +     NULL,
> > > > > +};
> > > > > +
> > > > > +static const struct attribute_group etm_pmu_sinks_group = {
> > > > > +     .name   = "sinks",
> > > > > +     .attrs  = etm_config_sinks_attr,
> > > > > +};
> > > > > +
> > > > >  static const struct attribute_group *etm_pmu_attr_groups[] = {
> > > > >       &etm_pmu_format_group,
> > > > > +     &etm_pmu_sinks_group,
> > > > >       NULL,
> > > > >  };
> > > > >
> > > > > @@ -479,6 +490,38 @@ int etm_perf_symlink(struct coresight_device *csdev, bool link)
> > > > >       return 0;
> > > > >  }
> > > > >
> > > > > +static ssize_t etm_perf_sink_name_show(struct device *dev,
> > > > > +                                    struct device_attribute *dattr,
> > > > > +                                    char *buf)
> > > > > +{
> > > > > +     /* See function coresight_sink_by_id() to know where this is used */
> > > > > +     u32 hash = hashlen_hash(hashlen_string(NULL, dattr->attr.name));
> > > > > +
> > > > > +     return scnprintf(buf, PAGE_SIZE, "%x\n", hash);
> > > > > +}
> > > > > +
> > > > > +int etm_perf_symlink_sink(struct coresight_device *csdev)
> > > > > +{
> > > > > +     struct device *pmu_dev = etm_pmu.dev;
> > > > > +     struct device *pdev = csdev->dev.parent;
> > > > > +     struct device_attribute *dev_attr;
> > > > > +
> > > > > +     if (csdev->type != CORESIGHT_DEV_TYPE_SINK &&
> > > > > +         csdev->type != CORESIGHT_DEV_TYPE_LINKSINK)
> > > > > +             return -EINVAL;
> > > > > +
> > > > > +     if (!etm_perf_up)
> > > > > +             return -EPROBE_DEFER;
> > > > > +
> > > > > +     dev_attr = kzalloc(sizeof(*dev_attr), GFP_KERNEL);
> > > > > +     dev_attr->attr.name = kstrdup(dev_name(pdev), GFP_KERNEL);
> > > > > +     dev_attr->attr.mode = 0444;
> > > > > +     dev_attr->show = etm_perf_sink_name_show;
> > > > > +
> > > > > +     return sysfs_add_file_to_group(&pmu_dev->kobj,
> > > > > +                                    &dev_attr->attr, "sinks");
> > > >
> > > > What is so odd about this call that you needed me to review this?
> > >
> > > As far as I can tell nobody is feeding a dynamic struct attribute to
> > > the function and I wasn't sure if it was because they were told not to
> > > or simply because it wasn't needed, hence asking for a second opinion.
> >
> > Ah.  Well, again, this is a good question to answer:
> >
> > > > And what happens if this call fails, do you leak memory?
> 
> That's something I will fix in the next revision.
> 
> >
> > And also, what happens when you unload the device, who frees the
> > attribute's memory?
> 
> If configured, coresight devices aren't removable.

But is the driver able to be unloaded?  Unbound from the device manually
through sysfs?  There's lots of ways devices are "removed" that don't
involved physical removal :)

thanks,

greg k-h

Re: [PATCH 2/7] coresight: perf: Add "sinks" group to PMU directory

[Mathieu Poirier]

On Wed, 16 Jan 2019 at 10:13, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jan 16, 2019 at 09:38:09AM -0700, Mathieu Poirier wrote:
> > On Wed, 16 Jan 2019 at 09:33, Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Wed, Jan 16, 2019 at 09:14:33AM -0700, Mathieu Poirier wrote:
> > > > On Wed, 16 Jan 2019 at 08:39, Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Tue, Jan 15, 2019 at 04:07:37PM -0700, Mathieu Poirier wrote:
> > > > > > Add a "sinks" directory entry so that users can see all the sinks
> > > > > > available in the system in a single place.  Individual sink are added
> > > > > > as they are registered with the coresight bus.
> > > > > >
> > > > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > > > > > ---
> > > > > >  .../hwtracing/coresight/coresight-etm-perf.c  | 43 +++++++++++++++++++
> > > > > >  .../hwtracing/coresight/coresight-etm-perf.h  |  1 +
> > > > > >  drivers/hwtracing/coresight/coresight.c       | 17 ++++++++
> > > > > >  3 files changed, 61 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > > > > > index f21eb28b6782..292bd409a68c 100644
> > > > > > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> > > > > > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > > > > > @@ -14,6 +14,7 @@
> > > > > >  #include <linux/perf_event.h>
> > > > > >  #include <linux/percpu-defs.h>
> > > > > >  #include <linux/slab.h>
> > > > > > +#include <linux/stringhash.h>
> > > > > >  #include <linux/types.h>
> > > > > >  #include <linux/workqueue.h>
> > > > > >
> > > > > > @@ -43,8 +44,18 @@ static const struct attribute_group etm_pmu_format_group = {
> > > > > >       .attrs  = etm_config_formats_attr,
> > > > > >  };
> > > > > >
> > > > > > +static struct attribute *etm_config_sinks_attr[] = {
> > > > > > +     NULL,
> > > > > > +};
> > > > > > +
> > > > > > +static const struct attribute_group etm_pmu_sinks_group = {
> > > > > > +     .name   = "sinks",
> > > > > > +     .attrs  = etm_config_sinks_attr,
> > > > > > +};
> > > > > > +
> > > > > >  static const struct attribute_group *etm_pmu_attr_groups[] = {
> > > > > >       &etm_pmu_format_group,
> > > > > > +     &etm_pmu_sinks_group,
> > > > > >       NULL,
> > > > > >  };
> > > > > >
> > > > > > @@ -479,6 +490,38 @@ int etm_perf_symlink(struct coresight_device *csdev, bool link)
> > > > > >       return 0;
> > > > > >  }
> > > > > >
> > > > > > +static ssize_t etm_perf_sink_name_show(struct device *dev,
> > > > > > +                                    struct device_attribute *dattr,
> > > > > > +                                    char *buf)
> > > > > > +{
> > > > > > +     /* See function coresight_sink_by_id() to know where this is used */
> > > > > > +     u32 hash = hashlen_hash(hashlen_string(NULL, dattr->attr.name));
> > > > > > +
> > > > > > +     return scnprintf(buf, PAGE_SIZE, "%x\n", hash);
> > > > > > +}
> > > > > > +
> > > > > > +int etm_perf_symlink_sink(struct coresight_device *csdev)
> > > > > > +{
> > > > > > +     struct device *pmu_dev = etm_pmu.dev;
> > > > > > +     struct device *pdev = csdev->dev.parent;
> > > > > > +     struct device_attribute *dev_attr;
> > > > > > +
> > > > > > +     if (csdev->type != CORESIGHT_DEV_TYPE_SINK &&
> > > > > > +         csdev->type != CORESIGHT_DEV_TYPE_LINKSINK)
> > > > > > +             return -EINVAL;
> > > > > > +
> > > > > > +     if (!etm_perf_up)
> > > > > > +             return -EPROBE_DEFER;
> > > > > > +
> > > > > > +     dev_attr = kzalloc(sizeof(*dev_attr), GFP_KERNEL);
> > > > > > +     dev_attr->attr.name = kstrdup(dev_name(pdev), GFP_KERNEL);
> > > > > > +     dev_attr->attr.mode = 0444;
> > > > > > +     dev_attr->show = etm_perf_sink_name_show;
> > > > > > +
> > > > > > +     return sysfs_add_file_to_group(&pmu_dev->kobj,
> > > > > > +                                    &dev_attr->attr, "sinks");
> > > > >
> > > > > What is so odd about this call that you needed me to review this?
> > > >
> > > > As far as I can tell nobody is feeding a dynamic struct attribute to
> > > > the function and I wasn't sure if it was because they were told not to
> > > > or simply because it wasn't needed, hence asking for a second opinion.
> > >
> > > Ah.  Well, again, this is a good question to answer:
> > >
> > > > > And what happens if this call fails, do you leak memory?
> >
> > That's something I will fix in the next revision.
> >
> > >
> > > And also, what happens when you unload the device, who frees the
> > > attribute's memory?
> >
> > If configured, coresight devices aren't removable.
>
> But is the driver able to be unloaded?  Unbound from the device manually
> through sysfs?  There's lots of ways devices are "removed" that don't
> involved physical removal :)

All drivers are built-in and thus preventing them from being unloaded.
They also have their ".suppress_bin_attrs" field set to take care of
unbinding through sysfs.

>
> thanks,
>
> greg k-h

[PATCH 3/7] coresight: Use event attributes for sink selection

[Mathieu Poirier]

This patch uses the information conveyed by perf_event::attr::config2
to select a sink to use for the session.  That way a sink can easily be
selected to be used by more than one source, something that isn't currently
possible with the sysfs implementation.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 .../hwtracing/coresight/coresight-etm-perf.c  | 16 ++------
 drivers/hwtracing/coresight/coresight-priv.h  |  1 +
 drivers/hwtracing/coresight/coresight.c       | 39 +++++++++++++++++++
 3 files changed, 44 insertions(+), 12 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 292bd409a68c..685d16001d0b 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -201,18 +201,10 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
 		return NULL;
 	INIT_WORK(&event_data->work, free_event_data);
 
-	/*
-	 * In theory nothing prevent tracers in a trace session from being
-	 * associated with different sinks, nor having a sink per tracer.  But
-	 * until we have HW with this kind of topology we need to assume tracers
-	 * in a trace session are using the same sink.  Therefore go through
-	 * the coresight bus and pick the first enabled sink.
-	 *
-	 * When operated from sysFS users are responsible to enable the sink
-	 * while from perf, the perf tools will do it based on the choice made
-	 * on the cmd line.  As such the "enable_sink" flag in sysFS is reset.
-	 */
-	sink = coresight_get_enabled_sink(true);
+	/* First get the selected sink from user space. */
+	sink = coresight_get_sink_by_id(event->attr.config2);
+	if (!sink)
+		sink = coresight_get_enabled_sink(true);
 	if (!sink || !sink_ops(sink)->alloc_buffer)
 		goto err;
 
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index 579f34943bf1..071bb98d358f 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -147,6 +147,7 @@ void coresight_disable_path(struct list_head *path);
 int coresight_enable_path(struct list_head *path, u32 mode, void *sink_data);
 struct coresight_device *coresight_get_sink(struct list_head *path);
 struct coresight_device *coresight_get_enabled_sink(bool reset);
+struct coresight_device *coresight_get_sink_by_id(u64 id);
 struct list_head *coresight_build_path(struct coresight_device *csdev,
 				       struct coresight_device *sink);
 void coresight_release_path(struct list_head *path);
diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index 526f122a38ee..7e2ce0beb2a0 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -11,6 +11,7 @@
 #include <linux/err.h>
 #include <linux/export.h>
 #include <linux/slab.h>
+#include <linux/stringhash.h>
 #include <linux/mutex.h>
 #include <linux/clk.h>
 #include <linux/coresight.h>
@@ -541,6 +542,44 @@ struct coresight_device *coresight_get_enabled_sink(bool deactivate)
 	return dev ? to_coresight_device(dev) : NULL;
 }
 
+static int coresight_sink_by_id(struct device *dev, void *data)
+{
+	struct coresight_device *csdev = to_coresight_device(dev);
+	unsigned long hash;
+
+	if (csdev->type == CORESIGHT_DEV_TYPE_SINK ||
+	     csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) {
+		/*
+		 * See function etm_perf_sink_name_show() to know where this
+		 * comes from.
+		 */
+		hash = hashlen_hash(hashlen_string(NULL, dev_name(dev)));
+
+		if (hash == (*(unsigned long *)data))
+			return 1;
+	}
+
+	return 0;
+}
+
+/**
+ * coresight_get_sink_by_id - returns the sink that matches the id
+ * @id: Id of the sink to match
+ *
+ * The name of a sink is unique, whether it is found on the AMBA bus or
+ * otherwise.  As such the hash of that name can easily be used to identify
+ * a sink.
+ */
+struct coresight_device *coresight_get_sink_by_id(u64 id)
+{
+	struct device *dev = NULL;
+
+	dev = bus_find_device(&coresight_bustype, NULL, &id,
+			      coresight_sink_by_id);
+
+	return dev ? to_coresight_device(dev) : NULL;
+}
+
 /*
  * coresight_grab_device - Power up this device and any of the helper
  * devices connected to it for trace operation. Since the helper devices
-- 
2.17.1

Re: [PATCH 3/7] coresight: Use event attributes for sink selection

[Suzuki K Poulose]

Hi Mathieu,

On 15/01/2019 23:07, Mathieu Poirier wrote:
> This patch uses the information conveyed by perf_event::attr::config2
> to select a sink to use for the session.  That way a sink can easily be
> selected to be used by more than one source, something that isn't currently
> possible with the sysfs implementation.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>   .../hwtracing/coresight/coresight-etm-perf.c  | 16 ++------
>   drivers/hwtracing/coresight/coresight-priv.h  |  1 +
>   drivers/hwtracing/coresight/coresight.c       | 39 +++++++++++++++++++
>   3 files changed, 44 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index 292bd409a68c..685d16001d0b 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -201,18 +201,10 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
>   		return NULL;
>   	INIT_WORK(&event_data->work, free_event_data);
>   
> -	/*
> -	 * In theory nothing prevent tracers in a trace session from being
> -	 * associated with different sinks, nor having a sink per tracer.  But
> -	 * until we have HW with this kind of topology we need to assume tracers
> -	 * in a trace session are using the same sink.  Therefore go through
> -	 * the coresight bus and pick the first enabled sink.
> -	 *
> -	 * When operated from sysFS users are responsible to enable the sink
> -	 * while from perf, the perf tools will do it based on the choice made
> -	 * on the cmd line.  As such the "enable_sink" flag in sysFS is reset.
> -	 */
> -	sink = coresight_get_enabled_sink(true);
> +	/* First get the selected sink from user space. */
> +	sink = coresight_get_sink_by_id(event->attr.config2);

Please could we also expose this information under "format", so that the
userspace knows where to fill in the sink id ? The other advantage is, we
could always change the size of the sink_id with the above, if we decide
to do something different with the sinks.

And also, I think this should be :

	if (event->attr.config2) {
		sink = coresight_get_sink_by_id(event->attr.config2)
		if (!sink)
			return -ENODEV;
	} else {
		sink = coresight_get_enabled_sink(true);
	}
So that we don't fallback to an enabled sink when a sink id is explicitly
specified ?

> +	if (!sink)
> +		sink = coresight_get_enabled_sink(true);
>   	if (!sink || !sink_ops(sink)->alloc_buffer)
>   		goto err;
>   
> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> index 579f34943bf1..071bb98d358f 100644
> --- a/drivers/hwtracing/coresight/coresight-priv.h
> +++ b/drivers/hwtracing/coresight/coresight-priv.h
> @@ -147,6 +147,7 @@ void coresight_disable_path(struct list_head *path);
>   int coresight_enable_path(struct list_head *path, u32 mode, void *sink_data);
>   struct coresight_device *coresight_get_sink(struct list_head *path);
>   struct coresight_device *coresight_get_enabled_sink(bool reset);
> +struct coresight_device *coresight_get_sink_by_id(u64 id);
>   struct list_head *coresight_build_path(struct coresight_device *csdev,
>   				       struct coresight_device *sink);
>   void coresight_release_path(struct list_head *path);
> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> index 526f122a38ee..7e2ce0beb2a0 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight.c
> @@ -11,6 +11,7 @@
>   #include <linux/err.h>
>   #include <linux/export.h>
>   #include <linux/slab.h>
> +#include <linux/stringhash.h>
>   #include <linux/mutex.h>
>   #include <linux/clk.h>
>   #include <linux/coresight.h>
> @@ -541,6 +542,44 @@ struct coresight_device *coresight_get_enabled_sink(bool deactivate)
>   	return dev ? to_coresight_device(dev) : NULL;
>   }
>   
> +static int coresight_sink_by_id(struct device *dev, void *data)
> +{
> +	struct coresight_device *csdev = to_coresight_device(dev);
> +	unsigned long hash;
> +
> +	if (csdev->type == CORESIGHT_DEV_TYPE_SINK ||
> +	     csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) {
> +		/*
> +		 * See function etm_perf_sink_name_show() to know where this
> +		 * comes from.
> +		 */
> +		hash = hashlen_hash(hashlen_string(NULL, dev_name(dev)));
> +
> +		if (hash == (*(unsigned long *)data))
> +			return 1;

nit: is it worth storing the sink_id of a sink in the csdev ? May be if we add
attribute field to the csdev (as mentioned in the previous patch), we could
simply use it here to compare.

Rest looks fine to me.

Cheers
Suzuki

Re: [PATCH 3/7] coresight: Use event attributes for sink selection

[Mathieu Poirier]

On Wed, 16 Jan 2019 at 03:36, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>
> Hi Mathieu,
>
> On 15/01/2019 23:07, Mathieu Poirier wrote:
> > This patch uses the information conveyed by perf_event::attr::config2
> > to select a sink to use for the session.  That way a sink can easily be
> > selected to be used by more than one source, something that isn't currently
> > possible with the sysfs implementation.
> >
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >   .../hwtracing/coresight/coresight-etm-perf.c  | 16 ++------
> >   drivers/hwtracing/coresight/coresight-priv.h  |  1 +
> >   drivers/hwtracing/coresight/coresight.c       | 39 +++++++++++++++++++
> >   3 files changed, 44 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > index 292bd409a68c..685d16001d0b 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > @@ -201,18 +201,10 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
> >               return NULL;
> >       INIT_WORK(&event_data->work, free_event_data);
> >
> > -     /*
> > -      * In theory nothing prevent tracers in a trace session from being
> > -      * associated with different sinks, nor having a sink per tracer.  But
> > -      * until we have HW with this kind of topology we need to assume tracers
> > -      * in a trace session are using the same sink.  Therefore go through
> > -      * the coresight bus and pick the first enabled sink.
> > -      *
> > -      * When operated from sysFS users are responsible to enable the sink
> > -      * while from perf, the perf tools will do it based on the choice made
> > -      * on the cmd line.  As such the "enable_sink" flag in sysFS is reset.
> > -      */
> > -     sink = coresight_get_enabled_sink(true);
> > +     /* First get the selected sink from user space. */
> > +     sink = coresight_get_sink_by_id(event->attr.config2);
>
> Please could we also expose this information under "format", so that the
> userspace knows where to fill in the sink id ? The other advantage is, we
> could always change the size of the sink_id with the above, if we decide
> to do something different with the sinks.

Humm...  From my point of view we don't want user space to do anything
with the sink ID other than read it.  In what  scenario do envison
user space filling the sink ID?

>
> And also, I think this should be :
>
>         if (event->attr.config2) {
>                 sink = coresight_get_sink_by_id(event->attr.config2)
>                 if (!sink)
>                         return -ENODEV;
>         } else {
>                 sink = coresight_get_enabled_sink(true);
>         }
> So that we don't fallback to an enabled sink when a sink id is explicitly
> specified ?

The code in user space refuses to go forward if an invalid sink has
been specified but enforcing the same in kernel space is a good idea.
I'll heed your advice.

>
> > +     if (!sink)
> > +             sink = coresight_get_enabled_sink(true);
> >       if (!sink || !sink_ops(sink)->alloc_buffer)
> >               goto err;
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> > index 579f34943bf1..071bb98d358f 100644
> > --- a/drivers/hwtracing/coresight/coresight-priv.h
> > +++ b/drivers/hwtracing/coresight/coresight-priv.h
> > @@ -147,6 +147,7 @@ void coresight_disable_path(struct list_head *path);
> >   int coresight_enable_path(struct list_head *path, u32 mode, void *sink_data);
> >   struct coresight_device *coresight_get_sink(struct list_head *path);
> >   struct coresight_device *coresight_get_enabled_sink(bool reset);
> > +struct coresight_device *coresight_get_sink_by_id(u64 id);
> >   struct list_head *coresight_build_path(struct coresight_device *csdev,
> >                                      struct coresight_device *sink);
> >   void coresight_release_path(struct list_head *path);
> > diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> > index 526f122a38ee..7e2ce0beb2a0 100644
> > --- a/drivers/hwtracing/coresight/coresight.c
> > +++ b/drivers/hwtracing/coresight/coresight.c
> > @@ -11,6 +11,7 @@
> >   #include <linux/err.h>
> >   #include <linux/export.h>
> >   #include <linux/slab.h>
> > +#include <linux/stringhash.h>
> >   #include <linux/mutex.h>
> >   #include <linux/clk.h>
> >   #include <linux/coresight.h>
> > @@ -541,6 +542,44 @@ struct coresight_device *coresight_get_enabled_sink(bool deactivate)
> >       return dev ? to_coresight_device(dev) : NULL;
> >   }
> >
> > +static int coresight_sink_by_id(struct device *dev, void *data)
> > +{
> > +     struct coresight_device *csdev = to_coresight_device(dev);
> > +     unsigned long hash;
> > +
> > +     if (csdev->type == CORESIGHT_DEV_TYPE_SINK ||
> > +          csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) {
> > +             /*
> > +              * See function etm_perf_sink_name_show() to know where this
> > +              * comes from.
> > +              */
> > +             hash = hashlen_hash(hashlen_string(NULL, dev_name(dev)));
> > +
> > +             if (hash == (*(unsigned long *)data))
> > +                     return 1;
>
> nit: is it worth storing the sink_id of a sink in the csdev ? May be if we add
> attribute field to the csdev (as mentioned in the previous patch), we could
> simply use it here to compare.

That depends on what we end up doing in the previous patch (see my
comment).  Since we are not even close to modularize coresight drivers
(unless you're already spending your weekends working on that,
something I doubt very much :o) ) I don't think we need to go this
way.  Again, let me know if you really want me to change this.

Thank you for your time,
Mathieu

>
> Rest looks fine to me.
>
> Cheers
> Suzuki

Re: [PATCH 3/7] coresight: Use event attributes for sink selection

[Suzuki K Poulose]

Hi Mathieu,

On 16/01/2019 23:57, Mathieu Poirier wrote:
> On Wed, 16 Jan 2019 at 03:36, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>>
>> Hi Mathieu,
>>
>> On 15/01/2019 23:07, Mathieu Poirier wrote:
>>> This patch uses the information conveyed by perf_event::attr::config2
>>> to select a sink to use for the session.  That way a sink can easily be
>>> selected to be used by more than one source, something that isn't currently
>>> possible with the sysfs implementation.
>>>
>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>> ---
>>>    .../hwtracing/coresight/coresight-etm-perf.c  | 16 ++------
>>>    drivers/hwtracing/coresight/coresight-priv.h  |  1 +
>>>    drivers/hwtracing/coresight/coresight.c       | 39 +++++++++++++++++++
>>>    3 files changed, 44 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
>>> index 292bd409a68c..685d16001d0b 100644
>>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
>>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
>>> @@ -201,18 +201,10 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
>>>                return NULL;
>>>        INIT_WORK(&event_data->work, free_event_data);
>>>
>>> -     /*
>>> -      * In theory nothing prevent tracers in a trace session from being
>>> -      * associated with different sinks, nor having a sink per tracer.  But
>>> -      * until we have HW with this kind of topology we need to assume tracers
>>> -      * in a trace session are using the same sink.  Therefore go through
>>> -      * the coresight bus and pick the first enabled sink.
>>> -      *
>>> -      * When operated from sysFS users are responsible to enable the sink
>>> -      * while from perf, the perf tools will do it based on the choice made
>>> -      * on the cmd line.  As such the "enable_sink" flag in sysFS is reset.
>>> -      */
>>> -     sink = coresight_get_enabled_sink(true);
>>> +     /* First get the selected sink from user space. */
>>> +     sink = coresight_get_sink_by_id(event->attr.config2);
>>
>> Please could we also expose this information under "format", so that the
>> userspace knows where to fill in the sink id ? The other advantage is, we
>> could always change the size of the sink_id with the above, if we decide
>> to do something different with the sinks.
> 
> Humm...  From my point of view we don't want user space to do anything
> with the sink ID other than read it.  In what  scenario do envison
> user space filling the sink ID?

The userspace needs to know where to put the "sink_id" in. (in this case
the driver expects it in attr.config2). So having this exposed in "format"
helps us to expose this information to all users (not just the perf tool, where
we hard-code it).

>>> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
>>> index 579f34943bf1..071bb98d358f 100644
>>> --- a/drivers/hwtracing/coresight/coresight-priv.h
>>> +++ b/drivers/hwtracing/coresight/coresight-priv.h
>>> @@ -147,6 +147,7 @@ void coresight_disable_path(struct list_head *path);
>>>    int coresight_enable_path(struct list_head *path, u32 mode, void *sink_data);
>>>    struct coresight_device *coresight_get_sink(struct list_head *path);
>>>    struct coresight_device *coresight_get_enabled_sink(bool reset);
>>> +struct coresight_device *coresight_get_sink_by_id(u64 id);
>>>    struct list_head *coresight_build_path(struct coresight_device *csdev,
>>>                                       struct coresight_device *sink);
>>>    void coresight_release_path(struct list_head *path);
>>> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
>>> index 526f122a38ee..7e2ce0beb2a0 100644
>>> --- a/drivers/hwtracing/coresight/coresight.c
>>> +++ b/drivers/hwtracing/coresight/coresight.c
>>> @@ -11,6 +11,7 @@
>>>    #include <linux/err.h>
>>>    #include <linux/export.h>
>>>    #include <linux/slab.h>
>>> +#include <linux/stringhash.h>
>>>    #include <linux/mutex.h>
>>>    #include <linux/clk.h>
>>>    #include <linux/coresight.h>
>>> @@ -541,6 +542,44 @@ struct coresight_device *coresight_get_enabled_sink(bool deactivate)
>>>        return dev ? to_coresight_device(dev) : NULL;
>>>    }
>>>
>>> +static int coresight_sink_by_id(struct device *dev, void *data)
>>> +{
>>> +     struct coresight_device *csdev = to_coresight_device(dev);
>>> +     unsigned long hash;
>>> +
>>> +     if (csdev->type == CORESIGHT_DEV_TYPE_SINK ||
>>> +          csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) {
>>> +             /*
>>> +              * See function etm_perf_sink_name_show() to know where this
>>> +              * comes from.
>>> +              */
>>> +             hash = hashlen_hash(hashlen_string(NULL, dev_name(dev)));
>>> +
>>> +             if (hash == (*(unsigned long *)data))
>>> +                     return 1;
>>
>> nit: is it worth storing the sink_id of a sink in the csdev ? May be if we add
>> attribute field to the csdev (as mentioned in the previous patch), we could
>> simply use it here to compare.
> 
> That depends on what we end up doing in the previous patch (see my
> comment).  Since we are not even close to modularize coresight drivers
> (unless you're already spending your weekends working on that,
> something I doubt very much :o) ) I don't think we need to go this
> way.  Again, let me know if you really want me to change this.

As I said, I understand the complexity of the modularization effort. However
it would be good to be future proof than figure this out the hard way when we
get there and we don't loose much except a few bytes in the coresight_device.

Cheers
Suzuki

Re: [PATCH 3/7] coresight: Use event attributes for sink selection

[Peter Zijlstra]

On Tue, Jan 15, 2019 at 04:07:38PM -0700, Mathieu Poirier wrote:
> +	/* First get the selected sink from user space. */
> +	sink = coresight_get_sink_by_id(event->attr.config2);

(I know there's a new version out, I'll go look there shortly)

should this patch not also have something like:

PMU_FORMAT_ATTR(sinkid, "config2:0-63");

> +	if (!sink)
> +		sink = coresight_get_enabled_sink(true);
>  	if (!sink || !sink_ops(sink)->alloc_buffer)
>  		goto err;

Re: [PATCH 3/7] coresight: Use event attributes for sink selection

[Peter Zijlstra]

On Wed, Jan 23, 2019 at 10:03:33PM +0100, Peter Zijlstra wrote:
> On Tue, Jan 15, 2019 at 04:07:38PM -0700, Mathieu Poirier wrote:
> > +	/* First get the selected sink from user space. */
> > +	sink = coresight_get_sink_by_id(event->attr.config2);
> 
> (I know there's a new version out, I'll go look there shortly)
> 
> should this patch not also have something like:
> 
> PMU_FORMAT_ATTR(sinkid, "config2:0-63");

n/m, I see it's already there.

[PATCH 4/7] perf pmu: Moving EVENT_SOURCE_DEVICE_PATH to PMU header file

[Mathieu Poirier]

From: linaro <linaro@localhost.localdomain>

Moving definition of EVENT_SOURCE_DEVICE_PATH to pmu.h so that it can be
used by other files than pmu.c

Signed-off-by: linaro <linaro@localhost.localdomain>
---
 tools/perf/util/pmu.c | 2 --
 tools/perf/util/pmu.h | 1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 11a234740632..51d437f55d18 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -29,8 +29,6 @@ struct perf_pmu_format {
 	struct list_head list;
 };
 
-#define EVENT_SOURCE_DEVICE_PATH "/bus/event_source/devices/"
-
 int perf_pmu_parse(struct list_head *list, char *name);
 extern FILE *perf_pmu_in;
 
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 76fecec7b3f9..350c54e0bd3d 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -16,6 +16,7 @@ enum {
 };
 
 #define PERF_PMU_FORMAT_BITS 64
+#define EVENT_SOURCE_DEVICE_PATH "/bus/event_source/devices/"
 
 struct perf_event_attr;
 
-- 
2.17.1

Re: [PATCH 4/7] perf pmu: Moving EVENT_SOURCE_DEVICE_PATH to PMU header file

[Suzuki K Poulose]

On 15/01/2019 23:07, Mathieu Poirier wrote:
> From: linaro <linaro@localhost.localdomain>

nit: I assume you have noticed this already ^^

Otherwise, looks fine.

Suzuki

[PATCH 5/7] perf tools: Use event attributes to send sink information to kernel

[Mathieu Poirier]

The communication of sink information for a trace session doesn't work when
more than on CPU is involved in the scenario due to the static nature of
sysfs.  As such communicate the sink information to each event by using the
perf_event::attr:config2 attribute.  The information sent to the kernel is
an hash of the sink's name, which is unique in a system.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 tools/perf/arch/arm/util/cs-etm.c | 40 +++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
index 2f595cd73da6..4aa6193dcb50 100644
--- a/tools/perf/arch/arm/util/cs-etm.c
+++ b/tools/perf/arch/arm/util/cs-etm.c
@@ -22,6 +22,7 @@
 #include "../../util/thread_map.h"
 #include "../../util/cs-etm.h"
 
+#include <errno.h>
 #include <stdlib.h>
 #include <sys/stat.h>
 
@@ -60,10 +61,45 @@ static int cs_etm_parse_snapshot_options(struct auxtrace_record *itr,
 	return 0;
 }
 
+static int cs_etm_set_sink_attr(struct perf_pmu *pmu,
+				struct perf_evsel *evsel)
+{
+	char msg[BUFSIZ], path[PATH_MAX], *sink;
+	struct perf_evsel_config_term *term;
+	int ret = -EINVAL;
+	u32 hash;
+
+	list_for_each_entry(term, &evsel->config_terms, list) {
+		if (term->type != PERF_EVSEL__CONFIG_TERM_DRV_CFG)
+			continue;
+
+		sink = term->val.drv_cfg;
+		snprintf(path, PATH_MAX, "sinks/%s", sink);
+
+		ret = perf_pmu__scan_file(pmu, path, "%x", &hash);
+		if (ret != 1) {
+			pr_err("failed to set sink \"%s\" on event %s with %d (%s)\n",
+			       sink, perf_evsel__name(evsel), errno,
+			       str_error_r(errno, msg, sizeof(msg)));
+			return ret;
+		}
+
+		evsel->attr.config2 = (u64)hash;
+		return 0;
+	}
+
+	/*
+	 * No sink was provided on the command line - for _now_ treat
+	 * this as an error.
+	 */
+	return ret;
+}
+
 static int cs_etm_recording_options(struct auxtrace_record *itr,
 				    struct perf_evlist *evlist,
 				    struct record_opts *opts)
 {
+	int ret;
 	struct cs_etm_recording *ptr =
 				container_of(itr, struct cs_etm_recording, itr);
 	struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu;
@@ -92,6 +128,10 @@ static int cs_etm_recording_options(struct auxtrace_record *itr,
 	if (!cs_etm_evsel)
 		return 0;
 
+	ret = cs_etm_set_sink_attr(cs_etm_pmu, cs_etm_evsel);
+	if (ret)
+		return ret;
+
 	if (opts->use_clockid) {
 		pr_err("Cannot use clockid (-k option) with %s\n",
 		       CORESIGHT_ETM_PMU_NAME);
-- 
2.17.1

[PATCH 6/7] perf tools: Removing CoreSight set_drv_config() API

[Mathieu Poirier]

Now that event's config2 attribute is used to communicate sink selection
to the kernel, remove the old set_drv_config() implementation since it
is no longer needed.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 tools/perf/arch/arm/util/cs-etm.c | 54 -------------------------------
 tools/perf/arch/arm/util/cs-etm.h |  3 --
 tools/perf/arch/arm/util/pmu.c    |  2 --
 3 files changed, 59 deletions(-)

diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
index 4aa6193dcb50..9a4a8153e4c2 100644
--- a/tools/perf/arch/arm/util/cs-etm.c
+++ b/tools/perf/arch/arm/util/cs-etm.c
@@ -26,9 +26,6 @@
 #include <stdlib.h>
 #include <sys/stat.h>
 
-#define ENABLE_SINK_MAX	128
-#define CS_BUS_DEVICE_PATH "/bus/coresight/devices/"
-
 struct cs_etm_recording {
 	struct auxtrace_record	itr;
 	struct perf_pmu		*cs_etm_pmu;
@@ -638,54 +635,3 @@ struct auxtrace_record *cs_etm_record_init(int *err)
 out:
 	return NULL;
 }
-
-static FILE *cs_device__open_file(const char *name)
-{
-	struct stat st;
-	char path[PATH_MAX];
-	const char *sysfs;
-
-	sysfs = sysfs__mountpoint();
-	if (!sysfs)
-		return NULL;
-
-	snprintf(path, PATH_MAX,
-		 "%s" CS_BUS_DEVICE_PATH "%s", sysfs, name);
-
-	if (stat(path, &st) < 0)
-		return NULL;
-
-	return fopen(path, "w");
-
-}
-
-static int __printf(2, 3) cs_device__print_file(const char *name, const char *fmt, ...)
-{
-	va_list args;
-	FILE *file;
-	int ret = -EINVAL;
-
-	va_start(args, fmt);
-	file = cs_device__open_file(name);
-	if (file) {
-		ret = vfprintf(file, fmt, args);
-		fclose(file);
-	}
-	va_end(args);
-	return ret;
-}
-
-int cs_etm_set_drv_config(struct perf_evsel_config_term *term)
-{
-	int ret;
-	char enable_sink[ENABLE_SINK_MAX];
-
-	snprintf(enable_sink, ENABLE_SINK_MAX, "%s/%s",
-		 term->val.drv_cfg, "enable_sink");
-
-	ret = cs_device__print_file(enable_sink, "%d", 1);
-	if (ret < 0)
-		return ret;
-
-	return 0;
-}
diff --git a/tools/perf/arch/arm/util/cs-etm.h b/tools/perf/arch/arm/util/cs-etm.h
index 1a12e64f5127..a3354bda4fe8 100644
--- a/tools/perf/arch/arm/util/cs-etm.h
+++ b/tools/perf/arch/arm/util/cs-etm.h
@@ -7,9 +7,6 @@
 #ifndef INCLUDE__PERF_CS_ETM_H__
 #define INCLUDE__PERF_CS_ETM_H__
 
-#include "../../util/evsel.h"
-
 struct auxtrace_record *cs_etm_record_init(int *err);
-int cs_etm_set_drv_config(struct perf_evsel_config_term *term);
 
 #endif
diff --git a/tools/perf/arch/arm/util/pmu.c b/tools/perf/arch/arm/util/pmu.c
index e047571e6080..e4619f2fe7ce 100644
--- a/tools/perf/arch/arm/util/pmu.c
+++ b/tools/perf/arch/arm/util/pmu.c
@@ -8,7 +8,6 @@
 #include <linux/coresight-pmu.h>
 #include <linux/perf_event.h>
 
-#include "cs-etm.h"
 #include "arm-spe.h"
 #include "../../util/pmu.h"
 
@@ -19,7 +18,6 @@ struct perf_event_attr
 	if (!strcmp(pmu->name, CORESIGHT_ETM_PMU_NAME)) {
 		/* add ETM default config here */
 		pmu->selectable = true;
-		pmu->set_drv_config = cs_etm_set_drv_config;
 #if defined(__aarch64__)
 	} else if (strstarts(pmu->name, ARM_SPE_PMU_NAME)) {
 		return arm_spe_pmu_default_config(pmu);
-- 
2.17.1

[PATCH 7/7] perf tools: Remove PMU::set_drv_config API

[Mathieu Poirier]

CoreSight was the only client of the PMU's set_drv_config() API.  Now
that it is no longer needed by CoreSight remove it from the code base.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 tools/perf/builtin-record.c   | 10 -----
 tools/perf/builtin-stat.c     |  9 ----
 tools/perf/builtin-top.c      | 13 ------
 tools/perf/util/Build         |  1 -
 tools/perf/util/drv_configs.c | 78 -----------------------------------
 tools/perf/util/drv_configs.h | 26 ------------
 tools/perf/util/pmu.h         |  1 -
 7 files changed, 138 deletions(-)
 delete mode 100644 tools/perf/util/drv_configs.c
 delete mode 100644 tools/perf/util/drv_configs.h

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 882285fb9f64..b637c37b8479 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -23,7 +23,6 @@
 #include "util/evlist.h"
 #include "util/evsel.h"
 #include "util/debug.h"
-#include "util/drv_configs.h"
 #include "util/session.h"
 #include "util/tool.h"
 #include "util/symbol.h"
@@ -566,7 +565,6 @@ static int record__open(struct record *rec)
 	struct perf_evlist *evlist = rec->evlist;
 	struct perf_session *session = rec->session;
 	struct record_opts *opts = &rec->opts;
-	struct perf_evsel_config_term *err_term;
 	int rc = 0;
 
 	/*
@@ -619,14 +617,6 @@ static int record__open(struct record *rec)
 		goto out;
 	}
 
-	if (perf_evlist__apply_drv_configs(evlist, &pos, &err_term)) {
-		pr_err("failed to set config \"%s\" on event %s with %d (%s)\n",
-		      err_term->val.drv_cfg, perf_evsel__name(pos), errno,
-		      str_error_r(errno, msg, sizeof(msg)));
-		rc = -1;
-		goto out;
-	}
-
 	rc = record__mmap(rec);
 	if (rc)
 		goto out;
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 63a3afc7f32b..9c5b3b94bb38 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -52,7 +52,6 @@
 #include "util/evlist.h"
 #include "util/evsel.h"
 #include "util/debug.h"
-#include "util/drv_configs.h"
 #include "util/color.h"
 #include "util/stat.h"
 #include "util/header.h"
@@ -418,7 +417,6 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 	int status = 0;
 	const bool forks = (argc > 0);
 	bool is_pipe = STAT_RECORD ? perf_stat.data.is_pipe : false;
-	struct perf_evsel_config_term *err_term;
 
 	if (interval) {
 		ts.tv_sec  = interval / USEC_PER_MSEC;
@@ -515,13 +513,6 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 		return -1;
 	}
 
-	if (perf_evlist__apply_drv_configs(evsel_list, &counter, &err_term)) {
-		pr_err("failed to set config \"%s\" on event %s with %d (%s)\n",
-		      err_term->val.drv_cfg, perf_evsel__name(counter), errno,
-		      str_error_r(errno, msg, sizeof(msg)));
-		return -1;
-	}
-
 	if (STAT_RECORD) {
 		int err, fd = perf_data__fd(&perf_stat.data);
 
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index f64e312db787..33e448f702b6 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -24,7 +24,6 @@
 #include "util/annotate.h"
 #include "util/config.h"
 #include "util/color.h"
-#include "util/drv_configs.h"
 #include "util/evlist.h"
 #include "util/evsel.h"
 #include "util/event.h"
@@ -1184,10 +1183,6 @@ static void init_process_thread(struct perf_top *top)
 
 static int __cmd_top(struct perf_top *top)
 {
-	char msg[512];
-	struct perf_evsel *pos;
-	struct perf_evsel_config_term *err_term;
-	struct perf_evlist *evlist = top->evlist;
 	struct record_opts *opts = &top->record_opts;
 	pthread_t thread, thread_process;
 	int ret;
@@ -1232,14 +1227,6 @@ static int __cmd_top(struct perf_top *top)
 	if (ret)
 		goto out_delete;
 
-	ret = perf_evlist__apply_drv_configs(evlist, &pos, &err_term);
-	if (ret) {
-		pr_err("failed to set config \"%s\" on event %s with %d (%s)\n",
-			err_term->val.drv_cfg, perf_evsel__name(pos), errno,
-			str_error_r(errno, msg, sizeof(msg)));
-		goto out_delete;
-	}
-
 	top->session->evlist = top->evlist;
 	perf_session__set_id_hdr_size(top->session);
 
diff --git a/tools/perf/util/Build b/tools/perf/util/Build
index af72be7f5b3b..71e697e7df2d 100644
--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -104,7 +104,6 @@ libperf-y += term.o
 libperf-y += help-unknown-cmd.o
 libperf-y += mem-events.o
 libperf-y += vsprintf.o
-libperf-y += drv_configs.o
 libperf-y += units.o
 libperf-y += time-utils.o
 libperf-y += expr-bison.o
diff --git a/tools/perf/util/drv_configs.c b/tools/perf/util/drv_configs.c
deleted file mode 100644
index eec754243f4d..000000000000
--- a/tools/perf/util/drv_configs.c
+++ /dev/null
@@ -1,78 +0,0 @@
-/*
- * drv_configs.h: Interface to apply PMU specific configuration
- * Copyright (c) 2016-2018, Linaro Ltd.
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms and conditions of the GNU General Public License,
- * version 2, as published by the Free Software Foundation.
- *
- * This program is distributed in the hope it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
- * more details.
- *
- */
-
-#include "drv_configs.h"
-#include "evlist.h"
-#include "evsel.h"
-#include "pmu.h"
-#include <errno.h>
-
-static int
-perf_evsel__apply_drv_configs(struct perf_evsel *evsel,
-			      struct perf_evsel_config_term **err_term)
-{
-	bool found = false;
-	int err = 0;
-	struct perf_evsel_config_term *term;
-	struct perf_pmu *pmu = NULL;
-
-	while ((pmu = perf_pmu__scan(pmu)) != NULL)
-		if (pmu->type == evsel->attr.type) {
-			found = true;
-			break;
-		}
-
-	list_for_each_entry(term, &evsel->config_terms, list) {
-		if (term->type != PERF_EVSEL__CONFIG_TERM_DRV_CFG)
-			continue;
-
-		/*
-		 * We have a configuration term, report an error if we
-		 * can't find the PMU or if the PMU driver doesn't support
-		 * cmd line driver configuration.
-		 */
-		if (!found || !pmu->set_drv_config) {
-			err = -EINVAL;
-			*err_term = term;
-			break;
-		}
-
-		err = pmu->set_drv_config(term);
-		if (err) {
-			*err_term = term;
-			break;
-		}
-	}
-
-	return err;
-}
-
-int perf_evlist__apply_drv_configs(struct perf_evlist *evlist,
-				   struct perf_evsel **err_evsel,
-				   struct perf_evsel_config_term **err_term)
-{
-	struct perf_evsel *evsel;
-	int err = 0;
-
-	evlist__for_each_entry(evlist, evsel) {
-		err = perf_evsel__apply_drv_configs(evsel, err_term);
-		if (err) {
-			*err_evsel = evsel;
-			break;
-		}
-	}
-
-	return err;
-}
diff --git a/tools/perf/util/drv_configs.h b/tools/perf/util/drv_configs.h
deleted file mode 100644
index 32bc9babc2e0..000000000000
--- a/tools/perf/util/drv_configs.h
+++ /dev/null
@@ -1,26 +0,0 @@
-/*
- * drv_configs.h: Interface to apply PMU specific configuration
- * Copyright (c) 2016-2018, Linaro Ltd.
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms and conditions of the GNU General Public License,
- * version 2, as published by the Free Software Foundation.
- *
- * This program is distributed in the hope it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
- * more details.
- *
- */
-
-#ifndef __PERF_DRV_CONFIGS_H
-#define __PERF_DRV_CONFIGS_H
-
-#include "drv_configs.h"
-#include "evlist.h"
-#include "evsel.h"
-
-int perf_evlist__apply_drv_configs(struct perf_evlist *evlist,
-				   struct perf_evsel **err_evsel,
-				   struct perf_evsel_config_term **term);
-#endif
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 350c54e0bd3d..569a6cbffd5a 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -30,7 +30,6 @@ struct perf_pmu {
 	struct list_head format;  /* HEAD struct perf_pmu_format -> list */
 	struct list_head aliases; /* HEAD struct perf_pmu_alias -> list */
 	struct list_head list;    /* ELEM */
-	int (*set_drv_config)	(struct perf_evsel_config_term *term);
 };
 
 struct perf_pmu_info {
-- 
2.17.1