[PATCH V3] coresight: Make sysFS functional on topologies with per core sink

[PATCH V3] coresight: Make sysFS functional on topologies with per core sink

[Linu Cherian]

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;
-- 
2.25.1


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

Re: [PATCH V3] coresight: Make sysFS functional on topologies with per core sink

[Mathieu Poirier]

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;
>  	/* 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
> 

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

Re: [PATCH V3] coresight: Make sysFS functional on topologies with per core sink

[Linu Cherian]

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.

-- 
Linu cherian

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

Re: [PATCH V3] coresight: Make sysFS functional on topologies with per core sink

[Mathieu Poirier]

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

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

Re: [PATCH V3] coresight: Make sysFS functional on topologies with per core sink

[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

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