[PATCH v5 0/5] Update CoreSight infrastructure to select a default sink.

[PATCH v5 0/5] Update CoreSight infrastructure to select a default sink.

[Mike Leach]

This patchset provides an infrastructure to allow for the automatic
selection of a sink during CoreSight tracing operations.

Currently starting tracing using perf requires a sink selection on the
command line:-

sudo ./perf record -e cs_etm/@tmc_etr0/ --per-thread uname -a

After this set (and the follow-up perf change set) the infrastructure will
be able to select a default sink:-

sudo ./perf record -e cs_etm// --per-thread uname -a

This matches with the default operation provided with perf and intelpt.

Where no sink is specified at the start of a trace session, the CoreSight
system will walk the connection graph from the source ETM, to find a
suitable sink using the first encountered highest priority device.

The CoreSight infrastructure is updated to define sink sub_types to
differentiate between sinks with built in buffers (ETB / ETF) - BUFFER
type, and those that use system memory (ETR) - SYSMEM - types.

SYSMEM types are considered higher priority.

When two sinks are found of equal priority, then the closest sink to the
source in terms of connection nodes is chosen.

The automatic sink selection will also operate if an ETM is enabled using
sysfs commands, and no sink is currently enabled. A last_sink attribute is
added to trace sources that is set to the value of the sink used when a
source is enabled via sysfs. This is set in both default and user enabled
sink scenarios.

Applies to Linux 5.8-rc1

Tested on Dragonboard DB410c.

Changes since v4:
1) Added reviewed-by etc that were missing from previous sets.
2) Added last_sink attribute to source devices.
3) Added documentation patch to update docs for default sinks.
4) Moved comment fix patch into separate misc fixes set.

Mike Leach (5):
  coresight: Add default sink selection to CoreSight base.
  coresight: tmc: Update sink types for default selection.
  coresight: etm: perf: Add default sink selection to etm perf.
  coresight: sysfs: Allow select default sink on source enable.
  documentation: coresight: Update CoreSight document for default sink.

 Documentation/trace/coresight/coresight.rst   |  48 ++--
 .../hwtracing/coresight/coresight-etm-perf.c  |  17 +-
 drivers/hwtracing/coresight/coresight-priv.h  |   2 +
 drivers/hwtracing/coresight/coresight-tmc.c   |   3 +-
 drivers/hwtracing/coresight/coresight.c       | 205 +++++++++++++++++-
 include/linux/coresight.h                     |   6 +
 6 files changed, 261 insertions(+), 20 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v5 1/5] coresight: Add default sink selection to CoreSight base.

[Mike Leach]

Adds a method to select a suitable sink connected to a given source.

In cases where no sink is defined, the coresight_find_default_sink
routine can search from a given source, through the child connections
until a suitable sink is found.

The suitability is defined in by the sink coresight_dev_subtype on the
CoreSight device, and the distance from the source by counting
connections.

Higher value subtype is preferred - where these are equal, shorter
distance from source is used as a tie-break.

This allows for default sink to be discovered were none is specified
(e.g. perf command line)

Signed-off-by: Mike Leach <mike.leach@linaro.org>
Suggested-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Reviewed-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/hwtracing/coresight/coresight-priv.h |   2 +
 drivers/hwtracing/coresight/coresight.c      | 166 +++++++++++++++++++
 include/linux/coresight.h                    |   3 +
 3 files changed, 171 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index 36c943ae94d5..f2dc625ea585 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -150,6 +150,8 @@ int coresight_enable_path(struct list_head *path, u32 mode, void *sink_data);
 struct coresight_device *coresight_get_sink(struct list_head *path);
 struct coresight_device *coresight_get_enabled_sink(bool reset);
 struct coresight_device *coresight_get_sink_by_id(u32 id);
+struct coresight_device *
+coresight_find_default_sink(struct coresight_device *csdev);
 struct list_head *coresight_build_path(struct coresight_device *csdev,
 				       struct coresight_device *sink);
 void coresight_release_path(struct list_head *path);
diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index f3efbb3b2b4d..e9c90f2de34a 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -769,6 +769,171 @@ void coresight_release_path(struct list_head *path)
 	path = NULL;
 }
 
