[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 at 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