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

Commit Message

Tao Zhang July 11, 2024, 8:17 a.m. UTC

In order to enable the output ports of multi-port output devices,
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.

Signed-off-by: Tao Zhang <[e-mail]>
---
 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(-)

Comments

Suzuki K Poulose July 11, 2024, 1:05 p.m. UTC | #1

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 <[e-mail]>
> ---
>   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 ?

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

kernel test robot July 12, 2024, 6:49 a.m. UTC | #2

Hi Tao,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v6.10-rc7 next-20240711]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Tao-Zhang/dt-bindings-arm-qcom-coresight-static-replicator-Add-property-for-source-filtering/20240711-162200
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20240711081750.21792-3-quic_taozha%40quicinc.com
patch subject: [PATCH v2 2/3] coresight: Add source filtering for multi-port output
config: arm-randconfig-004-20240712 (https://download.01.org/0day-ci/archive/20240712/[e-mail]/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240712/[e-mail]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[e-mail]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/

All warnings (new ones prefixed by >>):

>> drivers/hwtracing/coresight/coresight-platform.c:261:23: warning: more '%' conversions than data arguments [-Wformat-insufficient-args]
                                     "Filter source %s is not a source device\n");
                                                    ~^
   include/linux/dev_printk.h:156:62: note: expanded from macro 'dev_warn'
           dev_printk_index_wrap(_dev_warn, KERN_WARNING, dev, dev_fmt(fmt), ##__VA_ARGS__)
                                                                       ^~~
   include/linux/dev_printk.h:19:22: note: expanded from macro 'dev_fmt'
   #define dev_fmt(fmt) fmt
                        ^~~
   include/linux/dev_printk.h:110:16: note: expanded from macro 'dev_printk_index_wrap'
                   _p_func(dev, fmt, ##__VA_ARGS__);                       \
                                ^~~
   1 warning generated.


vim +261 drivers/hwtracing/coresight/coresight-platform.c

   185	
   186	/*
   187	 * of_coresight_parse_endpoint : Parse the given output endpoint @ep
   188	 * and fill the connection information in @pdata->out_conns
   189	 *
   190	 * Parses the local port, remote device name and the remote port.
   191	 *
   192	 * Returns :
   193	 *	 0	- If the parsing completed without any fatal errors.
   194	 *	-Errno	- Fatal error, abort the scanning.
   195	 */
   196	static int of_coresight_parse_endpoint(struct device *dev,
   197					       struct device_node *ep,
   198					       struct coresight_platform_data *pdata)
   199	{
   200		int ret = 0;
   201		struct of_endpoint endpoint, rendpoint;
   202		struct device_node *rparent = NULL;
   203		struct device_node *rep = NULL;
   204		struct device *rdev = NULL;
   205		struct fwnode_handle *rdev_fwnode;
   206		struct coresight_connection conn = {};
   207		struct coresight_connection *new_conn;
   208	
   209		do {
   210			/* Parse the local port details */
   211			if (of_graph_parse_endpoint(ep, &endpoint))
   212				break;
   213			/*
   214			 * Get a handle on the remote endpoint and the device it is
   215			 * attached to.
   216			 */
   217			rep = of_graph_get_remote_endpoint(ep);
   218			if (!rep)
   219				break;
   220			rparent = of_coresight_get_port_parent(rep);
   221			if (!rparent)
   222				break;
   223			if (of_graph_parse_endpoint(rep, &rendpoint))
   224				break;
   225	
   226			rdev_fwnode = of_fwnode_handle(rparent);
   227			/* If the remote device is not available, defer probing */
   228			rdev = coresight_find_device_by_fwnode(rdev_fwnode);
   229			if (!rdev) {
   230				ret = -EPROBE_DEFER;
   231				break;
   232			}
   233	
   234			conn.src_port = endpoint.port;
   235			/*
   236			 * Hold the refcount to the target device. This could be
   237			 * released via:
   238			 * 1) coresight_release_platform_data() if the probe fails or
   239			 *    this device is unregistered.
   240			 * 2) While removing the target device via
   241			 *    coresight_remove_match()
   242			 */
   243			conn.dest_fwnode = fwnode_handle_get(rdev_fwnode);
   244			conn.dest_port = rendpoint.port;
   245	
   246			/*
   247			 * Get the firmware node of the filter source through the
   248			 * reference. This could be used to filter the source in
   249			 * building path.
   250			 */
   251			conn.filter_src_fwnode =
   252				fwnode_find_reference(&ep->fwnode, "filter-src", 0);
   253			if (IS_ERR(conn.filter_src_fwnode))
   254				conn.filter_src_fwnode = NULL;
   255			else {
   256				conn.filter_src_dev =
   257				 coresight_find_csdev_by_fwnode(conn.filter_src_fwnode);
   258				if (conn.filter_src_dev && (conn.filter_src_dev->type
   259				    != CORESIGHT_DEV_TYPE_SOURCE))
   260					dev_warn(&conn.filter_src_dev->dev,
 > 261					  "Filter source %s is not a source device\n");
   262			}
   263	
   264			new_conn = coresight_add_out_conn(dev, pdata, &conn);
   265			if (IS_ERR_VALUE(new_conn)) {
   266				fwnode_handle_put(conn.dest_fwnode);
   267				return PTR_ERR(new_conn);
   268			}
   269			/* Connection record updated */
   270		} while (0);
   271	
   272		of_node_put(rparent);
   273		of_node_put(rep);
   274		put_device(rdev);
   275	
   276		return ret;
   277	}
   278

kernel test robot July 12, 2024, 7:12 a.m. UTC | #3

Hi Tao,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v6.10-rc7 next-20240711]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Tao-Zhang/dt-bindings-arm-qcom-coresight-static-replicator-Add-property-for-source-filtering/20240711-162200
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20240711081750.21792-3-quic_taozha%40quicinc.com
patch subject: [PATCH v2 2/3] coresight: Add source filtering for multi-port output
config: arm-allmodconfig (https://download.01.org/0day-ci/archive/20240712/[e-mail]/config)
compiler: arm-linux-gnueabi-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240712/[e-mail]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[e-mail]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/

All warnings (new ones prefixed by >>):

   In file included from include/linux/device.h:15,
                    from include/linux/acpi.h:14,
                    from drivers/hwtracing/coresight/coresight-platform.c:6:
   drivers/hwtracing/coresight/coresight-platform.c: In function 'of_coresight_parse_endpoint':
>> drivers/hwtracing/coresight/coresight-platform.c:261:35: warning: format '%s' expects a matching 'char *' argument [-Wformat=]
     261 |                                   "Filter source %s is not a source device\n");
         |                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                              ^~~
   include/linux/dev_printk.h:156:61: note: in expansion of macro 'dev_fmt'
     156 |         dev_printk_index_wrap(_dev_warn, KERN_WARNING, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                             ^~~~~~~
   drivers/hwtracing/coresight/coresight-platform.c:260:33: note: in expansion of macro 'dev_warn'
     260 |                                 dev_warn(&conn.filter_src_dev->dev,
         |                                 ^~~~~~~~
   drivers/hwtracing/coresight/coresight-platform.c:261:51: note: format string is defined here
     261 |                                   "Filter source %s is not a source device\n");
         |                                                  ~^
         |                                                   |
         |                                                   char *


vim +261 drivers/hwtracing/coresight/coresight-platform.c

   185	
   186	/*
   187	 * of_coresight_parse_endpoint : Parse the given output endpoint @ep
   188	 * and fill the connection information in @pdata->out_conns
   189	 *
   190	 * Parses the local port, remote device name and the remote port.
   191	 *
   192	 * Returns :
   193	 *	 0	- If the parsing completed without any fatal errors.
   194	 *	-Errno	- Fatal error, abort the scanning.
   195	 */
   196	static int of_coresight_parse_endpoint(struct device *dev,
   197					       struct device_node *ep,
   198					       struct coresight_platform_data *pdata)
   199	{
   200		int ret = 0;
   201		struct of_endpoint endpoint, rendpoint;
   202		struct device_node *rparent = NULL;
   203		struct device_node *rep = NULL;
   204		struct device *rdev = NULL;
   205		struct fwnode_handle *rdev_fwnode;
   206		struct coresight_connection conn = {};
   207		struct coresight_connection *new_conn;
   208	
   209		do {
   210			/* Parse the local port details */
   211			if (of_graph_parse_endpoint(ep, &endpoint))
   212				break;
   213			/*
   214			 * Get a handle on the remote endpoint and the device it is
   215			 * attached to.
   216			 */
   217			rep = of_graph_get_remote_endpoint(ep);
   218			if (!rep)
   219				break;
   220			rparent = of_coresight_get_port_parent(rep);
   221			if (!rparent)
   222				break;
   223			if (of_graph_parse_endpoint(rep, &rendpoint))
   224				break;
   225	
   226			rdev_fwnode = of_fwnode_handle(rparent);
   227			/* If the remote device is not available, defer probing */
   228			rdev = coresight_find_device_by_fwnode(rdev_fwnode);
   229			if (!rdev) {
   230				ret = -EPROBE_DEFER;
   231				break;
   232			}
   233	
   234			conn.src_port = endpoint.port;
   235			/*
   236			 * Hold the refcount to the target device. This could be
   237			 * released via:
   238			 * 1) coresight_release_platform_data() if the probe fails or
   239			 *    this device is unregistered.
   240			 * 2) While removing the target device via
   241			 *    coresight_remove_match()
   242			 */
   243			conn.dest_fwnode = fwnode_handle_get(rdev_fwnode);
   244			conn.dest_port = rendpoint.port;
   245	
   246			/*
   247			 * Get the firmware node of the filter source through the
   248			 * reference. This could be used to filter the source in
   249			 * building path.
   250			 */
   251			conn.filter_src_fwnode =
   252				fwnode_find_reference(&ep->fwnode, "filter-src", 0);
   253			if (IS_ERR(conn.filter_src_fwnode))
   254				conn.filter_src_fwnode = NULL;
   255			else {
   256				conn.filter_src_dev =
   257				 coresight_find_csdev_by_fwnode(conn.filter_src_fwnode);
   258				if (conn.filter_src_dev && (conn.filter_src_dev->type
   259				    != CORESIGHT_DEV_TYPE_SOURCE))
   260					dev_warn(&conn.filter_src_dev->dev,
 > 261					  "Filter source %s is not a source device\n");
   262			}
   263	
   264			new_conn = coresight_add_out_conn(dev, pdata, &conn);
   265			if (IS_ERR_VALUE(new_conn)) {
   266				fwnode_handle_put(conn.dest_fwnode);
   267				return PTR_ERR(new_conn);
   268			}
   269			/* Connection record updated */
   270		} while (0);
   271	
   272		of_node_put(rparent);
   273		of_node_put(rep);
   274		put_device(rdev);
   275	
   276		return ret;
   277	}
   278

Tao Zhang July 18, 2024, 2:22 a.m. UTC | #4

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.
>
I will update according to your comments to the next version.
>
>>
>> Signed-off-by: Tao Zhang <[e-mail]>
>> ---
>>   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 agree pass the source directly, but how can we get the source device 
of the path

without this function? The solution I can think of is to pass source 
directly from

"coresight_enable"like below.

coresight_enable ---pass source---> coresight_enable_path -> 
coresight_enable_link

-> coresight_find_out_connection -> coresight_blocks_source

Does this solution meet your expectations? Or do you have a better 
suggestion?

>
>> +/**
>> + * 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);
> }
I will update to the next version.
>
>> +
>> +    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.
I will update to the next version.
>
>>   {
>>       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.

How about if I pass the source to " coresight_disable_path_from" 
directly from

"coresight_enable" or "coresight_disable" like below?

coresight_enable ----|                             |---> 
coresight_disable_patch_from

                                      |---pass source---|

coresight_disable----|                             |---> 
coresight_disable_path -> coresight_disable_patch_from

This can ensure that the source passed into coresight_disable_patch_from is

the correct source.

>
>
>>         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".

I really didn't consider this test case in my testing. There was a 
problem with

the logic of the code I modified. If it is modified to the following, 
can it solve the

test case you proposed?

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

         /*Add the following code to fix filter source device before 
skip the port*/
+        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");
+        }

         /* Skip the port if it's already connected. */
         if (conn->dest_dev)
             continue;
     ... ... ...
             ret = coresight_add_in_conn(conn);
             if (ret)
                 return ret;

               /* Add the following code to make sure the device still 
orphan if there is a

                 * filter source firmware node but the filter source is 
null.*/

+            if (conn->filter_src_fwnode && !conn->filter_src_dev)
+                still_orphan = true;
         } else {
             /* This component still has an orphan */
             still_orphan = true;
         }
     }

     src_csdev->orphan = still_orphan;
     ... ... ...
}

