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: <2b06e962-419f-47f9-a097-a936eddabfa9@quicinc.com>
Date: Fri, 9 Aug 2024 10:32:30 +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 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 <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.
>>>
>>> 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;
>>>>>>   };
>>>>>
>>>
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