[PATCH v2 00/10] coresight: perf: Support for tmc-etr backed buffers

[PATCH v2 00/10] coresight: perf: Support for tmc-etr backed buffers

[Suzuki K Poulose]

This series adds the support for using the tmc-etr in perf mode for
storing the trace to system RAM. The ETR uses a separate buffer (double
buffering) for storing the trace. This is copied back to the ring buffer
when the event is stopped. We try to match the ETR buffer to the larger
of perf ring buffer or the size configured for the ETR via sysfs. This
allows tuning the buffer size to prevent overflows and loosing trace
data, as we don't have overflow interrupt support (yet).

Applies on coresight/next

Changes since v1 :
 - Fix path for each CPU where the event might be recorded.
 - Handle errors in enabling source
 - Remove set_buffer callback to avoid complicating the error handling.
 - Fix a bug in handling sysfs mode specific buffers

Changes since [0] :
 - Drop buffer rotation logic for etr-buf
 - Do not use perf ring buffer. (Add support later)
 - Fix handling of sink, preventing mixed modes of operation.

[0] - TMC ETR perf support
 - http://lists.infradead.org/pipermail/linux-arm-kernel/2018-May/574875.html


Suzuki K Poulose (10):
  coresight: Fix handling of sinks
  coresight: perf: Fix per cpu path management
  coresight: perf: Disable trace path upon source error
  coresight: tmc-etr: Handle driver mode specific ETR buffers
  coresight: tmc-etr: Relax collection of trace from sysfs mode
  coresight: Convert driver messages to dev_dbg
  coresight: perf: Remove reset_buffer call back for sinks
  coresight: perf: Add helper to retrieve sink configuration
  coresight: perf: Remove set_buffer call back
  coresight: etm-perf: Add support for ETR backend

 .../coresight/coresight-dynamic-replicator.c       |   4 +-
 drivers/hwtracing/coresight/coresight-etb10.c      |  84 ++----
 drivers/hwtracing/coresight/coresight-etm-perf.c   |  91 +++---
 drivers/hwtracing/coresight/coresight-etm-perf.h   |  26 ++
 drivers/hwtracing/coresight/coresight-etm3x.c      |   4 +-
 drivers/hwtracing/coresight/coresight-etm4x.c      |   4 +-
 drivers/hwtracing/coresight/coresight-funnel.c     |   4 +-
 drivers/hwtracing/coresight/coresight-priv.h       |   2 +-
 drivers/hwtracing/coresight/coresight-replicator.c |   4 +-
 drivers/hwtracing/coresight/coresight-stm.c        |   4 +-
 drivers/hwtracing/coresight/coresight-tmc-etf.c    |  94 +++---
 drivers/hwtracing/coresight/coresight-tmc-etr.c    | 327 ++++++++++++++++++---
 drivers/hwtracing/coresight/coresight-tmc.c        |   4 +-
 drivers/hwtracing/coresight/coresight-tmc.h        |   4 +
 drivers/hwtracing/coresight/coresight-tpiu.c       |   6 +-
 drivers/hwtracing/coresight/coresight.c            |  31 +-
 include/linux/coresight.h                          |  12 +-
 17 files changed, 473 insertions(+), 232 deletions(-)

-- 
2.7.4

[PATCH v2 01/10] coresight: Fix handling of sinks

[Suzuki K Poulose]

The coresight components could be operated either in sysfs mode or in perf
mode. For some of the components, the mode of operation doesn't matter as
they simply relay the data to the next component in the trace path. But for
sinks, they need to be able to provide the trace data back to the user.
Thus we need to make sure that "mode" is handled appropriately. e.g,
the sysfs mode could have multiple sources driving the trace data, while
perf mode doesn't allow sharing the sink.

The coresight_enable_sink() however doesn't really allow this check to
trigger as it skips the "enable_sink" callback if the component is
already enabled, irrespective of the mode. This could cause mixing
of data from different modes or even same mode (in perf), if the
sources are different. Also, if we fail to enable the sink while
enabling a path (where sink is the first component enabled),
we could end up in disabling the components in the "entire"
path which were not enabled in this trial, causing disruptions
in the existing trace paths.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index 3e07fd3..c0dabbd 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -132,12 +132,14 @@ static int coresight_enable_sink(struct coresight_device *csdev, u32 mode)
 {
 	int ret;
 
-	if (!csdev->enable) {
-		if (sink_ops(csdev)->enable) {
-			ret = sink_ops(csdev)->enable(csdev, mode);
-			if (ret)
-				return ret;
-		}
+	/*
+	 * We need to make sure the "new" session is compatible with the
+	 * existing "mode" of operation.
+	 */
+	if (sink_ops(csdev)->enable) {
+		ret = sink_ops(csdev)->enable(csdev, mode);
+		if (ret)
+			return ret;
 		csdev->enable = true;
 	}
 
@@ -339,8 +341,14 @@ int coresight_enable_path(struct list_head *path, u32 mode)
 		switch (type) {
 		case CORESIGHT_DEV_TYPE_SINK:
 			ret = coresight_enable_sink(csdev, mode);
+			/*
+			 * Sink is the first component turned on. If we
+			 * failed to enable the sink, there are no components
+			 * that need disabling. Disabling the path here
+			 * would mean we could disrupt an existing session.
+			 */
 			if (ret)
-				goto err;
+				goto out;
 			break;
 		case CORESIGHT_DEV_TYPE_SOURCE:
 			/* sources are enabled from either sysFS or Perf */
-- 
2.7.4

[PATCH v2 02/10] coresight: perf: Fix per cpu path management

[Suzuki K Poulose]

We create a coresight trace path for each online CPU when
we start the event. We rely on the number of online CPUs
and then go on to allocate an array matching the "number of
online CPUs" for holding the path and then uses normal
CPU id as the index to the array. This is problematic as
we could have some offline CPUs causing us to access beyond
the actual array size (e.g, on a dual SMP system, if CPU0 is
offline, CPU1 could be really accessing beyond the array).
The solution is to switch to per-cpu array for holding the path.
Also we get rid of the {get/put}_online_cpus() which is
not really needed.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-etm-perf.c | 59 ++++++++++++++++--------
 1 file changed, 39 insertions(+), 20 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 6776956..ab2a6e8 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -12,6 +12,7 @@
 #include <linux/mm.h>
 #include <linux/init.h>
 #include <linux/perf_event.h>
+#include <linux/percpu-defs.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <linux/workqueue.h>
@@ -33,7 +34,7 @@ struct etm_event_data {
 	struct work_struct work;
 	cpumask_t mask;
 	void *snk_config;
-	struct list_head **path;
+	struct list_head * __percpu *path;
 };
 
 static DEFINE_PER_CPU(struct perf_output_handle, ctx_handle);
@@ -61,6 +62,18 @@ static const struct attribute_group *etm_pmu_attr_groups[] = {
 	NULL,
 };
 
+static inline struct list_head **
+etm_event_cpu_path_ptr(struct etm_event_data *data, int cpu)
+{
+	return per_cpu_ptr(data->path, cpu);
+}
+
+static inline struct list_head *
+etm_event_cpu_path(struct etm_event_data *data, int cpu)
+{
+	return *etm_event_cpu_path_ptr(data, cpu);
+}
+
 static void etm_event_read(struct perf_event *event) {}
 
 static int etm_addr_filters_alloc(struct perf_event *event)
@@ -120,23 +133,26 @@ static void free_event_data(struct work_struct *work)
 	 */
 	if (event_data->snk_config) {
 		cpu = cpumask_first(mask);
-		sink = coresight_get_sink(event_data->path[cpu]);
+		sink = coresight_get_sink(etm_event_cpu_path(event_data, cpu));
 		if (sink_ops(sink)->free_buffer)
 			sink_ops(sink)->free_buffer(event_data->snk_config);
 	}
 
 	for_each_cpu(cpu, mask) {
-		if (!(IS_ERR_OR_NULL(event_data->path[cpu])))
-			coresight_release_path(event_data->path[cpu]);
+		struct list_head **ppath;
+
+		ppath = etm_event_cpu_path_ptr(event_data, cpu);
+		if (!(IS_ERR_OR_NULL(*ppath)))
+			coresight_release_path(*ppath);
+		*ppath = NULL;
 	}
 
-	kfree(event_data->path);
+	free_percpu(event_data->path);
 	kfree(event_data);
 }
 
 static void *alloc_event_data(int cpu)
 {
-	int size;
 	cpumask_t *mask;
 	struct etm_event_data *event_data;
 
@@ -145,17 +161,11 @@ static void *alloc_event_data(int cpu)
 	if (!event_data)
 		return NULL;
 
-	/* Make sure nothing disappears under us */
-	get_online_cpus();
-	size = num_online_cpus();
-
 	mask = &event_data->mask;
 	if (cpu != -1)
 		cpumask_set_cpu(cpu, mask);
 	else
 		cpumask_copy(mask, cpu_online_mask);
-	put_online_cpus();
-
 	/*
 	 * Each CPU has a single path between source and destination.  As such
 	 * allocate an array using CPU numbers as indexes.  That way a path
@@ -164,8 +174,8 @@ static void *alloc_event_data(int cpu)
 	 * unused memory when dealing with single CPU trace scenarios is small
 	 * compared to the cost of searching through an optimized array.
 	 */
-	event_data->path = kcalloc(size,
-				   sizeof(struct list_head *), GFP_KERNEL);
+	event_data->path = alloc_percpu(struct list_head *);
+
 	if (!event_data->path) {
 		kfree(event_data);
 		return NULL;
@@ -213,6 +223,7 @@ static void *etm_setup_aux(int event_cpu, void **pages,
 
 	/* Setup the path for each CPU in a trace session */
 	for_each_cpu(cpu, mask) {
+		struct list_head *path;
 		struct coresight_device *csdev;
 
 		csdev = per_cpu(csdev_src, cpu);
@@ -224,9 +235,10 @@ static void *etm_setup_aux(int event_cpu, void **pages,
 		 * list of devices from source to sink that can be
 		 * referenced later when the path is actually needed.
 		 */
-		event_data->path[cpu] = coresight_build_path(csdev, sink);
-		if (IS_ERR(event_data->path[cpu]))
+		path = coresight_build_path(csdev, sink);
+		if (IS_ERR(path))
 			goto err;
+		*etm_event_cpu_path_ptr(event_data, cpu) = path;
 	}
 
 	if (!sink_ops(sink)->alloc_buffer)
@@ -255,6 +267,7 @@ static void etm_event_start(struct perf_event *event, int flags)
 	struct etm_event_data *event_data;
 	struct perf_output_handle *handle = this_cpu_ptr(&ctx_handle);
 	struct coresight_device *sink, *csdev = per_cpu(csdev_src, cpu);
+	struct list_head *path;
 
 	if (!csdev)
 		goto fail;
@@ -267,8 +280,9 @@ static void etm_event_start(struct perf_event *event, int flags)
 	if (!event_data)
 		goto fail;
 
+	path = etm_event_cpu_path(event_data, cpu);
 	/* We need a sink, no need to continue without one */
-	sink = coresight_get_sink(event_data->path[cpu]);
+	sink = coresight_get_sink(path);
 	if (WARN_ON_ONCE(!sink || !sink_ops(sink)->set_buffer))
 		goto fail_end_stop;
 
@@ -278,7 +292,7 @@ static void etm_event_start(struct perf_event *event, int flags)
 		goto fail_end_stop;
 
 	/* Nothing will happen without a path */
-	if (coresight_enable_path(event_data->path[cpu], CS_MODE_PERF))
+	if (coresight_enable_path(path, CS_MODE_PERF))
 		goto fail_end_stop;
 
 	/* Tell the perf core the event is alive */
@@ -306,6 +320,7 @@ static void etm_event_stop(struct perf_event *event, int mode)
 	struct coresight_device *sink, *csdev = per_cpu(csdev_src, cpu);
 	struct perf_output_handle *handle = this_cpu_ptr(&ctx_handle);
 	struct etm_event_data *event_data = perf_get_aux(handle);
+	struct list_head *path;
 
 	if (event->hw.state == PERF_HES_STOPPED)
 		return;
@@ -313,7 +328,11 @@ static void etm_event_stop(struct perf_event *event, int mode)
 	if (!csdev)
 		return;
 
-	sink = coresight_get_sink(event_data->path[cpu]);
+	path = etm_event_cpu_path(event_data, cpu);
+	if (!path)
+		return;
+
+	sink = coresight_get_sink(path);
 	if (!sink)
 		return;
 
@@ -344,7 +363,7 @@ static void etm_event_stop(struct perf_event *event, int mode)
 	}
 
 	/* Disabling the path make its elements available to other sessions */
-	coresight_disable_path(event_data->path[cpu]);
+	coresight_disable_path(path);
 }
 
 static int etm_event_add(struct perf_event *event, int mode)
-- 
2.7.4

[PATCH v2 03/10] coresight: perf: Disable trace path upon source error

[Suzuki K Poulose]

We enable the trace path, before activating the source.
If we fail to enable the source, we must disable the path
to make sure it is available for another session.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-etm-perf.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index ab2a6e8..48b7d65b 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -300,11 +300,13 @@ static void etm_event_start(struct perf_event *event, int flags)
 
 	/* Finally enable the tracer */
 	if (source_ops(csdev)->enable(csdev, event, CS_MODE_PERF))
-		goto fail_end_stop;
+		goto fail_disable_path;
 
 out:
 	return;
 
+fail_disable_path:
+	coresight_disable_path(path);
 fail_end_stop:
 	perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
 	perf_aux_output_end(handle, 0);
-- 
2.7.4

[PATCH v2 04/10] coresight: tmc-etr: Handle driver mode specific ETR buffers

[Suzuki K Poulose]

Since the ETR could be driven either by SYSFS or by perf, it
becomes complicated how we deal with the buffers used for each
of these modes. The ETR driver cannot simply free the current
attached buffer without knowing the provider (i.e, sysfs vs perf).

To solve this issue, we provide:
1) the driver-mode specific etr buffer to be retained in the drvdata
2) the etr_buf for a session should be passed on when enabling the
   hardware, which will be stored in drvdata->etr_buf. This will be
   replaced (not free'd) as soon as the hardware is disabled, after
   necessary sync operation.

The advantages of this are :

1) The common code path doesn't need to worry about how to dispose
   an existing buffer, if it is about to start a new session with a
   different buffer, possibly in a different mode.
2) The driver mode can control its buffers and can get access to the
   saved session even when the hardware is operating in a different
   mode. (e.g, we can still access a trace buffer from a sysfs mode
   even if the etr is now used in perf mode, without disrupting the
   current session.)

Towards this, we introduce a sysfs specific data which will hold the
etr_buf used for sysfs mode of operation, controlled solely by the
sysfs mode handling code.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-tmc-etr.c | 58 ++++++++++++++++---------
 drivers/hwtracing/coresight/coresight-tmc.h     |  2 +
 2 files changed, 40 insertions(+), 20 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index 2eda5de..58c7935 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -895,10 +895,15 @@ static void tmc_sync_etr_buf(struct tmc_drvdata *drvdata)
 		tmc_etr_buf_insert_barrier_packet(etr_buf, etr_buf->offset);
 }
 