>
>
>>       }
>>         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();

That's right. My current understanding is that there are two cases that 
need to be considered.

#1 Unregister the source device

In this case, we need to walk through the entire coresight bus and fixup 
the filter_src_dev

#2 Unregister the replicator

Iterate over all output connections to set filter_src_dev and 
filter_src_fwnode to NULL.

Is this logically correct? I have attached my sample code below, please 
help review it.

+static void 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;
+    }

+}

/* 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 (csdev->type == CORESIGHT_DEV_TYPE_SOURCE) //walk through the 
entire coresight bus and fixup the filter_src_dev
+        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) { //Iterate over all output 
connections to set filter_src_dev and filter_src_fwnode to NULL.
+            conn->filter_src_dev = NULL;
+            conn->filter_src_fwnode = NULL;
+        }

         if (!conn->dest_dev)
             continue;

        ... ... ...
}


Best,

Tao

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

Suzuki K Poulose Aug. 7, 2024, 2:59 p.m. UTC | #5

On 18/07/2024 03:22, 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.
>>
> I will update according to your comments to the next version.
>>
>>>
>>> Signed-off-by: Tao Zhang <[e-mail]>
>>> ---
>>>   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 agree pass the source directly, but how can we get the source device 
> of the path
> 
> without this function? The solution I can think of is to pass source 
> directly from
> 
> "coresight_enable"like below.
> 
> coresight_enable ---pass source---> coresight_enable_path -> 
> coresight_enable_link
> 
> -> coresight_find_out_connection -> coresight_blocks_source
> 
> Does this solution meet your expectations? Or do you have a better 
> suggestion?
> 
>>
>>> +/**
>>> + * 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);
>> }
> I will update to the next version.
>>
>>> +
>>> +    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.
> I will update to the next version.
>>
>>>   {
>>>       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.
> 
> How about if I pass the source to " coresight_disable_path_from" 
> directly from
> 
> "coresight_enable" or "coresight_disable" like below?
> 
> coresight_enable ----|                             |---> 
> coresight_disable_patch_from
> 
>                                       |---pass source---|
> 
> coresight_disable----|                             |---> 
> coresight_disable_path -> coresight_disable_patch_from
> 
> This can ensure that the source passed into coresight_disable_patch_from is
> 
> the correct source.

coresight_disable_path_from() must be given the "source" separately,
as the "path" could be "partial". Rest could stay as it is in your
patch.


> 
>>
>>
>>>         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".
> 
> I really didn't consider this test case in my testing. There was a 
> problem with
> 
> the logic of the code I modified. If it is modified to the following, 
> can it solve the
> 
> test case you proposed?
> 
> 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];
> 
>          /*Add the following code to fix filter source device before 
> skip the port*/
> +        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");

