[PATCH V3] coresight: Make sysFS functional on topologies with per core sink
Mathieu Poirier
mathieu.poirier at linaro.org
Thu Aug 20 13:02:59 EDT 2020
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 at 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;
> /* 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;
> --
> 2.25.1
>
More information about the linux-arm-kernel
mailing list