-static void tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
+static void tmc_etr_enable_hw(struct tmc_drvdata *drvdata,
+			      struct etr_buf *etr_buf)
 {
 	u32 axictl, sts;
-	struct etr_buf *etr_buf = drvdata->etr_buf;
+
+	/* Callers should provide an appropriate buffer for use */
+	if (WARN_ON(!etr_buf || drvdata->etr_buf))
+		return;
+	drvdata->etr_buf = etr_buf;
 
 	/*
 	 * If this ETR is connected to a CATU, enable it before we turn
@@ -960,13 +965,16 @@ static void tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
  * also updating the @bufpp on where to find it. Since the trace data
  * starts at anywhere in the buffer, depending on the RRP, we adjust the
  * @len returned to handle buffer wrapping around.
+ *
+ * We are protected here by drvdata->reading != 0, which ensures the
+ * sysfs_buf stays alive.
  */
 ssize_t tmc_etr_get_sysfs_trace(struct tmc_drvdata *drvdata,
 				loff_t pos, size_t len, char **bufpp)
 {
 	s64 offset;
 	ssize_t actual = len;
-	struct etr_buf *etr_buf = drvdata->etr_buf;
+	struct etr_buf *etr_buf = drvdata->sysfs_buf;
 
 	if (pos + actual > etr_buf->len)
 		actual = etr_buf->len - pos;
@@ -996,7 +1004,14 @@ tmc_etr_free_sysfs_buf(struct etr_buf *buf)
 
 static void tmc_etr_sync_sysfs_buf(struct tmc_drvdata *drvdata)
 {
-	tmc_sync_etr_buf(drvdata);
+	struct etr_buf *etr_buf = drvdata->etr_buf;
+
+	if (WARN_ON(drvdata->sysfs_buf != etr_buf)) {
+		tmc_etr_free_sysfs_buf(drvdata->sysfs_buf);
+		drvdata->sysfs_buf = NULL;
+	} else {
+		tmc_sync_etr_buf(drvdata);
+	}
 }
 
 static void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
@@ -1017,6 +1032,8 @@ static void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
 
 	/* Disable CATU device if this ETR is connected to one */
 	tmc_etr_disable_catu(drvdata);
+	/* Reset the ETR buf used by hardware */
+	drvdata->etr_buf = NULL;
 }
 
 static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
@@ -1024,7 +1041,7 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
 	int ret = 0;
 	unsigned long flags;
 	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
-	struct etr_buf *new_buf = NULL, *free_buf = NULL;
+	struct etr_buf *sysfs_buf = NULL, *new_buf = NULL, *free_buf = NULL;
 
 	/*
 	 * If we are enabling the ETR from disabled state, we need to make
@@ -1035,7 +1052,8 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
 	 * with the lock released.
 	 */
 	spin_lock_irqsave(&drvdata->spinlock, flags);
-	if (!drvdata->etr_buf || (drvdata->etr_buf->size != drvdata->size)) {
+	sysfs_buf = READ_ONCE(drvdata->sysfs_buf);
+	if (!sysfs_buf || (sysfs_buf->size != drvdata->size)) {
 		spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
 		/* Allocate memory with the locks released */
@@ -1064,14 +1082,14 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
 	 * If we don't have a buffer or it doesn't match the requested size,
 	 * use the buffer allocated above. Otherwise reuse the existing buffer.
 	 */
-	if (!drvdata->etr_buf ||
-	    (new_buf && drvdata->etr_buf->size != new_buf->size)) {
-		free_buf = drvdata->etr_buf;
-		drvdata->etr_buf = new_buf;
+	sysfs_buf = READ_ONCE(drvdata->sysfs_buf);
+	if (!sysfs_buf || (new_buf && sysfs_buf->size != new_buf->size)) {
+		free_buf = sysfs_buf;
+		drvdata->sysfs_buf = new_buf;
 	}
 
 	drvdata->mode = CS_MODE_SYSFS;
-	tmc_etr_enable_hw(drvdata);
+	tmc_etr_enable_hw(drvdata, drvdata->sysfs_buf);
 out:
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
@@ -1156,13 +1174,13 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
 		goto out;
 	}
 
-	/* If drvdata::etr_buf is NULL the trace data has been read already */
-	if (drvdata->etr_buf == NULL) {
+	/* If sysfs_buf is NULL the trace data has been read already */
+	if (!drvdata->sysfs_buf) {
 		ret = -EINVAL;
 		goto out;
 	}
 
-	/* Disable the TMC if need be */
+	/* Disable the TMC if we are trying to read from a running session */
 	if (drvdata->mode == CS_MODE_SYSFS)
 		tmc_etr_disable_hw(drvdata);
 
@@ -1176,7 +1194,7 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
 int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)
 {
 	unsigned long flags;
-	struct etr_buf *etr_buf = NULL;
+	struct etr_buf *sysfs_buf = NULL;
 
 	/* config types are set a boot time and never change */
 	if (WARN_ON_ONCE(drvdata->config_type != TMC_CONFIG_TYPE_ETR))
@@ -1191,22 +1209,22 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)
 		 * buffer. Since the tracer is still enabled drvdata::buf can't
 		 * be NULL.
 		 */
-		tmc_etr_enable_hw(drvdata);
+		tmc_etr_enable_hw(drvdata, drvdata->sysfs_buf);
 	} else {
 		/*
 		 * The ETR is not tracing and the buffer was just read.
 		 * As such prepare to free the trace buffer.
 		 */
-		etr_buf =  drvdata->etr_buf;
-		drvdata->etr_buf = NULL;
+		sysfs_buf = drvdata->sysfs_buf;
+		drvdata->sysfs_buf = NULL;
 	}
 
 	drvdata->reading = false;
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
 	/* Free allocated memory out side of the spinlock */
-	if (etr_buf)
-		tmc_free_etr_buf(etr_buf);
+	if (sysfs_buf)
+		tmc_etr_free_sysfs_buf(sysfs_buf);
 
 	return 0;
 }
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index 7027bd6..872f63e 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -170,6 +170,7 @@ struct etr_buf {
  * @trigger_cntr: amount of words to store after a trigger.
  * @etr_caps:	Bitmask of capabilities of the TMC ETR, inferred from the
  *		device configuration register (DEVID)
+ * @sysfs_data:	SYSFS buffer for ETR.
  */
 struct tmc_drvdata {
 	void __iomem		*base;
@@ -189,6 +190,7 @@ struct tmc_drvdata {
 	enum tmc_mem_intf_width	memwidth;
 	u32			trigger_cntr;
 	u32			etr_caps;
+	struct etr_buf		*sysfs_buf;
 };
 
 struct etr_buf_operations {
-- 
2.7.4

[PATCH v2 05/10] coresight: tmc-etr: Relax collection of trace from sysfs mode

[Suzuki K Poulose]

Since the ETR now uses mode specific buffers, we can reliably
provide the trace data captured in sysfs mode, even when the ETR
is operating in PERF mode.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-tmc-etr.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index 58c7935..671a2ba 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -1168,19 +1168,17 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
 		goto out;
 	}
 
-	/* Don't interfere if operated from Perf */
-	if (drvdata->mode == CS_MODE_PERF) {
-		ret = -EINVAL;
-		goto out;
-	}
-
-	/* If sysfs_buf is NULL the trace data has been read already */
+	/*
+	 * We can safely allow reads even if the ETR is operating in PERF mode,
+	 * since the sysfs session is captured in mode specific data.
+	 * If drvdata::sysfs_data is NULL the trace data has been read already.
+	 */
 	if (!drvdata->sysfs_buf) {
 		ret = -EINVAL;
 		goto out;
 	}
 
-	/* Disable the TMC if we are trying to read from a running session */
+	/* Disable the TMC if we are trying to read from a running session. */
 	if (drvdata->mode == CS_MODE_SYSFS)
 		tmc_etr_disable_hw(drvdata);
 
-- 
2.7.4

[PATCH v2 06/10] coresight: Convert driver messages to dev_dbg

[Suzuki K Poulose]

Convert component enable/disable messages from dev_info to dev_dbg.
When used with perf, the components in the paths are enabled/disabled
during each schedule of the run, which can flood the dmesg with these
messages. Moreover, they are only useful for debug purposes. So,
convert such messages to dev_dbg() which can be turned on as
needed.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-dynamic-replicator.c | 4 ++--
 drivers/hwtracing/coresight/coresight-etb10.c              | 6 +++---
 drivers/hwtracing/coresight/coresight-etm3x.c              | 4 ++--
 drivers/hwtracing/coresight/coresight-etm4x.c              | 4 ++--
 drivers/hwtracing/coresight/coresight-funnel.c             | 4 ++--
 drivers/hwtracing/coresight/coresight-replicator.c         | 4 ++--
 drivers/hwtracing/coresight/coresight-stm.c                | 4 ++--
 drivers/hwtracing/coresight/coresight-tmc-etf.c            | 8 ++++----
 drivers/hwtracing/coresight/coresight-tmc-etr.c            | 4 ++--
 drivers/hwtracing/coresight/coresight-tmc.c                | 4 ++--
 drivers/hwtracing/coresight/coresight-tpiu.c               | 4 ++--
 11 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-dynamic-replicator.c b/drivers/hwtracing/coresight/coresight-dynamic-replicator.c
index f6d0571..ebb8043 100644
--- a/drivers/hwtracing/coresight/coresight-dynamic-replicator.c
+++ b/drivers/hwtracing/coresight/coresight-dynamic-replicator.c
@@ -56,7 +56,7 @@ static int replicator_enable(struct coresight_device *csdev, int inport,
 
 	CS_LOCK(drvdata->base);
 
-	dev_info(drvdata->dev, "REPLICATOR enabled\n");
+	dev_dbg(drvdata->dev, "REPLICATOR enabled\n");
 	return 0;
 }
 
@@ -75,7 +75,7 @@ static void replicator_disable(struct coresight_device *csdev, int inport,
 
 	CS_LOCK(drvdata->base);
 
-	dev_info(drvdata->dev, "REPLICATOR disabled\n");
+	dev_dbg(drvdata->dev, "REPLICATOR disabled\n");
 }
 
 static const struct coresight_ops_link replicator_link_ops = {
diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
index 306119e..73878a6 100644
--- a/drivers/hwtracing/coresight/coresight-etb10.c
+++ b/drivers/hwtracing/coresight/coresight-etb10.c
@@ -156,7 +156,7 @@ static int etb_enable(struct coresight_device *csdev, u32 mode)
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
 out:
-	dev_info(drvdata->dev, "ETB enabled\n");
+	dev_dbg(drvdata->dev, "ETB enabled\n");
 	return 0;
 }
 
@@ -262,7 +262,7 @@ static void etb_disable(struct coresight_device *csdev)
 
 	local_set(&drvdata->mode, CS_MODE_DISABLED);
 
-	dev_info(drvdata->dev, "ETB disabled\n");
+	dev_dbg(drvdata->dev, "ETB disabled\n");
 }
 
 static void *etb_alloc_buffer(struct coresight_device *csdev, int cpu,
@@ -505,7 +505,7 @@ static void etb_dump(struct etb_drvdata *drvdata)
 	}
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
-	dev_info(drvdata->dev, "ETB dumped\n");
+	dev_dbg(drvdata->dev, "ETB dumped\n");
 }
 
 static int etb_open(struct inode *inode, struct file *file)
diff --git a/drivers/hwtracing/coresight/coresight-etm3x.c b/drivers/hwtracing/coresight/coresight-etm3x.c
index 7c74263..9ce8fba 100644
--- a/drivers/hwtracing/coresight/coresight-etm3x.c
+++ b/drivers/hwtracing/coresight/coresight-etm3x.c
@@ -501,7 +501,7 @@ static int etm_enable_sysfs(struct coresight_device *csdev)
 	drvdata->sticky_enable = true;
 	spin_unlock(&drvdata->spinlock);
 
-	dev_info(drvdata->dev, "ETM tracing enabled\n");
+	dev_dbg(drvdata->dev, "ETM tracing enabled\n");
 	return 0;
 
 err:
@@ -604,7 +604,7 @@ static void etm_disable_sysfs(struct coresight_device *csdev)
 	spin_unlock(&drvdata->spinlock);
 	cpus_read_unlock();
 
-	dev_info(drvdata->dev, "ETM tracing disabled\n");
+	dev_dbg(drvdata->dev, "ETM tracing disabled\n");
 }
 
 static void etm_disable(struct coresight_device *csdev,
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
index 1d94ebe..c1dcc7c 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x.c
@@ -267,7 +267,7 @@ static int etm4_enable_sysfs(struct coresight_device *csdev)
 	drvdata->sticky_enable = true;
 	spin_unlock(&drvdata->spinlock);
 
-	dev_info(drvdata->dev, "ETM tracing enabled\n");
+	dev_dbg(drvdata->dev, "ETM tracing enabled\n");
 	return 0;
 
 err:
@@ -380,7 +380,7 @@ static void etm4_disable_sysfs(struct coresight_device *csdev)
 	spin_unlock(&drvdata->spinlock);
 	cpus_read_unlock();
 
-	dev_info(drvdata->dev, "ETM tracing disabled\n");
+	dev_dbg(drvdata->dev, "ETM tracing disabled\n");
 }
 
 static void etm4_disable(struct coresight_device *csdev,
diff --git a/drivers/hwtracing/coresight/coresight-funnel.c b/drivers/hwtracing/coresight/coresight-funnel.c
index 448145a..ee7a30b 100644
--- a/drivers/hwtracing/coresight/coresight-funnel.c
+++ b/drivers/hwtracing/coresight/coresight-funnel.c
@@ -65,7 +65,7 @@ static int funnel_enable(struct coresight_device *csdev, int inport,
 
 	funnel_enable_hw(drvdata, inport);
 
-	dev_info(drvdata->dev, "FUNNEL inport %d enabled\n", inport);
+	dev_dbg(drvdata->dev, "FUNNEL inport %d enabled\n", inport);
 	return 0;
 }
 
@@ -89,7 +89,7 @@ static void funnel_disable(struct coresight_device *csdev, int inport,
 
 	funnel_disable_hw(drvdata, inport);
 
-	dev_info(drvdata->dev, "FUNNEL inport %d disabled\n", inport);
+	dev_dbg(drvdata->dev, "FUNNEL inport %d disabled\n", inport);
 }
 
 static const struct coresight_ops_link funnel_link_ops = {
diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c
index 8d2eaaa..feac983 100644
--- a/drivers/hwtracing/coresight/coresight-replicator.c
+++ b/drivers/hwtracing/coresight/coresight-replicator.c
@@ -35,7 +35,7 @@ static int replicator_enable(struct coresight_device *csdev, int inport,
 {
 	struct replicator_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
 
-	dev_info(drvdata->dev, "REPLICATOR enabled\n");
+	dev_dbg(drvdata->dev, "REPLICATOR enabled\n");
 	return 0;
 }
 
@@ -44,7 +44,7 @@ static void replicator_disable(struct coresight_device *csdev, int inport,
 {
 	struct replicator_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
 
-	dev_info(drvdata->dev, "REPLICATOR disabled\n");
+	dev_dbg(drvdata->dev, "REPLICATOR disabled\n");
 }
 
 static const struct coresight_ops_link replicator_link_ops = {
diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
index c46c70a..35d6f97 100644
--- a/drivers/hwtracing/coresight/coresight-stm.c
+++ b/drivers/hwtracing/coresight/coresight-stm.c
@@ -211,7 +211,7 @@ static int stm_enable(struct coresight_device *csdev,
 	stm_enable_hw(drvdata);
 	spin_unlock(&drvdata->spinlock);
 
-	dev_info(drvdata->dev, "STM tracing enabled\n");
+	dev_dbg(drvdata->dev, "STM tracing enabled\n");
 	return 0;
 }
 
@@ -274,7 +274,7 @@ static void stm_disable(struct coresight_device *csdev,
 		pm_runtime_put(drvdata->dev);
 
 		local_set(&drvdata->mode, CS_MODE_DISABLED);
-		dev_info(drvdata->dev, "STM tracing disabled\n");
+		dev_dbg(drvdata->dev, "STM tracing disabled\n");
 	}
 }
 
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index 0549249..434003a 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -233,7 +233,7 @@ static int tmc_enable_etf_sink(struct coresight_device *csdev, u32 mode)
 	if (ret)
 		return ret;
 
-	dev_info(drvdata->dev, "TMC-ETB/ETF enabled\n");
+	dev_dbg(drvdata->dev, "TMC-ETB/ETF enabled\n");
 	return 0;
 }
 
@@ -256,7 +256,7 @@ static void tmc_disable_etf_sink(struct coresight_device *csdev)
 
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
-	dev_info(drvdata->dev, "TMC-ETB/ETF disabled\n");
+	dev_dbg(drvdata->dev, "TMC-ETB/ETF disabled\n");
 }
 
 static int tmc_enable_etf_link(struct coresight_device *csdev,
@@ -275,7 +275,7 @@ static int tmc_enable_etf_link(struct coresight_device *csdev,
 	drvdata->mode = CS_MODE_SYSFS;
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
-	dev_info(drvdata->dev, "TMC-ETF enabled\n");
+	dev_dbg(drvdata->dev, "TMC-ETF enabled\n");
 	return 0;
 }
 
@@ -295,7 +295,7 @@ static void tmc_disable_etf_link(struct coresight_device *csdev,
 	drvdata->mode = CS_MODE_DISABLED;
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
-	dev_info(drvdata->dev, "TMC-ETF disabled\n");
+	dev_dbg(drvdata->dev, "TMC-ETF disabled\n");
 }
 
 static void *tmc_alloc_etf_buffer(struct coresight_device *csdev, int cpu,
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index 671a2ba..85a3f59 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -1098,7 +1098,7 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
 		tmc_etr_free_sysfs_buf(free_buf);
 
 	if (!ret)
-		dev_info(drvdata->dev, "TMC-ETR enabled\n");
+		dev_dbg(drvdata->dev, "TMC-ETR enabled\n");
 
 	return ret;
 }
@@ -1141,7 +1141,7 @@ static void tmc_disable_etr_sink(struct coresight_device *csdev)
 
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
-	dev_info(drvdata->dev, "TMC-ETR disabled\n");
+	dev_dbg(drvdata->dev, "TMC-ETR disabled\n");
 }
 
 static const struct coresight_ops_sink tmc_etr_sink_ops = {
diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
index 1b817ec..ea249f0 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.c
+++ b/drivers/hwtracing/coresight/coresight-tmc.c
@@ -81,7 +81,7 @@ static int tmc_read_prepare(struct tmc_drvdata *drvdata)
 	}
 
 	if (!ret)
-		dev_info(drvdata->dev, "TMC read start\n");
+		dev_dbg(drvdata->dev, "TMC read start\n");
 
 	return ret;
 }
@@ -103,7 +103,7 @@ static int tmc_read_unprepare(struct tmc_drvdata *drvdata)
 	}
 
 	if (!ret)
-		dev_info(drvdata->dev, "TMC read end\n");
+		dev_dbg(drvdata->dev, "TMC read end\n");
 
 	return ret;
 }
diff --git a/drivers/hwtracing/coresight/coresight-tpiu.c b/drivers/hwtracing/coresight/coresight-tpiu.c
index 01b7457..7daeb084 100644
--- a/drivers/hwtracing/coresight/coresight-tpiu.c
+++ b/drivers/hwtracing/coresight/coresight-tpiu.c
@@ -73,7 +73,7 @@ static int tpiu_enable(struct coresight_device *csdev, u32 mode)
 
 	tpiu_enable_hw(drvdata);
 
-	dev_info(drvdata->dev, "TPIU enabled\n");
+	dev_dbg(drvdata->dev, "TPIU enabled\n");
 	return 0;
 }
 
@@ -99,7 +99,7 @@ static void tpiu_disable(struct coresight_device *csdev)
 
 	tpiu_disable_hw(drvdata);
 