+/* return true if the device is a suitable type for a default sink */
+static inline bool coresight_is_def_sink_type(struct coresight_device *csdev)
+{
+	/* sink & correct subtype */
+	if (((csdev->type == CORESIGHT_DEV_TYPE_SINK) ||
+	     (csdev->type == CORESIGHT_DEV_TYPE_LINKSINK)) &&
+	    (csdev->subtype.sink_subtype >= CORESIGHT_DEV_SUBTYPE_SINK_BUFFER))
+		return true;
+	return false;
+}
+
+/**
+ * coresight_select_best_sink - return the best sink for use as default from
+ * the two provided.
+ *
+ * @sink:	current best sink.
+ * @depth:      search depth where current sink was found.
+ * @new_sink:	new sink for comparison with current sink.
+ * @new_depth:  search depth where new sink was found.
+ *
+ * Sinks prioritised according to coresight_dev_subtype_sink, with only
+ * subtypes CORESIGHT_DEV_SUBTYPE_SINK_BUFFER or higher being used.
+ *
+ * Where two sinks of equal priority are found, the sink closest to the
+ * source is used (smallest search depth).
+ *
+ * return @new_sink & update @depth if better than @sink, else return @sink.
+ */
+static struct coresight_device *
+coresight_select_best_sink(struct coresight_device *sink, int *depth,
+			   struct coresight_device *new_sink, int new_depth)
+{
+	bool update = false;
+
+	if (!sink) {
+		/* first found at this level */
+		update = true;
+	} else if (new_sink->subtype.sink_subtype >
+		   sink->subtype.sink_subtype) {
+		/* found better sink */
+		update = true;
+	} else if ((new_sink->subtype.sink_subtype ==
+		    sink->subtype.sink_subtype) &&
+		   (*depth > new_depth)) {
+		/* found same but closer sink */
+		update = true;
+	}
+
+	if (update)
+		*depth = new_depth;
+	return update ? new_sink : sink;
+}
+
+/**
+ * coresight_find_sink - recursive function to walk trace connections from
+ * source to find a suitable default sink.
+ *
+ * @csdev: source / current device to check.
+ * @depth: [in] search depth of calling dev, [out] depth of found sink.
+ *
+ * This will walk the connection path from a source (ETM) till a suitable
+ * sink is encountered and return that sink to the original caller.
+ *
+ * If current device is a plain sink return that & depth, otherwise recursively
+ * call child connections looking for a sink. Select best possible using
+ * coresight_select_best_sink.
+ *
+ * return best sink found, or NULL if not found at this node or child nodes.
+ */
+static struct coresight_device *
+coresight_find_sink(struct coresight_device *csdev, int *depth)
+{
+	int i, curr_depth = *depth + 1, found_depth = 0;
+	struct coresight_device *found_sink = NULL;
+
+	if (coresight_is_def_sink_type(csdev)) {
+		found_depth = curr_depth;
+		found_sink = csdev;
+		if (csdev->type == CORESIGHT_DEV_TYPE_SINK)
+			goto return_def_sink;
+		/* look past LINKSINK for something better */
+	}
+
+	/*
+	 * Not a sink we want - or possible child sink may be better.
+	 * recursively explore each port found on this element.
+	 */
+	for (i = 0; i < csdev->pdata->nr_outport; i++) {
+		struct coresight_device *child_dev, *sink = NULL;
+		int child_depth = curr_depth;
+
+		child_dev = csdev->pdata->conns[i].child_dev;
+		if (child_dev)
+			sink = coresight_find_sink(child_dev, &child_depth);
+
+		if (sink)
+			found_sink = coresight_select_best_sink(found_sink,
+								&found_depth,
+								sink,
+								child_depth);
+	}
+
+return_def_sink:
+	/* return found sink and depth */
+	if (found_sink)
+		*depth = found_depth;
+	return found_sink;
+}
+
+/**
+ * coresight_find_default_sink: Find a sink suitable for use as a
+ * default sink.
+ *
+ * @csdev: starting source to find a connected sink.
+ *
+ * Walks connections graph looking for a suitable sink to enable for the
+ * supplied source. Uses CoreSight device subtypes and distance from source
+ * to select the best sink.
+ *
+ * If a sink is found, then the default sink for this device is set and
+ * will be automatically used in future.
+ *
+ * Used in cases where the CoreSight user (perf / sysfs) has not selected a
+ * sink.
+ */
+struct coresight_device *
+coresight_find_default_sink(struct coresight_device *csdev)
+{
+	int depth = 0;
+
+	/* look for a default sink if we have not found for this device */
+	if (!csdev->def_sink)
+		csdev->def_sink = coresight_find_sink(csdev, &depth);
+	return csdev->def_sink;
+}
+
+static int coresight_remove_sink_ref(struct device *dev, void *data)
+{
+	struct coresight_device *sink = data;
+	struct coresight_device *source = to_coresight_device(dev);
+
+	if (source->def_sink == sink)
+		source->def_sink = NULL;
+	return 0;
+}
+
+/**
+ * coresight_clear_default_sink: Remove all default sink references to the
+ * supplied sink.
+ *
+ * If supplied device is a sink, then check all the bus devices and clear
+ * out all the references to this sink from the coresight_device def_sink
+ * parameter.
+ *
+ * @csdev: coresight sink - remove references to this from all sources.
+ */
+static void coresight_clear_default_sink(struct coresight_device *csdev)
+{
+	if ((csdev->type == CORESIGHT_DEV_TYPE_SINK) ||
+	    (csdev->type == CORESIGHT_DEV_TYPE_LINKSINK)) {
+		bus_for_each_dev(&coresight_bustype, NULL, csdev,
+				 coresight_remove_sink_ref);
+	}
+}
+
 /** coresight_validate_source - make sure a source has the right credentials
  *  @csdev:	the device structure for a source.
  *  @function:	the function this was called from.
@@ -1358,6 +1523,7 @@ void coresight_unregister(struct coresight_device *csdev)
 	etm_perf_del_symlink_sink(csdev);
 	/* Remove references of that device in the topology */
 	coresight_remove_conns(csdev);
+	coresight_clear_default_sink(csdev);
 	coresight_release_platform_data(csdev, csdev->pdata);
 	device_unregister(&csdev->dev);
 }
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index 84dc695e87d4..58fffdecdbfd 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -48,6 +48,7 @@ enum coresight_dev_subtype_sink {
 	CORESIGHT_DEV_SUBTYPE_SINK_NONE,
 	CORESIGHT_DEV_SUBTYPE_SINK_PORT,
 	CORESIGHT_DEV_SUBTYPE_SINK_BUFFER,
+	CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM,
 };
 
 enum coresight_dev_subtype_link {
@@ -182,6 +183,7 @@ struct coresight_sysfs_link {
  *		happens when a source has been selected and a path is enabled
  *		from source to that sink.
  * @ea:		Device attribute for sink representation under PMU directory.
+ * @def_sink:	cached reference to default sink found for this device.
  * @ect_dev:	Associated cross trigger device. Not part of the trace data
  *		path or connections.
  * @nr_links:   number of sysfs links created to other components from this
@@ -200,6 +202,7 @@ struct coresight_device {
 	/* sink specific fields */
 	bool activated;	/* true only if a sink is part of a path */
 	struct dev_ext_attribute *ea;
+	struct coresight_device *def_sink;
 	/* cross trigger handling */
 	struct coresight_device *ect_dev;
 	/* sysfs links between components */
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v5 2/5] coresight: tmc: Update sink types for default selection.

[Mike Leach]

An additional sink subtype is added to differentiate ETB/ETF buffer
sinks and ETR type system memory sinks.

This allows the prioritised selection of default sinks.

Signed-off-by: Mike Leach <mike.leach@linaro.org>
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/hwtracing/coresight/coresight-tmc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
index 39fba1d16e6e..0d2eb7e0e1bb 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.c
+++ b/drivers/hwtracing/coresight/coresight-tmc.c
@@ -484,7 +484,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id)
 		break;
 	case TMC_CONFIG_TYPE_ETR:
 		desc.type = CORESIGHT_DEV_TYPE_SINK;
-		desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
+		desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM;
 		desc.ops = &tmc_etr_cs_ops;
 		ret = tmc_etr_setup_caps(dev, devid,
 					 coresight_get_uci_data(id));
@@ -496,6 +496,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id)
 		break;
 	case TMC_CONFIG_TYPE_ETF:
 		desc.type = CORESIGHT_DEV_TYPE_LINKSINK;
+		desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
 		desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_FIFO;
 		desc.ops = &tmc_etf_cs_ops;
 		dev_list = &etf_devs;
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v5 3/5] coresight: etm: perf: Add default sink selection to etm perf.

[Mike Leach]

Add default sink selection to the perf trace handling in the etm driver.
Uses the select default sink infrastructure to select a sink for the perf
session, if no other sink is specified.

Signed-off-by: Mike Leach <mike.leach@linaro.org>
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 .../hwtracing/coresight/coresight-etm-perf.c    | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 84f1dcb69827..1a3169e69bb1 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -226,9 +226,6 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
 		sink = coresight_get_enabled_sink(true);
 	}
 
-	if (!sink)
-		goto err;
-
 	mask = &event_data->mask;
 
 	/*
@@ -253,6 +250,16 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
 			continue;
 		}
 
+		/*
+		 * No sink provided - look for a default sink for one of the
+		 * devices. At present we only support topology where all CPUs
+		 * use the same sink [N:1], so only need to find one sink. The
+		 * coresight_build_path later will remove any CPU that does not
+		 * attach to the sink, or if we have not found a sink.
+		 */
+		if (!sink)
+			sink = coresight_find_default_sink(csdev);
+
 		/*
 		 * Building a path doesn't enable it, it simply builds a
 		 * list of devices from source to sink that can be
@@ -267,6 +274,10 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
 		*etm_event_cpu_path_ptr(event_data, cpu) = path;
 	}
 
+	/* no sink found for any CPU - cannot trace */
+	if (!sink)
+		goto err;
+
 	/* If we don't have any CPUs ready for tracing, abort */
 	cpu = cpumask_first(mask);
 	if (cpu >= nr_cpu_ids)
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v5 4/5] coresight: sysfs: Allow select default sink on source enable.

