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] [day] [month] [year] [list]
Message-ID: <b8fd3d5a-fc61-4ab0-8fb1-7cacf4cab9f5@linaro.org>
Date: Wed, 15 Jan 2025 12:29:03 +0000
From: James Clark <james.clark@...aro.org>
To: Jie Gan <quic_jiegan@...cinc.com>
Cc: Jinlong Mao <quic_jinlmao@...cinc.com>, 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>,
 linux-arm-msm@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com,
 Suzuki K Poulose <suzuki.poulose@....com>, Mike Leach
 <mike.leach@...aro.org>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Maxime Coquelin <mcoquelin.stm32@...il.com>,
 Alexandre Torgue <alexandre.torgue@...s.st.com>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Bjorn Andersson <andersson@...nel.org>,
 Konrad Dybcio <konradybcio@...nel.org>
Subject: Re: [PATCH v8 2/5] Coresight: Add trace_id function to retrieving the
 trace ID



On 15/01/2025 1:44 am, Jie Gan wrote:
> 
> 
> On 1/14/2025 6:07 PM, James Clark wrote:
>>
>>
>> On 14/01/2025 2:51 am, Jie Gan wrote:
>>>
>>>
>>> On 1/13/2025 8:02 PM, James Clark wrote:
>>>>
>>>>
>>>> On 26/12/2024 1:10 am, Jie Gan wrote:
>>>>> Add 'trace_id' function pointer in ops. It's responsible for
>>>>> retrieving the device's trace ID.
>>>>>
>>>>> Add 'struct cs_sink_data' to store the data that is needed by
>>>>> coresight_enable_path/coresight_disable_path. The structure
>>>>> will be transmitted to the helper and sink device to enable
>>>>> related funcationalities.
>>>>>
>>>>
>>>> The new cs_sink_data struct is quite specific to this change. Can we 
>>>> start passing the path around to enable/disable functions, that will 
>>>> allow devices to gather anything they want in the future. Because we 
>>>> already have coresight_get_sink(path), coresight_get_source(path) etc.
>>>>
>>>> And see below, but for this case we can also change the path struct 
>>>> to contain the trace ID. Then all the new functions, allocations and 
>>>> searches for the trace ID are unecessary. The CTCU will have access 
>>>> to the path, and by the time its enable function is called the trace 
>>>> ID is already assigned.
>>>>
>>>> It's also easier to understand at which point a trace ID is 
>>>> allocated, rather than adding the trace_id() callbacks from 
>>>> everywhere which could potentially either read or allocate. I 
>>>> suppose that's "safer" because maybe it's not allocated, but I can't 
>>>> see what case it would happen in reverse.
>>>>
>>> Thank you for comment. I will try this solution.
>>> The biggest challenge for the patch is how to correctly read trace_id 
>>> from source device and passthrough it to helper device as the source 
>>> device always the last one to enable. I believe your proposed 
>>> solution is better than mine and has minimal impact on the basic 
>>> framework, but I think we still need read_trace in source_ops and 
>>> link_ops. Then we can read the trace_id in coresight_build_path 
>>> function and save it to the coresight_path to avoid redundant searching?
>>>
>>>
>>>>> Signed-off-by: Jie Gan <quic_jiegan@...cinc.com>
>>>>> ---
>>>>>   drivers/hwtracing/coresight/coresight-core.c  | 59 ++++++++++++++ 
>>>>> +----
>>>>>   drivers/hwtracing/coresight/coresight-etb10.c |  3 +-
>>>>>   .../hwtracing/coresight/coresight-etm-perf.c  | 37 ++++++++++--
>>>>>   .../coresight/coresight-etm3x-core.c          | 30 ++++++++++
>>>>>   .../coresight/coresight-etm4x-core.c          | 29 +++++++++
>>>>>   drivers/hwtracing/coresight/coresight-priv.h  | 13 +++-
>>>>>   drivers/hwtracing/coresight/coresight-stm.c   | 22 +++++++
>>>>>   drivers/hwtracing/coresight/coresight-sysfs.c | 24 +++++++-
>>>>>   .../hwtracing/coresight/coresight-tmc-etf.c   |  3 +-
>>>>>   .../hwtracing/coresight/coresight-tmc-etr.c   |  6 +-
>>>>>   drivers/hwtracing/coresight/coresight-tpda.c  | 20 +++++++
>>>>>   drivers/hwtracing/coresight/coresight-trbe.c  |  4 +-
>>>>>   drivers/hwtracing/coresight/ultrasoc-smb.c    |  3 +-
>>>>>   include/linux/coresight.h                     |  6 ++
>>>>>   14 files changed, 234 insertions(+), 25 deletions(-)
>>>>>
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/ 
>>>>> drivers/ hwtracing/coresight/coresight-core.c
>>>>> index 0a9380350fb5..2e560b425fd4 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-core.c
>>>>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>>>>> @@ -23,6 +23,7 @@
>>>>>   #include "coresight-etm-perf.h"
>>>>>   #include "coresight-priv.h"
>>>>>   #include "coresight-syscfg.h"
>>>>> +#include "coresight-trace-id.h"
>>>>>   /*
>>>>>    * Mutex used to lock all sysfs enable and disable actions and 
>>>>> loading and
>>>>> @@ -331,12 +332,12 @@ static int coresight_enable_helper(struct 
>>>>> coresight_device *csdev,
>>>>>       return helper_ops(csdev)->enable(csdev, mode, data);
>>>>>   }
>>>>> -static void coresight_disable_helper(struct coresight_device *csdev)
>>>>> +static void coresight_disable_helper(struct coresight_device 
>>>>> *csdev, void *data)
>>>>>   {
>>>>> -    helper_ops(csdev)->disable(csdev, NULL);
>>>>> +    helper_ops(csdev)->disable(csdev, data);
>>>>>   }
>>>>> -static void coresight_disable_helpers(struct coresight_device *csdev)
>>>>> +static void coresight_disable_helpers(struct coresight_device 
>>>>> *csdev, void *data)
>>>>>   {
>>>>>       int i;
>>>>>       struct coresight_device *helper;
>>>>> @@ -344,7 +345,7 @@ static void coresight_disable_helpers(struct 
>>>>> coresight_device *csdev)
>>>>>       for (i = 0; i < csdev->pdata->nr_outconns; ++i) {
>>>>>           helper = csdev->pdata->out_conns[i]->dest_dev;
>>>>>           if (helper && coresight_is_helper(helper))
>>>>> -            coresight_disable_helper(helper);
>>>>> +            coresight_disable_helper(helper, data);
>>>>>       }
>>>>>   }
>>>>> @@ -361,7 +362,7 @@ static void coresight_disable_helpers(struct 
>>>>> coresight_device *csdev)
>>>>>   void coresight_disable_source(struct coresight_device *csdev, 
>>>>> void *data)
>>>>>   {
>>>>>       source_ops(csdev)->disable(csdev, data);
>>>>> -    coresight_disable_helpers(csdev);
>>>>> +    coresight_disable_helpers(csdev, NULL);
>>>>>   }
>>>>>   EXPORT_SYMBOL_GPL(coresight_disable_source);
>>>>> @@ -371,7 +372,8 @@ EXPORT_SYMBOL_GPL(coresight_disable_source);
>>>>>    * disabled.
>>>>>    */
>>>>>   static void coresight_disable_path_from(struct list_head *path,
>>>>> -                    struct coresight_node *nd)
>>>>> +                    struct coresight_node *nd,
>>>>> +                    void *sink_data)
>>>>>   {
>>>>>       u32 type;
>>>>>       struct coresight_device *csdev, *parent, *child;
>>>>> @@ -417,13 +419,13 @@ static void 
>>>>> coresight_disable_path_from(struct list_head *path,
>>>>>           }
>>>>>           /* Disable all helpers adjacent along the path last */
>>>>> -        coresight_disable_helpers(csdev);
>>>>> +        coresight_disable_helpers(csdev, sink_data);
>>>>>       }
>>>>>   }
>>>>> -void coresight_disable_path(struct list_head *path)
>>>>> +void coresight_disable_path(struct list_head *path, void *sink_data)
>>>>>   {
>>>>> -    coresight_disable_path_from(path, NULL);
>>>>> +    coresight_disable_path_from(path, NULL, sink_data);
>>>>>   }
>>>>>   EXPORT_SYMBOL_GPL(coresight_disable_path);
>>>>> @@ -505,10 +507,47 @@ int coresight_enable_path(struct list_head 
>>>>> *path, enum cs_mode mode,
>>>>>   out:
>>>>>       return ret;
>>>>>   err:
>>>>> -    coresight_disable_path_from(path, nd);
>>>>> +    coresight_disable_path_from(path, nd, sink_data);
>>>>>       goto out;
>>>>>   }
>>>>> +int coresight_read_traceid(struct list_head *path, enum cs_mode mode,
>>>>> +               struct coresight_trace_id_map *id_map)
>>>>> +{
>>>>> +    int trace_id, type;
>>>>> +    struct coresight_device *csdev;
>>>>> +    struct coresight_node *nd;
>>>>> +
>>>>> +    list_for_each_entry(nd, path, link) {
>>>>
>>>> What do you think about also changing the path to this:
>>>>
>>>>   struct coresight_path {
>>>>     struct list_head *path,
>>>>     u8 trace_id
>>>>   };
>>>>
>>> That's better, I can simplify the coresight_read_traceid function 
>>> without traverse the path.
>>>
>>> But we still need to check the type of the coresight device, because 
>>> the TPDM does not have traceid and we use the trace_id from the TPDA 
>>> device that the TPDM connected. That's why I added trace_id to link_ops.
>>>
>>
>> But if any device that allocates a trace ID saves it into the path, 
>> then as long as any other device that needs the ID is enabled after 
>> that it just reads it from the path directly. Assuming we pass the 
>> path to every enable and disable function.
>>
>> We wouldn't need coresight_read_traceid() if it always happens that 
>> way around, which I think it currently does?
>>
> I got your point here. You are right. If we passed path to the helper 
> device, just use coresight_get_source to obtain the source device, then 
> call the source_ops->trace_id to obtain the trace_id. So we definitely 
> dont need a standalone function, coresight_read_traceid().
> 
> Besides, I still need a function to retrive the trace_id of the TPDA 
> device if the source device is TPDM, right?
> 
> 
> Thanks,
> Jie
> 

Yes, and that would require a search as the TPDA not always at one end 
of the path like coresight_get_source() and coresight_get_sink(). Which 
is why I was thinking it might be good to save the trace ID in the path 
struct to avoid it.

>>>
>>>> That would avoid having to traverse the path on every enable and 
>>>> would remove this function. You could also cache the trace ID in the 
>>>> CTCU for a similar benefit, but it wouldn't remove the need to call 
>>>> this at least once.
>>>>
>>>> The expensive part should be the create path part, after that enable 
>>>> and disable should be cheap because they happen on schedule for Perf 
>>>> mode. We should be avoiding allocations and searches.
>>>>
>>> I can move the read trace_id logic to build_path function, I believe 
>>> it's better than current solution.
>>>
>>>>> +        csdev = nd->csdev;
>>>>> +        type = csdev->type;
>>>>> +
>>>>> +        switch (type) {
>>>>> +        case CORESIGHT_DEV_TYPE_SOURCE:
>>>>> +            if (source_ops(csdev)->trace_id != NULL) {
>>>>> +                trace_id = source_ops(csdev)->trace_id(csdev,
>>>>> +                                       mode,
>>>>> +                                       id_map);
>>>>> +                if (IS_VALID_CS_TRACE_ID(trace_id))
>>>>> +                    goto out;
>>>>> +            }
>>>>> +            break;
>>>>> +        case CORESIGHT_DEV_TYPE_LINK:
>>>>> +            if (link_ops(csdev)->trace_id != NULL) {
>>>>> +                trace_id = link_ops(csdev)->trace_id(csdev);
>>>>> +                if (IS_VALID_CS_TRACE_ID(trace_id))
>>>>> +                    goto out;
>>>>> +            }
>>>>> +            break;
>>>>> +        default:
>>>>> +            break;
>>>>> +        }
>>>>> +    }
>>>>> +    return -EINVAL;
>>>>> +out:
>>>>> +    return trace_id;
>>>>> +}
>>>>> +
>>>>>   struct coresight_device *coresight_get_sink(struct list_head *path)
>>>>>   {
>>>>>       struct coresight_device *csdev;
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/ 
>>>>> drivers/ hwtracing/coresight/coresight-etb10.c
>>>>> index aea9ac9c4bd0..904b5531c256 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-etb10.c
>>>>> +++ b/drivers/hwtracing/coresight/coresight-etb10.c
>>>>> @@ -173,7 +173,8 @@ static int etb_enable_perf(struct 
>>>>> coresight_device *csdev, void *data)
>>>>>       pid_t pid;
>>>>>       unsigned long flags;
>>>>>       struct etb_drvdata *drvdata = dev_get_drvdata(csdev- 
>>>>> >dev.parent);
>>>>> -    struct perf_output_handle *handle = data;
>>>>> +    struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
>>>>> +    struct perf_output_handle *handle = sink_data->handle;
>>>>>       struct cs_buffers *buf = etm_perf_sink_config(handle);
>>>>>       spin_lock_irqsave(&drvdata->spinlock, flags);
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/ 
>>>>> drivers/hwtracing/coresight/coresight-etm-perf.c
>>>>> index ad6a8f4b70b6..e676edd42ddc 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
>>>>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
>>>>> @@ -459,6 +459,7 @@ static void etm_event_start(struct perf_event 
>>>>> *event, int flags)
>>>>>       struct perf_output_handle *handle = &ctxt->handle;
>>>>>       struct coresight_device *sink, *csdev = per_cpu(csdev_src, cpu);
>>>>>       struct list_head *path;
>>>>> +    struct cs_sink_data *sink_data = NULL;
>>>>>       u64 hw_id;
>>>>>       u8 trace_id;
>>>>> @@ -498,9 +499,20 @@ static void etm_event_start(struct perf_event 
>>>>> *event, int flags)
>>>>>       if (WARN_ON_ONCE(!sink))
>>>>>           goto fail_end_stop;
>>>>> +    sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL);
>>>>
>>>> kzalloc can't be called from here. Check dmesg for the warning. 
>>>> That's another reason to do this change on the path. Because the 
>>>> path is allocated on etm_setup_aux() where allocations are allowed.
>>>>
>>> Will fix it.
>>>
>>>
>>>>> +    if (!sink_data)
>>>>> +        goto fail_end_stop;
>>>>> +
>>>>> +    sink_data->sink = sink;
>>>>> +    sink_data->traceid = coresight_read_traceid(path, CS_MODE_PERF,
>>>>> +                            &sink->perf_sink_id_map);
>>>>> +    sink_data->handle = handle;
>>>>> +
>>>>>       /* Nothing will happen without a path */
>>>>> -    if (coresight_enable_path(path, CS_MODE_PERF, handle))
>>>>> +    if (coresight_enable_path(path, CS_MODE_PERF, sink_data)) {
>>>>> +        kfree(sink_data);
>>>>>           goto fail_end_stop;
>>>>> +    }
>>>>>       /* Finally enable the tracer */
>>>>>       if (source_ops(csdev)->enable(csdev, event, CS_MODE_PERF,
>>>>> @@ -526,6 +538,7 @@ static void etm_event_start(struct perf_event 
>>>>> *event, int flags)
>>>>>           perf_report_aux_output_id(event, hw_id);
>>>>>       }
>>>>> +    kfree(sink_data);
>>>>>   out:
>>>>>       /* Tell the perf core the event is alive */
>>>>>       event->hw.state = 0;
>>>>> @@ -534,7 +547,8 @@ static void etm_event_start(struct perf_event 
>>>>> *event, int flags)
>>>>>       return;
>>>>>   fail_disable_path:
>>>>> -    coresight_disable_path(path);
>>>>> +    coresight_disable_path(path, sink_data);
>>>>> +    kfree(sink_data);
>>>>>   fail_end_stop:
>>>>>       /*
>>>>>        * Check if the handle is still associated with the event,
>>>>> @@ -559,6 +573,7 @@ static void etm_event_stop(struct perf_event 
>>>>> *event, int mode)
>>>>>       struct perf_output_handle *handle = &ctxt->handle;
>>>>>       struct etm_event_data *event_data;
>>>>>       struct list_head *path;
>>>>> +    struct cs_sink_data *sink_data = NULL;
>>>>>       /*
>>>>>        * If we still have access to the event_data via handle,
>>>>> @@ -603,6 +618,10 @@ static void etm_event_stop(struct perf_event 
>>>>> *event, int mode)
>>>>>       if (!sink)
>>>>>           return;
>>>>> +    sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL);
>>>>
>>>> Same problem as above.
>>>>
>>>>> +    if (!sink_data)
>>>>> +        return;
>>>>> +
>>>>>       /* stop tracer */
>>>>>       coresight_disable_source(csdev, event);
>>>>> @@ -616,12 +635,16 @@ static void etm_event_stop(struct perf_event 
>>>>> *event, int mode)
>>>>>        * have to do anything here.
>>>>>        */
>>>>>       if (handle->event && (mode & PERF_EF_UPDATE)) {
>>>>> -        if (WARN_ON_ONCE(handle->event != event))
>>>>> +        if (WARN_ON_ONCE(handle->event != event)) {
>>>>> +            kfree(sink_data);
>>>>>               return;
>>>>> +        }
>>>>>           /* update trace information */
>>>>> -        if (!sink_ops(sink)->update_buffer)
>>>>> +        if (!sink_ops(sink)->update_buffer) {
>>>>> +            kfree(sink_data);
>>>>>               return;
>>>>> +        }
>>>>>           size = sink_ops(sink)->update_buffer(sink, handle,
>>>>>                             event_data->snk_config);
>>>>> @@ -642,8 +665,12 @@ static void etm_event_stop(struct perf_event 
>>>>> *event, int mode)
>>>>>               WARN_ON(size);
>>>>>       }
>>>>> +    sink_data->sink = sink;
>>>>> +    sink_data->traceid = coresight_read_traceid(path, CS_MODE_PERF,
>>>>> +                            &sink->perf_sink_id_map);
>>>>>       /* Disabling the path make its elements available to other 
>>>>> sessions */
>>>>> -    coresight_disable_path(path);
>>>>> +    coresight_disable_path(path, sink_data);
>>>>> +    kfree(sink_data);
>>>>>   }
>>>>>   static int etm_event_add(struct perf_event *event, int mode)
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/ 
>>>>> drivers/hwtracing/coresight/coresight-etm3x-core.c
>>>>> index c103f4c70f5d..0f095fab7e0a 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
>>>>> +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
>>>>> @@ -697,10 +697,40 @@ static void etm_disable(struct 
>>>>> coresight_device *csdev,
>>>>>           coresight_set_mode(csdev, CS_MODE_DISABLED);
>>>>>   }
>>>>> +static int etm_trace_id(struct coresight_device *csdev,
>>>>> +            enum cs_mode mode,
>>>>> +            struct coresight_trace_id_map *id_map)
>>>>> +{
>>>>> +    int trace_id;
>>>>> +    struct etm_drvdata *drvdata;
>>>>> +
>>>>> +    if (csdev == NULL)
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    drvdata = dev_get_drvdata(csdev->dev.parent);
>>>>> +    switch (mode) {
>>>>> +    case CS_MODE_SYSFS:
>>>>> +        trace_id = etm_read_alloc_trace_id(drvdata);
>>>>> +        break;
>>>>> +    case CS_MODE_PERF:
>>>>> +        trace_id = coresight_trace_id_read_cpu_id_map(drvdata- 
>>>>> >cpu, id_map);
>>>>> +        if (IS_VALID_CS_TRACE_ID(trace_id))
>>>>> +            drvdata->traceid = (u8)trace_id;
>>>>> +        break;
>>>>> +    default:
>>>>> +        trace_id = -EINVAL;
>>>>> +        break;
>>>>> +    }
>>>>> +
>>>>> +    return trace_id;
>>>>> +}
>>>>> +
>>>>> +
>>>>>   static const struct coresight_ops_source etm_source_ops = {
>>>>>       .cpu_id        = etm_cpu_id,
>>>>>       .enable        = etm_enable,
>>>>>       .disable    = etm_disable,
>>>>> +    .trace_id    = etm_trace_id,
>>>>>   };
>>>>>   static const struct coresight_ops etm_cs_ops = {
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/ 
>>>>> drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>>> index dd8c74f893db..bac9ea371688 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>>> @@ -1025,10 +1025,39 @@ static void etm4_disable(struct 
>>>>> coresight_device *csdev,
>>>>>           coresight_set_mode(csdev, CS_MODE_DISABLED);
>>>>>   }
>>>>> +static int etm4_trace_id(struct coresight_device *csdev,
>>>>> +             enum cs_mode mode,
>>>>> +             struct coresight_trace_id_map *id_map)
>>>>> +{
>>>>> +    int trace_id;
>>>>> +    struct etmv4_drvdata *drvdata;
>>>>> +
>>>>> +    if (csdev == NULL)
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    drvdata = dev_get_drvdata(csdev->dev.parent);
>>>>> +    switch (mode) {
>>>>> +    case CS_MODE_SYSFS:
>>>>> +        trace_id = etm4_read_alloc_trace_id(drvdata);
>>>>> +        break;
>>>>> +    case CS_MODE_PERF:
>>>>> +        trace_id = coresight_trace_id_read_cpu_id_map(drvdata- 
>>>>> >cpu, id_map);
>>>>> +        if (IS_VALID_CS_TRACE_ID(trace_id))
>>>>> +            drvdata->trcid = (u8)trace_id;
>>>>> +        break;
>>>>> +    default:
>>>>> +        trace_id = -EINVAL;
>>>>> +        break;
>>>>> +    }
>>>>> +
>>>>> +    return trace_id;
>>>>> +}
>>>>> +
>>>>>   static const struct coresight_ops_source etm4_source_ops = {
>>>>>       .cpu_id        = etm4_cpu_id,
>>>>>       .enable        = etm4_enable,
>>>>>       .disable    = etm4_disable,
>>>>> +    .trace_id    = etm4_trace_id,
>>>>>   };
>>>>>   static const struct coresight_ops etm4_cs_ops = {
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/ 
>>>>> drivers/ hwtracing/coresight/coresight-priv.h
>>>>> index 05f891ca6b5c..6a95a3ba9339 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-priv.h
>>>>> +++ b/drivers/hwtracing/coresight/coresight-priv.h
>>>>> @@ -105,6 +105,15 @@ struct cs_buffers {
>>>>>       void            **data_pages;
>>>>>   };
>>>>> +/**
>>>>> + * struct cs_sink_data - data used by coresight_enable_path/ 
>>>>> coresight_disable_path
>>>>> + */
>>>>> +struct cs_sink_data {
>>>>> +    struct perf_output_handle    *handle;
>>>>> +    struct coresight_device        *sink;
>>>>> +    u8                traceid;
>>>>> +};
>>>>> +
>>>>>   static inline void coresight_insert_barrier_packet(void *buf)
>>>>>   {
>>>>>       if (buf)
>>>>> @@ -129,9 +138,11 @@ static inline void CS_UNLOCK(void __iomem *addr)
>>>>>       } while (0);
>>>>>   }
>>>>> -void coresight_disable_path(struct list_head *path);
>>>>> +void coresight_disable_path(struct list_head *path, void *sink_data);
>>>>>   int coresight_enable_path(struct list_head *path, enum cs_mode mode,
>>>>>                 void *sink_data);
>>>>> +int coresight_read_traceid(struct list_head *path, enum cs_mode mode,
>>>>> +               struct coresight_trace_id_map *id_map);
>>>>>   struct coresight_device *coresight_get_sink(struct list_head *path);
>>>>>   struct coresight_device *coresight_get_sink_by_id(u32 id);
>>>>>   struct coresight_device *
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/ 
>>>>> hwtracing/coresight/coresight-stm.c
>>>>> index b581a30a1cd9..5709eda77d9b 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-stm.c
>>>>> +++ b/drivers/hwtracing/coresight/coresight-stm.c
>>>>> @@ -281,9 +281,31 @@ static void stm_disable(struct 
>>>>> coresight_device *csdev,
>>>>>       }
>>>>>   }
>>>>> +static int stm_trace_id(struct coresight_device *csdev,
>>>>> +            enum cs_mode mode,
>>>>> +            struct coresight_trace_id_map *id_map)
>>>>> +{
>>>>> +    int trace_id;
>>>>> +    struct stm_drvdata *drvdata;
>>>>> +
>>>>> +    if (csdev == NULL)
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    drvdata = dev_get_drvdata(csdev->dev.parent);
>>>>> +    trace_id = drvdata->traceid;
>>>>> +    if (!IS_VALID_CS_TRACE_ID(trace_id)) {
>>>>
>>>> Trace ID is static and always set for STM so this isn't necessary, 
>>>> just return drvdata->traceid which would already be valid otherwise 
>>>> the device wouldn't be registered.
>>>>
>>> Thanks for comment.
>>> Will fix in next version
>>>
>>>
>>>>> +        trace_id = coresight_trace_id_get_system_id();
>>>>> +        if (IS_VALID_CS_TRACE_ID(trace_id))
>>>>> +            drvdata->traceid = (u8)trace_id;
>>>>> +    }
>>>>> +
>>>>> +    return trace_id;
>>>>> +}
>>>>> +
>>>>>   static const struct coresight_ops_source stm_source_ops = {
>>>>>       .enable        = stm_enable,
>>>>>       .disable    = stm_disable,
>>>>> +    .trace_id    = stm_trace_id,
>>>>>   };
>>>>>   static const struct coresight_ops stm_cs_ops = {
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/ 
>>>>> drivers/ hwtracing/coresight/coresight-sysfs.c
>>>>> index a01c9e54e2ed..8816bccba374 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-sysfs.c
>>>>> +++ b/drivers/hwtracing/coresight/coresight-sysfs.c
>>>>> @@ -168,6 +168,7 @@ int coresight_enable_sysfs(struct 
>>>>> coresight_device *csdev)
>>>>>       int cpu, ret = 0;
>>>>>       struct coresight_device *sink;
>>>>>       struct list_head *path;
>>>>> +    struct cs_sink_data *sink_data;
>>>>>       enum coresight_dev_subtype_source subtype;
>>>>>       u32 hash;
>>>>> @@ -209,7 +210,14 @@ int coresight_enable_sysfs(struct 
>>>>> coresight_device *csdev)
>>>>>           goto out;
>>>>>       }
>>>>> -    ret = coresight_enable_path(path, CS_MODE_SYSFS, NULL);
>>>>> +    sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL);
>>>>> +    if (!sink_data) {
>>>>> +        ret = -ENOMEM;
>>>>> +        goto out;
>>>>> +    }
>>>>> +    sink_data->traceid = coresight_read_traceid(path, 
>>>>> CS_MODE_SYSFS, NULL);
>>>>> +    sink_data->sink = sink;
>>>>> +    ret = coresight_enable_path(path, CS_MODE_SYSFS, sink_data);
>>>>>       if (ret)
>>>>>           goto err_path;
>>>>> @@ -246,15 +254,17 @@ int coresight_enable_sysfs(struct 
>>>>> coresight_device *csdev)
>>>>>           break;
>>>>>       }
>>>>> +    kfree(sink_data);
>>>>>   out:
>>>>>       mutex_unlock(&coresight_mutex);
>>>>>       return ret;
>>>>>   err_source:
>>>>> -    coresight_disable_path(path);
>>>>> +    coresight_disable_path(path, sink_data);
>>>>>   err_path:
>>>>>       coresight_release_path(path);
>>>>> +    kfree(sink_data);
>>>>>       goto out;
>>>>>   }
>>>>>   EXPORT_SYMBOL_GPL(coresight_enable_sysfs);
>>>>> @@ -263,6 +273,7 @@ void coresight_disable_sysfs(struct 
>>>>> coresight_device *csdev)
>>>>>   {
>>>>>       int cpu, ret;
>>>>>       struct list_head *path = NULL;
>>>>> +    struct cs_sink_data *sink_data = NULL;
>>>>>       u32 hash;
>>>>>       mutex_lock(&coresight_mutex);
>>>>> @@ -274,6 +285,10 @@ void coresight_disable_sysfs(struct 
>>>>> coresight_device *csdev)
>>>>>       if (!coresight_disable_source_sysfs(csdev, NULL))
>>>>>           goto out;
>>>>> +    sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL);
>>>>> +    if (!sink_data)
>>>>> +        goto out;
>>>>> +
>>>>>       switch (csdev->subtype.source_subtype) {
>>>>>       case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC:
>>>>>           cpu = source_ops(csdev)->cpu_id(csdev);
>>>>> @@ -297,8 +312,11 @@ void coresight_disable_sysfs(struct 
>>>>> coresight_device *csdev)
>>>>>           break;
>>>>>       }
>>>>> -    coresight_disable_path(path);
>>>>> +    sink_data->sink = coresight_find_activated_sysfs_sink(csdev);
>>>>> +    sink_data->traceid = coresight_read_traceid(path, 
>>>>> CS_MODE_SYSFS, NULL);
>>>>> +    coresight_disable_path(path, sink_data);
>>>>>       coresight_release_path(path);
>>>>> +    kfree(sink_data);
>>>>>   out:
>>>>>       mutex_unlock(&coresight_mutex);
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/ 
>>>>> drivers/hwtracing/coresight/coresight-tmc-etf.c
>>>>> index d4f641cd9de6..7dc536eba3e2 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
>>>>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
>>>>> @@ -250,7 +250,8 @@ static int tmc_enable_etf_sink_perf(struct 
>>>>> coresight_device *csdev, void *data)
>>>>>       pid_t pid;
>>>>>       unsigned long flags;
>>>>>       struct tmc_drvdata *drvdata = dev_get_drvdata(csdev- 
>>>>> >dev.parent);
>>>>> -    struct perf_output_handle *handle = data;
>>>>> +    struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
>>>>> +    struct perf_output_handle *handle = sink_data->handle;
>>>>>       struct cs_buffers *buf = etm_perf_sink_config(handle);
>>>>>       spin_lock_irqsave(&drvdata->spinlock, flags);
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/ 
>>>>> drivers/hwtracing/coresight/coresight-tmc-etr.c
>>>>> index a48bb85d0e7f..90a4058c9959 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
>>>>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>>>>> @@ -1254,7 +1254,8 @@ static int tmc_enable_etr_sink_sysfs(struct 
>>>>> coresight_device *csdev)
>>>>>   struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev,
>>>>>                      enum cs_mode mode, void *data)
>>>>>   {
>>>>> -    struct perf_output_handle *handle = data;
>>>>> +    struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
>>>>> +    struct perf_output_handle *handle = sink_data->handle;
>>>>>       struct etr_perf_buffer *etr_perf;
>>>>>       switch (mode) {
>>>>> @@ -1648,7 +1649,8 @@ static int tmc_enable_etr_sink_perf(struct 
>>>>> coresight_device *csdev, void *data)
>>>>>       pid_t pid;
>>>>>       unsigned long flags;
>>>>>       struct tmc_drvdata *drvdata = dev_get_drvdata(csdev- 
>>>>> >dev.parent);
>>>>> -    struct perf_output_handle *handle = data;
>>>>> +    struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
>>>>> +    struct perf_output_handle *handle = sink_data->handle;
>>>>>       struct etr_perf_buffer *etr_perf = etm_perf_sink_config(handle);
>>>>>       spin_lock_irqsave(&drvdata->spinlock, flags);
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/ 
>>>>> drivers/ hwtracing/coresight/coresight-tpda.c
>>>>> index 189a4abc2561..05622333f2d2 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>>>> @@ -241,9 +241,29 @@ static void tpda_disable(struct 
>>>>> coresight_device *csdev,
>>>>>       dev_dbg(drvdata->dev, "TPDA inport %d disabled\n", in- 
>>>>> >dest_port);
>>>>>   }
>>>>> +static int tpda_trace_id(struct coresight_device *csdev)
>>>>> +{
>>>>> +    int trace_id;
>>>>> +    struct tpda_drvdata *drvdata;
>>>>> +
>>>>> +    if (csdev == NULL)
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    drvdata = dev_get_drvdata(csdev->dev.parent);
>>>>> +    trace_id = drvdata->atid;
>>>>> +    if (!IS_VALID_CS_TRACE_ID(trace_id)) {
>>>>
>>>> Same comment as STM. This is always set so no ned to try to assign 
>>>> another one.
>>>>
>>> Will fix.
>>>
>>> [...]
>>>
>>> Thanks,
>>> Jie
>>>
>>
> 


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