And we should not set the device as the filter_src_dev in this case.

> +        }
> 
>          /* Skip the port if it's already connected. */
>          if (conn->dest_dev)
>              continue;
>      ... ... ...
>              ret = coresight_add_in_conn(conn);
>              if (ret)
>                  return ret;
> 
>                /* Add the following code to make sure the device still 
> orphan if there is a
> 
>                  * filter source firmware node but the filter source is 
> null.*/
> 
> +            if (conn->filter_src_fwnode && !conn->filter_src_dev)
> +                still_orphan = true;
>          } else {
>              /* This component still has an orphan */
>              still_orphan = true;
>          }
>      }
> 
>      src_csdev->orphan = still_orphan;
>      ... ... ...
> }
> 
>>
>>
>>>       }
>>>         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();
> 
> That's right. My current understanding is that there are two cases that 
> need to be considered.
> 
> #1 Unregister the source device
> 
> In this case, we need to walk through the entire coresight bus and fixup 
> the filter_src_dev
> 
> #2 Unregister the replicator
> 
> Iterate over all output connections to set filter_src_dev and 
> filter_src_fwnode to NULL.
> 
> Is this logically correct? I have attached my sample code below, please 
> help review it.
> 
> +static void 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;
> +    }
> 
> +}

That looks good to me.