[Mike Leach]

When enabling a trace source using sysfs, allow the CoreSight system to
auto-select a default sink if none has been enabled by the user.

Uses the sink select algorithm that uses the default select priorities
set when sinks are registered with the system. At present this will
prefer ETR over ETB / ETF.

Adds a new attribute 'last_sink' to source CoreSight devices. This is set
when a source is enabled using sysfs, to the sink that the device will
trace into. This applies for both user enabled and default enabled sinks.

Signed-off-by: Mike Leach <mike.leach@linaro.org>
---
 drivers/hwtracing/coresight/coresight.c | 39 +++++++++++++++++++++++--
 include/linux/coresight.h               |  3 ++
 2 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index e9c90f2de34a..db39e0b56994 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -934,6 +934,16 @@ static void coresight_clear_default_sink(struct coresight_device *csdev)
 	}
 }
 
+static void coresight_set_last_sink_name(struct coresight_device *source,
+					 struct coresight_device *sink)
+{
+	/* remove current value and set new one if *sink not NULL */
+	kfree(source->last_sink);
+	source->last_sink = NULL;
+	if (sink)
+		source->last_sink = kstrdup(dev_name(&sink->dev), GFP_KERNEL);
+}
+
 /** coresight_validate_source - make sure a source has the right credentials
  *  @csdev:	the device structure for a source.
  *  @function:	the function this was called from.
@@ -994,8 +1004,15 @@ int coresight_enable(struct coresight_device *csdev)
 	 */
 	sink = coresight_get_enabled_sink(false);
 	if (!sink) {
-		ret = -EINVAL;
-		goto out;
+		/* look for a default sink if nothing enabled */
+		sink = coresight_find_default_sink(csdev);
+		if (!sink) {
+			ret = -EINVAL;
+			goto out;
+		}
+		/* mark the default as enabled */
+		sink->activated = true;
+		dev_info(&sink->dev, "Enabled default sink.");
 	}
 
 	path = coresight_build_path(csdev, sink);
@@ -1033,6 +1050,9 @@ int coresight_enable(struct coresight_device *csdev)
 		break;
 	}
 
+	/* record name of sink used for this session */
+	coresight_set_last_sink_name(csdev, sink);
+
 out:
 	mutex_unlock(&coresight_mutex);
 	return ret;
@@ -1145,6 +1165,19 @@ static ssize_t enable_source_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(enable_source);
 
