| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a1ae3a0c-5a58-4ed5-94c1-d64c512cc87c@quicinc.com>
Date: Thu, 18 Jul 2024 10:22:06 +0800
From: Tao Zhang <quic_taozha@...cinc.com>
To: Suzuki K Poulose <suzuki.poulose@....com>,
Mike Leach
<mike.leach@...aro.org>, James Clark <james.clark@....com>,
Rob Herring
<robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley
<conor+dt@...nel.org>,
Mathieu Poirier <mathieu.poirier@...aro.org>,
Leo Yan
<leo.yan@...ux.dev>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>
CC: <coresight@...ts.linaro.org>, <linux-arm-kernel@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH v2 2/3] coresight: Add source filtering for multi-port
output
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 <quic_taozha@...cinc.com>
>> ---
>> 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;
>> };
>
Powered by blists - more mailing lists