-	dev_info(drvdata->dev, "TPIU disabled\n");
+	dev_dbg(drvdata->dev, "TPIU disabled\n");
 }
 
 static const struct coresight_ops_sink tpiu_sink_ops = {
-- 
2.7.4

[PATCH v2 07/10] coresight: perf: Remove reset_buffer call back for sinks

[Suzuki K Poulose]

Right now we issue an update_buffer() and reset_buffer() call backs
in succession when we stop tracing an event. The update_buffer is
supposed to check the status of the buffer and make sure the ring buffer
is updated with the trace data. And we store information about the
size of the data collected only to be consumed by the reset_buffer
callback which always follows the update_buffer. This was originally
designed for handling future IPs which could trigger a buffer overflow
interrupt. This patch gets rid of the reset_buffer callback altogether
and performs the actions in update_buffer, making it return the size
collected. We can always add the support for handling the overflow
interrupt case later.

This removes some not-so pretty hack (storing the new head in the
size field for snapshot mode) and cleans it up a little bit.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-etb10.c    | 56 +++++------------------
 drivers/hwtracing/coresight/coresight-etm-perf.c |  9 +---
 drivers/hwtracing/coresight/coresight-tmc-etf.c  | 58 +++++-------------------
 include/linux/coresight.h                        |  6 +--
 4 files changed, 26 insertions(+), 103 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
index 73878a6..a729a7b 100644
--- a/drivers/hwtracing/coresight/coresight-etb10.c
+++ b/drivers/hwtracing/coresight/coresight-etb10.c
@@ -315,37 +315,7 @@ static int etb_set_buffer(struct coresight_device *csdev,
 	return ret;
 }
 
-static unsigned long etb_reset_buffer(struct coresight_device *csdev,
-				      struct perf_output_handle *handle,
-				      void *sink_config)
-{
-	unsigned long size = 0;
-	struct cs_buffers *buf = sink_config;
-
-	if (buf) {
-		/*
-		 * In snapshot mode ->data_size holds the new address of the
-		 * ring buffer's head.  The size itself is the whole address
-		 * range since we want the latest information.
-		 */
-		if (buf->snapshot)
-			handle->head = local_xchg(&buf->data_size,
-						  buf->nr_pages << PAGE_SHIFT);
-
-		/*
-		 * Tell the tracer PMU how much we got in this run and if
-		 * something went wrong along the way.  Nobody else can use
-		 * this cs_buffers instance until we are done.  As such
-		 * resetting parameters here and squaring off with the ring
-		 * buffer API in the tracer PMU is fine.
-		 */
-		size = local_xchg(&buf->data_size, 0);
-	}
-
-	return size;
-}
-
-static void etb_update_buffer(struct coresight_device *csdev,
+static unsigned long etb_update_buffer(struct coresight_device *csdev,
 			      struct perf_output_handle *handle,
 			      void *sink_config)
 {
@@ -354,13 +324,13 @@ static void etb_update_buffer(struct coresight_device *csdev,
 	u8 *buf_ptr;
 	const u32 *barrier;
 	u32 read_ptr, write_ptr, capacity;
-	u32 status, read_data, to_read;
-	unsigned long offset;
+	u32 status, read_data;
+	unsigned long offset, to_read;
 	struct cs_buffers *buf = sink_config;
 	struct etb_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
 
 	if (!buf)
-		return;
+		return 0;
 
 	capacity = drvdata->buffer_depth * ETB_FRAME_SIZE_WORDS;
 
@@ -465,18 +435,17 @@ static void etb_update_buffer(struct coresight_device *csdev,
 	writel_relaxed(0x0, drvdata->base + ETB_RAM_WRITE_POINTER);
 
 	/*
-	 * In snapshot mode all we have to do is communicate to
-	 * perf_aux_output_end() the address of the current head.  In full
-	 * trace mode the same function expects a size to move rb->aux_head
-	 * forward.
+	 * In snapshot mode we have to update the handle->head to point
+	 * to the new location.
 	 */
-	if (buf->snapshot)
-		local_set(&buf->data_size, (cur * PAGE_SIZE) + offset);
-	else
-		local_add(to_read, &buf->data_size);
-
+	if (buf->snapshot) {
+		handle->head = (cur * PAGE_SIZE) + offset;
+		to_read = buf->nr_pages << PAGE_SHIFT;
+	}
 	etb_enable_hw(drvdata);
 	CS_LOCK(drvdata->base);
+
+	return to_read;
 }
 
 static const struct coresight_ops_sink etb_sink_ops = {
@@ -485,7 +454,6 @@ static const struct coresight_ops_sink etb_sink_ops = {
 	.alloc_buffer	= etb_alloc_buffer,
 	.free_buffer	= etb_free_buffer,
 	.set_buffer	= etb_set_buffer,
-	.reset_buffer	= etb_reset_buffer,
 	.update_buffer	= etb_update_buffer,
 };
 
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 48b7d65b..6a4252b 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -352,15 +352,8 @@ static void etm_event_stop(struct perf_event *event, int mode)
 		if (!sink_ops(sink)->update_buffer)
 			return;
 
-		sink_ops(sink)->update_buffer(sink, handle,
+		size = sink_ops(sink)->update_buffer(sink, handle,
 					      event_data->snk_config);
-
-		if (!sink_ops(sink)->reset_buffer)
-			return;
-
-		size = sink_ops(sink)->reset_buffer(sink, handle,
-						    event_data->snk_config);
-
 		perf_aux_output_end(handle, size);
 	}
 
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index 434003a..31a98f9 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -349,36 +349,7 @@ static int tmc_set_etf_buffer(struct coresight_device *csdev,
 	return ret;
 }
 
-static unsigned long tmc_reset_etf_buffer(struct coresight_device *csdev,
-					  struct perf_output_handle *handle,
-					  void *sink_config)
-{
-	long size = 0;
-	struct cs_buffers *buf = sink_config;
-
-	if (buf) {
-		/*
-		 * In snapshot mode ->data_size holds the new address of the
-		 * ring buffer's head.  The size itself is the whole address
-		 * range since we want the latest information.
-		 */
-		if (buf->snapshot)
-			handle->head = local_xchg(&buf->data_size,
-						  buf->nr_pages << PAGE_SHIFT);
-		/*
-		 * Tell the tracer PMU how much we got in this run and if
-		 * something went wrong along the way.  Nobody else can use
-		 * this cs_buffers instance until we are done.  As such
-		 * resetting parameters here and squaring off with the ring
-		 * buffer API in the tracer PMU is fine.
-		 */
-		size = local_xchg(&buf->data_size, 0);
-	}
-
-	return size;
-}
-
-static void tmc_update_etf_buffer(struct coresight_device *csdev,
+static unsigned long tmc_update_etf_buffer(struct coresight_device *csdev,
 				  struct perf_output_handle *handle,
 				  void *sink_config)
 {
@@ -387,17 +358,17 @@ static void tmc_update_etf_buffer(struct coresight_device *csdev,
 	const u32 *barrier;
 	u32 *buf_ptr;
 	u64 read_ptr, write_ptr;
-	u32 status, to_read;
-	unsigned long offset;
+	u32 status;
+	unsigned long offset, to_read;
 	struct cs_buffers *buf = sink_config;
 	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
 
 	if (!buf)
-		return;
+		return 0;
 
 	/* This shouldn't happen */
 	if (WARN_ON_ONCE(drvdata->mode != CS_MODE_PERF))
-		return;
+		return 0;
 
 	CS_UNLOCK(drvdata->base);
 
@@ -486,18 +457,14 @@ static void tmc_update_etf_buffer(struct coresight_device *csdev,
 		}
 	}
 
-	/*
-	 * In snapshot mode all we have to do is communicate to
-	 * perf_aux_output_end() the address of the current head.  In full
-	 * trace mode the same function expects a size to move rb->aux_head
-	 * forward.
-	 */
-	if (buf->snapshot)
-		local_set(&buf->data_size, (cur * PAGE_SIZE) + offset);
-	else
-		local_add(to_read, &buf->data_size);
-
+	/* In snapshot mode we have to update the head */
+	if (buf->snapshot) {
+		handle->head = (cur * PAGE_SIZE) + offset;
+		to_read = buf->nr_pages << PAGE_SHIFT;
+	}
 	CS_LOCK(drvdata->base);
+
+	return to_read;
 }
 
 static const struct coresight_ops_sink tmc_etf_sink_ops = {
@@ -506,7 +473,6 @@ static const struct coresight_ops_sink tmc_etf_sink_ops = {
 	.alloc_buffer	= tmc_alloc_etf_buffer,
 	.free_buffer	= tmc_free_etf_buffer,
 	.set_buffer	= tmc_set_etf_buffer,
-	.reset_buffer	= tmc_reset_etf_buffer,
 	.update_buffer	= tmc_update_etf_buffer,
 };
 
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index d828a6e..eafe4d1 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -191,7 +191,6 @@ struct coresight_device {
  * @alloc_buffer:	initialises perf's ring buffer for trace collection.
  * @free_buffer:	release memory allocated in @get_config.
  * @set_buffer:		initialises buffer mechanic before a trace session.
- * @reset_buffer:	finalises buffer mechanic after a trace session.
  * @update_buffer:	update buffer pointers after a trace session.
  */
 struct coresight_ops_sink {
@@ -203,10 +202,7 @@ struct coresight_ops_sink {
 	int (*set_buffer)(struct coresight_device *csdev,
 			  struct perf_output_handle *handle,
 			  void *sink_config);
-	unsigned long (*reset_buffer)(struct coresight_device *csdev,
-				      struct perf_output_handle *handle,
-				      void *sink_config);
-	void (*update_buffer)(struct coresight_device *csdev,
+	unsigned long (*update_buffer)(struct coresight_device *csdev,
 			      struct perf_output_handle *handle,
 			      void *sink_config);
 };
-- 
2.7.4

[PATCH v2 08/10] coresight: perf: Add helper to retrieve sink configuration

[Suzuki K Poulose]

We can always find the sink configuration for a given perf_output_handle.
Add a helper to retrieve the sink configuration for a given
perf_output_handle. This will be used to get rid of the set_buffer()
call back.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-etm-perf.c | 14 -------------
 drivers/hwtracing/coresight/coresight-etm-perf.h | 26 ++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 6a4252b..3cc4a0b 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -23,20 +23,6 @@
 static struct pmu etm_pmu;
 static bool etm_perf_up;
 
-/**
- * struct etm_event_data - Coresight specifics associated to an event
- * @work:		Handle to free allocated memory outside IRQ context.
- * @mask:		Hold the CPU(s) this event was set for.
- * @snk_config:		The sink configuration.
- * @path:		An array of path, each slot for one CPU.
- */
-struct etm_event_data {
-	struct work_struct work;
-	cpumask_t mask;
-	void *snk_config;
-	struct list_head * __percpu *path;
-};
-
 static DEFINE_PER_CPU(struct perf_output_handle, ctx_handle);
 static DEFINE_PER_CPU(struct coresight_device *, csdev_src);
 
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h
index 4197df4..da7d933 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.h
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.h
@@ -7,6 +7,7 @@
 #ifndef _CORESIGHT_ETM_PERF_H
 #define _CORESIGHT_ETM_PERF_H
 
+#include <linux/percpu-defs.h>
 #include "coresight-priv.h"
 
 struct coresight_device;
@@ -42,14 +43,39 @@ struct etm_filters {
 	bool			ssstatus;
 };
 
+/**
+ * struct etm_event_data - Coresight specifics associated to an event
+ * @work:		Handle to free allocated memory outside IRQ context.
+ * @mask:		Hold the CPU(s) this event was set for.
+ * @snk_config:		The sink configuration.
+ * @path:		An array of path, each slot for one CPU.
+ */
+struct etm_event_data {
+	struct work_struct work;
+	cpumask_t mask;
+	void *snk_config;
+	struct list_head * __percpu *path;
+};
 
 #ifdef CONFIG_CORESIGHT
 int etm_perf_symlink(struct coresight_device *csdev, bool link);
+static inline void *etm_perf_sink_config(struct perf_output_handle *handle)
+{
+	struct etm_event_data *data = perf_get_aux(handle);
 
+	if (data)
+		return data->snk_config;
+	return NULL;
+}
 #else
 static inline int etm_perf_symlink(struct coresight_device *csdev, bool link)
 { return -EINVAL; }
 
+static inline void *etm_perf_sink_config(struct perf_output_handle *handle)
+{
+	return NULL;
+}
+
 #endif /* CONFIG_CORESIGHT */
 
 #endif
-- 
2.7.4

[PATCH v2 09/10] coresight: perf: Remove set_buffer call back

[Suzuki K Poulose]

In coresight perf mode, we need to prepare the sink before
starting a session, which is done via set_buffer call back.
We then proceed to enable the tracing. If we fail to start
the session successfully, we leave the sink configuration
unchanged. This was fine for the existing backends as they
don't have any state associated with the buffers. But with
ETR, we need to keep track of the buffer details and need
to be cleaned up if we fail. In order to make the operation
atomic and to avoid yet another call back, we get rid of
the "set_buffer" call back and pass the buffer details
via enable() call back to the sink.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-etb10.c    | 24 ++++++++++++++------
 drivers/hwtracing/coresight/coresight-etm-perf.c |  9 ++------
 drivers/hwtracing/coresight/coresight-priv.h     |  2 +-
 drivers/hwtracing/coresight/coresight-tmc-etf.c  | 28 ++++++++++++++++--------
 drivers/hwtracing/coresight/coresight-tmc-etr.c  |  7 +++---
 drivers/hwtracing/coresight/coresight-tpiu.c     |  2 +-
 drivers/hwtracing/coresight/coresight.c          | 11 +++++-----
 include/linux/coresight.h                        |  6 +----
 8 files changed, 51 insertions(+), 38 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
index a729a7b..4e829db 100644
--- a/drivers/hwtracing/coresight/coresight-etb10.c
+++ b/drivers/hwtracing/coresight/coresight-etb10.c
@@ -28,6 +28,7 @@
 
 
 #include "coresight-priv.h"
+#include "coresight-etm-perf.h"
 
 #define ETB_RAM_DEPTH_REG	0x004
 #define ETB_STATUS_REG		0x00c
@@ -90,6 +91,9 @@ struct etb_drvdata {
 	u32			trigger_cntr;
 };
 
+static int etb_set_buffer(struct coresight_device *csdev,
+			  struct perf_output_handle *handle);
+
 static unsigned int etb_get_buffer_depth(struct etb_drvdata *drvdata)
 {
 	u32 depth = 0;
@@ -131,12 +135,19 @@ static void etb_enable_hw(struct etb_drvdata *drvdata)
 	CS_LOCK(drvdata->base);
 }
 
-static int etb_enable(struct coresight_device *csdev, u32 mode)
+static int etb_enable(struct coresight_device *csdev, u32 mode, void *data)
 {
+	int ret = 0;
 	u32 val;
 	unsigned long flags;
 	struct etb_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
 
+	if (mode == CS_MODE_PERF) {
+		ret = etb_set_buffer(csdev, (struct perf_output_handle *)data);
+		if (ret)
+			goto out;
+	}
+
 	val = local_cmpxchg(&drvdata->mode,
 			    CS_MODE_DISABLED, mode);
 	/*
@@ -156,8 +167,9 @@ static int etb_enable(struct coresight_device *csdev, u32 mode)
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
 out:
-	dev_dbg(drvdata->dev, "ETB enabled\n");
-	return 0;
+	if (!ret)
+		dev_dbg(drvdata->dev, "ETB enabled\n");
+	return ret;
 }
 
 static void etb_disable_hw(struct etb_drvdata *drvdata)
@@ -294,12 +306,11 @@ static void etb_free_buffer(void *config)
 }
 
 static int etb_set_buffer(struct coresight_device *csdev,
-			  struct perf_output_handle *handle,
-			  void *sink_config)
+			  struct perf_output_handle *handle)
 {
 	int ret = 0;
 	unsigned long head;
-	struct cs_buffers *buf = sink_config;
+	struct cs_buffers *buf = etm_perf_sink_config(handle);
 
 	/* wrap head around to the amount of space we have */
 	head = handle->head & ((buf->nr_pages << PAGE_SHIFT) - 1);
@@ -453,7 +464,6 @@ static const struct coresight_ops_sink etb_sink_ops = {
 	.disable	= etb_disable,
 	.alloc_buffer	= etb_alloc_buffer,
 	.free_buffer	= etb_free_buffer,
-	.set_buffer	= etb_set_buffer,
 	.update_buffer	= etb_update_buffer,
 };
 
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 3cc4a0b..12a247d 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -269,16 +269,11 @@ static void etm_event_start(struct perf_event *event, int flags)
 	path = etm_event_cpu_path(event_data, cpu);
 	/* We need a sink, no need to continue without one */
 	sink = coresight_get_sink(path);
-	if (WARN_ON_ONCE(!sink || !sink_ops(sink)->set_buffer))
-		goto fail_end_stop;
-
-	/* Configure the sink */
-	if (sink_ops(sink)->set_buffer(sink, handle,
-				       event_data->snk_config))
+	if (WARN_ON_ONCE(!sink))
 		goto fail_end_stop;
 
 	/* Nothing will happen without a path */
-	if (coresight_enable_path(path, CS_MODE_PERF))
+	if (coresight_enable_path(path, CS_MODE_PERF, handle))
 		goto fail_end_stop;
 
 	/* Tell the perf core the event is alive */
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index 1a6cf35..c11da55 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -137,7 +137,7 @@ static inline void coresight_write_reg_pair(void __iomem *addr, u64 val,
 }
 
 void coresight_disable_path(struct list_head *path);
-int coresight_enable_path(struct list_head *path, u32 mode);
+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 list_head *coresight_build_path(struct coresight_device *csdev,
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index 31a98f9..4156c95 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -10,6 +10,10 @@
 #include <linux/slab.h>
 #include "coresight-priv.h"
 #include "coresight-tmc.h"
+#include "coresight-etm-perf.h"
+
+static int tmc_set_etf_buffer(struct coresight_device *csdev,
+			      struct perf_output_handle *handle);
 
 static void tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
 {
@@ -182,11 +186,12 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev)
 	return ret;
 }
 
-static int tmc_enable_etf_sink_perf(struct coresight_device *csdev)
+static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, void *data)
 {
 	int ret = 0;
 	unsigned long flags;
 	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+	struct perf_output_handle *handle = data;
 
 	spin_lock_irqsave(&drvdata->spinlock, flags);
 	if (drvdata->reading) {
@@ -204,15 +209,19 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev)
 		goto out;
 	}
 
-	drvdata->mode = CS_MODE_PERF;
-	tmc_etb_enable_hw(drvdata);
+	ret = tmc_set_etf_buffer(csdev, handle);
+	if (!ret) {
+		drvdata->mode = CS_MODE_PERF;
+		tmc_etb_enable_hw(drvdata);
+	}
 out:
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
 	return ret;
 }
 
-static int tmc_enable_etf_sink(struct coresight_device *csdev, u32 mode)
+static int tmc_enable_etf_sink(struct coresight_device *csdev,
+			       u32 mode, void *data)
 {
 	int ret;
 	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
@@ -222,7 +231,7 @@ static int tmc_enable_etf_sink(struct coresight_device *csdev, u32 mode)
 		ret = tmc_enable_etf_sink_sysfs(csdev);
 		break;
 	case CS_MODE_PERF:
-		ret = tmc_enable_etf_sink_perf(csdev);
+		ret = tmc_enable_etf_sink_perf(csdev, data);
 		break;
 	/* We shouldn't be here */
 	default:
@@ -328,12 +337,14 @@ static void tmc_free_etf_buffer(void *config)
 }
 
 static int tmc_set_etf_buffer(struct coresight_device *csdev,
-			      struct perf_output_handle *handle,
-			      void *sink_config)
+			      struct perf_output_handle *handle)
 {
 	int ret = 0;
 	unsigned long head;
-	struct cs_buffers *buf = sink_config;
+	struct cs_buffers *buf = etm_perf_sink_config(handle);
+
+	if (!buf)
+		return -EINVAL;
 
 	/* wrap head around to the amount of space we have */
 	head = handle->head & ((buf->nr_pages << PAGE_SHIFT) - 1);
@@ -472,7 +483,6 @@ static const struct coresight_ops_sink tmc_etf_sink_ops = {
 	.disable	= tmc_disable_etf_sink,
 	.alloc_buffer	= tmc_alloc_etf_buffer,
 	.free_buffer	= tmc_free_etf_buffer,
-	.set_buffer	= tmc_set_etf_buffer,
 	.update_buffer	= tmc_update_etf_buffer,
 };
 
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index 85a3f59..bc0c932 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -1103,19 +1103,20 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
 	return ret;
 }
 
-static int tmc_enable_etr_sink_perf(struct coresight_device *csdev)
+static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
 {
 	/* We don't support perf mode yet ! */
 	return -EINVAL;
 }
 
-static int tmc_enable_etr_sink(struct coresight_device *csdev, u32 mode)
+static int tmc_enable_etr_sink(struct coresight_device *csdev,
+			       u32 mode, void *data)
 {
 	switch (mode) {
 	case CS_MODE_SYSFS:
 		return tmc_enable_etr_sink_sysfs(csdev);
 	case CS_MODE_PERF:
-		return tmc_enable_etr_sink_perf(csdev);
+		return tmc_enable_etr_sink_perf(csdev, data);
 	}
 
 	/* We shouldn't be here */
diff --git a/drivers/hwtracing/coresight/coresight-tpiu.c b/drivers/hwtracing/coresight/coresight-tpiu.c
index 7daeb084..d7dd5e4 100644
--- a/drivers/hwtracing/coresight/coresight-tpiu.c
+++ b/drivers/hwtracing/coresight/coresight-tpiu.c
@@ -67,7 +67,7 @@ static void tpiu_enable_hw(struct tpiu_drvdata *drvdata)
 	CS_LOCK(drvdata->base);
 }
 
-static int tpiu_enable(struct coresight_device *csdev, u32 mode)
+static int tpiu_enable(struct coresight_device *csdev, u32 mode, void *__unused)
 {
 	struct tpiu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
 
diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index c0dabbd..fab87c9 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -128,7 +128,8 @@ static int coresight_find_link_outport(struct coresight_device *csdev,
 	return -ENODEV;
 }
 
-static int coresight_enable_sink(struct coresight_device *csdev, u32 mode)
+static int coresight_enable_sink(struct coresight_device *csdev,
+				 u32 mode, void *data)
 {
 	int ret;
 
@@ -137,7 +138,7 @@ static int coresight_enable_sink(struct coresight_device *csdev, u32 mode)
 	 * existing "mode" of operation.
 	 */
 	if (sink_ops(csdev)->enable) {
-		ret = sink_ops(csdev)->enable(csdev, mode);
+		ret = sink_ops(csdev)->enable(csdev, mode, data);
 		if (ret)
 			return ret;
 		csdev->enable = true;
@@ -315,7 +316,7 @@ void coresight_disable_path(struct list_head *path)
 	}
 }
 
-int coresight_enable_path(struct list_head *path, u32 mode)
+int coresight_enable_path(struct list_head *path, u32 mode, void *sink_data)
 {
 
 	int ret = 0;
@@ -340,7 +341,7 @@ int coresight_enable_path(struct list_head *path, u32 mode)
 
 		switch (type) {
 		case CORESIGHT_DEV_TYPE_SINK:
-			ret = coresight_enable_sink(csdev, mode);
+			ret = coresight_enable_sink(csdev, mode, sink_data);
 			/*
 			 * Sink is the first component turned on. If we
 			 * failed to enable the sink, there are no components
@@ -643,7 +644,7 @@ int coresight_enable(struct coresight_device *csdev)
 		goto out;
 	}
 
-	ret = coresight_enable_path(path, CS_MODE_SYSFS);
+	ret = coresight_enable_path(path, CS_MODE_SYSFS, NULL);
 	if (ret)
 		goto err_path;
 
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index eafe4d1..3434758 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -190,18 +190,14 @@ struct coresight_device {
  * @disable:		disables the sink.
  * @alloc_buffer:	initialises perf's ring buffer for trace collection.
  * @free_buffer:	release memory allocated in @get_config.
- * @set_buffer:		initialises buffer mechanic before a trace session.
  * @update_buffer:	update buffer pointers after a trace session.
  */
 struct coresight_ops_sink {
-	int (*enable)(struct coresight_device *csdev, u32 mode);
+	int (*enable)(struct coresight_device *csdev, u32 mode, void *data);
 	void (*disable)(struct coresight_device *csdev);
 	void *(*alloc_buffer)(struct coresight_device *csdev, int cpu,
 			      void **pages, int nr_pages, bool overwrite);
 	void (*free_buffer)(void *config);
-	int (*set_buffer)(struct coresight_device *csdev,
-			  struct perf_output_handle *handle,
-			  void *sink_config);
 	unsigned long (*update_buffer)(struct coresight_device *csdev,
 			      struct perf_output_handle *handle,
 			      void *sink_config);
-- 
2.7.4

[PATCH v2 10/10] coresight: etm-perf: Add support for ETR backend

[Suzuki K Poulose]

Add support for using TMC-ETR as backend for ETM perf tracing.
We use software double buffering at the moment. i.e, the TMC-ETR
uses a separate buffer than the perf ring buffer. The data is
copied to the perf ring buffer once a session completes.

The TMC-ETR would try to match the larger of perf ring buffer
or the ETR buffer size configured via sysfs, scaling down to
a minimum limit of 1MB.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-tmc-etr.c | 248 +++++++++++++++++++++++-
 drivers/hwtracing/coresight/coresight-tmc.h     |   2 +
 2 files changed, 248 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index bc0c932..6280790 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -10,6 +10,7 @@
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
 #include "coresight-catu.h"
+#include "coresight-etm-perf.h"
 #include "coresight-priv.h"
 #include "coresight-tmc.h"
 
@@ -21,6 +22,28 @@ struct etr_flat_buf {
 };
 
 /*
+ * etr_perf_buffer - Perf buffer used for ETR
+ * @etr_buf		- Actual buffer used by the ETR
+ * @snaphost		- Perf session mode
+ * @head		- handle->head at the beginning of the session.
+ * @nr_pages		- Number of pages in the ring buffer.
+ * @pages		- Array of Pages in the ring buffer.
+ */
+struct etr_perf_buffer {
+	struct etr_buf		*etr_buf;
+	bool			snapshot;
+	unsigned long		head;
+	int			nr_pages;
+	void			**pages;
+};
+
+/* Convert the perf index to an offset within the ETR buffer */
+#define PERF_IDX2OFF(idx, buf)	((idx) % ((buf)->nr_pages << PAGE_SHIFT))
+
+/* Lower limit for ETR hardware buffer */
+#define TMC_ETR_PERF_MIN_BUF_SIZE	SZ_1M
+
+/*
  * The TMC ETR SG has a page size of 4K. The SG table contains pointers
  * to 4KB buffers. However, the OS may use a PAGE_SIZE different from
  * 4K (i.e, 16KB or 64KB). This implies that a single OS page could
@@ -1103,10 +1126,228 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
 	return ret;
 }
 
+/*
+ * tmc_etr_setup_perf_buf: Allocate ETR buffer for use by perf.
+ * The size of the hardware buffer is dependent on the size configured
+ * via sysfs and the perf ring buffer size. We prefer to allocate the
+ * largest possible size, scaling down the size by half until it
+ * reaches a minimum limit (1M), beyond which we give up.
+ */
+static struct etr_perf_buffer *
+tmc_etr_setup_perf_buf(struct tmc_drvdata *drvdata, int node, int nr_pages,
+		       void **pages, bool snapshot)
+{
+	struct etr_buf *etr_buf;
+	struct etr_perf_buffer *etr_perf;
+	unsigned long size;
+
+	etr_perf = kzalloc_node(sizeof(*etr_perf), GFP_KERNEL, node);
+	if (!etr_perf)
+		return ERR_PTR(-ENOMEM);
+
+	/*
+	 * Try to match the perf ring buffer size if it is larger
+	 * than the size requested via sysfs.
+	 */
+	if ((nr_pages << PAGE_SHIFT) > drvdata->size) {
+		etr_buf = tmc_alloc_etr_buf(drvdata, (nr_pages << PAGE_SHIFT),
+					    0, node, NULL);
+		if (!IS_ERR(etr_buf))
+			goto done;
+	}
+
+	/*
+	 * Else switch to configured size for this ETR
+	 * and scale down until we hit the minimum limit.
+	 */
+	size = drvdata->size;
+	do {
+		etr_buf = tmc_alloc_etr_buf(drvdata, size, 0, node, NULL);
+		if (!IS_ERR(etr_buf))
+			goto done;
+		size /= 2;
+	} while (size >= TMC_ETR_PERF_MIN_BUF_SIZE);
+
+	kfree(etr_perf);
+	return ERR_PTR(-ENOMEM);
+
+done:
+	etr_perf->etr_buf = etr_buf;
+	return etr_perf;
+}
+
+
+static void *tmc_etr_alloc_perf_buffer(struct coresight_device *csdev,
+				       int cpu, void **pages, int nr_pages,
+				       bool snapshot)
+{
+	struct etr_perf_buffer *etr_perf;
+	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	if (cpu == -1)
+		cpu = smp_processor_id();
+
+	etr_perf = tmc_etr_setup_perf_buf(drvdata, cpu_to_node(cpu),
+					  nr_pages, pages, snapshot);
+	if (IS_ERR(etr_perf)) {
+		dev_dbg(drvdata->dev, "Unable to allocate ETR buffer\n");
+		return NULL;
+	}
+
+	etr_perf->snapshot = snapshot;
+	etr_perf->nr_pages = nr_pages;
+	etr_perf->pages = pages;
+
+	return etr_perf;
+}
+
+static void tmc_etr_free_perf_buffer(void *config)
+{
+	struct etr_perf_buffer *etr_perf = config;
+
+	if (etr_perf->etr_buf)
+		tmc_free_etr_buf(etr_perf->etr_buf);
+	kfree(etr_perf);
+}
+
+/*
+ * tmc_etr_sync_perf_buffer: Copy the actual trace data from the hardware
+ * buffer to the perf ring buffer.
+ */
+static void tmc_etr_sync_perf_buffer(struct etr_perf_buffer *etr_perf)
+{
+	long bytes, to_copy;
+	long pg_idx, pg_offset, src_offset;
+	unsigned long head = etr_perf->head;
+	char **dst_pages, *src_buf;
+	struct etr_buf *etr_buf = etr_perf->etr_buf;
+
+	head = etr_perf->head;
+	pg_idx = head >> PAGE_SHIFT;
+	pg_offset = head & (PAGE_SIZE - 1);
+	dst_pages = (char **)etr_perf->pages;
+	src_offset = etr_buf->offset;
+	to_copy = etr_buf->len;
+
+	while (to_copy > 0) {
+		/*
+		 * In one iteration, we can copy minimum of :
+		 *  1) what is available in the source buffer,
+		 *  2) what is available in the source buffer, before it
+		 *     wraps around.
+		 *  3) what is available in the destination page.
+		 * in one iteration.
+		 */
+		bytes = tmc_etr_buf_get_data(etr_buf, src_offset, to_copy,
+					     &src_buf);
+		if (WARN_ON_ONCE(bytes <= 0))
+			break;
+		bytes = min(bytes, (long)(PAGE_SIZE - pg_offset));
+
+		memcpy(dst_pages[pg_idx] + pg_offset, src_buf, bytes);
+
+		to_copy -= bytes;
+
+		/* Move destination pointers */
+		pg_offset += bytes;
+		if (pg_offset == PAGE_SIZE) {
+			pg_offset = 0;
+			if (++pg_idx == etr_perf->nr_pages)
+				pg_idx = 0;
+		}
+
+		/* Move source pointers */
+		src_offset += bytes;
+		if (src_offset >= etr_buf->size)
+			src_offset -= etr_buf->size;
+	}
+}
+
+/*
+ * tmc_etr_update_perf_buffer : Update the perf ring buffer with the
+ * available trace data. We use software double buffering at the moment.
+ *
+ * TODO: Add support for reusing the perf ring buffer.
+ */
+static unsigned long
+tmc_etr_update_perf_buffer(struct coresight_device *csdev,
+			   struct perf_output_handle *handle,
+			   void *config)
+{
+	bool lost = false;
+	unsigned long flags, size = 0;
+	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+	struct etr_perf_buffer *etr_perf = config;
+	struct etr_buf *etr_buf = etr_perf->etr_buf;
+
+	spin_lock_irqsave(&drvdata->spinlock, flags);
+	if (WARN_ON(drvdata->perf_data != etr_perf)) {
+		lost = true;
+		spin_unlock_irqrestore(&drvdata->spinlock, flags);
+		goto out;
+	}
+
+	CS_UNLOCK(drvdata->base);
+
+	tmc_flush_and_stop(drvdata);
+	tmc_sync_etr_buf(drvdata);
+
+	CS_UNLOCK(drvdata->base);
+	/* Reset perf specific data */
+	drvdata->perf_data = NULL;
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+
+	size = etr_buf->len;
+	tmc_etr_sync_perf_buffer(etr_perf);
+
+	/*
+	 * Update handle->head in snapshot mode. Also update the size to the
+	 * hardware buffer size if there was an overflow.
+	 */
+	if (etr_perf->snapshot) {
+		handle->head += size;
+		if (etr_buf->full)
+			size = etr_buf->size;
+	}
+
+	lost |= etr_buf->full;
+out:
+	if (lost)
+		perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
+	return size;
+}
+
 static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
 {
-	/* We don't support perf mode yet ! */
-	return -EINVAL;
+	int rc = 0;
+	unsigned long flags;
+	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+	struct perf_output_handle *handle = data;
+	struct etr_perf_buffer *etr_perf = etm_perf_sink_config(handle);
+
+	spin_lock_irqsave(&drvdata->spinlock, flags);
+	/*
+	 * There can be only one writer per sink in perf mode. If the sink
+	 * is already open in SYSFS mode, we can't use it.
+	 */
+	if (drvdata->mode != CS_MODE_DISABLED || drvdata->perf_data) {
+		rc = -EBUSY;
+		goto unlock_out;
+	}
+
+	if (WARN_ON(!etr_perf || !etr_perf->etr_buf)) {
+		rc = -EINVAL;
+		goto unlock_out;
+	}
+
+	etr_perf->head = PERF_IDX2OFF(handle->head, etr_perf);
+	drvdata->perf_data = etr_perf;
+	drvdata->mode = CS_MODE_PERF;
+	tmc_etr_enable_hw(drvdata, etr_perf->etr_buf);
+
+unlock_out:
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+	return rc;
 }
 
 static int tmc_enable_etr_sink(struct coresight_device *csdev,
@@ -1148,6 +1389,9 @@ static void tmc_disable_etr_sink(struct coresight_device *csdev)
 static const struct coresight_ops_sink tmc_etr_sink_ops = {
 	.enable		= tmc_enable_etr_sink,
 	.disable	= tmc_disable_etr_sink,
+	.alloc_buffer	= tmc_etr_alloc_perf_buffer,
+	.update_buffer	= tmc_etr_update_perf_buffer,
+	.free_buffer	= tmc_etr_free_perf_buffer,
 };
 
 const struct coresight_ops tmc_etr_cs_ops = {
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index 872f63e..487c537 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -170,6 +170,7 @@ struct etr_buf {
  * @trigger_cntr: amount of words to store after a trigger.
  * @etr_caps:	Bitmask of capabilities of the TMC ETR, inferred from the
  *		device configuration register (DEVID)
+ * @perf_data:	PERF buffer for ETR.
  * @sysfs_data:	SYSFS buffer for ETR.
  */
 struct tmc_drvdata {
@@ -191,6 +192,7 @@ struct tmc_drvdata {
 	u32			trigger_cntr;
 	u32			etr_caps;
 	struct etr_buf		*sysfs_buf;
+	void			*perf_data;
 };
 
 struct etr_buf_operations {
-- 
2.7.4

Re: [PATCH v2 10/10] coresight: etm-perf: Add support for ETR backend

[Mathieu Poirier]

On Tue, Jul 17, 2018 at 06:11:41PM +0100, Suzuki K Poulose wrote:
> Add support for using TMC-ETR as backend for ETM perf tracing.
> We use software double buffering at the moment. i.e, the TMC-ETR
> uses a separate buffer than the perf ring buffer. The data is
> copied to the perf ring buffer once a session completes.
> 
> The TMC-ETR would try to match the larger of perf ring buffer
> or the ETR buffer size configured via sysfs, scaling down to
> a minimum limit of 1MB.
> 
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight-tmc-etr.c | 248 +++++++++++++++++++++++-
>  drivers/hwtracing/coresight/coresight-tmc.h     |   2 +
>  2 files changed, 248 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index bc0c932..6280790 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -10,6 +10,7 @@
>  #include <linux/slab.h>
>  #include <linux/vmalloc.h>
>  #include "coresight-catu.h"
> +#include "coresight-etm-perf.h"
>  #include "coresight-priv.h"
>  #include "coresight-tmc.h"
>  
> @@ -21,6 +22,28 @@ struct etr_flat_buf {
>  };
>  
>  /*
> + * etr_perf_buffer - Perf buffer used for ETR
> + * @etr_buf		- Actual buffer used by the ETR
> + * @snaphost		- Perf session mode
> + * @head		- handle->head at the beginning of the session.
> + * @nr_pages		- Number of pages in the ring buffer.
> + * @pages		- Array of Pages in the ring buffer.
> + */
> +struct etr_perf_buffer {
> +	struct etr_buf		*etr_buf;
> +	bool			snapshot;
> +	unsigned long		head;
> +	int			nr_pages;
> +	void			**pages;
> +};
> +
> +/* Convert the perf index to an offset within the ETR buffer */
> +#define PERF_IDX2OFF(idx, buf)	((idx) % ((buf)->nr_pages << PAGE_SHIFT))
> +
> +/* Lower limit for ETR hardware buffer */
> +#define TMC_ETR_PERF_MIN_BUF_SIZE	SZ_1M
> +
> +/*
>   * The TMC ETR SG has a page size of 4K. The SG table contains pointers
>   * to 4KB buffers. However, the OS may use a PAGE_SIZE different from
>   * 4K (i.e, 16KB or 64KB). This implies that a single OS page could
> @@ -1103,10 +1126,228 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
>  	return ret;
>  }
>  
> +/*
> + * tmc_etr_setup_perf_buf: Allocate ETR buffer for use by perf.
> + * The size of the hardware buffer is dependent on the size configured
> + * via sysfs and the perf ring buffer size. We prefer to allocate the
> + * largest possible size, scaling down the size by half until it
> + * reaches a minimum limit (1M), beyond which we give up.
> + */
> +static struct etr_perf_buffer *
> +tmc_etr_setup_perf_buf(struct tmc_drvdata *drvdata, int node, int nr_pages,
> +		       void **pages, bool snapshot)
> +{
> +	struct etr_buf *etr_buf;
> +	struct etr_perf_buffer *etr_perf;
> +	unsigned long size;
> +
> +	etr_perf = kzalloc_node(sizeof(*etr_perf), GFP_KERNEL, node);
> +	if (!etr_perf)
> +		return ERR_PTR(-ENOMEM);
> +
> +	/*
> +	 * Try to match the perf ring buffer size if it is larger
> +	 * than the size requested via sysfs.
> +	 */
> +	if ((nr_pages << PAGE_SHIFT) > drvdata->size) {
> +		etr_buf = tmc_alloc_etr_buf(drvdata, (nr_pages << PAGE_SHIFT),
> +					    0, node, NULL);
> +		if (!IS_ERR(etr_buf))
> +			goto done;
> +	}
> +
> +	/*
> +	 * Else switch to configured size for this ETR
> +	 * and scale down until we hit the minimum limit.
> +	 */
> +	size = drvdata->size;
> +	do {
> +		etr_buf = tmc_alloc_etr_buf(drvdata, size, 0, node, NULL);
> +		if (!IS_ERR(etr_buf))
> +			goto done;
> +		size /= 2;
> +	} while (size >= TMC_ETR_PERF_MIN_BUF_SIZE);
> +
> +	kfree(etr_perf);
> +	return ERR_PTR(-ENOMEM);
> +
> +done:
> +	etr_perf->etr_buf = etr_buf;
> +	return etr_perf;
> +}
> +
> +
> +static void *tmc_etr_alloc_perf_buffer(struct coresight_device *csdev,
> +				       int cpu, void **pages, int nr_pages,
> +				       bool snapshot)
> +{
> +	struct etr_perf_buffer *etr_perf;
> +	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> +	if (cpu == -1)
> +		cpu = smp_processor_id();
> +
> +	etr_perf = tmc_etr_setup_perf_buf(drvdata, cpu_to_node(cpu),
> +					  nr_pages, pages, snapshot);
> +	if (IS_ERR(etr_perf)) {
> +		dev_dbg(drvdata->dev, "Unable to allocate ETR buffer\n");
> +		return NULL;
> +	}
> +
> +	etr_perf->snapshot = snapshot;
> +	etr_perf->nr_pages = nr_pages;
> +	etr_perf->pages = pages;
> +
> +	return etr_perf;
> +}
> +
> +static void tmc_etr_free_perf_buffer(void *config)
> +{
> +	struct etr_perf_buffer *etr_perf = config;
> +
> +	if (etr_perf->etr_buf)
> +		tmc_free_etr_buf(etr_perf->etr_buf);
> +	kfree(etr_perf);
> +}
> +
> +/*
> + * tmc_etr_sync_perf_buffer: Copy the actual trace data from the hardware
> + * buffer to the perf ring buffer.
> + */
> +static void tmc_etr_sync_perf_buffer(struct etr_perf_buffer *etr_perf)
> +{
> +	long bytes, to_copy;
> +	long pg_idx, pg_offset, src_offset;
> +	unsigned long head = etr_perf->head;
> +	char **dst_pages, *src_buf;
> +	struct etr_buf *etr_buf = etr_perf->etr_buf;
> +
> +	head = etr_perf->head;
> +	pg_idx = head >> PAGE_SHIFT;
> +	pg_offset = head & (PAGE_SIZE - 1);
> +	dst_pages = (char **)etr_perf->pages;
> +	src_offset = etr_buf->offset;
> +	to_copy = etr_buf->len;
> +
> +	while (to_copy > 0) {
> +		/*
> +		 * In one iteration, we can copy minimum of :
> +		 *  1) what is available in the source buffer,
> +		 *  2) what is available in the source buffer, before it
> +		 *     wraps around.
> +		 *  3) what is available in the destination page.
> +		 * in one iteration.
> +		 */
> +		bytes = tmc_etr_buf_get_data(etr_buf, src_offset, to_copy,
> +					     &src_buf);
> +		if (WARN_ON_ONCE(bytes <= 0))
> +			break;
> +		bytes = min(bytes, (long)(PAGE_SIZE - pg_offset));
> +
> +		memcpy(dst_pages[pg_idx] + pg_offset, src_buf, bytes);
> +
> +		to_copy -= bytes;
> +
> +		/* Move destination pointers */
> +		pg_offset += bytes;
> +		if (pg_offset == PAGE_SIZE) {
> +			pg_offset = 0;
> +			if (++pg_idx == etr_perf->nr_pages)
> +				pg_idx = 0;
> +		}
> +
> +		/* Move source pointers */
> +		src_offset += bytes;
> +		if (src_offset >= etr_buf->size)
> +			src_offset -= etr_buf->size;
> +	}
> +}
> +
> +/*
> + * tmc_etr_update_perf_buffer : Update the perf ring buffer with the
> + * available trace data. We use software double buffering at the moment.
> + *
> + * TODO: Add support for reusing the perf ring buffer.
> + */
> +static unsigned long
> +tmc_etr_update_perf_buffer(struct coresight_device *csdev,
> +			   struct perf_output_handle *handle,
> +			   void *config)
> +{
> +	bool lost = false;
> +	unsigned long flags, size = 0;
> +	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +	struct etr_perf_buffer *etr_perf = config;
> +	struct etr_buf *etr_buf = etr_perf->etr_buf;
> +
> +	spin_lock_irqsave(&drvdata->spinlock, flags);
> +	if (WARN_ON(drvdata->perf_data != etr_perf)) {
> +		lost = true;
> +		spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +		goto out;
> +	}
> +
> +	CS_UNLOCK(drvdata->base);
> +
> +	tmc_flush_and_stop(drvdata);
> +	tmc_sync_etr_buf(drvdata);
> +
> +	CS_UNLOCK(drvdata->base);

This is a copy/paste oversight, here you want to lock again.

> +	/* Reset perf specific data */
> +	drvdata->perf_data = NULL;
> +	spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +
> +	size = etr_buf->len;
> +	tmc_etr_sync_perf_buffer(etr_perf);
> +
> +	/*
> +	 * Update handle->head in snapshot mode. Also update the size to the
> +	 * hardware buffer size if there was an overflow.
> +	 */
> +	if (etr_perf->snapshot) {
> +		handle->head += size;
> +		if (etr_buf->full)
> +			size = etr_buf->size;
> +	}
> +
> +	lost |= etr_buf->full;
> +out:
> +	if (lost)
> +		perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> +	return size;
> +}
> +
>  static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
>  {
> -	/* We don't support perf mode yet ! */
> -	return -EINVAL;
> +	int rc = 0;
> +	unsigned long flags;
> +	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +	struct perf_output_handle *handle = data;
> +	struct etr_perf_buffer *etr_perf = etm_perf_sink_config(handle);
> +
> +	spin_lock_irqsave(&drvdata->spinlock, flags);
> +	/*
> +	 * There can be only one writer per sink in perf mode. If the sink
> +	 * is already open in SYSFS mode, we can't use it.
> +	 */
> +	if (drvdata->mode != CS_MODE_DISABLED || drvdata->perf_data) {
> +		rc = -EBUSY;
> +		goto unlock_out;
> +	}
> +
> +	if (WARN_ON(!etr_perf || !etr_perf->etr_buf)) {
> +		rc = -EINVAL;
> +		goto unlock_out;
> +	}
> +
> +	etr_perf->head = PERF_IDX2OFF(handle->head, etr_perf);
> +	drvdata->perf_data = etr_perf;
> +	drvdata->mode = CS_MODE_PERF;
> +	tmc_etr_enable_hw(drvdata, etr_perf->etr_buf);
> +
> +unlock_out:
> +	spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +	return rc;
>  }
>  
>  static int tmc_enable_etr_sink(struct coresight_device *csdev,
> @@ -1148,6 +1389,9 @@ static void tmc_disable_etr_sink(struct coresight_device *csdev)
>  static const struct coresight_ops_sink tmc_etr_sink_ops = {
>  	.enable		= tmc_enable_etr_sink,
>  	.disable	= tmc_disable_etr_sink,
> +	.alloc_buffer	= tmc_etr_alloc_perf_buffer,
> +	.update_buffer	= tmc_etr_update_perf_buffer,
> +	.free_buffer	= tmc_etr_free_perf_buffer,


	.alloc_buffer	= tmc_alloc_etr_buffer,
	.update_buffer	= tmc_update_etr_buffer,
	.free_buffer	= tmc_free_etr_buffer,

To be consistant with .enable and .disable along with the naming convention on
the ETF side.

>  };
>  
>  const struct coresight_ops tmc_etr_cs_ops = {
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
> index 872f63e..487c537 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.h
> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
> @@ -170,6 +170,7 @@ struct etr_buf {
>   * @trigger_cntr: amount of words to store after a trigger.
>   * @etr_caps:	Bitmask of capabilities of the TMC ETR, inferred from the
>   *		device configuration register (DEVID)
> + * @perf_data:	PERF buffer for ETR.
>   * @sysfs_data:	SYSFS buffer for ETR.
>   */
>  struct tmc_drvdata {
> @@ -191,6 +192,7 @@ struct tmc_drvdata {
>  	u32			trigger_cntr;
>  	u32			etr_caps;
>  	struct etr_buf		*sysfs_buf;
> +	void			*perf_data;
>  };
>  
>  struct etr_buf_operations {
> -- 
> 2.7.4
> 

Re: [PATCH v2 08/10] coresight: perf: Add helper to retrieve sink configuration

[Mathieu Poirier]

On Tue, Jul 17, 2018 at 06:11:39PM +0100, Suzuki K Poulose wrote:
> We can always find the sink configuration for a given perf_output_handle.
> Add a helper to retrieve the sink configuration for a given
> perf_output_handle. This will be used to get rid of the set_buffer()
> call back.
> 
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight-etm-perf.c | 14 -------------
>  drivers/hwtracing/coresight/coresight-etm-perf.h | 26 ++++++++++++++++++++++++
>  2 files changed, 26 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index 6a4252b..3cc4a0b 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -23,20 +23,6 @@
>  static struct pmu etm_pmu;
>  static bool etm_perf_up;
>  
> -/**
> - * struct etm_event_data - Coresight specifics associated to an event
> - * @work:		Handle to free allocated memory outside IRQ context.
> - * @mask:		Hold the CPU(s) this event was set for.
> - * @snk_config:		The sink configuration.
> - * @path:		An array of path, each slot for one CPU.
> - */
> -struct etm_event_data {
> -	struct work_struct work;
> -	cpumask_t mask;
> -	void *snk_config;
> -	struct list_head * __percpu *path;
> -};
> -

If this is moved to coresight-etm-perf.h, the #include <linux/percpu-defs.h> can
be removed.

>  static DEFINE_PER_CPU(struct perf_output_handle, ctx_handle);
>  static DEFINE_PER_CPU(struct coresight_device *, csdev_src);
>  
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h
> index 4197df4..da7d933 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.h
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.h
> @@ -7,6 +7,7 @@
>  #ifndef _CORESIGHT_ETM_PERF_H
>  #define _CORESIGHT_ETM_PERF_H
>  
> +#include <linux/percpu-defs.h>
>  #include "coresight-priv.h"
>  
>  struct coresight_device;
> @@ -42,14 +43,39 @@ struct etm_filters {
>  	bool			ssstatus;
>  };
>  
> +/**
> + * struct etm_event_data - Coresight specifics associated to an event
> + * @work:		Handle to free allocated memory outside IRQ context.
> + * @mask:		Hold the CPU(s) this event was set for.
> + * @snk_config:		The sink configuration.
> + * @path:		An array of path, each slot for one CPU.
> + */
> +struct etm_event_data {
> +	struct work_struct work;
> +	cpumask_t mask;
> +	void *snk_config;
> +	struct list_head * __percpu *path;
> +};
>  
>  #ifdef CONFIG_CORESIGHT
>  int etm_perf_symlink(struct coresight_device *csdev, bool link);
> +static inline void *etm_perf_sink_config(struct perf_output_handle *handle)
> +{
> +	struct etm_event_data *data = perf_get_aux(handle);
>  
> +	if (data)
> +		return data->snk_config;
> +	return NULL;
> +}
>  #else
>  static inline int etm_perf_symlink(struct coresight_device *csdev, bool link)
>  { return -EINVAL; }
>  
> +static inline void *etm_perf_sink_config(struct perf_output_handle *handle)
> +{
> +	return NULL;
> +}
> +

I think we can do without those... See my comment in the next patch.

Thanks,
Mathieu

>  #endif /* CONFIG_CORESIGHT */
>  
>  #endif
> -- 
> 2.7.4
> 

Re: [PATCH v2 09/10] coresight: perf: Remove set_buffer call back

[Mathieu Poirier]

On Tue, Jul 17, 2018 at 06:11:40PM +0100, Suzuki K Poulose wrote:
> In coresight perf mode, we need to prepare the sink before
> starting a session, which is done via set_buffer call back.
> We then proceed to enable the tracing. If we fail to start
> the session successfully, we leave the sink configuration
> unchanged. This was fine for the existing backends as they
> don't have any state associated with the buffers. But with
> ETR, we need to keep track of the buffer details and need
> to be cleaned up if we fail. In order to make the operation
> atomic and to avoid yet another call back, we get rid of
> the "set_buffer" call back and pass the buffer details
> via enable() call back to the sink.

Suzuki,

I'm not sure I understand the problem you're trying to fix there.  From the
implementation of tmc_enable_etr_sink_perf() in the next patch, wouldn't the
same result been achievable using a callback?

I'm fine with this patch and supportive of getting rid of callbacks if we can, I
just need to understand the exact problem you're after.  From looking a your
code (and the current implementation), if we succeed in setting the memory for
the sink but fail in any of the subsequent steps i.e, enabling the rest of the
compoment on the path or the source, the sink is left unchanged.


> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight-etb10.c    | 24 ++++++++++++++------
>  drivers/hwtracing/coresight/coresight-etm-perf.c |  9 ++------
>  drivers/hwtracing/coresight/coresight-priv.h     |  2 +-
>  drivers/hwtracing/coresight/coresight-tmc-etf.c  | 28 ++++++++++++++++--------
>  drivers/hwtracing/coresight/coresight-tmc-etr.c  |  7 +++---
>  drivers/hwtracing/coresight/coresight-tpiu.c     |  2 +-
>  drivers/hwtracing/coresight/coresight.c          | 11 +++++-----
>  include/linux/coresight.h                        |  6 +----
>  8 files changed, 51 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
> index a729a7b..4e829db 100644
> --- a/drivers/hwtracing/coresight/coresight-etb10.c
> +++ b/drivers/hwtracing/coresight/coresight-etb10.c
> @@ -28,6 +28,7 @@
>  
>  
>  #include "coresight-priv.h"
> +#include "coresight-etm-perf.h"
>  
>  #define ETB_RAM_DEPTH_REG	0x004
>  #define ETB_STATUS_REG		0x00c
> @@ -90,6 +91,9 @@ struct etb_drvdata {
>  	u32			trigger_cntr;
>  };
>  
> +static int etb_set_buffer(struct coresight_device *csdev,
> +			  struct perf_output_handle *handle);
> +
>  static unsigned int etb_get_buffer_depth(struct etb_drvdata *drvdata)
>  {
>  	u32 depth = 0;
> @@ -131,12 +135,19 @@ static void etb_enable_hw(struct etb_drvdata *drvdata)
>  	CS_LOCK(drvdata->base);
>  }
>  
> -static int etb_enable(struct coresight_device *csdev, u32 mode)
> +static int etb_enable(struct coresight_device *csdev, u32 mode, void *data)
>  {
> +	int ret = 0;
>  	u32 val;
>  	unsigned long flags;
>  	struct etb_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>  
> +	if (mode == CS_MODE_PERF) {
> +		ret = etb_set_buffer(csdev, (struct perf_output_handle *)data);
> +		if (ret)
> +			goto out;
> +	}
> +
>  	val = local_cmpxchg(&drvdata->mode,
>  			    CS_MODE_DISABLED, mode);
>  	/*
> @@ -156,8 +167,9 @@ static int etb_enable(struct coresight_device *csdev, u32 mode)
>  	spin_unlock_irqrestore(&drvdata->spinlock, flags);
>  
>  out:
> -	dev_dbg(drvdata->dev, "ETB enabled\n");
> -	return 0;
> +	if (!ret)
> +		dev_dbg(drvdata->dev, "ETB enabled\n");
> +	return ret;
>  }
>  
>  static void etb_disable_hw(struct etb_drvdata *drvdata)
> @@ -294,12 +306,11 @@ static void etb_free_buffer(void *config)
>  }
>  
>  static int etb_set_buffer(struct coresight_device *csdev,
> -			  struct perf_output_handle *handle,
> -			  void *sink_config)
> +			  struct perf_output_handle *handle)
>  {
>  	int ret = 0;
>  	unsigned long head;
> -	struct cs_buffers *buf = sink_config;
> +	struct cs_buffers *buf = etm_perf_sink_config(handle);
>  
>  	/* wrap head around to the amount of space we have */
>  	head = handle->head & ((buf->nr_pages << PAGE_SHIFT) - 1);
> @@ -453,7 +464,6 @@ static const struct coresight_ops_sink etb_sink_ops = {
>  	.disable	= etb_disable,
>  	.alloc_buffer	= etb_alloc_buffer,
>  	.free_buffer	= etb_free_buffer,
> -	.set_buffer	= etb_set_buffer,
>  	.update_buffer	= etb_update_buffer,
>  };
>  
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index 3cc4a0b..12a247d 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -269,16 +269,11 @@ static void etm_event_start(struct perf_event *event, int flags)
>  	path = etm_event_cpu_path(event_data, cpu);
>  	/* We need a sink, no need to continue without one */
>  	sink = coresight_get_sink(path);
> -	if (WARN_ON_ONCE(!sink || !sink_ops(sink)->set_buffer))
> -		goto fail_end_stop;
> -
> -	/* Configure the sink */
> -	if (sink_ops(sink)->set_buffer(sink, handle,
> -				       event_data->snk_config))
> +	if (WARN_ON_ONCE(!sink))
>  		goto fail_end_stop;
>  
>  	/* Nothing will happen without a path */
> -	if (coresight_enable_path(path, CS_MODE_PERF))
> +	if (coresight_enable_path(path, CS_MODE_PERF, handle))

Here we already have a handle on "event_data".  As such I think this is what we
should feed to coresight_enable_path() rather than the handle.  That way we
don't need to call etm_perf_sink_config(), we just use the data.

Thanks,
Mathieu 

>  		goto fail_end_stop;
>  
>  	/* Tell the perf core the event is alive */
> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> index 1a6cf35..c11da55 100644
> --- a/drivers/hwtracing/coresight/coresight-priv.h
> +++ b/drivers/hwtracing/coresight/coresight-priv.h
> @@ -137,7 +137,7 @@ static inline void coresight_write_reg_pair(void __iomem *addr, u64 val,
>  }
>  
>  void coresight_disable_path(struct list_head *path);
> -int coresight_enable_path(struct list_head *path, u32 mode);
> +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 list_head *coresight_build_path(struct coresight_device *csdev,
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> index 31a98f9..4156c95 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> @@ -10,6 +10,10 @@
>  #include <linux/slab.h>
>  #include "coresight-priv.h"
>  #include "coresight-tmc.h"
> +#include "coresight-etm-perf.h"
> +
> +static int tmc_set_etf_buffer(struct coresight_device *csdev,
> +			      struct perf_output_handle *handle);
>  
>  static void tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
>  {
> @@ -182,11 +186,12 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev)
>  	return ret;
>  }
>  
> -static int tmc_enable_etf_sink_perf(struct coresight_device *csdev)
> +static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, void *data)
>  {
>  	int ret = 0;
>  	unsigned long flags;
>  	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +	struct perf_output_handle *handle = data;
>  
>  	spin_lock_irqsave(&drvdata->spinlock, flags);
>  	if (drvdata->reading) {
> @@ -204,15 +209,19 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev)
>  		goto out;
>  	}
>  
> -	drvdata->mode = CS_MODE_PERF;
> -	tmc_etb_enable_hw(drvdata);
> +	ret = tmc_set_etf_buffer(csdev, handle);
> +	if (!ret) {
> +		drvdata->mode = CS_MODE_PERF;
> +		tmc_etb_enable_hw(drvdata);
> +	}
>  out:
>  	spin_unlock_irqrestore(&drvdata->spinlock, flags);
>  
>  	return ret;
>  }
>  
> -static int tmc_enable_etf_sink(struct coresight_device *csdev, u32 mode)
> +static int tmc_enable_etf_sink(struct coresight_device *csdev,
> +			       u32 mode, void *data)
>  {
>  	int ret;
>  	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> @@ -222,7 +231,7 @@ static int tmc_enable_etf_sink(struct coresight_device *csdev, u32 mode)
>  		ret = tmc_enable_etf_sink_sysfs(csdev);
>  		break;
>  	case CS_MODE_PERF:
> -		ret = tmc_enable_etf_sink_perf(csdev);
> +		ret = tmc_enable_etf_sink_perf(csdev, data);
>  		break;
>  	/* We shouldn't be here */
>  	default:
> @@ -328,12 +337,14 @@ static void tmc_free_etf_buffer(void *config)
>  }
>  
>  static int tmc_set_etf_buffer(struct coresight_device *csdev,
> -			      struct perf_output_handle *handle,
> -			      void *sink_config)
> +			      struct perf_output_handle *handle)
>  {
>  	int ret = 0;
>  	unsigned long head;
> -	struct cs_buffers *buf = sink_config;
> +	struct cs_buffers *buf = etm_perf_sink_config(handle);
> +
> +	if (!buf)
> +		return -EINVAL;
>  
>  	/* wrap head around to the amount of space we have */
>  	head = handle->head & ((buf->nr_pages << PAGE_SHIFT) - 1);
> @@ -472,7 +483,6 @@ static const struct coresight_ops_sink tmc_etf_sink_ops = {
>  	.disable	= tmc_disable_etf_sink,
>  	.alloc_buffer	= tmc_alloc_etf_buffer,
>  	.free_buffer	= tmc_free_etf_buffer,
> -	.set_buffer	= tmc_set_etf_buffer,
>  	.update_buffer	= tmc_update_etf_buffer,
>  };
>  
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index 85a3f59..bc0c932 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -1103,19 +1103,20 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
>  	return ret;
>  }
>  
> -static int tmc_enable_etr_sink_perf(struct coresight_device *csdev)
> +static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
>  {
>  	/* We don't support perf mode yet ! */
>  	return -EINVAL;
>  }
>  
> -static int tmc_enable_etr_sink(struct coresight_device *csdev, u32 mode)
> +static int tmc_enable_etr_sink(struct coresight_device *csdev,
> +			       u32 mode, void *data)
>  {
>  	switch (mode) {
>  	case CS_MODE_SYSFS:
>  		return tmc_enable_etr_sink_sysfs(csdev);
>  	case CS_MODE_PERF:
> -		return tmc_enable_etr_sink_perf(csdev);
> +		return tmc_enable_etr_sink_perf(csdev, data);
>  	}
>  
>  	/* We shouldn't be here */
> diff --git a/drivers/hwtracing/coresight/coresight-tpiu.c b/drivers/hwtracing/coresight/coresight-tpiu.c
> index 7daeb084..d7dd5e4 100644
> --- a/drivers/hwtracing/coresight/coresight-tpiu.c
> +++ b/drivers/hwtracing/coresight/coresight-tpiu.c
> @@ -67,7 +67,7 @@ static void tpiu_enable_hw(struct tpiu_drvdata *drvdata)
>  	CS_LOCK(drvdata->base);
>  }
>  
> -static int tpiu_enable(struct coresight_device *csdev, u32 mode)
> +static int tpiu_enable(struct coresight_device *csdev, u32 mode, void *__unused)
>  {
>  	struct tpiu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>  
> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> index c0dabbd..fab87c9 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight.c
> @@ -128,7 +128,8 @@ static int coresight_find_link_outport(struct coresight_device *csdev,
>  	return -ENODEV;
>  }
>  
> -static int coresight_enable_sink(struct coresight_device *csdev, u32 mode)
> +static int coresight_enable_sink(struct coresight_device *csdev,
> +				 u32 mode, void *data)
>  {
>  	int ret;
>  
> @@ -137,7 +138,7 @@ static int coresight_enable_sink(struct coresight_device *csdev, u32 mode)
>  	 * existing "mode" of operation.
>  	 */
>  	if (sink_ops(csdev)->enable) {
> -		ret = sink_ops(csdev)->enable(csdev, mode);
> +		ret = sink_ops(csdev)->enable(csdev, mode, data);
>  		if (ret)
>  			return ret;
>  		csdev->enable = true;
> @@ -315,7 +316,7 @@ void coresight_disable_path(struct list_head *path)
>  	}
>  }
>  
> -int coresight_enable_path(struct list_head *path, u32 mode)
> +int coresight_enable_path(struct list_head *path, u32 mode, void *sink_data)
>  {
>  
>  	int ret = 0;
> @@ -340,7 +341,7 @@ int coresight_enable_path(struct list_head *path, u32 mode)
>  
>  		switch (type) {
>  		case CORESIGHT_DEV_TYPE_SINK:
> -			ret = coresight_enable_sink(csdev, mode);
> +			ret = coresight_enable_sink(csdev, mode, sink_data);
>  			/*
>  			 * Sink is the first component turned on. If we
>  			 * failed to enable the sink, there are no components
> @@ -643,7 +644,7 @@ int coresight_enable(struct coresight_device *csdev)
>  		goto out;
>  	}
>  
> -	ret = coresight_enable_path(path, CS_MODE_SYSFS);
> +	ret = coresight_enable_path(path, CS_MODE_SYSFS, NULL);
>  	if (ret)
>  		goto err_path;
>  
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index eafe4d1..3434758 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -190,18 +190,14 @@ struct coresight_device {
>   * @disable:		disables the sink.
>   * @alloc_buffer:	initialises perf's ring buffer for trace collection.
>   * @free_buffer:	release memory allocated in @get_config.
> - * @set_buffer:		initialises buffer mechanic before a trace session.
>   * @update_buffer:	update buffer pointers after a trace session.
>   */
>  struct coresight_ops_sink {
> -	int (*enable)(struct coresight_device *csdev, u32 mode);
> +	int (*enable)(struct coresight_device *csdev, u32 mode, void *data);
>  	void (*disable)(struct coresight_device *csdev);
>  	void *(*alloc_buffer)(struct coresight_device *csdev, int cpu,
>  			      void **pages, int nr_pages, bool overwrite);
>  	void (*free_buffer)(void *config);
> -	int (*set_buffer)(struct coresight_device *csdev,
> -			  struct perf_output_handle *handle,
> -			  void *sink_config);
>  	unsigned long (*update_buffer)(struct coresight_device *csdev,
>  			      struct perf_output_handle *handle,
>  			      void *sink_config);
> -- 
> 2.7.4
> 

Re: [PATCH v2 08/10] coresight: perf: Add helper to retrieve sink configuration

[Suzuki K Poulose]

On 19/07/18 21:07, Mathieu Poirier wrote:
> On Tue, Jul 17, 2018 at 06:11:39PM +0100, Suzuki K Poulose wrote:
>> We can always find the sink configuration for a given perf_output_handle.
>> Add a helper to retrieve the sink configuration for a given
>> perf_output_handle. This will be used to get rid of the set_buffer()
>> call back.
>>
>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-etm-perf.c | 14 -------------
>>   drivers/hwtracing/coresight/coresight-etm-perf.h | 26 ++++++++++++++++++++++++
>>   2 files changed, 26 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
>> index 6a4252b..3cc4a0b 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
>> @@ -23,20 +23,6 @@
>>   static struct pmu etm_pmu;
>>   static bool etm_perf_up;
>>   
>> -/**
>> - * struct etm_event_data - Coresight specifics associated to an event
>> - * @work:		Handle to free allocated memory outside IRQ context.
>> - * @mask:		Hold the CPU(s) this event was set for.
>> - * @snk_config:		The sink configuration.
>> - * @path:		An array of path, each slot for one CPU.
>> - */
>> -struct etm_event_data {
>> -	struct work_struct work;
>> -	cpumask_t mask;
>> -	void *snk_config;
>> -	struct list_head * __percpu *path;
>> -};
>> -
> 
> If this is moved to coresight-etm-perf.h, the #include <linux/percpu-defs.h> can
> be removed.

Actually, we do have the PER_CPU variables in the file already, which is why I left
them there.  See the next line.
> 
>>   static DEFINE_PER_CPU(struct perf_output_handle, ctx_handle);
>>   static DEFINE_PER_CPU(struct coresight_device *, csdev_src);
>>   


>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h
>> index 4197df4..da7d933 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.h
>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.h
>> @@ -7,6 +7,7 @@
>>   #ifndef _CORESIGHT_ETM_PERF_H
>>   #define _CORESIGHT_ETM_PERF_H
>>   
>> +#include <linux/percpu-defs.h>
>>   #include "coresight-priv.h"
>>   
>>   struct coresight_device;
>> @@ -42,14 +43,39 @@ struct etm_filters {
>>   	bool			ssstatus;
>>   };
>>   
>> +/**
>> + * struct etm_event_data - Coresight specifics associated to an event
>> + * @work:		Handle to free allocated memory outside IRQ context.
>> + * @mask:		Hold the CPU(s) this event was set for.
>> + * @snk_config:		The sink configuration.
>> + * @path:		An array of path, each slot for one CPU.
>> + */
>> +struct etm_event_data {
>> +	struct work_struct work;
>> +	cpumask_t mask;
>> +	void *snk_config;
>> +	struct list_head * __percpu *path;
>> +};
>>   
>>   #ifdef CONFIG_CORESIGHT
>>   int etm_perf_symlink(struct coresight_device *csdev, bool link);
>> +static inline void *etm_perf_sink_config(struct perf_output_handle *handle)
>> +{
>> +	struct etm_event_data *data = perf_get_aux(handle);
>>   
>> +	if (data)
>> +		return data->snk_config;
>> +	return NULL;
>> +}
>>   #else
>>   static inline int etm_perf_symlink(struct coresight_device *csdev, bool link)
>>   { return -EINVAL; }
>>   
>> +static inline void *etm_perf_sink_config(struct perf_output_handle *handle)
>> +{
>> +	return NULL;
>> +}
>> +
> 
> I think we can do without those... See my comment in the next patch.

Sure

Re: [PATCH v2 09/10] coresight: perf: Remove set_buffer call back

[Suzuki K Poulose]

Mathieu,

On 19/07/18 21:36, Mathieu Poirier wrote:
> On Tue, Jul 17, 2018 at 06:11:40PM +0100, Suzuki K Poulose wrote:
>> In coresight perf mode, we need to prepare the sink before
>> starting a session, which is done via set_buffer call back.
>> We then proceed to enable the tracing. If we fail to start
>> the session successfully, we leave the sink configuration
>> unchanged. This was fine for the existing backends as they
>> don't have any state associated with the buffers. But with
>> ETR, we need to keep track of the buffer details and need
>> to be cleaned up if we fail. In order to make the operation
>> atomic and to avoid yet another call back, we get rid of
>> the "set_buffer" call back and pass the buffer details
>> via enable() call back to the sink.
> 
> Suzuki,
> 
> I'm not sure I understand the problem you're trying to fix there.  From the
> implementation of tmc_enable_etr_sink_perf() in the next patch, wouldn't the
> same result been achievable using a callback?

We can definitely achieve the results using "set_buffer". But for ETR,
we track the "perf_buf" in drvdata->perf_data when we do "set_buffer".
But if we failed to enable_path(), we leave the drvdata->perf_data
and doesn't clean it up. Now when another session is about to set_buf,
we check if perf_data is empty and WARNs otherwise.
Because we can't be sure if it belongs to an abandoned session or
another active session and we completely messed somewhere in the driver.
So, we need a clear_buffer call back if the enable fails, something
not really worth. Anyways, there is no point in separating set_buffer
and enabling the sink, as the error handling becomes cumbersome as explained
above.

> 
> I'm fine with this patch and supportive of getting rid of callbacks if we can, I
> just need to understand the exact problem you're after.  From looking a your
> code (and the current implementation), if we succeed in setting the memory for
> the sink but fail in any of the subsequent steps i.e, enabling the rest of the
> compoment on the path or the source, the sink is left unchanged.

Yes, thats right. And we should WARN (which I missed in this version) if
there is a perf_data already for a disabled ETR. Please see my response to the
next patch.

>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
>> index 3cc4a0b..12a247d 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
>> @@ -269,16 +269,11 @@ static void etm_event_start(struct perf_event *event, int flags)
>>   	path = etm_event_cpu_path(event_data, cpu);
>>   	/* We need a sink, no need to continue without one */
>>   	sink = coresight_get_sink(path);
>> -	if (WARN_ON_ONCE(!sink || !sink_ops(sink)->set_buffer))
>> -		goto fail_end_stop;
>> -
>> -	/* Configure the sink */
>> -	if (sink_ops(sink)->set_buffer(sink, handle,
>> -				       event_data->snk_config))
>> +	if (WARN_ON_ONCE(!sink))
>>   		goto fail_end_stop;
>>   
>>   	/* Nothing will happen without a path */
>> -	if (coresight_enable_path(path, CS_MODE_PERF))
>> +	if (coresight_enable_path(path, CS_MODE_PERF, handle))
> 
> Here we already have a handle on "event_data".  As such I think this is what we
> should feed to coresight_enable_path() rather than the handle.  That way we
> don't need to call etm_perf_sink_config(), we just use the data.

The advantage of passing on the handle is, we could get all the way upto the
"perf_event" for the given session. Passing the event_data will loose that
information.

i.e, perf_event-> |perf_ouput_handle | -> |event_data | -> sink_config
                   |  <-event         |    |           |

The purpose of the wrapper "etm_perf_sink_config()" is to abstract the way we
handle the information under the event_data. i.e, if we decide to make some
changes in the way we store event_data, we need to spill the changes every
where. But the perf_ouput_handle has much more stable ABI than event_data,
hence the choice of passing handle.

Cheers
Suzuki

Re: [PATCH v2 10/10] coresight: etm-perf: Add support for ETR backend

[Suzuki K Poulose]

On 19/07/18 20:59, Mathieu Poirier wrote:
> On Tue, Jul 17, 2018 at 06:11:41PM +0100, Suzuki K Poulose wrote:
>> Add support for using TMC-ETR as backend for ETM perf tracing.
>> We use software double buffering at the moment. i.e, the TMC-ETR
>> uses a separate buffer than the perf ring buffer. The data is
>> copied to the perf ring buffer once a session completes.
>>
>> The TMC-ETR would try to match the larger of perf ring buffer
>> or the ETR buffer size configured via sysfs, scaling down to
>> a minimum limit of 1MB.
>>
>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---

>> +	CS_UNLOCK(drvdata->base);
>> +
>> +	tmc_flush_and_stop(drvdata);
>> +	tmc_sync_etr_buf(drvdata);
>> +
>> +	CS_UNLOCK(drvdata->base);
> 
> This is a copy/paste oversight, here you want to lock again.

Thanks for catching that, will fix it.

> 
>> +	/* Reset perf specific data */
>> +	drvdata->perf_data = NULL;
>> +	spin_unlock_irqrestore(&drvdata->spinlock, flags);
>> +
>> +	size = etr_buf->len;
>> +	tmc_etr_sync_perf_buffer(etr_perf);
>> +
>> +	/*
>> +	 * Update handle->head in snapshot mode. Also update the size to the
>> +	 * hardware buffer size if there was an overflow.
>> +	 */
>> +	if (etr_perf->snapshot) {
>> +		handle->head += size;
>> +		if (etr_buf->full)
>> +			size = etr_buf->size;
>> +	}
>> +
>> +	lost |= etr_buf->full;
>> +out:
>> +	if (lost)
>> +		perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
>> +	return size;
>> +}
>> +
>>   static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
>>   {
>> -	/* We don't support perf mode yet ! */
>> -	return -EINVAL;
>> +	int rc = 0;
>> +	unsigned long flags;
>> +	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> +	struct perf_output_handle *handle = data;
>> +	struct etr_perf_buffer *etr_perf = etm_perf_sink_config(handle);
>> +
>> +	spin_lock_irqsave(&drvdata->spinlock, flags);
>> +	/*
>> +	 * There can be only one writer per sink in perf mode. If the sink
>> +	 * is already open in SYSFS mode, we can't use it.
>> +	 */
>> +	if (drvdata->mode != CS_MODE_DISABLED || drvdata->perf_data) {
>> +		rc = -EBUSY;

As discussed in the previous patch, I am missing a WARN_ON here : i.e,

    +	if (drvdata->mode != CS_MODE_DISABLED || WARN_ON(drvdata->perf_data))

which was there in the previous version in set_buffer, which triggered some
warnings for me with testing.

>> +		goto unlock_out;
>> +	}
>> +
>> +	if (WARN_ON(!etr_perf || !etr_perf->etr_buf)) {
>> +		rc = -EINVAL;
>> +		goto unlock_out;
>> +	}
>> +
>> +	etr_perf->head = PERF_IDX2OFF(handle->head, etr_perf);
>> +	drvdata->perf_data = etr_perf;
>> +	drvdata->mode = CS_MODE_PERF;
>> +	tmc_etr_enable_hw(drvdata, etr_perf->etr_buf);
>> +
>> +unlock_out:
>> +	spin_unlock_irqrestore(&drvdata->spinlock, flags);
>> +	return rc;
>>   }
>>   
>>   static int tmc_enable_etr_sink(struct coresight_device *csdev,
>> @@ -1148,6 +1389,9 @@ static void tmc_disable_etr_sink(struct coresight_device *csdev)
>>   static const struct coresight_ops_sink tmc_etr_sink_ops = {
>>   	.enable		= tmc_enable_etr_sink,
>>   	.disable	= tmc_disable_etr_sink,
>> +	.alloc_buffer	= tmc_etr_alloc_perf_buffer,
>> +	.update_buffer	= tmc_etr_update_perf_buffer,
>> +	.free_buffer	= tmc_etr_free_perf_buffer,
> 
> 
> 	.alloc_buffer	= tmc_alloc_etr_buffer,
> 	.update_buffer	= tmc_update_etr_buffer,
> 	.free_buffer	= tmc_free_etr_buffer,
> 
> To be consistant with .enable and .disable along with the naming convention on
> the ETF side.


Sure, I can change them.

Cheers
Suzuki

Re: [PATCH v2 08/10] coresight: perf: Add helper to retrieve sink configuration

[Mathieu Poirier]

On Fri, 20 Jul 2018 at 02:43, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
>
> On 19/07/18 21:07, Mathieu Poirier wrote:
> > On Tue, Jul 17, 2018 at 06:11:39PM +0100, Suzuki K Poulose wrote:
> >> We can always find the sink configuration for a given perf_output_handle.
> >> Add a helper to retrieve the sink configuration for a given
> >> perf_output_handle. This will be used to get rid of the set_buffer()
> >> call back.
> >>
> >> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> >> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> >> ---
> >>   drivers/hwtracing/coresight/coresight-etm-perf.c | 14 -------------
> >>   drivers/hwtracing/coresight/coresight-etm-perf.h | 26 ++++++++++++++++++++++++
> >>   2 files changed, 26 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> >> index 6a4252b..3cc4a0b 100644
> >> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> >> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> >> @@ -23,20 +23,6 @@
> >>   static struct pmu etm_pmu;
> >>   static bool etm_perf_up;
> >>
> >> -/**
> >> - * struct etm_event_data - Coresight specifics associated to an event
> >> - * @work:           Handle to free allocated memory outside IRQ context.
> >> - * @mask:           Hold the CPU(s) this event was set for.
> >> - * @snk_config:             The sink configuration.
> >> - * @path:           An array of path, each slot for one CPU.
> >> - */
> >> -struct etm_event_data {
> >> -    struct work_struct work;
> >> -    cpumask_t mask;
> >> -    void *snk_config;
> >> -    struct list_head * __percpu *path;
> >> -};
> >> -
> >
> > If this is moved to coresight-etm-perf.h, the #include <linux/percpu-defs.h> can
> > be removed.
>
> Actually, we do have the PER_CPU variables in the file already, which is why I left
> them there.  See the next line.

Of course...

> >
> >>   static DEFINE_PER_CPU(struct perf_output_handle, ctx_handle);
> >>   static DEFINE_PER_CPU(struct coresight_device *, csdev_src);
> >>
>
>
> >> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h
> >> index 4197df4..da7d933 100644
> >> --- a/drivers/hwtracing/coresight/coresight-etm-perf.h
> >> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.h
> >> @@ -7,6 +7,7 @@
> >>   #ifndef _CORESIGHT_ETM_PERF_H
> >>   #define _CORESIGHT_ETM_PERF_H
> >>
> >> +#include <linux/percpu-defs.h>
> >>   #include "coresight-priv.h"
> >>
> >>   struct coresight_device;
> >> @@ -42,14 +43,39 @@ struct etm_filters {
> >>      bool                    ssstatus;
> >>   };
> >>
> >> +/**
> >> + * struct etm_event_data - Coresight specifics associated to an event
> >> + * @work:           Handle to free allocated memory outside IRQ context.
> >> + * @mask:           Hold the CPU(s) this event was set for.
> >> + * @snk_config:             The sink configuration.
> >> + * @path:           An array of path, each slot for one CPU.
> >> + */
> >> +struct etm_event_data {
> >> +    struct work_struct work;
> >> +    cpumask_t mask;
> >> +    void *snk_config;
> >> +    struct list_head * __percpu *path;
> >> +};
> >>
> >>   #ifdef CONFIG_CORESIGHT
> >>   int etm_perf_symlink(struct coresight_device *csdev, bool link);
> >> +static inline void *etm_perf_sink_config(struct perf_output_handle *handle)
> >> +{
> >> +    struct etm_event_data *data = perf_get_aux(handle);
> >>
> >> +    if (data)
> >> +            return data->snk_config;
> >> +    return NULL;
> >> +}
> >>   #else
> >>   static inline int etm_perf_symlink(struct coresight_device *csdev, bool link)
> >>   { return -EINVAL; }
> >>
> >> +static inline void *etm_perf_sink_config(struct perf_output_handle *handle)
> >> +{
> >> +    return NULL;
> >> +}
> >> +
> >
> > I think we can do without those... See my comment in the next patch.
>
> Sure

Re: [PATCH v2 09/10] coresight: perf: Remove set_buffer call back

[Mathieu Poirier]

On Fri, 20 Jul 2018 at 03:04, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
>
> Mathieu,
>
> On 19/07/18 21:36, Mathieu Poirier wrote:
> > On Tue, Jul 17, 2018 at 06:11:40PM +0100, Suzuki K Poulose wrote:
> >> In coresight perf mode, we need to prepare the sink before
> >> starting a session, which is done via set_buffer call back.
> >> We then proceed to enable the tracing. If we fail to start
> >> the session successfully, we leave the sink configuration
> >> unchanged. This was fine for the existing backends as they
> >> don't have any state associated with the buffers. But with
> >> ETR, we need to keep track of the buffer details and need
> >> to be cleaned up if we fail. In order to make the operation
> >> atomic and to avoid yet another call back, we get rid of
> >> the "set_buffer" call back and pass the buffer details
> >> via enable() call back to the sink.
> >
> > Suzuki,
> >
> > I'm not sure I understand the problem you're trying to fix there.  From the
> > implementation of tmc_enable_etr_sink_perf() in the next patch, wouldn't the
> > same result been achievable using a callback?
>
> We can definitely achieve the results using "set_buffer". But for ETR,
> we track the "perf_buf" in drvdata->perf_data when we do "set_buffer".
> But if we failed to enable_path(), we leave the drvdata->perf_data
> and doesn't clean it up. Now when another session is about to set_buf,
> we check if perf_data is empty and WARNs otherwise.
> Because we can't be sure if it belongs to an abandoned session or
> another active session and we completely messed somewhere in the driver.
> So, we need a clear_buffer call back if the enable fails, something
> not really worth. Anyways, there is no point in separating set_buffer
> and enabling the sink, as the error handling becomes cumbersome as explained
> above.
>
> >
> > I'm fine with this patch and supportive of getting rid of callbacks if we can, I
> > just need to understand the exact problem you're after.  From looking a your
> > code (and the current implementation), if we succeed in setting the memory for
> > the sink but fail in any of the subsequent steps i.e, enabling the rest of the
> > compoment on the path or the source, the sink is left unchanged.
>
> Yes, thats right. And we should WARN (which I missed in this version) if
> there is a perf_data already for a disabled ETR. Please see my response to the
> next patch.

The changelog for this patch states the following: "But with ETR, we
need to keep track of the buffer details and need to be cleaned up if
we fail."

I did a deep dive in the code and in the current implementation if the
source fails to be enabled in etm_event_start() the path and the sink
remains unchanged.  With your patchset this get fixed because a goto
was added to disable the path when such condition occur.  As such each
component in the path will see its ->disable() callback invoked.  In
tmc_disable_etr_sink(), drvdata->perf_data is set to NULL in
tmc_etr_disable_hw(), so the cleanup on error condition is done
properly.  As such we wouldn't need a clean_buffer() callback.

As I said I'm in favour of removing the set_buffer() callback but I
wouldn't associated it with ETR state cleanup.  If the code can be
rearranged in a way that code can be removed then that alone is enough
to justify the change.

>
> >> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> >> index 3cc4a0b..12a247d 100644
> >> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> >> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> >> @@ -269,16 +269,11 @@ static void etm_event_start(struct perf_event *event, int flags)
> >>      path = etm_event_cpu_path(event_data, cpu);
> >>      /* We need a sink, no need to continue without one */
> >>      sink = coresight_get_sink(path);
> >> -    if (WARN_ON_ONCE(!sink || !sink_ops(sink)->set_buffer))
> >> -            goto fail_end_stop;
> >> -
> >> -    /* Configure the sink */
> >> -    if (sink_ops(sink)->set_buffer(sink, handle,
> >> -                                   event_data->snk_config))
> >> +    if (WARN_ON_ONCE(!sink))
> >>              goto fail_end_stop;
> >>
> >>      /* Nothing will happen without a path */
> >> -    if (coresight_enable_path(path, CS_MODE_PERF))
> >> +    if (coresight_enable_path(path, CS_MODE_PERF, handle))
> >
> > Here we already have a handle on "event_data".  As such I think this is what we
> > should feed to coresight_enable_path() rather than the handle.  That way we
> > don't need to call etm_perf_sink_config(), we just use the data.
>
> The advantage of passing on the handle is, we could get all the way upto the
> "perf_event" for the given session. Passing the event_data will loose that
> information.
>
> i.e, perf_event-> |perf_ouput_handle | -> |event_data | -> sink_config
>                    |  <-event         |    |           |
>
> The purpose of the wrapper "etm_perf_sink_config()" is to abstract the way we
> handle the information under the event_data. i.e, if we decide to make some
> changes in the way we store event_data, we need to spill the changes every
> where. But the perf_ouput_handle has much more stable ABI than event_data,
> hence the choice of passing handle.

I agree that etm_perf_sink_config() has value but it should take a
void * as parameter (i.e what gets returned from perf_get_aux())
rather than a perf_output_handle *.

Thanks,
Mathieu

>
> Cheers
> Suzuki

Re: [PATCH v2 09/10] coresight: perf: Remove set_buffer call back

[Suzuki K Poulose]

Mathieu,

On 07/23/2018 07:22 PM, Mathieu Poirier wrote:
> On Fri, 20 Jul 2018 at 03:04, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
>>
>> Mathieu,
>>
>> On 19/07/18 21:36, Mathieu Poirier wrote:
>>> On Tue, Jul 17, 2018 at 06:11:40PM +0100, Suzuki K Poulose wrote:
>>>> In coresight perf mode, we need to prepare the sink before
>>>> starting a session, which is done via set_buffer call back.
>>>> We then proceed to enable the tracing. If we fail to start
>>>> the session successfully, we leave the sink configuration
>>>> unchanged. This was fine for the existing backends as they
>>>> don't have any state associated with the buffers. But with
>>>> ETR, we need to keep track of the buffer details and need
>>>> to be cleaned up if we fail. In order to make the operation
>>>> atomic and to avoid yet another call back, we get rid of
>>>> the "set_buffer" call back and pass the buffer details
>>>> via enable() call back to the sink.
>>>
>>> Suzuki,
>>>
>>> I'm not sure I understand the problem you're trying to fix there.  From the
>>> implementation of tmc_enable_etr_sink_perf() in the next patch, wouldn't the
>>> same result been achievable using a callback?
>>
>> We can definitely achieve the results using "set_buffer". But for ETR,
>> we track the "perf_buf" in drvdata->perf_data when we do "set_buffer".
>> But if we failed to enable_path(), we leave the drvdata->perf_data
>> and doesn't clean it up. Now when another session is about to set_buf,
>> we check if perf_data is empty and WARNs otherwise.
>> Because we can't be sure if it belongs to an abandoned session or
>> another active session and we completely messed somewhere in the driver.
>> So, we need a clear_buffer call back if the enable fails, something
>> not really worth. Anyways, there is no point in separating set_buffer
>> and enabling the sink, as the error handling becomes cumbersome as explained
>> above.
>>
>>>
>>> I'm fine with this patch and supportive of getting rid of callbacks if we can, I
>>> just need to understand the exact problem you're after.  From looking a your
>>> code (and the current implementation), if we succeed in setting the memory for
>>> the sink but fail in any of the subsequent steps i.e, enabling the rest of the
>>> compoment on the path or the source, the sink is left unchanged.
>>
>> Yes, thats right. And we should WARN (which I missed in this version) if
>> there is a perf_data already for a disabled ETR. Please see my response to the
>> next patch.
> 
> The changelog for this patch states the following: "But with ETR, we
> need to keep track of the buffer details and need to be cleaned up if
> we fail."
> 
> I did a deep dive in the code and in the current implementation if the
> source fails to be enabled in etm_event_start() the path and the sink
> remains unchanged.  With your patchset this get fixed because a goto
> was added to disable the path when such condition occur.  As such each
> component in the path will see its ->disable() callback invoked.  In
> tmc_disable_etr_sink(), drvdata->perf_data is set to NULL in
> tmc_etr_disable_hw(), so the cleanup on error condition is done
> properly.  As such we wouldn't need a clean_buffer() callback.

All of this is right. But we still have a case. e.g, if the ETR is
enabled in sysfs mode, coresight_enable_path() will fail after we
have set the buffer. And since we don't try to disable the path
when we fail at SINK (which is the right thing to do, as we could
be potentially disabling the ETR operated in sysfs mode), we leave
the perf_data around. And the next session finds a non-empty data.

> 
> As I said I'm in favour of removing the set_buffer() callback but I
> wouldn't associated it with ETR state cleanup.  If the code can be
> rearranged in a way that code can be removed then that alone is enough
> to justify the change.
> 
>>
>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
>>>> index 3cc4a0b..12a247d 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
>>>> @@ -269,16 +269,11 @@ static void etm_event_start(struct perf_event *event, int flags)
>>>>       path = etm_event_cpu_path(event_data, cpu);
>>>>       /* We need a sink, no need to continue without one */
>>>>       sink = coresight_get_sink(path);
>>>> -    if (WARN_ON_ONCE(!sink || !sink_ops(sink)->set_buffer))
>>>> -            goto fail_end_stop;
>>>> -
>>>> -    /* Configure the sink */
>>>> -    if (sink_ops(sink)->set_buffer(sink, handle,
>>>> -                                   event_data->snk_config))
>>>> +    if (WARN_ON_ONCE(!sink))
>>>>               goto fail_end_stop;
>>>>
>>>>       /* Nothing will happen without a path */
>>>> -    if (coresight_enable_path(path, CS_MODE_PERF))
>>>> +    if (coresight_enable_path(path, CS_MODE_PERF, handle))
>>>
>>> Here we already have a handle on "event_data".  As such I think this is what we
>>> should feed to coresight_enable_path() rather than the handle.  That way we
>>> don't need to call etm_perf_sink_config(), we just use the data.
>>
>> The advantage of passing on the handle is, we could get all the way upto the
>> "perf_event" for the given session. Passing the event_data will loose that
>> information.
>>
>> i.e, perf_event-> |perf_ouput_handle | -> |event_data | -> sink_config
>>                     |  <-event         |    |           |
>>
>> The purpose of the wrapper "etm_perf_sink_config()" is to abstract the way we
>> handle the information under the event_data. i.e, if we decide to make some
>> changes in the way we store event_data, we need to spill the changes every
>> where. But the perf_ouput_handle has much more stable ABI than event_data,
>> hence the choice of passing handle.
> 
> I agree that etm_perf_sink_config() has value but it should take a
> void * as parameter (i.e what gets returned from perf_get_aux())
> rather than a perf_output_handle *.

Ok, did you mean :

	sink_config = etm_perf_sink_config(perf_get_aux(handle)); ?

Or do you want to change the parameter passed to the enable_sink() as
well ?


Suzuki

Re: [PATCH v2 09/10] coresight: perf: Remove set_buffer call back

[Mathieu Poirier]

On Mon, 23 Jul 2018 at 16:27, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>
> Mathieu,
>
> On 07/23/2018 07:22 PM, Mathieu Poirier wrote:
> > On Fri, 20 Jul 2018 at 03:04, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
> >>
> >> Mathieu,
> >>
> >> On 19/07/18 21:36, Mathieu Poirier wrote:
> >>> On Tue, Jul 17, 2018 at 06:11:40PM +0100, Suzuki K Poulose wrote:
> >>>> In coresight perf mode, we need to prepare the sink before
> >>>> starting a session, which is done via set_buffer call back.
> >>>> We then proceed to enable the tracing. If we fail to start
> >>>> the session successfully, we leave the sink configuration
> >>>> unchanged. This was fine for the existing backends as they
> >>>> don't have any state associated with the buffers. But with
> >>>> ETR, we need to keep track of the buffer details and need
> >>>> to be cleaned up if we fail. In order to make the operation
> >>>> atomic and to avoid yet another call back, we get rid of
> >>>> the "set_buffer" call back and pass the buffer details
> >>>> via enable() call back to the sink.
> >>>
> >>> Suzuki,
> >>>
> >>> I'm not sure I understand the problem you're trying to fix there.  From the
> >>> implementation of tmc_enable_etr_sink_perf() in the next patch, wouldn't the
> >>> same result been achievable using a callback?
> >>
> >> We can definitely achieve the results using "set_buffer". But for ETR,
> >> we track the "perf_buf" in drvdata->perf_data when we do "set_buffer".
> >> But if we failed to enable_path(), we leave the drvdata->perf_data
> >> and doesn't clean it up. Now when another session is about to set_buf,
> >> we check if perf_data is empty and WARNs otherwise.
> >> Because we can't be sure if it belongs to an abandoned session or
> >> another active session and we completely messed somewhere in the driver.
> >> So, we need a clear_buffer call back if the enable fails, something
> >> not really worth. Anyways, there is no point in separating set_buffer
> >> and enabling the sink, as the error handling becomes cumbersome as explained
> >> above.
> >>
> >>>
> >>> I'm fine with this patch and supportive of getting rid of callbacks if we can, I
> >>> just need to understand the exact problem you're after.  From looking a your
> >>> code (and the current implementation), if we succeed in setting the memory for
> >>> the sink but fail in any of the subsequent steps i.e, enabling the rest of the
> >>> compoment on the path or the source, the sink is left unchanged.
> >>
> >> Yes, thats right. And we should WARN (which I missed in this version) if
> >> there is a perf_data already for a disabled ETR. Please see my response to the
> >> next patch.
> >
> > The changelog for this patch states the following: "But with ETR, we
> > need to keep track of the buffer details and need to be cleaned up if
> > we fail."
> >
> > I did a deep dive in the code and in the current implementation if the
> > source fails to be enabled in etm_event_start() the path and the sink
> > remains unchanged.  With your patchset this get fixed because a goto
> > was added to disable the path when such condition occur.  As such each
> > component in the path will see its ->disable() callback invoked.  In
> > tmc_disable_etr_sink(), drvdata->perf_data is set to NULL in
> > tmc_etr_disable_hw(), so the cleanup on error condition is done
> > properly.  As such we wouldn't need a clean_buffer() callback.
>
> All of this is right. But we still have a case. e.g, if the ETR is
> enabled in sysfs mode, coresight_enable_path() will fail after we
> have set the buffer. And since we don't try to disable the path
> when we fail at SINK (which is the right thing to do, as we could
> be potentially disabling the ETR operated in sysfs mode), we leave
> the perf_data around. And the next session finds a non-empty data.

We are in total agreement :o)

All I hoping for is to see the sentence "But with ETR, we need to keep
track of the buffer details and need to be cleaned up if we fail."
removed from the changelog.  To me that sounds like you're adding
cleanup work in enable() which isn't the case.  On the flip side you
are making the sink configuration atomic and that is the important
part.

>
> >
> > As I said I'm in favour of removing the set_buffer() callback but I
> > wouldn't associated it with ETR state cleanup.  If the code can be
> > rearranged in a way that code can be removed then that alone is enough
> > to justify the change.
> >
> >>
> >>>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> >>>> index 3cc4a0b..12a247d 100644
> >>>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> >>>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> >>>> @@ -269,16 +269,11 @@ static void etm_event_start(struct perf_event *event, int flags)
> >>>>       path = etm_event_cpu_path(event_data, cpu);
> >>>>       /* We need a sink, no need to continue without one */
> >>>>       sink = coresight_get_sink(path);
> >>>> -    if (WARN_ON_ONCE(!sink || !sink_ops(sink)->set_buffer))
> >>>> -            goto fail_end_stop;
> >>>> -
> >>>> -    /* Configure the sink */
> >>>> -    if (sink_ops(sink)->set_buffer(sink, handle,
> >>>> -                                   event_data->snk_config))
> >>>> +    if (WARN_ON_ONCE(!sink))
> >>>>               goto fail_end_stop;
> >>>>
> >>>>       /* Nothing will happen without a path */
> >>>> -    if (coresight_enable_path(path, CS_MODE_PERF))
> >>>> +    if (coresight_enable_path(path, CS_MODE_PERF, handle))
> >>>
> >>> Here we already have a handle on "event_data".  As such I think this is what we
> >>> should feed to coresight_enable_path() rather than the handle.  That way we
> >>> don't need to call etm_perf_sink_config(), we just use the data.
> >>
> >> The advantage of passing on the handle is, we could get all the way upto the
> >> "perf_event" for the given session. Passing the event_data will loose that
> >> information.
> >>
> >> i.e, perf_event-> |perf_ouput_handle | -> |event_data | -> sink_config
> >>                     |  <-event         |    |           |
> >>
> >> The purpose of the wrapper "etm_perf_sink_config()" is to abstract the way we
> >> handle the information under the event_data. i.e, if we decide to make some
> >> changes in the way we store event_data, we need to spill the changes every
> >> where. But the perf_ouput_handle has much more stable ABI than event_data,
> >> hence the choice of passing handle.
> >
> > I agree that etm_perf_sink_config() has value but it should take a
> > void * as parameter (i.e what gets returned from perf_get_aux())
> > rather than a perf_output_handle *.
>
> Ok, did you mean :
>
>         sink_config = etm_perf_sink_config(perf_get_aux(handle)); ?
>
> Or do you want to change the parameter passed to the enable_sink() as
> well ?

I've been thinking further on this and keeping things the way you've
implemented them is probably best at this time.  I'm currently adding
support for PMU HW configuration and in the end we may need the
information conveyed by event->hw.drv_config [1].  When it's all said
and done we can clean things up if need be.

Thanks,
Mathieu

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

Re: [PATCH v2 09/10] coresight: perf: Remove set_buffer call back

[Suzuki K Poulose]

On 07/24/2018 09:08 PM, Mathieu Poirier wrote:
> On Mon, 23 Jul 2018 at 16:27, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>>
>> Mathieu,
>>
>> On 07/23/2018 07:22 PM, Mathieu Poirier wrote:
>>> On Fri, 20 Jul 2018 at 03:04, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
>>>>
>>>> Mathieu,
>>>>
>>>> On 19/07/18 21:36, Mathieu Poirier wrote:
>>>>> On Tue, Jul 17, 2018 at 06:11:40PM +0100, Suzuki K Poulose wrote:
>>>>>> In coresight perf mode, we need to prepare the sink before
>>>>>> starting a session, which is done via set_buffer call back.
>>>>>> We then proceed to enable the tracing. If we fail to start
>>>>>> the session successfully, we leave the sink configuration
>>>>>> unchanged. This was fine for the existing backends as they
>>>>>> don't have any state associated with the buffers. But with
>>>>>> ETR, we need to keep track of the buffer details and need
>>>>>> to be cleaned up if we fail. In order to make the operation
>>>>>> atomic and to avoid yet another call back, we get rid of
>>>>>> the "set_buffer" call back and pass the buffer details
>>>>>> via enable() call back to the sink.
>>>>>
>>>>> Suzuki,
>>>>>
>>>>> I'm not sure I understand the problem you're trying to fix there.  From the
>>>>> implementation of tmc_enable_etr_sink_perf() in the next patch, wouldn't the
>>>>> same result been achievable using a callback?
>>>>
>>>> We can definitely achieve the results using "set_buffer". But for ETR,
>>>> we track the "perf_buf" in drvdata->perf_data when we do "set_buffer".
>>>> But if we failed to enable_path(), we leave the drvdata->perf_data
>>>> and doesn't clean it up. Now when another session is about to set_buf,
>>>> we check if perf_data is empty and WARNs otherwise.
>>>> Because we can't be sure if it belongs to an abandoned session or
>>>> another active session and we completely messed somewhere in the driver.
>>>> So, we need a clear_buffer call back if the enable fails, something
>>>> not really worth. Anyways, there is no point in separating set_buffer
>>>> and enabling the sink, as the error handling becomes cumbersome as explained
>>>> above.
>>>>
>>>>>
>>>>> I'm fine with this patch and supportive of getting rid of callbacks if we can, I
>>>>> just need to understand the exact problem you're after.  From looking a your
>>>>> code (and the current implementation), if we succeed in setting the memory for
>>>>> the sink but fail in any of the subsequent steps i.e, enabling the rest of the
>>>>> compoment on the path or the source, the sink is left unchanged.
>>>>
>>>> Yes, thats right. And we should WARN (which I missed in this version) if
>>>> there is a perf_data already for a disabled ETR. Please see my response to the
>>>> next patch.
>>>
>>> The changelog for this patch states the following: "But with ETR, we
>>> need to keep track of the buffer details and need to be cleaned up if
>>> we fail."
>>>
>>> I did a deep dive in the code and in the current implementation if the
>>> source fails to be enabled in etm_event_start() the path and the sink
>>> remains unchanged.  With your patchset this get fixed because a goto
>>> was added to disable the path when such condition occur.  As such each
>>> component in the path will see its ->disable() callback invoked.  In
>>> tmc_disable_etr_sink(), drvdata->perf_data is set to NULL in
>>> tmc_etr_disable_hw(), so the cleanup on error condition is done
>>> properly.  As such we wouldn't need a clean_buffer() callback.
>>
>> All of this is right. But we still have a case. e.g, if the ETR is
>> enabled in sysfs mode, coresight_enable_path() will fail after we
>> have set the buffer. And since we don't try to disable the path
>> when we fail at SINK (which is the right thing to do, as we could
>> be potentially disabling the ETR operated in sysfs mode), we leave
>> the perf_data around. And the next session finds a non-empty data.
> 
> We are in total agreement :o)
> 
> All I hoping for is to see the sentence "But with ETR, we need to keep
> track of the buffer details and need to be cleaned up if we fail."
> removed from the changelog.  To me that sounds like you're adding
> cleanup work in enable() which isn't the case.  On the flip side you
> are making the sink configuration atomic and that is the important
> part.

:-), I will update the change log accordingly.

> 
>>
>>>
>>> As I said I'm in favour of removing the set_buffer() callback but I
>>> wouldn't associated it with ETR state cleanup.  If the code can be
>>> rearranged in a way that code can be removed then that alone is enough
>>> to ju


>>>>>> +    if (coresight_enable_path(path, CS_MODE_PERF, handle))
>>>>>
>>>>> Here we already have a handle on "event_data".  As such I think this is what we
>>>>> should feed to coresight_enable_path() rather than the handle.  That way we
>>>>> don't need to call etm_perf_sink_config(), we just use the data.
>>>>
>>>> The advantage of passing on the handle is, we could get all the way upto the
>>>> "perf_event" for the given session. Passing the event_data will loose that
>>>> information.
>>>>
>>>> i.e, perf_event-> |perf_ouput_handle | -> |event_data | -> sink_config
>>>>                      |  <-event         |    |           |
>>>>
>>>> The purpose of the wrapper "etm_perf_sink_config()" is to abstract the way we
>>>> handle the information under the event_data. i.e, if we decide to make some
>>>> changes in the way we store event_data, we need to spill the changes every
>>>> where. But the perf_ouput_handle has much more stable ABI than event_data,
>>>> hence the choice of passing handle.
>>>
>>> I agree that etm_perf_sink_config() has value but it should take a
>>> void * as parameter (i.e what gets returned from perf_get_aux())
>>> rather than a perf_output_handle *.
>>
>> Ok, did you mean :
>>
>>          sink_config = etm_perf_sink_config(perf_get_aux(handle)); ?
>>
>> Or do you want to change the parameter passed to the enable_sink() as
>> well ?
> 
> I've been thinking further on this and keeping things the way you've
> implemented them is probably best at this time.  I'm currently adding
> support for PMU HW configuration and in the end we may need the
> information conveyed by event->hw.drv_config [1].  When it's all said
> and done we can clean things up if need be.

Ok.


Thanks for the review.

Cheers
Suzuki