+static ssize_t last_sink_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	struct coresight_device *csdev = to_coresight_device(dev);
+	ssize_t size = 0;
+
+	if (csdev->last_sink)
+		size = scnprintf(buf, PAGE_SIZE, "%s\n", csdev->last_sink);
+	return size;
+}
+static DEVICE_ATTR_RO(last_sink);
+
+
 static struct attribute *coresight_sink_attrs[] = {
 	&dev_attr_enable_sink.attr,
 	NULL,
@@ -1153,6 +1186,7 @@ ATTRIBUTE_GROUPS(coresight_sink);
 
 static struct attribute *coresight_source_attrs[] = {
 	&dev_attr_enable_source.attr,
+	&dev_attr_last_sink.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(coresight_source);
@@ -1524,6 +1558,7 @@ void coresight_unregister(struct coresight_device *csdev)
 	/* Remove references of that device in the topology */
 	coresight_remove_conns(csdev);
 	coresight_clear_default_sink(csdev);
+	coresight_set_last_sink_name(csdev, NULL);
 	coresight_release_platform_data(csdev, csdev->pdata);
 	device_unregister(&csdev->dev);
 }
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index 58fffdecdbfd..fc320dd2cedc 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -184,6 +184,8 @@ struct coresight_sysfs_link {
  *		from source to that sink.
  * @ea:		Device attribute for sink representation under PMU directory.
  * @def_sink:	cached reference to default sink found for this device.
+ * @last_sink:	Name of last sink used for this source to trace into. Set when
+ *		enabling a source using sysfs - only set on a source device.
  * @ect_dev:	Associated cross trigger device. Not part of the trace data
  *		path or connections.
  * @nr_links:   number of sysfs links created to other components from this
@@ -203,6 +205,7 @@ struct coresight_device {
 	bool activated;	/* true only if a sink is part of a path */
 	struct dev_ext_attribute *ea;
 	struct coresight_device *def_sink;
+	const char *last_sink;
 	/* cross trigger handling */
 	struct coresight_device *ect_dev;
 	/* sysfs links between components */
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v5 5/5] documentation: coresight: Update CoreSight document for default sink.

[Mike Leach]

Updates the CoreSight documentation to cover the use of default sinks for
both perf and sysfs operations.

Signed-off-by: Mike Leach <mike.leach@linaro.org>
---
 Documentation/trace/coresight/coresight.rst | 48 +++++++++++++++------
 1 file changed, 34 insertions(+), 14 deletions(-)

diff --git a/Documentation/trace/coresight/coresight.rst b/Documentation/trace/coresight/coresight.rst
index 0b73acb44efa..917d89f74c2e 100644
--- a/Documentation/trace/coresight/coresight.rst
+++ b/Documentation/trace/coresight/coresight.rst
@@ -341,17 +341,18 @@ provide details on using both methods.
 1) Using the sysFS interface:
 
 Before trace collection can start, a coresight sink needs to be identified.
-There is no limit on the amount of sinks (nor sources) that can be enabled at
-any given moment.  As a generic operation, all device pertaining to the sink
-class will have an "active" entry in sysfs::
+There is no limit on the amount of sources and sinks that can be enabled at
+any given moment. However, any source will only trace into a single sink.
+As a generic operation, all device pertaining to the sink class will have an
+"active" entry in sysfs::
 
     root:/sys/bus/coresight/devices# ls
-    replicator  20030000.tpiu    2201c000.ptm  2203c000.etm  2203e000.etm
-    20010000.etb         20040000.funnel  2201d000.ptm  2203d000.etm
-    root:/sys/bus/coresight/devices# ls 20010000.etb
+    replicator0  tpiu0  ptm0  etm2  etm3
+    etb0  funnel0  ptm1  etm4
+    root:/sys/bus/coresight/devices# ls etb0
     enable_sink  status  trigger_cntr
-    root:/sys/bus/coresight/devices# echo 1 > 20010000.etb/enable_sink
-    root:/sys/bus/coresight/devices# cat 20010000.etb/enable_sink
+    root:/sys/bus/coresight/devices# echo 1 > etb0/enable_sink
+    root:/sys/bus/coresight/devices# cat etb0/enable_sink
     1
     root:/sys/bus/coresight/devices#
 
@@ -360,10 +361,10 @@ comparator with "_stext" and "_etext", essentially tracing any instruction
 that falls within that range.  As such "enabling" a source will immediately
 trigger a trace capture::
 
-    root:/sys/bus/coresight/devices# echo 1 > 2201c000.ptm/enable_source
-    root:/sys/bus/coresight/devices# cat 2201c000.ptm/enable_source
+    root:/sys/bus/coresight/devices# echo 1 > ptm0/enable_source
+    root:/sys/bus/coresight/devices# cat ptm0/enable_source
     1
-    root:/sys/bus/coresight/devices# cat 20010000.etb/status
+    root:/sys/bus/coresight/devices# cat etb0/status
     Depth:          0x2000
     Status:         0x1
     RAM read ptr:   0x0
@@ -376,13 +377,22 @@ trigger a trace capture::
 
 Trace collection is stopped the same way::
 
-    root:/sys/bus/coresight/devices# echo 0 > 2201c000.ptm/enable_source
+    root:/sys/bus/coresight/devices# echo 0 > ptm0/enable_source
     root:/sys/bus/coresight/devices#
 
+If no sink is enabled before the source is enabled, then a default sink will
+be selected and enabled automatically. Once the source is disabled, then the
+sink used can be read from <source>/last_sink.::
+
+    root:/sys/bus/coresight/devices# echo 1 > ptm0/enable_source
+    root:/sys/bus/coresight/devices# echo 0 > ptm0/enable_source
+    root:/sys/bus/coresight/devices# cat ptm0/last_sink
+    etb0
+    root:/sys/bus/coresight/devices# echo 0 > etb0/enable_sink
+
 The content of the ETB buffer can be harvested directly from /dev::
 
-    root:/sys/bus/coresight/devices# dd if=/dev/20010000.etb \
-    of=~/cstrace.bin
+    root:/sys/bus/coresight/devices# dd if=/dev/etb0 of=~/cstrace.bin
     64+0 records in
     64+0 records out
     32768 bytes (33 kB) copied, 0.00125258 s, 26.2 MB/s
@@ -490,6 +500,16 @@ The syntax within the forward slashes '/' is important.  The '@' character
 tells the parser that a sink is about to be specified and that this is the sink
 to use for the trace session.
 
+Alternatively, if no sink name is given between the //, then the CoreSight
+system will select a default sink::
+
+	root@linaro-nano:~# perf record -e cs_etm//u --per-thread program
+
+The system selects a sink by searching connection path from the source ETM to
+any sink that is on the path.The system will prefer ETR devices over ETB/ETF,
+and where two of the same type are found, the closest to the ETM, in terms of
+connection links.
+
 More information on the above and other example on how to use Coresight with
 the perf tools can be found in the "HOWTO.md" file of the openCSD gitHub
 repository [#third]_.
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/5] coresight: sysfs: Allow select default sink on source enable.

[Mathieu Poirier]

Hi Mike,

I have applied patches 1 to 3 of this set.  Please see below for comments on
this patch.

On Tue, Jun 16, 2020 at 05:40:05PM +0100, Mike Leach wrote:
> When enabling a trace source using sysfs, allow the CoreSight system to
> auto-select a default sink if none has been enabled by the user.
> 
> Uses the sink select algorithm that uses the default select priorities
> set when sinks are registered with the system. At present this will
> prefer ETR over ETB / ETF.
> 
> Adds a new attribute 'last_sink' to source CoreSight devices. This is set
> when a source is enabled using sysfs, to the sink that the device will
> trace into. This applies for both user enabled and default enabled sinks.
> 
> Signed-off-by: Mike Leach <mike.leach@linaro.org>
> ---
>  drivers/hwtracing/coresight/coresight.c | 39 +++++++++++++++++++++++--
>  include/linux/coresight.h               |  3 ++
>  2 files changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> index e9c90f2de34a..db39e0b56994 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight.c
> @@ -934,6 +934,16 @@ static void coresight_clear_default_sink(struct coresight_device *csdev)
>  	}
>  }
>  
> +static void coresight_set_last_sink_name(struct coresight_device *source,
> +					 struct coresight_device *sink)
> +{
> +	/* remove current value and set new one if *sink not NULL */
> +	kfree(source->last_sink);
> +	source->last_sink = NULL;
> +	if (sink)
> +		source->last_sink = kstrdup(dev_name(&sink->dev), GFP_KERNEL);
> +}
> +
>  /** coresight_validate_source - make sure a source has the right credentials
>   *  @csdev:	the device structure for a source.
>   *  @function:	the function this was called from.
> @@ -994,8 +1004,15 @@ int coresight_enable(struct coresight_device *csdev)
>  	 */
>  	sink = coresight_get_enabled_sink(false);
>  	if (!sink) {
> -		ret = -EINVAL;
> -		goto out;
> +		/* look for a default sink if nothing enabled */
> +		sink = coresight_find_default_sink(csdev);
> +		if (!sink) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +		/* mark the default as enabled */
> +		sink->activated = true;
> +		dev_info(&sink->dev, "Enabled default sink.");

I'm very ambivalent about extending the automatic sink selection to the sysfs
interface, mainly because of the new sysfs entry it requires.  I find it
clunky that users don't have to specify the sink to use but have to explicitly
disable it after the trace session.  We could automatically disable the sink
after a trace session but that would break with the current sysfs heuristic
where sinks have to be explicitly enabled and disabled.

Thanks,
Mathieu 

>  	}
>  
>  	path = coresight_build_path(csdev, sink);
> @@ -1033,6 +1050,9 @@ int coresight_enable(struct coresight_device *csdev)
>  		break;
>  	}
>  
> +	/* record name of sink used for this session */
> +	coresight_set_last_sink_name(csdev, sink);
> +
>  out:
>  	mutex_unlock(&coresight_mutex);
>  	return ret;
> @@ -1145,6 +1165,19 @@ static ssize_t enable_source_store(struct device *dev,
>  }
>  static DEVICE_ATTR_RW(enable_source);
>  
> +static ssize_t last_sink_show(struct device *dev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	struct coresight_device *csdev = to_coresight_device(dev);
> +	ssize_t size = 0;
> +
> +	if (csdev->last_sink)
> +		size = scnprintf(buf, PAGE_SIZE, "%s\n", csdev->last_sink);
> +	return size;
> +}
> +static DEVICE_ATTR_RO(last_sink);
> +
> +
>  static struct attribute *coresight_sink_attrs[] = {
>  	&dev_attr_enable_sink.attr,
>  	NULL,
> @@ -1153,6 +1186,7 @@ ATTRIBUTE_GROUPS(coresight_sink);
>  
>  static struct attribute *coresight_source_attrs[] = {
>  	&dev_attr_enable_source.attr,
> +	&dev_attr_last_sink.attr,
>  	NULL,
>  };
>  ATTRIBUTE_GROUPS(coresight_source);
> @@ -1524,6 +1558,7 @@ void coresight_unregister(struct coresight_device *csdev)
>  	/* Remove references of that device in the topology */
>  	coresight_remove_conns(csdev);
>  	coresight_clear_default_sink(csdev);
> +	coresight_set_last_sink_name(csdev, NULL);
>  	coresight_release_platform_data(csdev, csdev->pdata);
>  	device_unregister(&csdev->dev);
>  }
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index 58fffdecdbfd..fc320dd2cedc 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -184,6 +184,8 @@ struct coresight_sysfs_link {
>   *		from source to that sink.
>   * @ea:		Device attribute for sink representation under PMU directory.
>   * @def_sink:	cached reference to default sink found for this device.
> + * @last_sink:	Name of last sink used for this source to trace into. Set when
> + *		enabling a source using sysfs - only set on a source device.
>   * @ect_dev:	Associated cross trigger device. Not part of the trace data
>   *		path or connections.
>   * @nr_links:   number of sysfs links created to other components from this
> @@ -203,6 +205,7 @@ struct coresight_device {
>  	bool activated;	/* true only if a sink is part of a path */
>  	struct dev_ext_attribute *ea;
>  	struct coresight_device *def_sink;
> +	const char *last_sink;
>  	/* cross trigger handling */
>  	struct coresight_device *ect_dev;
>  	/* sysfs links between components */
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/5] coresight: sysfs: Allow select default sink on source enable.

[Mike Leach]

Hi Mathieu,

On Mon, 29 Jun 2020 at 18:47, Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
>
> Hi Mike,
>
> I have applied patches 1 to 3 of this set.  Please see below for comments on
> this patch.
>
> On Tue, Jun 16, 2020 at 05:40:05PM +0100, Mike Leach wrote:
> > When enabling a trace source using sysfs, allow the CoreSight system to
> > auto-select a default sink if none has been enabled by the user.
> >
> > Uses the sink select algorithm that uses the default select priorities
> > set when sinks are registered with the system. At present this will
> > prefer ETR over ETB / ETF.
> >
> > Adds a new attribute 'last_sink' to source CoreSight devices. This is set
> > when a source is enabled using sysfs, to the sink that the device will
> > trace into. This applies for both user enabled and default enabled sinks.
> >
> > Signed-off-by: Mike Leach <mike.leach@linaro.org>
> > ---
> >  drivers/hwtracing/coresight/coresight.c | 39 +++++++++++++++++++++++--
> >  include/linux/coresight.h               |  3 ++
> >  2 files changed, 40 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> > index e9c90f2de34a..db39e0b56994 100644
> > --- a/drivers/hwtracing/coresight/coresight.c
> > +++ b/drivers/hwtracing/coresight/coresight.c
> > @@ -934,6 +934,16 @@ static void coresight_clear_default_sink(struct coresight_device *csdev)
> >       }
> >  }
> >
> > +static void coresight_set_last_sink_name(struct coresight_device *source,
> > +                                      struct coresight_device *sink)
> > +{
> > +     /* remove current value and set new one if *sink not NULL */
> > +     kfree(source->last_sink);
> > +     source->last_sink = NULL;
> > +     if (sink)
> > +             source->last_sink = kstrdup(dev_name(&sink->dev), GFP_KERNEL);
> > +}
> > +
> >  /** coresight_validate_source - make sure a source has the right credentials
> >   *  @csdev:  the device structure for a source.
> >   *  @function:       the function this was called from.
> > @@ -994,8 +1004,15 @@ int coresight_enable(struct coresight_device *csdev)
> >        */
> >       sink = coresight_get_enabled_sink(false);
> >       if (!sink) {
> > -             ret = -EINVAL;
> > -             goto out;
> > +             /* look for a default sink if nothing enabled */
> > +             sink = coresight_find_default_sink(csdev);
> > +             if (!sink) {
> > +                     ret = -EINVAL;
> > +                     goto out;
> > +             }
> > +             /* mark the default as enabled */
> > +             sink->activated = true;
> > +             dev_info(&sink->dev, "Enabled default sink.");
>
> I'm very ambivalent about extending the automatic sink selection to the sysfs
> interface, mainly because of the new sysfs entry it requires.

That's interesting - this was added to overcome Suzuki's objection
that it wasn't possible to determine which sink was in use!

However, I think it is important to allow this as once we see systems
with many cores + many sinks, determining the correct sink to enable
becomes much more difficult.

You said yourself, albeit in relation to perf, that for 1:1 systems,
sink selection should be implicit. This is something I completely
agree with, and hence the automatic selection algorithm that was
chosen to ensure that this is the case.
Is there any reason not to make the same assertion for sysfs?

Further, this allows sysfs users to write board agnostic tests
(similar to the one Leo wrote for perf) - effectively all we need to
do to test the coresight function on a board is iterate through the
cpus / etms without worrying about the sink in use, then name of which
can be read from the etm and then data read out.

As an aside - last_sink also shows which sink was used should you
happen to explicitly enable two sinks in the etm path (e.g. etf &
etr).

>  I find it
> clunky that users don't have to specify the sink to use but have to explicitly
> disable it after the trace session.

Sure - but is it not just as clunky to have to figure out which sink
attaches to your etm in the first place? (yes there are topolgy links
now but this is not the most straighforward thing to use)
Ultimately, if you are only using sysfs, you never actually need to
disable the sink to read back data if you don't want to. I am not sure
there are many people who use both syfs and perf in the same session
to collect trace - and these are the ones who would need to be careful
about disabling the sink.

From a debug tools perspective, anything we can do to make things
easier for users - especially at so little extra cost, can only be a
good thing.

> We could automatically disable the sink
> after a trace session but that would break with the current sysfs heuristic
> where sinks have to be explicitly enabled and disabled.
>

Or possibly only auto-disable if the sink was selected by default in
the first place. Another short cut would be to allow writing 0 to the
last_sink attribute to disable the named sink.


Regards

Mike




> Thanks,
> Mathieu
>
> >       }
> >
> >       path = coresight_build_path(csdev, sink);
> > @@ -1033,6 +1050,9 @@ int coresight_enable(struct coresight_device *csdev)
> >               break;
> >       }
> >
> > +     /* record name of sink used for this session */
> > +     coresight_set_last_sink_name(csdev, sink);
> > +
> >  out:
> >       mutex_unlock(&coresight_mutex);
> >       return ret;
> > @@ -1145,6 +1165,19 @@ static ssize_t enable_source_store(struct device *dev,
> >  }
> >  static DEVICE_ATTR_RW(enable_source);
> >
> > +static ssize_t last_sink_show(struct device *dev,
> > +                           struct device_attribute *attr, char *buf)
> > +{
> > +     struct coresight_device *csdev = to_coresight_device(dev);
> > +     ssize_t size = 0;
> > +
> > +     if (csdev->last_sink)
> > +             size = scnprintf(buf, PAGE_SIZE, "%s\n", csdev->last_sink);
> > +     return size;
> > +}
> > +static DEVICE_ATTR_RO(last_sink);
> > +
> > +
> >  static struct attribute *coresight_sink_attrs[] = {
> >       &dev_attr_enable_sink.attr,
> >       NULL,
> > @@ -1153,6 +1186,7 @@ ATTRIBUTE_GROUPS(coresight_sink);
> >
> >  static struct attribute *coresight_source_attrs[] = {
> >       &dev_attr_enable_source.attr,
> > +     &dev_attr_last_sink.attr,
> >       NULL,
> >  };
> >  ATTRIBUTE_GROUPS(coresight_source);
> > @@ -1524,6 +1558,7 @@ void coresight_unregister(struct coresight_device *csdev)
> >       /* Remove references of that device in the topology */
> >       coresight_remove_conns(csdev);
> >       coresight_clear_default_sink(csdev);
> > +     coresight_set_last_sink_name(csdev, NULL);
> >       coresight_release_platform_data(csdev, csdev->pdata);
> >       device_unregister(&csdev->dev);
> >  }
> > diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> > index 58fffdecdbfd..fc320dd2cedc 100644
> > --- a/include/linux/coresight.h
> > +++ b/include/linux/coresight.h
> > @@ -184,6 +184,8 @@ struct coresight_sysfs_link {
> >   *           from source to that sink.
> >   * @ea:              Device attribute for sink representation under PMU directory.
> >   * @def_sink:        cached reference to default sink found for this device.
> > + * @last_sink:       Name of last sink used for this source to trace into. Set when
> > + *           enabling a source using sysfs - only set on a source device.
> >   * @ect_dev: Associated cross trigger device. Not part of the trace data
> >   *           path or connections.
> >   * @nr_links:   number of sysfs links created to other components from this
> > @@ -203,6 +205,7 @@ struct coresight_device {
> >       bool activated; /* true only if a sink is part of a path */
> >       struct dev_ext_attribute *ea;
> >       struct coresight_device *def_sink;
> > +     const char *last_sink;
> >       /* cross trigger handling */
> >       struct coresight_device *ect_dev;
> >       /* sysfs links between components */
> > --
> > 2.17.1
> >



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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/5] coresight: sysfs: Allow select default sink on source enable.

[Suzuki K Poulose]

Hi Mike, Mathieu,

On 07/01/2020 05:40 PM, Mike Leach wrote:
> Hi Mathieu,
> 
> On Mon, 29 Jun 2020 at 18:47, Mathieu Poirier
> <mathieu.poirier@linaro.org> wrote:
>>
>> Hi Mike,
>>
>> I have applied patches 1 to 3 of this set.  Please see below for comments on
>> this patch.
>>
>> On Tue, Jun 16, 2020 at 05:40:05PM +0100, Mike Leach wrote:
>>> When enabling a trace source using sysfs, allow the CoreSight system to
>>> auto-select a default sink if none has been enabled by the user.
>>>
>>> Uses the sink select algorithm that uses the default select priorities
>>> set when sinks are registered with the system. At present this will
>>> prefer ETR over ETB / ETF.
>>>
>>> Adds a new attribute 'last_sink' to source CoreSight devices. This is set
>>> when a source is enabled using sysfs, to the sink that the device will
>>> trace into. This applies for both user enabled and default enabled sinks.
>>>
>>> Signed-off-by: Mike Leach <mike.leach@linaro.org>
>>> ---
>>>   drivers/hwtracing/coresight/coresight.c | 39 +++++++++++++++++++++++--
>>>   include/linux/coresight.h               |  3 ++
>>>   2 files changed, 40 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
>>> index e9c90f2de34a..db39e0b56994 100644
>>> --- a/drivers/hwtracing/coresight/coresight.c
>>> +++ b/drivers/hwtracing/coresight/coresight.c
>>> @@ -934,6 +934,16 @@ static void coresight_clear_default_sink(struct coresight_device *csdev)
>>>        }
>>>   }
>>>
>>> +static void coresight_set_last_sink_name(struct coresight_device *source,
>>> +                                      struct coresight_device *sink)
>>> +{
>>> +     /* remove current value and set new one if *sink not NULL */
>>> +     kfree(source->last_sink);
>>> +     source->last_sink = NULL;
>>> +     if (sink)
>>> +             source->last_sink = kstrdup(dev_name(&sink->dev), GFP_KERNEL);
>>> +}
>>> +
>>>   /** coresight_validate_source - make sure a source has the right credentials
>>>    *  @csdev:  the device structure for a source.
>>>    *  @function:       the function this was called from.
>>> @@ -994,8 +1004,15 @@ int coresight_enable(struct coresight_device *csdev)
>>>         */
>>>        sink = coresight_get_enabled_sink(false);
>>>        if (!sink) {
>>> -             ret = -EINVAL;
>>> -             goto out;
>>> +             /* look for a default sink if nothing enabled */
>>> +             sink = coresight_find_default_sink(csdev);
>>> +             if (!sink) {
>>> +                     ret = -EINVAL;
>>> +                     goto out;
>>> +             }
>>> +             /* mark the default as enabled */
>>> +             sink->activated = true;
>>> +             dev_info(&sink->dev, "Enabled default sink.");
>>
>> I'm very ambivalent about extending the automatic sink selection to the sysfs
>> interface, mainly because of the new sysfs entry it requires.
> 
> That's interesting - this was added to overcome Suzuki's objection
> that it wasn't possible to determine which sink was in use!

I personally don't prefer the auto selection for sysfs mode. And that
was one of the arguments to support it.

> 
> However, I think it is important to allow this as once we see systems
> with many cores + many sinks, determining the correct sink to enable
> becomes much more difficult.
> 
> You said yourself, albeit in relation to perf, that for 1:1 systems,
> sink selection should be implicit. This is something I completely
> agree with, and hence the automatic selection algorithm that was
> chosen to ensure that this is the case.
> Is there any reason not to make the same assertion for sysfs?
> 
> Further, this allows sysfs users to write board agnostic tests
> (similar to the one Leo wrote for perf) - effectively all we need to
> do to test the coresight function on a board is iterate through the
> cpus / etms without worrying about the sink in use, then name of which
> can be read from the etm and then data read out.

The tests could use the "connections" exposed via the sysfs to figure
out the appropriate sink for a given source.

> 
> As an aside - last_sink also shows which sink was used should you
> happen to explicitly enable two sinks in the etm path (e.g. etf &
> etr).
> 
>>   I find it
>> clunky that users don't have to specify the sink to use but have to explicitly
>> disable it after the trace session.
> 
> Sure - but is it not just as clunky to have to figure out which sink
> attaches to your etm in the first place? (yes there are topolgy links
> now but this is not the most straighforward thing to use)
> Ultimately, if you are only using sysfs, you never actually need to
> disable the sink to read back data if you don't want to. I am not sure
> there are many people who use both syfs and perf in the same session
> to collect trace - and these are the ones who would need to be careful
> about disabling the sink.

The problem lies exactly there. Just like we don't know how many actual
sysfs mode users are there, who consume the trace data and use it in a 
production environment compared to a bring up situation (verifying
that the board topology is detected fine and the components are working
fine), there could be users of the perf on these systems.

Debugging such cases where someone forgot to disable the trace can be
a painful process. Like I have said from the beginning, this is not
worth the benefit that we get from this code (i.e, figuring out which
sink is closer to a source in sysfs mode, when there is an existing
infrastructure, i.e, "connections" already available for this).

Cheers
Suzuki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 1/5] coresight: Add default sink selection to CoreSight base.

