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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <89b90002-fcb8-44c3-bc03-819941161c5d@quicinc.com>
Date: Thu, 11 Jul 2024 16:20:12 +0800
From: Tao Zhang <quic_taozha@...cinc.com>
To: Suzuki K Poulose <suzuki.poulose@....com>,
        Mathieu Poirier
	<mathieu.poirier@...aro.org>,
        Alexander Shishkin
	<alexander.shishkin@...ux.intel.com>,
        Mike Leach <mike.leach@...aro.org>, "Rob Herring" <robh+dt@...nel.org>,
        Krzysztof Kozlowski
	<krzysztof.kozlowski+dt@...aro.org>,
        James Clark <james.clark@....com>, <james.clark@...aro.org>
CC: Jinlong Mao <quic_jinlmao@...cinc.com>, Leo Yan <leo.yan@...aro.org>,
        <coresight@...ts.linaro.org>, <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
        Tingwei Zhang
	<quic_tingweiz@...cinc.com>,
        Yuanfang Zhang <quic_yuanfang@...cinc.com>,
        Trilok Soni <quic_tsoni@...cinc.com>,
        Song Chai <quic_songchai@...cinc.com>, <linux-arm-msm@...r.kernel.org>,
        Jie Gan <quic_jiegan@...cinc.com>
Subject: Re: [PATCH v1 2/3] coresight: Add source filtering for multi-port
 output


On 7/9/2024 9:42 PM, Suzuki K Poulose wrote:
> On 05/07/2024 09:51, Tao Zhang wrote:
>> 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 <quic_taozha@...cinc.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-core.c  | 89 ++++++++++++++++---
>>   .../hwtracing/coresight/coresight-platform.c  | 13 +++
>>   include/linux/coresight.h                     |  5 ++
>>   3 files changed, 94 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-core.c 
>> b/drivers/hwtracing/coresight/coresight-core.c
>> index 5dde597403b3..1c58b64f0031 100644
>> --- a/drivers/hwtracing/coresight/coresight-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>> @@ -113,15 +113,62 @@ 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.
>> + * @path:    The list of devices
>> + * @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 list_head *path,
>
> Instead of path, you may need to switch to "source" device. See below.
Got it. I will update in the next patch series.
>
>> +            struct coresight_connection *conn)
>> +{
>> +    int ret = 0;
>> +    struct coresight_device *source = NULL;
>> +
>> +    if (!conn->filter_src_dev)
>> +        return ret;
>> +
>> +    source = coresight_get_source(path);
>> +    if (!source)
>> +        return ret;
>> +
>> +    if (conn->filter_src_dev == 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 list_head *path)
>>   {
>>       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(path, conn))
>> +            continue;
>>           if (conn->dest_dev == dest_dev)
>>               return conn;
>>       }
>> @@ -312,7 +359,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 list_head *path)
>>   {
>>       int ret = 0;
>>       int link_subtype;
>> @@ -321,8 +369,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, path);
>> +    outconn = coresight_find_out_connection(csdev, child, path);
>>       link_subtype = csdev->subtype.link_subtype;
>>         if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG && 
>> IS_ERR(inconn))
>> @@ -341,7 +389,8 @@ 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 list_head *path)
>>   {
>>       int i;
>>       int link_subtype;
>> @@ -350,8 +399,8 @@ static void coresight_disable_link(struct 
>> coresight_device *csdev,
>>       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, path);
>> +    outconn = coresight_find_out_connection(csdev, child, path);
>>       link_subtype = csdev->subtype.link_subtype;
>>         if (link_ops(csdev)->disable) {
>> @@ -507,7 +556,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, path);
>>               break;
>
> Disable path from could be called with a "partial path" with the
> "source" stripped off. e.g, if the enabling of the components failed
> mid-point in the path. So, source_from_path() above is WRONG csdev.
> Instead you should make sure we pass the "source", which must be
> available with the caller.
I will update in the next patch series.
>
>
>>           default:
>>               break;
>> @@ -588,7 +637,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, path);
>>               if (ret)
>>                   goto err;
>>               break;
>> @@ -802,7 +851,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;
>> @@ -814,7 +864,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;
>>           }
>> @@ -825,8 +875,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;
>>           }
>> @@ -871,7 +926,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);
>> @@ -1395,6 +1450,9 @@ 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;
>>       }
>>         src_csdev->orphan = still_orphan;
>> @@ -1424,6 +1482,11 @@ 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 (conn->filter_src_dev) {
>> +            conn->filter_src_dev = NULL;
>> +            conn->filter_src_fwnode = NULL;
>> +        }
>> +
>
> Similarly we should reset the "filter_src_dev" if the "src" csdev is
> being removed.  You may need a new function for that, which scans
> through all devices and looks for a conn->filter_src_dev == csdev.
> Something like:
I will update in the next patch series.
>
>
>
>>           if (!conn->dest_dev)
>>               continue;
>>   diff --git a/drivers/hwtracing/coresight/coresight-platform.c 
>> b/drivers/hwtracing/coresight/coresight-platform.c
>> index 9d550f5697fa..a9f5b0700310 100644
>> --- a/drivers/hwtracing/coresight/coresight-platform.c
>> +++ b/drivers/hwtracing/coresight/coresight-platform.c
>> @@ -243,6 +243,19 @@ 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);
>
> We should warn, if the filter_src_dev is of not the type DEV_TYPE_SOURCE.

I will update in the next patch series.


Best,

Tao

>
>> +
>>           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 e8b6e388218c..0a7ec0978605 100644
>> --- a/include/linux/coresight.h
>> +++ b/include/linux/coresight.h
>> @@ -171,6 +171,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.
>> @@ -199,6 +202,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
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