Re: [PATCH v5 3/4] coresight: Add support for trace filtering by source

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

On 18/11/2024 09:28, Tao Zhang wrote:
> On 11/15/2024 5:40 PM, Suzuki K Poulose wrote:
> > On 15/11/2024 09:18, Tao Zhang wrote:
> > > On 11/13/2024 9:39 PM, Suzuki K Poulose wrote:
> > > > On 30/10/2024 09:32, Tao Zhang wrote:
> > > > > 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.
> > > > >
> > > > > The specified source will be marked like below in the Devicetree.
> > > > > test-replicator {
> > > > >      ... ... ... ...
> > > > >      out-ports {
> > > > >          ... ... ... ...
> > > > >          port@0 {
> > > > >              reg = <0>;
> > > > >              xyz: endpoint {
> > > > >                  remote-endpoint = <&zyx>;
> > > > >                  filter-source = <&source_1>; <-- To specify the 
> > > > > source to
> > > > >              };                           be filtered out here.
> > > > >          };
> > > > >
> > > > >          port@1 {
> > > > >              reg = <1>;
> > > > >              abc: endpoint {
> > > > >                  remote-endpoint = <&cba>;
> > > > >                  filter-source = <&source_2>; <-- To specify the 
> > > > > source to
> > > > >              };                           be filtered out here.
> > > > >          };
> > > > >      };
> > > > > };
> > > > >
> > > > > Signed-off-by: Tao Zhang <quic_taozha@xxxxxxxxxxx>
> > > > > ---
> > > > >   drivers/hwtracing/coresight/coresight-core.c  | 113 +++++++++++++ 
> > > > > ++---
> > > > >   .../hwtracing/coresight/coresight-platform.c  |  18 +++
> > > > >   include/linux/coresight.h                     |   5 +
> > > > >   3 files changed, 117 insertions(+), 19 deletions(-)
> > > > >
> > > > > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/ 
> > > > > drivers/hwtracing/coresight/coresight-core.c
> > > > > index ea38ecf26fcb..0a9380350fb5 100644
> > > > > --- a/drivers/hwtracing/coresight/coresight-core.c
> > > > > +++ b/drivers/hwtracing/coresight/coresight-core.c
> > > > > @@ -75,22 +75,54 @@ 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 (!coresight_is_device_source(csdev))
> > > > > +        return NULL;
> > > > > +
> > > > > +    return csdev;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * coresight_blocks_source - checks whether the connection matches 
> > > > > the source
> > > > > + * of path if connection is bound to specific source.
> > > > > + * @src:    The source device of the trace path
> > > > > + * @conn:    The connection of one outport
> > > > > + *
> > > > > + * Return false if the connection doesn't have a source binded or 
> > > > > source of the
> > > > > + * path matches the source binds to connection.
> > > > > + */
> > > > > +static bool coresight_blocks_source(struct coresight_device *src,
> > > > > +                    struct coresight_connection *conn)
> > > > > +{
> > > > > +    return conn->filter_src_fwnode && (conn->filter_src_dev != src);
> > > > > +}
> > > > > +
> > > > >   static struct coresight_connection *
> > > > > -coresight_find_out_connection(struct coresight_device *src_dev,
> > > > > -                  struct coresight_device *dest_dev)
> > > > > +coresight_find_out_connection(struct coresight_device *csdev,
> > > > > +                  struct coresight_device *out_dev,
> > > > > +                  struct coresight_device *trace_src)
> > > > >   {
> > > > >       int i;
> > > > >       struct coresight_connection *conn;
> > > > >   -    for (i = 0; i < src_dev->pdata->nr_outconns; i++) {
> > > > > -        conn = src_dev->pdata->out_conns[i];
> > > > > -        if (conn->dest_dev == dest_dev)
> > > > > +    for (i = 0; i < csdev->pdata->nr_outconns; i++) {
> > > > > +        conn = csdev->pdata->out_conns[i];
> > > > > +        if (coresight_blocks_source(trace_src, conn))
> > > > > +            continue;
> > > > > +        if (conn->dest_dev == out_dev)
> > > > >               return conn;
> > > > >       }
> > > > >   -    dev_err(&src_dev->dev,
> > > > > -        "couldn't find output connection, src_dev: %s, dest_dev: 
> > > > > %s\n",
> > > > > -        dev_name(&src_dev->dev), dev_name(&dest_dev->dev));
> > > > > +    dev_err(&csdev->dev,
> > > > > +        "couldn't find output connection, csdev: %s, out_dev: %s\n",
> > > > > +        dev_name(&csdev->dev), dev_name(&out_dev->dev));
> > > > >         return ERR_PTR(-ENODEV);
> > > > >   }
> > > > > @@ -251,7 +283,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 *source)
> > > > >   {
> > > > >       int link_subtype;
> > > > >       struct coresight_connection *inconn, *outconn;
> > > > > @@ -259,8 +292,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, source);
> > > > > +    outconn = coresight_find_out_connection(csdev, child, source);
> > > > >       link_subtype = csdev->subtype.link_subtype;
> > > > >         if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG && 
> > > > > IS_ERR(inconn))
> > > > > @@ -273,15 +306,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 *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, source);
> > > > > +    outconn = coresight_find_out_connection(csdev, child, source);
> > > > >         link_ops(csdev)->disable(csdev, inconn, outconn);
> > > > >   }
> > > > > @@ -375,7 +409,8 @@ 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,
> > > > > +                           coresight_get_source(path));
> > > > >               break;
> > > > >           default:
> > > > >               break;
> > > > > @@ -418,7 +453,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;
> > > > >           type = csdev->type;
> > > > > @@ -456,7 +493,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 +656,7 @@ static void coresight_drop_device(struct 
> > > > > coresight_device *csdev)
> > > > >   /**
> > > > >    * _coresight_build_path - recursively build a path from a @csdev 
> > > > > to a sink.
> > > > >    * @csdev:    The device to start from.
> > > > > + * @source:    The trace source device of the path.
> > > > >    * @sink:    The final sink we want in this path.
> > > > >    * @path:    The list to add devices to.
> > > > >    *
> > > > > @@ -628,6 +666,7 @@ static void coresight_drop_device(struct 
> > > > > coresight_device *csdev)
> > > > >    * the source is the first device and the sink the last one.
> > > > >    */
> > > > >   static int _coresight_build_path(struct coresight_device *csdev,
> > > > > +                 struct coresight_device *source,
> > > > >                    struct coresight_device *sink,
> > > > >                    struct list_head *path)
> > > > >   {
> > > > > @@ -641,7 +680,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, source, sink, path) == 0) {
> > > > >               found = true;
> > > > >               goto out;
> > > > >           }
> > > > > @@ -652,8 +691,12 @@ static int _coresight_build_path(struct 
> > > > > coresight_device *csdev,
> > > > >           struct coresight_device *child_dev;
> > > > >             child_dev = csdev->pdata->out_conns[i]->dest_dev;
> > > > > +
> > > > > +        if (coresight_blocks_source(source, csdev->pdata- 
> > > > > >out_conns[i]))
> > > > > +            continue;
> > > > > +
> > > > >           if (child_dev &&
> > > > > -            _coresight_build_path(child_dev, sink, path) == 0) {
> > > > > +            _coresight_build_path(child_dev, source, sink, path) 
> > > > > == 0) {
> > > > >               found = true;
> > > > >               break;
> > > > >           }
> > > > > @@ -698,7 +741,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, source, sink, path);
> > > > >       if (rc) {
> > > > >           kfree(path);
> > > > >           return ERR_PTR(rc);
> > > > > @@ -927,6 +970,16 @@ static int coresight_orphan_match(struct 
> > > > > device *dev, void *data)
> > > > >       for (i = 0; i < src_csdev->pdata->nr_outconns; i++) {
> > > > >           conn = src_csdev->pdata->out_conns[i];
> > > > >   +        /* Fix filter source device before skip the port */
> > > > > +        if (conn->filter_src_fwnode && !conn->filter_src_dev) {
> > > > > +            if (dst_csdev &&
> > > > > +                (conn->filter_src_fwnode == dst_csdev->dev.fwnode) &&
> > > > > + !WARN_ON_ONCE(!coresight_is_device_source(dst_csdev)))
> > > > > +                conn->filter_src_dev = dst_csdev;
> > > > > +            else
> > > > > +                still_orphan = true;
> > > > > +        }
> > > > > +
> > > > >           /* Skip the port if it's already connected. */
> > > > >           if (conn->dest_dev)
> > > > >               continue;
> > > > > @@ -977,18 +1030,40 @@ static int 
> > > > > coresight_fixup_orphan_conns(struct coresight_device *csdev)
> > > > >                csdev, coresight_orphan_match);
> > > > >   }
> > > > >   +static int coresight_clear_filter_source(struct device *dev, 
> > > > > void *data)
> > > > > +{
> > > > > +    int i;
> > > > > +    struct coresight_device *source = data;
> > > > > +    struct coresight_device *csdev = to_coresight_device(dev);
> > > > > +
> > > > > +    for (i = 0; i < csdev->pdata->nr_outconns; ++i) {
> > > > > +        if (csdev->pdata->out_conns[i]->filter_src_dev == source)
> > > > > + csdev->pdata->out_conns[i]->filter_src_dev = NULL;
> > > > > +    }
> > > > > +    return 0;
> > > > > +}
> > > > > +
> > > > >   /* coresight_remove_conns - Remove other device's references to 
> > > > > this device */
> > > > >   static void coresight_remove_conns(struct coresight_device *csdev)
> > > > >   {
> > > > >       int i, j;
> > > > >       struct coresight_connection *conn;
> > > > >   +    if (coresight_is_device_source(csdev))
> > > > > +        bus_for_each_dev(&coresight_bustype, NULL, csdev,
> > > > > +                 coresight_clear_filter_source);
> > > > > +
> > > > >       /*
> > > > >        * Remove the input connection references from the 
> > > > > destination device
> > > > >        * for each output connection.
> > > > >        */
> > > > >       for (i = 0; i < csdev->pdata->nr_outconns; i++) {
> > > > >           conn = csdev->pdata->out_conns[i];
> > > > > +        if (conn->filter_src_fwnode) {
> > > > > +            conn->filter_src_dev = NULL;
> > > > > +            fwnode_handle_put(conn->filter_src_fwnode);
> > > > > +        }
> > > > > +
> > > > >           if (!conn->dest_dev)
> > > > >               continue;
> > > > >   diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/ 
> > > > > drivers/hwtracing/coresight/coresight-platform.c
> > > > > index 64e171eaad82..d5532caa9e92 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-source", 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 &&
> > > > > + !coresight_is_device_source(conn.filter_src_dev))
> > > > > +                dev_warn(&conn.filter_src_dev->dev,
> > > > > +                  "Filter source is not a source device\n");
> > > >
> > > > Do we need to reset the conn.filter_src_fwnode ? Otherwise, we can 
> > > > warn from other places too.
> > >
> > > Agree, we need to reset conn.filter_src_fwnode and 
> > > conn.filter_src_dev here if it is not a SOURCE
> > >
> > > type device.
> > >
> > > I will update the following code to the next patch series. Could you 
> > > help check if it is fine to you?
> >
> > This looks good.
> >
> > > +            if (conn.filter_src_dev &&
> > > + !coresight_is_device_source(conn.filter_src_dev)) {
> > > +                dev_warn(&conn.filter_src_dev->dev,
> > > +                  "Filter source is not a source device\n");
> >
> > minor nit: It would be good to indicate which device has this 
> > "filtering". i.e,
> >           dev_warn(dev, "Filter source is not a trace source : %s\n", 
> > dev_name(&conn.filter_src_dev->dev));
> Sure, I will update this to the next patch series.

Also, it may be helpful, if we specify the "port number" which has this
wrong handle. I guess something like:

dev_warn(dev, "port %d: Filter handle is not a trace source : %s\n",
	 conn.src_port, dev_name(&conn.filter_src_dev->dev));

Suzuki
	
> > > +                conn.filter_src_dev = NULL；
> > > +                conn.filter_src_dev = NULL;
>
> Sorry for my mistake here. I also want to set "filter_src_fwnode " to 
> NULL if it is not a trace source.
>
> conn.filter_src_fwnode = NULL;


> Best,
>
> Tao
>
> > Duplicated line ^
> >
> > Suzuki
> >
> > > +            }
> > >
> > > Best,
> > >
> > > Tao
> > >
> > > > Suzuki
> > > >
> > > > > +        }
> > > > > +
> > > > >           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 9311df8538fc..f372c01ae2fc 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;
> > > > >   };