> 
> /* 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 (csdev->type == CORESIGHT_DEV_TYPE_SOURCE) //walk through the 
> entire coresight bus and fixup the filter_src_dev
> +        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) { //Iterate over all output 
> connections to set filter_src_dev and filter_src_fwnode to NULL.
> +            conn->filter_src_dev = NULL;
> +            conn->filter_src_fwnode = NULL;

We may need to "drop" the refcount for this one ?

Suzuki


> +        }
> 
>          if (!conn->dest_dev)
>              continue;
> 
>         ... ... ...
> }
> 
> 
> Best,
> 
> Tao
> 
>>
>>
>> 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;
>>>   };
>>

Tao Zhang Aug. 8, 2024, 2:43 p.m. UTC | #6

On 8/7/2024 10:59 PM, Suzuki K Poulose wrote:
> On 18/07/2024 03:22, 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.
>>>
>> I will update according to your comments to the next version.
>>>
>>>>
>>>> Signed-off-by: Tao Zhang <[e-mail]>
>>>> ---
>>>>   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 agree pass the source directly, but how can we get the source 
>> device of the path
>>
>> without this function? The solution I can think of is to pass source 
>> directly from
>>
>> "coresight_enable"like below.
>>
>> coresight_enable ---pass source---> coresight_enable_path -> 
>> coresight_enable_link
>>
>> -> coresight_find_out_connection -> coresight_blocks_source
>>
>> Does this solution meet your expectations? Or do you have a better 
>> suggestion?
>>
>>>
>>>> +/**
>>>> + * 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);
>>> }
>> I will update to the next version.
>>>
>>>> +
>>>> +    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.
>> I will update to the next version.
>>>
>>>>   {
>>>>       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.
>>
>> How about if I pass the source to " coresight_disable_path_from" 
>> directly from
>>
>> "coresight_enable" or "coresight_disable" like below?
>>
>> coresight_enable ----|                             |---> 
>> coresight_disable_patch_from
>>
>>                                       |---pass source---|
>>
>> coresight_disable----|                             |---> 
>> coresight_disable_path -> coresight_disable_patch_from
>>
>> This can ensure that the source passed into 
>> coresight_disable_patch_from is
>>
>> the correct source.
>
> coresight_disable_path_from() must be given the "source" separately,
> as the "path" could be "partial". Rest could stay as it is in your
> patch.
In what scenario is this "path" "partial"?

There are two place where "coresight_disable_path_from" will be called 
in coresight driver.

1. coresight_disable_path -> coresight_disable_path_from

In this case, I see all "coresight_disable_path" will be called after 
building path successfully. The "path" should

be "complete", right?

2. coresight_enable_path return with error and call 
coresight_disable_path_from to disable the "path" from

"nd". But in this case, it is called after building path completely, 
right? So this "path" should be complete as well?

Please let me know if there is any mistake in my understanding. If my 
understanding is correct, then there is no

need to pass "source" to "coresight_disable_path_from". Because the 
"source" can be collected from the "path"

directly.

>
>
>>
>>>
>>>
>>>>         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".
>>
>> I really didn't consider this test case in my testing. There was a 
>> problem with
>>
>> the logic of the code I modified. If it is modified to the following, 
>> can it solve the
>>
>> test case you proposed?
>>
>> 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];
>>
>>          /*Add the following code to fix filter source device before 
>> skip the port*/
>> +        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");
>
> And we should not set the device as the filter_src_dev in this case.