[Suzuki K Poulose]

Hi Mike,

On 06/16/2020 05:40 PM, Mike Leach wrote:
> Adds a method to select a suitable sink connected to a given source.
> 
> In cases where no sink is defined, the coresight_find_default_sink
> routine can search from a given source, through the child connections
> until a suitable sink is found.
> 
> The suitability is defined in by the sink coresight_dev_subtype on the
> CoreSight device, and the distance from the source by counting
> connections.
> 
> Higher value subtype is preferred - where these are equal, shorter
> distance from source is used as a tie-break.
> 
> This allows for default sink to be discovered were none is specified
> (e.g. perf command line)
> 
> Signed-off-by: Mike Leach <mike.leach@linaro.org>
> Suggested-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Reviewed-by: Leo Yan <leo.yan@linaro.org>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 3/5] coresight: etm: perf: Add default sink selection to etm perf.

[Suzuki K Poulose]

On 06/16/2020 05:40 PM, Mike Leach wrote:
> Add default sink selection to the perf trace handling in the etm driver.
> Uses the select default sink infrastructure to select a sink for the perf
> session, if no other sink is specified.
> 
> Signed-off-by: Mike Leach <mike.leach@linaro.org>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/5] coresight: sysfs: Allow select default sink on source enable.

[Mike Leach]

