Re: [PATCH v2 2/3] coresight: Add source filtering for multi-port output

[Suzuki K Poulose <suzuki.poulose@xxxxxxx>]

On 09/08/2024 11:21, Tao Zhang wrote:
> On 7/11/2024 9:05 PM, Suzuki K Poulose wrote:
> > On 11/07/2024 09:17, Tao Zhang wrote:
> > > In order to enable the output ports of multi-port output devices,
> >
> > This has nothing to do with multi-port. You wanted to pain the picture
> > of a multi-port device, while the reality is different.
> >
> > > such as static replicator, to correspond to designated sources,
> > > a mechanism for filtering data sources is introduced for the
> > > output ports.
> > >
> > > The specified source will be marked like below in the Devicetree.
> > > test-replicator {
> > >      ... ... ... ...
> > >      out-ports {
> > >          ... ... ... ...
> > >          port@0 {
> > >              reg = <0>;
> > >              xxx: endpoint {
> > >                  remote-endpoint = <&xxx>;
> > >                  filter_src = <&xxx>; <-- To specify the source to
> > >              };                           be filtered out here.
> > >          };
> > >
> > >          port@1 {
> > >              reg = <1>;
> > >              yyy: endpoint {
> > >                  remote-endpoint = <&yyy>;
> > >                  filter_src = <&yyy>; <-- To specify the source to
> > >              };                           be filtered out here.
> > >          };
> > >      };
> > > };
> > >
> > > Then driver will find the expected source marked in the Devicetree, and
> > > save it to the coresight path. When the function needs to filter the
> > > source, it could obtain it from coresight path parameter. Finally,
> > > the output port knows which source it corresponds to, and it also knows
> > > which input port it corresponds to.
> >
> > Minor nit: I think the commit description is full of "How" you are 
> > doing something, while it must rather be: "What and Why?"
> >
> > I would prefer something like :
> >
> > Subject: coresight: Add support for trace filtering by source
> >
> > Some replicators have hard coded filtering of "trace" data, based on the
> > source device. This is different from the trace filtering based on
> > TraceID, available in the standard programmable replicators. e.g.,
> > Qualcomm replicators have filtering based on custom trace protocol
> > format and is not programmable.
> >
> > The source device could be connected to the replicator via intermediate
> > components (e.g., a funnel). Thus we need platform information from
> > the firmware tables to decide the source device corresponding to a
> > given output port from the replicator. Given this affects "trace
> > path building" and traversing the path back from the sink to source,
> > add the concept of "filtering by source" to the generic coresight
> > connection.
> >
> >
> > > Signed-off-by: Tao Zhang <quic_taozha@xxxxxxxxxxx>
> > > ---
> > >   drivers/hwtracing/coresight/coresight-core.c  | 125 ++++++++++++++++--
> > >   .../hwtracing/coresight/coresight-platform.c  |  18 +++
> > >   include/linux/coresight.h                     |   5 +
> > >   3 files changed, 135 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/hwtracing/coresight/coresight-core.c 
> > > b/drivers/hwtracing/coresight/coresight-core.c
> > > index 9fc6f6b863e0..3f02a31b9328 100644
> > > --- a/drivers/hwtracing/coresight/coresight-core.c
> > > +++ b/drivers/hwtracing/coresight/coresight-core.c
> > > @@ -75,15 +75,60 @@ struct coresight_device 
> > > *coresight_get_percpu_sink(int cpu)
> > >   }
> > >   EXPORT_SYMBOL_GPL(coresight_get_percpu_sink);
> > >   +static struct coresight_device *coresight_get_source(struct 
> > > list_head *path)
> > > +{
> > > +    struct coresight_device *csdev;
> > > +
> > > +    if (!path)
> > > +        return NULL;
> > > +
> > > +    csdev = list_first_entry(path, struct coresight_node, link)->csdev;
> > > +    if (csdev->type != CORESIGHT_DEV_TYPE_SOURCE)
> > > +        return NULL;
> > > +
> > > +    return csdev;
> > > +}
> > > +
> >
> > Why is this still here ? Didn't agree to remove this and pass the source
> > directly ?
>
> I would like to discuss with you about passing the "source" directly. 
> Since the APIs "coresight_enable_path" and
>
> "coresight_disable_path" are called in coresight-etm-perf.c as well. If 
> passing the "source" to them directly, I need to
>
> modify the code in coresight-etm-perf.c as well. In order to reduce the 
> modification, how about if I keep using
>
> "coresight_get_source" in "coresight_enable_path" and 
> "coresight_disable_path" to get "source" for filtering
>
> the source in "coresight_block_source"?