Sure, I will update the code as the following.

+        if ((conn->filter_src_fwnode) && dst_csdev
+            && (conn->filter_src_fwnode == dst_csdev->dev.fwnode)) {
+             if (dst_csdev->type == CORESIGHT_DEV_TYPE_SOURCE)
+                 conn->filter_src_dev = dst_csdev;
+             else
+                 dev_warn(&dst_csdev->dev,
+                   "Filter source is not a source device\n");
>
>> +        }
>>
>>          /* Skip the port if it's already connected. */
>>          if (conn->dest_dev)
>>              continue;
>>      ... ... ...
>>              ret = coresight_add_in_conn(conn);
>>              if (ret)
>>                  return ret;
>>
>>                /* Add the following code to make sure the device 
>> still orphan if there is a
>>
>>                  * filter source firmware node but the filter source 
>> is null.*/
>>
>> +            if (conn->filter_src_fwnode && !conn->filter_src_dev)
>> +                still_orphan = true;
>>          } else {
>>              /* This component still has an orphan */
>>              still_orphan = true;
>>          }
>>      }
>>
>>      src_csdev->orphan = still_orphan;
>>      ... ... ...
>> }
>>
>>>
>>>
>>>>       }
>>>>         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();
>>
>> That's right. My current understanding is that there are two cases 
>> that need to be considered.
>>
>> #1 Unregister the source device
>>
>> In this case, we need to walk through the entire coresight bus and 
>> fixup the filter_src_dev
>>
>> #2 Unregister the replicator
>>
>> Iterate over all output connections to set filter_src_dev and 
>> filter_src_fwnode to NULL.
>>
>> Is this logically correct? I have attached my sample code below, 
>> please help review it.
>>
>> +static void 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;
>> +    }
>>
>> +}
>
> That looks good to me.
>
>>
>> /* 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 (csdev->type == CORESIGHT_DEV_TYPE_SOURCE) //walk through the 
>> entire coresight bus and fixup the filter_src_dev
>> +        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) { //Iterate over all output 
>> connections to set filter_src_dev and filter_src_fwnode to NULL.
>> +            conn->filter_src_dev = NULL;
>> +            conn->filter_src_fwnode = NULL;
>
> We may need to "drop" the refcount for this one ?

I'm not clear why need to "drop" the refcount here. I didn't add 
refcount to this patch series before.

Could you show me your sample code so I can understand what you mean?


Best,

Tao

>
> Suzuki
>
>
>> +        }
>>
>>          if (!conn->dest_dev)
>>              continue;
>>
>>         ... ... ...
>> }
>>
>>
>> Best,
>>
>> Tao
>>
>>>
>>>
>>> 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;
>>>>   };
>>>
>

Suzuki K Poulose Aug. 8, 2024, 3:13 p.m. UTC | #7

On 08/08/2024 15:43, Tao Zhang wrote:
> 
> On 8/7/2024 10:59 PM, Suzuki K Poulose wrote:
>> On 18/07/2024 03:22, 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.
>>>>
>>> I will update according to your comments to the next version.
>>>>
>>>>>
>>>>> Signed-off-by: Tao Zhang <[e-mail]>
>>>>> ---
>>>>>   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 agree pass the source directly, but how can we get the source 
>>> device of the path
>>>
>>> without this function? The solution I can think of is to pass source 
>>> directly from
>>>
>>> "coresight_enable"like below.
>>>
>>> coresight_enable ---pass source---> coresight_enable_path -> 
>>> coresight_enable_link
>>>
>>> -> coresight_find_out_connection -> coresight_blocks_source
>>>
>>> Does this solution meet your expectations? Or do you have a better 
>>> suggestion?
>>>
>>>>
>>>>> +/**
>>>>> + * 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);
>>>> }
>>> I will update to the next version.
>>>>
>>>>> +
>>>>> +    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.
>>> I will update to the next version.
>>>>
>>>>>   {
>>>>>       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.
>>>
>>> How about if I pass the source to " coresight_disable_path_from" 
>>> directly from
>>>
>>> "coresight_enable" or "coresight_disable" like below?
>>>
>>> coresight_enable ----|                             |---> 
>>> coresight_disable_patch_from
>>>
>>>                                       |---pass source---|
>>>
>>> coresight_disable----|                             |---> 
>>> coresight_disable_path -> coresight_disable_patch_from
>>>
>>> This can ensure that the source passed into 
>>> coresight_disable_patch_from is
>>>
>>> the correct source.
>>
>> coresight_disable_path_from() must be given the "source" separately,
>> as the "path" could be "partial". Rest could stay as it is in your
>> patch.
> In what scenario is this "path" "partial"?
> 
> There are two place where "coresight_disable_path_from" will be called 
> in coresight driver.
> 
> 1. coresight_disable_path -> coresight_disable_path_from
> 
> In this case, I see all "coresight_disable_path" will be called after 
> building path successfully. The "path" should
> 
> be "complete", right?
> 
> 2. coresight_enable_path return with error and call 
> coresight_disable_path_from to disable the "path" from
> 
> "nd". But in this case, it is called after building path completely, 
> right? So this "path" should be complete as well?
> 
> Please let me know if there is any mistake in my understanding. If my 
> understanding is correct, then there is no
> 
> need to pass "source" to "coresight_disable_path_from". Because the 
> "source" can be collected from the "path"\

Apologies. You are right. We are passing the full path in both cases.
Instead "nd" is specified for partial path operations.

> 
> directly.
> 
>>
>>
>>>
>>>>
>>>>
>>>>>         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".
>>>
>>> I really didn't consider this test case in my testing. There was a 
>>> problem with
>>>
>>> the logic of the code I modified. If it is modified to the following, 
>>> can it solve the
>>>
>>> test case you proposed?
>>>
>>> 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];
>>>
>>>          /*Add the following code to fix filter source device before 
>>> skip the port*/
>>> +        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");
>>
>> And we should not set the device as the filter_src_dev in this case.
> 
> Sure, I will update the code as the following.
> 
> +        if ((conn->filter_src_fwnode) && dst_csdev
> +            && (conn->filter_src_fwnode == dst_csdev->dev.fwnode)) {
> +             if (dst_csdev->type == CORESIGHT_DEV_TYPE_SOURCE)
> +                 conn->filter_src_dev = dst_csdev;
> +             else
> +                 dev_warn(&dst_csdev->dev,
> +                   "Filter source is not a source device\n");
>>
>>> +        }
>>>
>>>          /* Skip the port if it's already connected. */
>>>          if (conn->dest_dev)
>>>              continue;
>>>      ... ... ...
>>>              ret = coresight_add_in_conn(conn);
>>>              if (ret)
>>>                  return ret;
>>>
>>>                /* Add the following code to make sure the device 
>>> still orphan if there is a
>>>
>>>                  * filter source firmware node but the filter source 
>>> is null.*/
>>>
>>> +            if (conn->filter_src_fwnode && !conn->filter_src_dev)
>>> +                still_orphan = true;
>>>          } else {
>>>              /* This component still has an orphan */
>>>              still_orphan = true;
>>>          }
>>>      }
>>>
>>>      src_csdev->orphan = still_orphan;
>>>      ... ... ...
>>> }
>>>
>>>>
>>>>
>>>>>       }
>>>>>         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();
>>>
>>> That's right. My current understanding is that there are two cases 
>>> that need to be considered.
>>>
>>> #1 Unregister the source device
>>>
>>> In this case, we need to walk through the entire coresight bus and 
>>> fixup the filter_src_dev
>>>
>>> #2 Unregister the replicator
>>>
>>> Iterate over all output connections to set filter_src_dev and 
>>> filter_src_fwnode to NULL.
>>>
>>> Is this logically correct? I have attached my sample code below, 
>>> please help review it.
>>>
>>> +static void 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;
>>> +    }
>>>
>>> +}
>>
>> That looks good to me.
>>
>>>
>>> /* 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 (csdev->type == CORESIGHT_DEV_TYPE_SOURCE) //walk through the 
>>> entire coresight bus and fixup the filter_src_dev
>>> +        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) { //Iterate over all output 
>>> connections to set filter_src_dev and filter_src_fwnode to NULL.
>>> +            conn->filter_src_dev = NULL;
>>> +            conn->filter_src_fwnode = NULL;
>>
>> We may need to "drop" the refcount for this one ?
> 
> I'm not clear why need to "drop" the refcount here. I didn't add 
> refcount to this patch series before.

Whenever we save a fwnode in a structure, we do : fwnode_handle_get()
and drop them via while releasing it via fwnode_handle_put().

e.g. : of_coresight_parse_endpoint() / acpi_coresight_parse_link() for
connection and dropped in coresight_release_platform_data().

You should do that whenever you save the filter_src_fwnode in 
coresight-platform.c and drop ot when the device "using the fwnode" goes 
away (in
this case the replicator).

Suzuki


> 
> Could you show me your sample code so I can understand what you mean?
> 
> 
> Best,
> 
> Tao
> 
>>
>> Suzuki
>>
>>
>>> +        }
>>>
>>>          if (!conn->dest_dev)
>>>              continue;
>>>
>>>         ... ... ...
>>> }
>>>
>>>
>>> Best,
>>>
>>> Tao
>>>
>>>>
>>>>
>>>> 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;
>>>>>   };
>>>>
>>

Tao Zhang Aug. 9, 2024, 2:32 a.m. UTC | #8

On 8/8/2024 11:13 PM, Suzuki K Poulose wrote:
> On 08/08/2024 15:43, Tao Zhang wrote:
>>
>> On 8/7/2024 10:59 PM, Suzuki K Poulose wrote:
>>> On 18/07/2024 03:22, 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.
>>>>>
>>>> I will update according to your comments to the next version.
>>>>>
>>>>>>
>>>>>> Signed-off-by: Tao Zhang <[e-mail]>
>>>>>> ---
>>>>>>   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 agree pass the source directly, but how can we get the source 
>>>> device of the path
>>>>
>>>> without this function? The solution I can think of is to pass 
>>>> source directly from
>>>>
>>>> "coresight_enable"like below.
>>>>
>>>> coresight_enable ---pass source---> coresight_enable_path -> 
>>>> coresight_enable_link
>>>>
>>>> -> coresight_find_out_connection -> coresight_blocks_source
>>>>
>>>> Does this solution meet your expectations? Or do you have a better 
>>>> suggestion?
>>>>
>>>>>
>>>>>> +/**
>>>>>> + * 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);
>>>>> }
>>>> I will update to the next version.
>>>>>
>>>>>> +
>>>>>> +    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.
>>>> I will update to the next version.
>>>>>
>>>>>>   {
>>>>>>       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.
>>>>
>>>> How about if I pass the source to " coresight_disable_path_from" 
>>>> directly from
>>>>
>>>> "coresight_enable" or "coresight_disable" like below?
>>>>
>>>> coresight_enable ----|                             |---> 
>>>> coresight_disable_patch_from
>>>>
>>>>                                       |---pass source---|
>>>>
>>>> coresight_disable----|                             |---> 
>>>> coresight_disable_path -> coresight_disable_patch_from
>>>>
>>>> This can ensure that the source passed into 
>>>> coresight_disable_patch_from is
>>>>
>>>> the correct source.
>>>
>>> coresight_disable_path_from() must be given the "source" separately,
>>> as the "path" could be "partial". Rest could stay as it is in your
>>> patch.
>> In what scenario is this "path" "partial"?
>>
>> There are two place where "coresight_disable_path_from" will be 
>> called in coresight driver.
>>
>> 1. coresight_disable_path -> coresight_disable_path_from
>>
>> In this case, I see all "coresight_disable_path" will be called after 
>> building path successfully. The "path" should
>>
>> be "complete", right?
>>
>> 2. coresight_enable_path return with error and call 
>> coresight_disable_path_from to disable the "path" from
>>
>> "nd". But in this case, it is called after building path completely, 
>> right? So this "path" should be complete as well?
>>
>> Please let me know if there is any mistake in my understanding. If my 
>> understanding is correct, then there is no
>>
>> need to pass "source" to "coresight_disable_path_from". Because the 
>> "source" can be collected from the "path"\
>
> Apologies. You are right. We are passing the full path in both cases.
> Instead "nd" is specified for partial path operations.
Thanks for you confirmation. I will modify the code according to this 
idea to minimize the impact on the other code.
>
>>
>> directly.
>>
>>>
>>>
>>>>
>>>>>
>>>>>
>>>>>>         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".
>>>>
>>>> I really didn't consider this test case in my testing. There was a 
>>>> problem with
>>>>
>>>> the logic of the code I modified. If it is modified to the 
>>>> following, can it solve the
>>>>
>>>> test case you proposed?
>>>>
>>>> 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];
>>>>
>>>>          /*Add the following code to fix filter source device 
>>>> before skip the port*/
>>>> +        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");
>>>
>>> And we should not set the device as the filter_src_dev in this case.
>>
>> Sure, I will update the code as the following.
>>
>> +        if ((conn->filter_src_fwnode) && dst_csdev
>> +            && (conn->filter_src_fwnode == dst_csdev->dev.fwnode)) {
>> +             if (dst_csdev->type == CORESIGHT_DEV_TYPE_SOURCE)
>> +                 conn->filter_src_dev = dst_csdev;
>> +             else
>> +                 dev_warn(&dst_csdev->dev,
>> +                   "Filter source is not a source device\n");
>>>
>>>> +        }
>>>>
>>>>          /* Skip the port if it's already connected. */
>>>>          if (conn->dest_dev)
>>>>              continue;
>>>>      ... ... ...
>>>>              ret = coresight_add_in_conn(conn);
>>>>              if (ret)
>>>>                  return ret;
>>>>
>>>>                /* Add the following code to make sure the device 
>>>> still orphan if there is a
>>>>
>>>>                  * filter source firmware node but the filter 
>>>> source is null.*/
>>>>
>>>> +            if (conn->filter_src_fwnode && !conn->filter_src_dev)
>>>> +                still_orphan = true;
>>>>          } else {
>>>>              /* This component still has an orphan */
>>>>              still_orphan = true;
>>>>          }
>>>>      }
>>>>
>>>>      src_csdev->orphan = still_orphan;
>>>>      ... ... ...
>>>> }
>>>>
>>>>>
>>>>>
>>>>>>       }
>>>>>>         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();
>>>>
>>>> That's right. My current understanding is that there are two cases 
>>>> that need to be considered.
>>>>
>>>> #1 Unregister the source device
>>>>
>>>> In this case, we need to walk through the entire coresight bus and 
>>>> fixup the filter_src_dev
>>>>
>>>> #2 Unregister the replicator
>>>>
>>>> Iterate over all output connections to set filter_src_dev and 
>>>> filter_src_fwnode to NULL.
>>>>
>>>> Is this logically correct? I have attached my sample code below, 
>>>> please help review it.
>>>>
>>>> +static void 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;
>>>> +    }
>>>>
>>>> +}
>>>
>>> That looks good to me.
>>>
>>>>
>>>> /* 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 (csdev->type == CORESIGHT_DEV_TYPE_SOURCE) //walk through 
>>>> the entire coresight bus and fixup the filter_src_dev
>>>> +        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) { //Iterate over all output 
>>>> connections to set filter_src_dev and filter_src_fwnode to NULL.
>>>> +            conn->filter_src_dev = NULL;
>>>> +            conn->filter_src_fwnode = NULL;
>>>
>>> We may need to "drop" the refcount for this one ?
>>
>> I'm not clear why need to "drop" the refcount here. I didn't add 
>> refcount to this patch series before.
>
> Whenever we save a fwnode in a structure, we do : fwnode_handle_get()
> and drop them via while releasing it via fwnode_handle_put().
>
> e.g. : of_coresight_parse_endpoint() / acpi_coresight_parse_link() for
> connection and dropped in coresight_release_platform_data().
>
> You should do that whenever you save the filter_src_fwnode in 
> coresight-platform.c and drop ot when the device "using the fwnode" 
> goes away (in
> this case the replicator).

Got it. Thank you for your reminder, I will modify the code according to 
your suggestion.


Best,

Tao

>
> Suzuki
>
>
>>
>> Could you show me your sample code so I can understand what you mean?
>>
>>
>> Best,
>>
>> Tao
>>
>>>
>>> Suzuki
>>>
>>>
>>>> +        }
>>>>
>>>>          if (!conn->dest_dev)
>>>>              continue;
>>>>
>>>>         ... ... ...
>>>> }
>>>>
>>>>
>>>> Best,
>>>>
>>>> Tao
>>>>
>>>>>
>>>>>
>>>>> 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;
>>>>>>   };
>>>>>
>>>
>

Tao Zhang Aug. 9, 2024, 10:21 a.m. UTC | #9

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 <[e-mail]>
>> ---
>>   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"?


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

Suzuki K Poulose Aug. 9, 2024, 11 a.m. UTC | #10

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 <[e-mail]>
>>> ---
>>>   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;
>>>   };
>>

Patch

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;
+}
+
+/**
+ * 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)
+{
+	int ret = 0;
+
+	if (!conn->filter_src_dev)
+		return ret;
+
+	if (!trace_source)
+		return -1;
+
+	if (conn->filter_src_dev == trace_source)
+		ret = 0;
+	else
+		ret = -1;
+
+	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)
 {
 	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))
+			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);
 
 	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");
+		}
 	}
 
 	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;
+
+	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;
 };