Hi Suzuki,

On Wed, 1 Jul 2020 at 23:19, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>
> Hi Mike, Mathieu,
>
> On 07/01/2020 05:40 PM, Mike Leach wrote:
> > Hi Mathieu,
> >
> > On Mon, 29 Jun 2020 at 18:47, Mathieu Poirier
> > <mathieu.poirier@linaro.org> wrote:
> >>
> >> Hi Mike,
> >>
> >> I have applied patches 1 to 3 of this set.  Please see below for comments on
> >> this patch.
> >>
> >> On Tue, Jun 16, 2020 at 05:40:05PM +0100, Mike Leach wrote:
> >>> When enabling a trace source using sysfs, allow the CoreSight system to
> >>> auto-select a default sink if none has been enabled by the user.
> >>>
> >>> Uses the sink select algorithm that uses the default select priorities
> >>> set when sinks are registered with the system. At present this will
> >>> prefer ETR over ETB / ETF.
> >>>
> >>> Adds a new attribute 'last_sink' to source CoreSight devices. This is set
> >>> when a source is enabled using sysfs, to the sink that the device will
> >>> trace into. This applies for both user enabled and default enabled sinks.
> >>>
> >>> Signed-off-by: Mike Leach <mike.leach@linaro.org>
> >>> ---
> >>>   drivers/hwtracing/coresight/coresight.c | 39 +++++++++++++++++++++++--
> >>>   include/linux/coresight.h               |  3 ++
> >>>   2 files changed, 40 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> >>> index e9c90f2de34a..db39e0b56994 100644
> >>> --- a/drivers/hwtracing/coresight/coresight.c
> >>> +++ b/drivers/hwtracing/coresight/coresight.c
> >>> @@ -934,6 +934,16 @@ static void coresight_clear_default_sink(struct coresight_device *csdev)
> >>>        }
> >>>   }
> >>>
> >>> +static void coresight_set_last_sink_name(struct coresight_device *source,
> >>> +                                      struct coresight_device *sink)
> >>> +{
> >>> +     /* remove current value and set new one if *sink not NULL */
> >>> +     kfree(source->last_sink);
> >>> +     source->last_sink = NULL;
> >>> +     if (sink)
> >>> +             source->last_sink = kstrdup(dev_name(&sink->dev), GFP_KERNEL);
> >>> +}
> >>> +
> >>>   /** coresight_validate_source - make sure a source has the right credentials
> >>>    *  @csdev:  the device structure for a source.
> >>>    *  @function:       the function this was called from.
> >>> @@ -994,8 +1004,15 @@ int coresight_enable(struct coresight_device *csdev)
> >>>         */
> >>>        sink = coresight_get_enabled_sink(false);
> >>>        if (!sink) {
> >>> -             ret = -EINVAL;
> >>> -             goto out;
> >>> +             /* look for a default sink if nothing enabled */
> >>> +             sink = coresight_find_default_sink(csdev);
> >>> +             if (!sink) {
> >>> +                     ret = -EINVAL;
> >>> +                     goto out;
> >>> +             }
> >>> +             /* mark the default as enabled */
> >>> +             sink->activated = true;
> >>> +             dev_info(&sink->dev, "Enabled default sink.");
> >>
> >> I'm very ambivalent about extending the automatic sink selection to the sysfs
> >> interface, mainly because of the new sysfs entry it requires.
> >
> > That's interesting - this was added to overcome Suzuki's objection
> > that it wasn't possible to determine which sink was in use!
>
> I personally don't prefer the auto selection for sysfs mode. And that
> was one of the arguments to support it.
>
> >
> > However, I think it is important to allow this as once we see systems
> > with many cores + many sinks, determining the correct sink to enable
> > becomes much more difficult.
> >
> > You said yourself, albeit in relation to perf, that for 1:1 systems,
> > sink selection should be implicit. This is something I completely
> > agree with, and hence the automatic selection algorithm that was
> > chosen to ensure that this is the case.
> > Is there any reason not to make the same assertion for sysfs?
> >
> > Further, this allows sysfs users to write board agnostic tests
> > (similar to the one Leo wrote for perf) - effectively all we need to
> > do to test the coresight function on a board is iterate through the
> > cpus / etms without worrying about the sink in use, then name of which
> > can be read from the etm and then data read out.
>
> The tests could use the "connections" exposed via the sysfs to figure
> out the appropriate sink for a given source.
>
> >
> > As an aside - last_sink also shows which sink was used should you
> > happen to explicitly enable two sinks in the etm path (e.g. etf &
> > etr).
> >
> >>   I find it
> >> clunky that users don't have to specify the sink to use but have to explicitly
> >> disable it after the trace session.
> >
> > Sure - but is it not just as clunky to have to figure out which sink
> > attaches to your etm in the first place? (yes there are topolgy links
> > now but this is not the most straighforward thing to use)
> > Ultimately, if you are only using sysfs, you never actually need to
> > disable the sink to read back data if you don't want to. I am not sure
> > there are many people who use both syfs and perf in the same session
> > to collect trace - and these are the ones who would need to be careful
> > about disabling the sink.
>
> The problem lies exactly there. Just like we don't know how many actual
> sysfs mode users are there, who consume the trace data and use it in a
> production environment compared to a bring up situation (verifying
> that the board topology is detected fine and the components are working
> fine), there could be users of the perf on these systems.
>

This is an issue irrespective of how the trace sink is turned on, be
it automatically or explicitly.
Given that it is possible to read the sink data without disabling the
sink - the chances are it could happen either way.

> Debugging such cases where someone forgot to disable the trace can be
> a painful process. Like I have said from the beginning, this is not
> worth the benefit that we get from this code (i.e, figuring out which
> sink is closer to a source in sysfs mode, when there is an existing
> infrastructure, i.e, "connections" already available for this).
>

Actually all connections can tell you is the number sinks available to
the etm on the path - not which would be selected by the current
priority algorithm - unless the user is willing to dig into the driver
source code and figure out the priority mechanism.

Regards

Mike

> Cheers
> Suzuki



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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/5] coresight: sysfs: Allow select default sink on source enable.