Yes, please retain the helper. The above comment was based on my
"bad memory" of disable_path_from() was called with a partial path.

No need to change the function. Once again, apologies for the confusion.

Suzuki



> Best,
>
> Tao
>
> > > +/**
> > > + * coresight_source_filter - checks whether the connection matches 
> > > the source
> > > + * of path if connection is binded to specific source.
> > > + * @trace_source: The source device of the trace path
> > > + * @conn:      The connection of one outport
> > > + *
> > > + * Return zero if the connection doesn't have a source binded or 
> > > source of the
> > > + * path matches the source binds to connection.
> > > + */
> > > +static int coresight_source_filter(struct coresight_device 
> > > *trace_source,
> > > +            struct coresight_connection *conn)
> >
> > This name is a bit confusing. It doesn't tell you, whether it
> > allows the trace or blocks it.
> >
> > > +{
> > > +    int ret = 0;
> > > +
> > > +    if (!conn->filter_src_dev)
> > > +        return ret;
> >
> > This is not the correct check. It must be :
> >
> > if (!conn->filter_src_fwnode)
> >
> > Because, the device could have disappeared (or not appeared yet). 
> > e.g., (TPDM) driver module unloaded/not - loaded.
> >
> > > +
> > > +    if (!trace_source)
> >
> > Is that possible ?
> >
> > > +        return -1;
> > > +
> > > +    if (conn->filter_src_dev == trace_source)
> > > +        ret = 0;
> > > +    else
> > > +        ret = -1;
> >
> > Couldn't this simply be :
> >
> > /*
> >  * Returns true, if the trace path is not possible through @conn
> >  * for trace originating from @src
> >  */
> > static bool coresight_blocks_source(src, conn)
> > {
> >     return conn->filter_src_fwnode &&
> >         (conn->filter_src_dev != src);
> > }
> >
> > > +
> > > +    return ret;
> > > +}
> > > +
> > >   static struct coresight_connection *
> > >   coresight_find_out_connection(struct coresight_device *src_dev,
> > > -                  struct coresight_device *dest_dev)
> > > +                  struct coresight_device *dest_dev,
> > > +                  struct coresight_device *trace_source)
> >
> > Please could you rename :
> >
> >     src_dev => csdev
> >     dest_dev => out_dev
> >     trace_source => trace_src ?
> >
> > Having src_dev and trace_source in the same list is confusing.
> >
> > >   {
> > >       int i;
> > >       struct coresight_connection *conn;
> > >         for (i = 0; i < src_dev->pdata->nr_outconns; i++) {
> > >           conn = src_dev->pdata->out_conns[i];
> > > +        if (coresight_source_filter(trace_source, conn))
> >
> >         if (coresight_blocks_source(trace_source, conn))
> >             continue;
> >
> > > +            continue;
> > >           if (conn->dest_dev == dest_dev)
> > >               return conn;
> > >       }
> > > @@ -251,7 +296,8 @@ static void coresight_disable_sink(struct 
> > > coresight_device *csdev)
> > >     static int coresight_enable_link(struct coresight_device *csdev,
> > >                    struct coresight_device *parent,
> > > -                 struct coresight_device *child)
> > > +                 struct coresight_device *child,
> > > +                 struct coresight_device *trace_source)
> > >   {
> > >       int link_subtype;
> > >       struct coresight_connection *inconn, *outconn;
> > > @@ -259,8 +305,8 @@ static int coresight_enable_link(struct 
> > > coresight_device *csdev,
> > >       if (!parent || !child)
> > >           return -EINVAL;
> > >   -    inconn = coresight_find_out_connection(parent, csdev);
> > > -    outconn = coresight_find_out_connection(csdev, child);
> > > +    inconn = coresight_find_out_connection(parent, csdev, 
> > > trace_source);
> > > +    outconn = coresight_find_out_connection(csdev, child, 
> > > trace_source);
> > >       link_subtype = csdev->subtype.link_subtype;
> > >         if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG && 
> > > IS_ERR(inconn))
> > > @@ -273,15 +319,16 @@ static int coresight_enable_link(struct 
> > > coresight_device *csdev,
> > >     static void coresight_disable_link(struct coresight_device *csdev,
> > >                      struct coresight_device *parent,
> > > -                   struct coresight_device *child)
> > > +                   struct coresight_device *child,
> > > +                   struct coresight_device *trace_source)
> > >   {
> > >       struct coresight_connection *inconn, *outconn;
> > >         if (!parent || !child)
> > >           return;
> > >   -    inconn = coresight_find_out_connection(parent, csdev);
> > > -    outconn = coresight_find_out_connection(csdev, child);
> > > +    inconn = coresight_find_out_connection(parent, csdev, 
> > > trace_source);
> > > +    outconn = coresight_find_out_connection(csdev, child, 
> > > trace_source);
> > >         link_ops(csdev)->disable(csdev, inconn, outconn);
> > >   }
> > > @@ -341,6 +388,9 @@ static void coresight_disable_path_from(struct 
> > > list_head *path,
> > >   {
> > >       u32 type;
> > >       struct coresight_device *csdev, *parent, *child;
> > > +    struct coresight_device *source;
> > > +
> > > +    source = coresight_get_source(path);
> >
> > Grr! This must come from the "caller of the 
> > coresight_disable_path_from()", which was my comment. Please go back
> > and double check the comment on previous version.
> >
> >
> > >         if (!nd)
> > >           nd = list_first_entry(path, struct coresight_node, link);
> > > @@ -375,7 +425,7 @@ static void coresight_disable_path_from(struct 
> > > list_head *path,
> > >           case CORESIGHT_DEV_TYPE_LINK:
> > >               parent = list_prev_entry(nd, link)->csdev;
> > >               child = list_next_entry(nd, link)->csdev;
> > > -            coresight_disable_link(csdev, parent, child);
> > > +            coresight_disable_link(csdev, parent, child, source);
> > >               break;
> > >           default:
> > >               break;
> > > @@ -418,6 +468,9 @@ int coresight_enable_path(struct list_head *path, 
> > > enum cs_mode mode,
> > >       u32 type;
> > >       struct coresight_node *nd;
> > >       struct coresight_device *csdev, *parent, *child;
> > > +    struct coresight_device *source;
> > > +
> > > +    source = coresight_get_source(path);
> > >         list_for_each_entry_reverse(nd, path, link) {
> > >           csdev = nd->csdev;
> > > @@ -456,7 +509,7 @@ int coresight_enable_path(struct list_head *path, 
> > > enum cs_mode mode,
> > >           case CORESIGHT_DEV_TYPE_LINK:
> > >               parent = list_prev_entry(nd, link)->csdev;
> > >               child = list_next_entry(nd, link)->csdev;
> > > -            ret = coresight_enable_link(csdev, parent, child);
> > > +            ret = coresight_enable_link(csdev, parent, child, source);
> > >               if (ret)
> > >                   goto err;
> > >               break;
> > > @@ -619,6 +672,7 @@ static void coresight_drop_device(struct 
> > > coresight_device *csdev)
> > >    * @csdev:    The device to start from.
> > >    * @sink:    The final sink we want in this path.
> > >    * @path:    The list to add devices to.
> > > + * @trace_source: The trace source device of the path.
> > >    *
> > >    * The tree of Coresight device is traversed until @sink is found.
> > >    * From there the sink is added to the list along with all the 
> > > devices that led
> > > @@ -627,7 +681,8 @@ static void coresight_drop_device(struct 
> > > coresight_device *csdev)
> > >    */
> > >   static int _coresight_build_path(struct coresight_device *csdev,
> > >                    struct coresight_device *sink,
> > > -                 struct list_head *path)
> > > +                 struct list_head *path,
> > > +                 struct coresight_device *trace_source)
> > >   {
> > >       int i, ret;
> > >       bool found = false;
> > > @@ -639,7 +694,7 @@ static int _coresight_build_path(struct 
> > > coresight_device *csdev,
> > >         if (coresight_is_percpu_source(csdev) && 
> > > coresight_is_percpu_sink(sink) &&
> > >           sink == per_cpu(csdev_sink, 
> > > source_ops(csdev)->cpu_id(csdev))) {
> > > -        if (_coresight_build_path(sink, sink, path) == 0) {
> > > +        if (_coresight_build_path(sink, sink, path, trace_source) == 
> > > 0) {
> > >               found = true;
> > >               goto out;
> > >           }
> > > @@ -650,8 +705,13 @@ static int _coresight_build_path(struct 
> > > coresight_device *csdev,
> > >           struct coresight_device *child_dev;
> > >             child_dev = csdev->pdata->out_conns[i]->dest_dev;
> > > +
> > > +        if (csdev->pdata->out_conns[i]->filter_src_dev
> > > +            && (csdev->pdata->out_conns[i]->filter_src_dev != 
> > > trace_source))
> > > +            continue;
> > > +
> > >           if (child_dev &&
> > > -            _coresight_build_path(child_dev, sink, path) == 0) {
> > > +            _coresight_build_path(child_dev, sink, path, 
> > > trace_source) == 0) {
> > >               found = true;
> > >               break;
> > >           }
> > > @@ -696,7 +756,7 @@ struct list_head *coresight_build_path(struct 
> > > coresight_device *source,
> > >         INIT_LIST_HEAD(path);
> > >   -    rc = _coresight_build_path(source, sink, path);
> > > +    rc = _coresight_build_path(source, sink, path, source);
> > >       if (rc) {
> > >           kfree(path);
> > >           return ERR_PTR(rc);
> > > @@ -957,6 +1017,14 @@ static int coresight_orphan_match(struct device 
> > > *dev, void *data)
> > >               /* This component still has an orphan */
> > >               still_orphan = true;
> > >           }
> >
> >
> > > +        if ((conn->filter_src_fwnode) && dst_csdev
> > > +            && (conn->filter_src_fwnode == dst_csdev->dev.fwnode)) {
> > > +            conn->filter_src_dev = dst_csdev;
> > > +            if (conn->filter_src_dev->type
> > > +                != CORESIGHT_DEV_TYPE_SOURCE)
> > > +                dev_warn(&conn->filter_src_dev->dev,
> > > +                  "Filter source is not a source device\n");
> > > +        }
> >
> > Have you tested your code with the following order :
> >
> > 1) Build everything as module
> > 2) Load the replicator driver.
> > 3) Load the TPDM driver
> >
> > And are you able to get the trace working ? I suspect it doesn't work. 
> > The csdev->orphan must be used to track if there is any missing link
> > with unresolved "filter_src_dev".
> >
> >
> > >       }
> > >         src_csdev->orphan = still_orphan;
> > > @@ -974,6 +1042,30 @@ static int coresight_fixup_orphan_conns(struct 
> > > coresight_device *csdev)
> > >                csdev, coresight_orphan_match);
> > >   }
> > >   +/* reset_filter_src - Reset the filter source if the source is 
> > > being removed */
> > > +static int reset_filter_src(struct coresight_device *trace_source,
> > > +                struct coresight_connection *conn)
> > > +{
> > > +    int i;
> > > +    int ret = -1;
> > > +    struct coresight_device *csdev;
> > > +    struct coresight_connection *output_conn;
> >
> > I don't think this is sufficient. I would rather walk through the 
> > entire coresight bus and fixup the filter_src_dev, rather than 
> > traversing the
> > paths. e.g., one of the devices in the path could have been removed.
> > See coresight_clear_default_sink() for e.g. You should have something
> > similar :
> >
> > coresight_clear_filter_source();
> >
> >
> > Suzuki
> >
> >
> >
> > > +
> > > +    csdev = conn->dest_dev;
> > > +    for (i = 0; i < csdev->pdata->nr_outconns; i++) {
> > > +        output_conn = csdev->pdata->out_conns[i];
> > > +        if (output_conn->filter_src_dev
> > > +            && (output_conn->filter_src_dev == trace_source)) {
> > > +            output_conn->filter_src_dev = NULL;
> > > +            return 0;
> > > +        }
> > > +        ret = reset_filter_src(trace_source, output_conn);
> > > +        if (!ret)
> > > +            return ret;
> > > +    }
> > > +    return ret;
> > > +}
> > > +
> > >   /* coresight_remove_conns - Remove other device's references to 
> > > this device */
> > >   static void coresight_remove_conns(struct coresight_device *csdev)
> > >   {
> > > @@ -986,6 +1078,13 @@ static void coresight_remove_conns(struct 
> > > coresight_device *csdev)
> > >        */
> >
> >
> > >       for (i = 0; i < csdev->pdata->nr_outconns; i++) {
> > >           conn = csdev->pdata->out_conns[i];
> > > +        if (csdev->type == CORESIGHT_DEV_TYPE_SOURCE)
> > > +            reset_filter_src(csdev, conn);
> > > +        if (conn->filter_src_fwnode) {
> > > +            conn->filter_src_dev = NULL;
> > > +            conn->filter_src_fwnode = NULL;
> > > +        }
> > > +
> > >           if (!conn->dest_dev)
> > >               continue;
> > >   diff --git a/drivers/hwtracing/coresight/coresight-platform.c 
> > > b/drivers/hwtracing/coresight/coresight-platform.c
> > > index 64e171eaad82..b3c3e2361d07 100644
> > > --- a/drivers/hwtracing/coresight/coresight-platform.c
> > > +++ b/drivers/hwtracing/coresight/coresight-platform.c
> > > @@ -243,6 +243,24 @@ static int of_coresight_parse_endpoint(struct 
> > > device *dev,
> > >           conn.dest_fwnode = fwnode_handle_get(rdev_fwnode);
> > >           conn.dest_port = rendpoint.port;
> > >   +        /*
> > > +         * Get the firmware node of the filter source through the
> > > +         * reference. This could be used to filter the source in
> > > +         * building path.
> > > +         */
> > > +        conn.filter_src_fwnode =
> > > +            fwnode_find_reference(&ep->fwnode, "filter-src", 0);
> > > +        if (IS_ERR(conn.filter_src_fwnode))
> > > +            conn.filter_src_fwnode = NULL;
> > > +        else {
> > > +            conn.filter_src_dev =
> > > + coresight_find_csdev_by_fwnode(conn.filter_src_fwnode);
> > > +            if (conn.filter_src_dev && (conn.filter_src_dev->type
> > > +                != CORESIGHT_DEV_TYPE_SOURCE))
> > > +                dev_warn(&conn.filter_src_dev->dev,
> > > +                  "Filter source %s is not a source device\n");
> > > +        }
> > > +
> > >           new_conn = coresight_add_out_conn(dev, pdata, &conn);
> > >           if (IS_ERR_VALUE(new_conn)) {
> > >               fwnode_handle_put(conn.dest_fwnode);
> > > diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> > > index f09ace92176e..91a689b4514b 100644
> > > --- a/include/linux/coresight.h
> > > +++ b/include/linux/coresight.h
> > > @@ -172,6 +172,9 @@ struct coresight_desc {
> > >    * @dest_dev:    a @coresight_device representation of the component
> > >           connected to @src_port. NULL until the device is created
> > >    * @link: Representation of the connection as a sysfs link.
> > > + * @filter_src_fwnode: filter source component's fwnode handle.
> > > + * @filter_src_dev: a @coresight_device representation of the 
> > > component that
> > > +        needs to be filtered.
> > >    *
> > >    * The full connection structure looks like this, where in_conns store
> > >    * references to same connection as the source device's out_conns.
> > > @@ -200,6 +203,8 @@ struct coresight_connection {
> > >       struct coresight_device *dest_dev;
> > >       struct coresight_sysfs_link *link;
> > >       struct coresight_device *src_dev;
> > > +    struct fwnode_handle *filter_src_fwnode;
> > > +    struct coresight_device *filter_src_dev;
> > >       atomic_t src_refcnt;
> > >       atomic_t dest_refcnt;
> > >   };