Coresight driver assumes sink is common across all the ETMs, and tries to build a path between ETM and the first enabled sink found using bus based search. This breaks sysFS usage on implementations that has multiple per core sinks in enabled state.
For this, - coresight_find_sink API is updated with an additional flag so that it is able to return an enabled sink - coresight_get_enabled_sink API is updated to do a connection based search, when a source reference is given.
Signed-off-by: Linu Cherian lcherian@marvell.com --- Applies on https://git.linaro.org/kernel/coresight.git/log/?h=next
Changes in V3: Fixed checkpatch issue.
.../hwtracing/coresight/coresight-etm-perf.c | 2 +- drivers/hwtracing/coresight/coresight-priv.h | 5 +- drivers/hwtracing/coresight/coresight.c | 51 +++++++++++++++++-- 3 files changed, 51 insertions(+), 7 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 1a3169e69bb1..25041d2654e3 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -223,7 +223,7 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, id = (u32)event->attr.config2; sink = coresight_get_sink_by_id(id); } else { - sink = coresight_get_enabled_sink(true); + sink = coresight_get_enabled_sink(NULL, true); }
mask = &event_data->mask; diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h index f2dc625ea585..010ed26db340 100644 --- a/drivers/hwtracing/coresight/coresight-priv.h +++ b/drivers/hwtracing/coresight/coresight-priv.h @@ -148,10 +148,13 @@ 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, 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_enabled_sink(struct coresight_device *source, bool reset); struct coresight_device *coresight_get_sink_by_id(u32 id); struct coresight_device * coresight_find_default_sink(struct coresight_device *csdev); +struct coresight_device * +coresight_find_enabled_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 e9c90f2de34a..ae69169c58d3 100644 --- a/drivers/hwtracing/coresight/coresight.c +++ b/drivers/hwtracing/coresight/coresight.c @@ -566,6 +566,10 @@ static int coresight_enabled_sink(struct device *dev, const void *data)
/** * coresight_get_enabled_sink - returns the first enabled sink found on the bus + * When a source reference is given, enabled sink is found using connection based + * search. + * + * @source: Coresight source device reference * @deactivate: Whether the 'enable_sink' flag should be reset * * When operated from perf the deactivate parameter should be set to 'true'. @@ -576,10 +580,21 @@ static int coresight_enabled_sink(struct device *dev, const void *data) * parameter should be set to 'false', hence mandating users to explicitly * clear the flag. */ -struct coresight_device *coresight_get_enabled_sink(bool deactivate) +struct coresight_device * +coresight_get_enabled_sink(struct coresight_device *source, bool deactivate) { struct device *dev = NULL; + struct coresight_device *sink; + + if (!source) + goto bus_search; + sink = coresight_find_enabled_sink(source); + if (sink && deactivate) + sink->activated = false; + + return sink;
+bus_search: dev = bus_find_device(&coresight_bustype, NULL, &deactivate, coresight_enabled_sink);
@@ -828,6 +843,7 @@ coresight_select_best_sink(struct coresight_device *sink, int *depth, * * @csdev: source / current device to check. * @depth: [in] search depth of calling dev, [out] depth of found sink. + * @enabled: flag to search only enabled sinks * * This will walk the connection path from a source (ETM) till a suitable * sink is encountered and return that sink to the original caller. @@ -839,7 +855,7 @@ coresight_select_best_sink(struct coresight_device *sink, int *depth, * 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) +coresight_find_sink(struct coresight_device *csdev, int *depth, bool enabled) { int i, curr_depth = *depth + 1, found_depth = 0; struct coresight_device *found_sink = NULL; @@ -862,7 +878,8 @@ coresight_find_sink(struct coresight_device *csdev, int *depth)
child_dev = csdev->pdata->conns[i].child_dev; if (child_dev) - sink = coresight_find_sink(child_dev, &child_depth); + sink = coresight_find_sink(child_dev, &child_depth, + enabled);
if (sink) found_sink = coresight_select_best_sink(found_sink, @@ -872,6 +889,10 @@ coresight_find_sink(struct coresight_device *csdev, int *depth) }
return_def_sink: + /* Check if we need to return an enabled sink */ + if (enabled && found_sink) + if (!found_sink->activated) + found_sink = NULL; /* return found sink and depth */ if (found_sink) *depth = found_depth; @@ -901,10 +922,30 @@ coresight_find_default_sink(struct coresight_device *csdev)
/* 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); + csdev->def_sink = coresight_find_sink(csdev, &depth, false); return csdev->def_sink; }
+/** + * coresight_find_enabled_sink: Find the suitable enabled 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. + * + * Used in cases where the CoreSight user (sysfs) has selected a sink. + */ +struct coresight_device * +coresight_find_enabled_sink(struct coresight_device *csdev) +{ + int depth = 0; + + /* look for the enabled sink */ + return coresight_find_sink(csdev, &depth, true); +} + static int coresight_remove_sink_ref(struct device *dev, void *data) { struct coresight_device *sink = data; @@ -992,7 +1033,7 @@ int coresight_enable(struct coresight_device *csdev) * Search for a valid sink for this session but don't reset the * "enable_sink" flag in sysFS. Users get to do that explicitly. */ - sink = coresight_get_enabled_sink(false); + sink = coresight_get_enabled_sink(csdev, false); if (!sink) { ret = -EINVAL; goto out;
Hi Linu,
On Tue, Aug 11, 2020 at 07:58:42AM +0530, Linu Cherian wrote:
Coresight driver assumes sink is common across all the ETMs, and tries to build a path between ETM and the first enabled sink found using bus based search. This breaks sysFS usage on implementations that has multiple per core sinks in enabled state.
For this,
- coresight_find_sink API is updated with an additional flag so that it is able to return an enabled sink
- coresight_get_enabled_sink API is updated to do a connection based search, when a source reference is given.
Signed-off-by: Linu Cherian lcherian@marvell.com
Applies on https://git.linaro.org/kernel/coresight.git/log/?h=next
Changes in V3: Fixed checkpatch issue.
.../hwtracing/coresight/coresight-etm-perf.c | 2 +- drivers/hwtracing/coresight/coresight-priv.h | 5 +- drivers/hwtracing/coresight/coresight.c | 51 +++++++++++++++++-- 3 files changed, 51 insertions(+), 7 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 1a3169e69bb1..25041d2654e3 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -223,7 +223,7 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, id = (u32)event->attr.config2; sink = coresight_get_sink_by_id(id); } else {
sink = coresight_get_enabled_sink(true);
sink = coresight_get_enabled_sink(NULL, true);
It is time for this subsystem to move out of the prehistoric age. Please remove the call to coresight_get_enabled_sink() entirely. In the change log you can write that selecting a sink from sysfs is deprecated when using the perf interface. I will personally refactor the code if someone complains that it broke their user space.
} mask = &event_data->mask; diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h index f2dc625ea585..010ed26db340 100644 --- a/drivers/hwtracing/coresight/coresight-priv.h +++ b/drivers/hwtracing/coresight/coresight-priv.h @@ -148,10 +148,13 @@ 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, 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_enabled_sink(struct coresight_device *source, bool reset); struct coresight_device *coresight_get_sink_by_id(u32 id); struct coresight_device * coresight_find_default_sink(struct coresight_device *csdev); +struct coresight_device * +coresight_find_enabled_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 e9c90f2de34a..ae69169c58d3 100644 --- a/drivers/hwtracing/coresight/coresight.c +++ b/drivers/hwtracing/coresight/coresight.c @@ -566,6 +566,10 @@ static int coresight_enabled_sink(struct device *dev, const void *data) /**
- coresight_get_enabled_sink - returns the first enabled sink found on the bus
- When a source reference is given, enabled sink is found using connection based
- search.
- @source: Coresight source device reference
- @deactivate: Whether the 'enable_sink' flag should be reset
- When operated from perf the deactivate parameter should be set to 'true'.
@@ -576,10 +580,21 @@ static int coresight_enabled_sink(struct device *dev, const void *data)
- parameter should be set to 'false', hence mandating users to explicitly
- clear the flag.
*/ -struct coresight_device *coresight_get_enabled_sink(bool deactivate) +struct coresight_device * +coresight_get_enabled_sink(struct coresight_device *source, bool deactivate)
This code was written during a time when all sources were able to reach a sink, something that is no longer the case. Starting from a source is the right way to go.
{ struct device *dev = NULL;
- struct coresight_device *sink;
- if (!source)
goto bus_search;
Return NULL if a source was not given.
- sink = coresight_find_enabled_sink(source);
- if (sink && deactivate)
sink->activated = false;- return sink;
You get credits for trying to re-use code but in this case I would like to avoid modifying coresight_find_sink() to keep it as simple as possible. I suggest to copy the for() loop in coresight_find_sink() and stop the search when the first activated sink is found. It will introduce a little bit of code duplication but I think we will gain a lot in maintainability.
+bus_search: dev = bus_find_device(&coresight_bustype, NULL, &deactivate, coresight_enabled_sink);
Get rid of this. You can also get rid of the 'deactivate' parameter since it won't be used anymore.
Let me know if you have questions.
Thanks, Mathieu
@@ -828,6 +843,7 @@ coresight_select_best_sink(struct coresight_device *sink, int *depth,
- @csdev: source / current device to check.
- @depth: [in] search depth of calling dev, [out] depth of found sink.
- @enabled: flag to search only enabled sinks
- This will walk the connection path from a source (ETM) till a suitable
- sink is encountered and return that sink to the original caller.
@@ -839,7 +855,7 @@ coresight_select_best_sink(struct coresight_device *sink, int *depth,
- 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) +coresight_find_sink(struct coresight_device *csdev, int *depth, bool enabled) { int i, curr_depth = *depth + 1, found_depth = 0; struct coresight_device *found_sink = NULL; @@ -862,7 +878,8 @@ coresight_find_sink(struct coresight_device *csdev, int *depth) child_dev = csdev->pdata->conns[i].child_dev; if (child_dev)
sink = coresight_find_sink(child_dev, &child_depth);
sink = coresight_find_sink(child_dev, &child_depth,enabled);if (sink) found_sink = coresight_select_best_sink(found_sink, @@ -872,6 +889,10 @@ coresight_find_sink(struct coresight_device *csdev, int *depth) } return_def_sink:
- /* Check if we need to return an enabled sink */
- if (enabled && found_sink)
if (!found_sink->activated)found_sink = NULL;@@ -901,10 +922,30 @@ coresight_find_default_sink(struct coresight_device *csdev) /* 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);
csdev->def_sink = coresight_find_sink(csdev, &depth, false);} +/**
- coresight_find_enabled_sink: Find the suitable enabled 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.
- Used in cases where the CoreSight user (sysfs) has selected a sink.
- */
+struct coresight_device * +coresight_find_enabled_sink(struct coresight_device *csdev) +{
- int depth = 0;
- /* look for the enabled sink */
- return coresight_find_sink(csdev, &depth, true);
+}
static int coresight_remove_sink_ref(struct device *dev, void *data) { struct coresight_device *sink = data; @@ -992,7 +1033,7 @@ int coresight_enable(struct coresight_device *csdev) * Search for a valid sink for this session but don't reset the * "enable_sink" flag in sysFS. Users get to do that explicitly. */
- sink = coresight_get_enabled_sink(false);
- sink = coresight_get_enabled_sink(csdev, false); if (!sink) { ret = -EINVAL; goto out;
-- 2.25.1
Hi Mathieu,
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 1a3169e69bb1..25041d2654e3 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -223,7 +223,7 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, id = (u32)event->attr.config2; sink = coresight_get_sink_by_id(id); } else {
sink = coresight_get_enabled_sink(true);
sink = coresight_get_enabled_sink(NULL, true);It is time for this subsystem to move out of the prehistoric age. Please remove the call to coresight_get_enabled_sink() entirely. In the change log you can write that selecting a sink from sysfs is deprecated when using the perf interface. I will personally refactor the code if someone complains that it broke their user space.
Okay fine.
} mask = &event_data->mask; diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h index f2dc625ea585..010ed26db340 100644 --- a/drivers/hwtracing/coresight/coresight-priv.h +++ b/drivers/hwtracing/coresight/coresight-priv.h @@ -148,10 +148,13 @@ 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, 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_enabled_sink(struct coresight_device *source, bool reset); struct coresight_device *coresight_get_sink_by_id(u32 id); struct coresight_device * coresight_find_default_sink(struct coresight_device *csdev); +struct coresight_device * +coresight_find_enabled_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 e9c90f2de34a..ae69169c58d3 100644 --- a/drivers/hwtracing/coresight/coresight.c +++ b/drivers/hwtracing/coresight/coresight.c @@ -566,6 +566,10 @@ static int coresight_enabled_sink(struct device *dev, const void *data) /**
- coresight_get_enabled_sink - returns the first enabled sink found on the bus
- When a source reference is given, enabled sink is found using connection based
- search.
- @source: Coresight source device reference
- @deactivate: Whether the 'enable_sink' flag should be reset
- When operated from perf the deactivate parameter should be set to 'true'.
@@ -576,10 +580,21 @@ static int coresight_enabled_sink(struct device *dev, const void *data)
- parameter should be set to 'false', hence mandating users to explicitly
- clear the flag.
*/ -struct coresight_device *coresight_get_enabled_sink(bool deactivate) +struct coresight_device * +coresight_get_enabled_sink(struct coresight_device *source, bool deactivate)
This code was written during a time when all sources were able to reach a sink, something that is no longer the case. Starting from a source is the right way to go.
{ struct device *dev = NULL;
- struct coresight_device *sink;
- if (!source)
goto bus_search;Return NULL if a source was not given.
Agree, since we are removing invocations from perf.
- sink = coresight_find_enabled_sink(source);
- if (sink && deactivate)
sink->activated = false;- return sink;
You get credits for trying to re-use code but in this case I would like to avoid modifying coresight_find_sink() to keep it as simple as possible. I suggest to copy the for() loop in coresight_find_sink() and stop the search when the first activated sink is found. It will introduce a little bit of code duplication but I think we will gain a lot in maintainability.
Fine.
+bus_search: dev = bus_find_device(&coresight_bustype, NULL, &deactivate, coresight_enabled_sink);
Get rid of this. You can also get rid of the 'deactivate' parameter since it won't be used anymore.
Agree, since perf doesnt require this anymore.
Let me know if you have questions.
Sure. Thanks.
On Sat, 22 Aug 2020 at 07:18, Linu Cherian linuc.decode@gmail.com wrote:
Hi Mathieu,
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 1a3169e69bb1..25041d2654e3 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -223,7 +223,7 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, id = (u32)event->attr.config2; sink = coresight_get_sink_by_id(id); } else {
sink = coresight_get_enabled_sink(true);
sink = coresight_get_enabled_sink(NULL, true);It is time for this subsystem to move out of the prehistoric age. Please remove the call to coresight_get_enabled_sink() entirely. In the change log you can write that selecting a sink from sysfs is deprecated when using the perf interface. I will personally refactor the code if someone complains that it broke their user space.
I forgot to mention... Please do two patches, one that removes coresight_get_enabled_sink() from etm_setup_aux() and another one for the reset of your changes.
Okay fine.
} mask = &event_data->mask;diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h index f2dc625ea585..010ed26db340 100644 --- a/drivers/hwtracing/coresight/coresight-priv.h +++ b/drivers/hwtracing/coresight/coresight-priv.h @@ -148,10 +148,13 @@ 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, 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_enabled_sink(struct coresight_device *source, bool reset); struct coresight_device *coresight_get_sink_by_id(u32 id); struct coresight_device * coresight_find_default_sink(struct coresight_device *csdev); +struct coresight_device * +coresight_find_enabled_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 e9c90f2de34a..ae69169c58d3 100644 --- a/drivers/hwtracing/coresight/coresight.c +++ b/drivers/hwtracing/coresight/coresight.c @@ -566,6 +566,10 @@ static int coresight_enabled_sink(struct device *dev, const void *data)
/**
- coresight_get_enabled_sink - returns the first enabled sink found on the bus
- When a source reference is given, enabled sink is found using connection based
- search.
- @source: Coresight source device reference
- @deactivate: Whether the 'enable_sink' flag should be reset
- When operated from perf the deactivate parameter should be set to 'true'.
@@ -576,10 +580,21 @@ static int coresight_enabled_sink(struct device *dev, const void *data)
- parameter should be set to 'false', hence mandating users to explicitly
- clear the flag.
*/ -struct coresight_device *coresight_get_enabled_sink(bool deactivate) +struct coresight_device * +coresight_get_enabled_sink(struct coresight_device *source, bool deactivate)
This code was written during a time when all sources were able to reach a sink, something that is no longer the case. Starting from a source is the right way to go.
{ struct device *dev = NULL;
- struct coresight_device *sink;
- if (!source)
goto bus_search;Return NULL if a source was not given.
Agree, since we are removing invocations from perf.
- sink = coresight_find_enabled_sink(source);
- if (sink && deactivate)
sink->activated = false;- return sink;
You get credits for trying to re-use code but in this case I would like to avoid modifying coresight_find_sink() to keep it as simple as possible. I suggest to copy the for() loop in coresight_find_sink() and stop the search when the first activated sink is found. It will introduce a little bit of code duplication but I think we will gain a lot in maintainability.
Fine.
+bus_search: dev = bus_find_device(&coresight_bustype, NULL, &deactivate, coresight_enabled_sink);
Get rid of this. You can also get rid of the 'deactivate' parameter since it won't be used anymore.
Agree, since perf doesnt require this anymore.
Let me know if you have questions.
Sure. Thanks.
-- Linu cherian
Hi Mathieu,
On Mon Aug 24, 2020 at 08:46:33AM -0600, Mathieu Poirier wrote:
On Sat, 22 Aug 2020 at 07:18, Linu Cherian linuc.decode@gmail.com wrote:
Hi Mathieu,
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 1a3169e69bb1..25041d2654e3 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -223,7 +223,7 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, id = (u32)event->attr.config2; sink = coresight_get_sink_by_id(id); } else {
sink = coresight_get_enabled_sink(true);
sink = coresight_get_enabled_sink(NULL, true);It is time for this subsystem to move out of the prehistoric age. Please remove the call to coresight_get_enabled_sink() entirely. In the change log you can write that selecting a sink from sysfs is deprecated when using the perf interface. I will personally refactor the code if someone complains that it broke their user space.
I forgot to mention... Please do two patches, one that removes coresight_get_enabled_sink() from etm_setup_aux() and another one for the reset of your changes.
Yeah. Sure.
-
Linu Cherian -
Linu Cherian -
Mathieu Poirier