[Suzuki K Poulose]

On 07/02/2020 01:21 AM, Mike Leach wrote:
> Hi Suzuki,
> 
> On Wed, 1 Jul 2020 at 23:19, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>>
>> Hi Mike, Mathieu,
>>
>> On 07/01/2020 05:40 PM, Mike Leach wrote:
>>> Hi Mathieu,
>>>
>>> On Mon, 29 Jun 2020 at 18:47, Mathieu Poirier
>>> <mathieu.poirier@linaro.org> wrote:
>>>>
>>>> Hi Mike,
>>>>
>>>> I have applied patches 1 to 3 of this set.  Please see below for comments on
>>>> this patch.
>>>>
>>>> On Tue, Jun 16, 2020 at 05:40:05PM +0100, Mike Leach wrote:
>>>>> When enabling a trace source using sysfs, allow the CoreSight system to
>>>>> auto-select a default sink if none has been enabled by the user.
>>>>>
>>>>> Uses the sink select algorithm that uses the default select priorities
>>>>> set when sinks are registered with the system. At present this will
>>>>> prefer ETR over ETB / ETF.
>>>>>
>>>>> Adds a new attribute 'last_sink' to source CoreSight devices. This is set
>>>>> when a source is enabled using sysfs, to the sink that the device will
>>>>> trace into. This applies for both user enabled and default enabled sinks.
>>>>>
>>>>> Signed-off-by: Mike Leach <mike.leach@linaro.org>
>>>>> ---
>>>>>    drivers/hwtracing/coresight/coresight.c | 39 +++++++++++++++++++++++--
>>>>>    include/linux/coresight.h               |  3 ++
>>>>>    2 files changed, 40 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
>>>>> index e9c90f2de34a..db39e0b56994 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight.c
>>>>> +++ b/drivers/hwtracing/coresight/coresight.c
>>>>> @@ -934,6 +934,16 @@ static void coresight_clear_default_sink(struct coresight_device *csdev)
>>>>>         }
>>>>>    }
>>>>>
>>>>> +static void coresight_set_last_sink_name(struct coresight_device *source,
>>>>> +                                      struct coresight_device *sink)
>>>>> +{
>>>>> +     /* remove current value and set new one if *sink not NULL */
>>>>> +     kfree(source->last_sink);
>>>>> +     source->last_sink = NULL;
>>>>> +     if (sink)
>>>>> +             source->last_sink = kstrdup(dev_name(&sink->dev), GFP_KERNEL);
>>>>> +}
>>>>> +
>>>>>    /** coresight_validate_source - make sure a source has the right credentials
>>>>>     *  @csdev:  the device structure for a source.
>>>>>     *  @function:       the function this was called from.
>>>>> @@ -994,8 +1004,15 @@ int coresight_enable(struct coresight_device *csdev)
>>>>>          */
>>>>>         sink = coresight_get_enabled_sink(false);
>>>>>         if (!sink) {
>>>>> -             ret = -EINVAL;
>>>>> -             goto out;
>>>>> +             /* look for a default sink if nothing enabled */
>>>>> +             sink = coresight_find_default_sink(csdev);
>>>>> +             if (!sink) {
>>>>> +                     ret = -EINVAL;
>>>>> +                     goto out;
>>>>> +             }
>>>>> +             /* mark the default as enabled */
>>>>> +             sink->activated = true;
>>>>> +             dev_info(&sink->dev, "Enabled default sink.");
>>>>
>>>> I'm very ambivalent about extending the automatic sink selection to the sysfs
>>>> interface, mainly because of the new sysfs entry it requires.
>>>
>>> That's interesting - this was added to overcome Suzuki's objection
>>> that it wasn't possible to determine which sink was in use!
>>
>> I personally don't prefer the auto selection for sysfs mode. And that
>> was one of the arguments to support it.
>>
>>>
>>> However, I think it is important to allow this as once we see systems
>>> with many cores + many sinks, determining the correct sink to enable
>>> becomes much more difficult.
>>>
>>> You said yourself, albeit in relation to perf, that for 1:1 systems,
>>> sink selection should be implicit. This is something I completely
>>> agree with, and hence the automatic selection algorithm that was
>>> chosen to ensure that this is the case.
>>> Is there any reason not to make the same assertion for sysfs?
>>>
>>> Further, this allows sysfs users to write board agnostic tests
>>> (similar to the one Leo wrote for perf) - effectively all we need to
>>> do to test the coresight function on a board is iterate through the
>>> cpus / etms without worrying about the sink in use, then name of which
>>> can be read from the etm and then data read out.
>>
>> The tests could use the "connections" exposed via the sysfs to figure
>> out the appropriate sink for a given source.
>>
>>>
>>> As an aside - last_sink also shows which sink was used should you
>>> happen to explicitly enable two sinks in the etm path (e.g. etf &
>>> etr).
>>>
>>>>    I find it
>>>> clunky that users don't have to specify the sink to use but have to explicitly
>>>> disable it after the trace session.
>>>
>>> Sure - but is it not just as clunky to have to figure out which sink
>>> attaches to your etm in the first place? (yes there are topolgy links
>>> now but this is not the most straighforward thing to use)
>>> Ultimately, if you are only using sysfs, you never actually need to
>>> disable the sink to read back data if you don't want to. I am not sure
>>> there are many people who use both syfs and perf in the same session
>>> to collect trace - and these are the ones who would need to be careful
>>> about disabling the sink.
>>
>> The problem lies exactly there. Just like we don't know how many actual
>> sysfs mode users are there, who consume the trace data and use it in a
>> production environment compared to a bring up situation (verifying
>> that the board topology is detected fine and the components are working
>> fine), there could be users of the perf on these systems.
>>
> 
> This is an issue irrespective of how the trace sink is turned on, be
> it automatically or explicitly.
> Given that it is possible to read the sink data without disabling the
> sink - the chances are it could happen either way.
> 
>> Debugging such cases where someone forgot to disable the trace can be
>> a painful process. Like I have said from the beginning, this is not
>> worth the benefit that we get from this code (i.e, figuring out which
>> sink is closer to a source in sysfs mode, when there is an existing
>> infrastructure, i.e, "connections" already available for this).
>>
> 
> Actually all connections can tell you is the number sinks available to
> the etm on the path - not which would be selected by the current
> priority algorithm - unless the user is willing to dig into the driver
> source code and figure out the priority mechanism.

Exactly. My point is, don't do this for sysfs mode.
The user can figure this out for sysfs mode, if he/she wanted to (unlike 
the perf mode, where the event could be placed on any CPU). We
don't have to add this fragile change just because the user
don't want to do this himself, when he must know what was used for 
collecting the trace back (again, something the perf mode doesn't have
to worry about and is justifying the use there). In other words, the
change for sysfs mode is not justified enough.

Cheers
Suzuki


> 
> Regards
> 
> Mike
> 
>> Cheers
>> Suzuki
> 
> 
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel