CoreSight

coresight@lists.linaro.org

7 participants
2442 discussions

Re: [PATCH v9 3/6] Coresight: Introduce a new struct coresight_path

by James Clark

On 24/01/2025 7:25 am, Jie Gan wrote: > Add 'struct coresight_path' 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. > > Signed-off-by: Jie Gan <quic_jiegan(a)quicinc.com> > --- > drivers/hwtracing/coresight/coresight-core.c | 87 ++++++++++++++----- > drivers/hwtracing/coresight/coresight-etb10.c | 3 +- > .../hwtracing/coresight/coresight-etm-perf.c | 52 ++++++----- > .../hwtracing/coresight/coresight-etm-perf.h | 2 +- > drivers/hwtracing/coresight/coresight-priv.h | 21 +++-- > drivers/hwtracing/coresight/coresight-sysfs.c | 32 +++---- > .../hwtracing/coresight/coresight-tmc-etf.c | 3 +- > .../hwtracing/coresight/coresight-tmc-etr.c | 6 +- > drivers/hwtracing/coresight/coresight-trbe.c | 4 +- > drivers/hwtracing/coresight/ultrasoc-smb.c | 3 +- > 10 files changed, 137 insertions(+), 76 deletions(-) > [...] > INIT_LIST_HEAD(path); > + cs_path->path = path; > + /* > + * Since not all source devices have a defined trace_id function, > + * make sure to check for it before use. > + * > + * Assert the mode is CS_MODE_SYSFS, the trace_id will be assigned > + * again later if the mode is CS_MODE_PERF. > + */ > + if (source_ops(source)->trace_id != NULL) { > + rc = source_ops(source)->trace_id(source, CS_MODE_SYSFS, NULL); I don't think we should do this. Doesn't this consume two trace IDs for each session? And I'm not even sure if it's released properly if it's overwritten. It should be possible to consolidate the all the trace ID allocation to a single step when building the path, or another function that gets called just after the path is built. At the moment the ID can be allocated from about 5 different places and it's quite hard to understand, especially with these new changes. I have some of it coded up, let me finish it off and I can share it. > + if(IS_VALID_CS_TRACE_ID(rc)) > + cs_path->trace_id = rc; > + else > + cs_path->trace_id = 0; > + } > + else > + cs_path->trace_id = 0; [...] > +/** > + * struct coresight_path - data needed by enable/disable path > + * @handle: perf aux handle for ETM. > + * @path: path from source to sink. > + * @trace_id: trace_id of the whole path. > + */ > +struct coresight_path { > + struct perf_output_handle *handle; This is only needed to avoid adding *handle to the enable function call signature, but having it here implies it needs to be stored. And then we need to manage the lifecycle of it by nulling it on deletion. All of this can be avoided by just adding handle to enable(). Unrelated to this patch, but I'm not sure why we were passing around void* for handle either. It just makes the code hard to read and implies some flexibility that doesn't exist. It's always "struct perf_output_handle", so we can change void* to that in the enable functions. I also have a patch for this that I'll share in a bit. > + struct list_head *path; > + u8 trace_id; > +}; > + > static inline void coresight_insert_barrier_packet(void *buf) > { > if (buf) > @@ -132,16 +144,15 @@ static inline void CS_UNLOCK(void __iomem *addr) > } while (0); > } > > -void coresight_disable_path(struct list_head *path); > -int coresight_enable_path(struct list_head *path, enum cs_mode mode, > - void *sink_data); > +void coresight_disable_path(struct coresight_path *cs_path); > +int coresight_enable_path(struct coresight_path *cs_path, enum cs_mode mode); > struct coresight_device *coresight_get_sink(struct list_head *path); This needs to be exported otherwise the build fails because you use it in a module in another commit. I assume you are building as static?

6 months, 4 weeks

Re: [PATCH v8 2/5] Coresight: Add trace_id function to retrieving the trace ID

by James Clark

On 23/01/2025 6:28 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. >> >>> Signed-off-by: Jie Gan <quic_jiegan(a)quicinc.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 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. >> >>> + 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. >> > Hi, James > I just tried with following command and did not observe any warning info > from dmesg, may I ask what's the issue may suffered here? > You might be missing some debugging configs like lockdep etc. The warning is that etm_event_start() is a non-sleepable context and kzalloc is sleepable. Even if it wasn't an error we still wouldn't want to do it, etm_event_start() and stop are called too frequently. > root@qemuarm64:/data# ./perf record -e cs_etm/@tmc_etr0/ --per-thread ls > configs kernel.txt logs lost+found misc > perf perf.data perf.data.old root time > tzstorage weston > [ perf record: Woken up 1 times to write data ] > [ perf record: Captured and wrote 0.145 MB perf.data ] > > For the new patch version, I implemented an 8-bit hash table in the CTCU > driver data to handle situations where multiple TPDMs are connected to > the same TPDA device have been enabled. As we know, TPDMs share the > trace_id of the TPDA device they are connected to. If we reset the bit > based on the trace_id without checking the enabled refcount, it causes > an issue where trace data from other enabled TPDM devices (sharing the > same trace_id) cannot enter the ETR buffer, as it gets filtered out by > the CTCU. I think sharing the code or a diagram might be easier to follow here. The mention of a refcount makes sense but I don't follow the need for a hash table. There are other places where single devices are shared by multiple paths, like funnels, and they're all done with refcounts. > I need allocate memory when implement hash table(add/remove key entry) > in coresight_enable_path flow, but you mentioned we cannot call kzalloc > from here. > > Thanks, > Jie > Why not allocate on setup_aux()? That's called by userspace before the session starts, and then the path is fixed from that point onwards so you shouldn't need to do any more allocations. That's how it's setup currently anyway.

7 months

[PATCH v4 0/2] coresight: Add remote etm support

by Mao Jinlong

The system on chip (SoC) consists of main APSS(Applications processor subsytem) and additional processors like modem, lpass. There is coresight-etm driver for etm trace of APSS. Coresight remote etm driver is for enabling and disabling the etm trace of remote processors. It uses QMI interface to communicate with remote processors' software and uses coresight framework to configure the connection from remote etm source to TMC sinks. Example to capture the remote etm trace: Enable source: echo 1 > /sys/bus/coresight/devices/tmc_etf0/enable_sink echo 1 > /sys/bus/coresight/devices/remote_etm0/enable_source Capture the trace: cat /dev/tmc_etf0 > /data/remote_etm.bin Disable source: echo 0 > /sys/bus/coresight/devices/remote_etm0/enable_source Changes since V3: 1. Use different compatible for different remote etms in dt. 2. Get qmi instance id from the match table data in driver. Change since V2: 1. Change qcom,inst-id to qcom,qmi-id 2. Fix the error in code for type of remote_etm_remove 3. Depend on QMI helper in Kconfig Changes since V1: 1. Remove unused content 2. Use CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS as remote etm source type. 3. Use enabled instead of enable in driver data. 4. Validate instance id value where it's read from the DT. Mao Jinlong (2): dt-bindings: arm: Update compatible for remote etm coresight: Add remote etm support .../arm/qcom,coresight-remote-etm.yaml | 11 +- drivers/hwtracing/coresight/Kconfig | 13 + drivers/hwtracing/coresight/Makefile | 1 + drivers/hwtracing/coresight/coresight-qmi.h | 89 +++++ .../coresight/coresight-remote-etm.c | 316 ++++++++++++++++++ 5 files changed, 428 insertions(+), 2 deletions(-) create mode 100644 drivers/hwtracing/coresight/coresight-qmi.h create mode 100644 drivers/hwtracing/coresight/coresight-remote-etm.c -- 2.17.1

7 months

Re: [PATCH] coresight: Use str_enable_disable-like helpers

by James Clark

On 14/01/2025 7:27 pm, Krzysztof Kozlowski wrote: > Replace ternary (condition ? "enable" : "disable") syntax with helpers > from string_choices.h because: > 1. Simple function call with one argument is easier to read. Ternary > operator has three arguments and with wrapping might lead to quite > long code. > 2. Is slightly shorter thus also easier to read. > 3. It brings uniformity in the text - same string. > 4. Allows deduping by the linker, which results in a smaller binary > file. > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski(a)linaro.org> > --- > drivers/hwtracing/coresight/coresight-config.c | 3 ++- > drivers/hwtracing/coresight/coresight-cpu-debug.c | 3 ++- > 2 files changed, 4 insertions(+), 2 deletions(-) > Reviewed-by: James Clark <james.clark(a)linaro.org>

7 months, 1 week

Re: [PATCH v8 2/5] Coresight: Add trace_id function to retrieving the trace ID

by James Clark

On 16/01/2025 3:01 am, Jie Gan wrote: > > > On 1/15/2025 8:29 PM, James Clark wrote: >> >> >> 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(a)quicinc.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. >> > As you proposed, I created coresight_path structure as below: > struct coresight_path { > struct perf_output_handle *handle; What's the perf handle for? Seems like this change for the CTCU only requires access to the trace_id which is added below. > struct list_head *path; > u8 trace_id; > }; > > In coresight_enable_path, I modified the parameters that transmitted to > helper device: > struct coresight_path *cs_path; > > coresight_enable_helpers(csdev, mode, sink_data) -> > coresight_enable_helpers(csdev, mode, cs_path) > > The cs_path will be constructed and initialized in coresight_build_path > function. > > For perf mode, the trace_id is collected within etm_setup_aux and stored > in cs_path->trace_id to avoid extra cost of retrieving the trace_id; > Looks good. > For sysfs mode, I let the CTCU device to retrieving the trace_id with > path. The TPDA device is located close to the TPDM, making the cost of > searching for the TPDA device acceptable. > > The cs_path will be stored in the same manner as the previous path. > > How do you think about this solution? > Can it not be done the same way as Perf, at least for consistency? If the enable helper function already gets access to the path, and the path already has the trace ID, why is any search required? > Thanks, > Jie > > [...] >

7 months, 1 week

[PATCH v3] coresight-etm4x: add isb() before reading the TRCSTATR

by yuanfang zhang

From: Yuanfang Zhang <quic_yuanfang(a)quicinc.com> As recommended by section 4.3.7 ("Synchronization when using system instructions to progrom the trace unit") of ARM IHI 0064H.b, the self-hosted trace analyzer must perform a Context synchronization event between writing to the TRCPRGCTLR and reading the TRCSTATR. Fixes: ebddaad09e10 ("coresight: etm4x: Add missing single-shot control API to sysfs") Signed-off-by: Yuanfang Zhang <quic_yuanfang(a)quicinc.com> --- Changes in V3: 1. Remove dsb(sy) after polling TRCSTATR. 2. Add isb() after polling TRCSTATR. --- .../hwtracing/coresight/coresight-etm4x-core.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 66d44a404ad0..c6ea00bba0cc 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -531,7 +531,6 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata) * As recommended by section 4.3.7 ("Synchronization when using the * memory-mapped interface") of ARM IHI 0064D */ - dsb(sy); isb(); done: @@ -906,10 +905,25 @@ static void etm4_disable_hw(void *info) tsb_csync(); etm4x_relaxed_write32(csa, control, TRCPRGCTLR); + /* + * As recommended by section 4.3.7 ("Synchronization when using system + * instructions to progrom the trace unit") of ARM IHI 0064H.b, the + * self-hosted trace analyzer must perform a Context synchronization + * event between writing to the TRCPRGCTLR and reading the TRCSTATR. + */ + if (!csa->io_mem) + isb(); + /* wait for TRCSTATR.PMSTABLE to go to '1' */ if (coresight_timeout(csa, TRCSTATR, TRCSTATR_PMSTABLE_BIT, 1)) dev_err(etm_dev, "timeout while waiting for PM stable Trace Status\n"); + /* + * As recommended by section 4.3.7 (Synchronization of register updates) + * of ARM IHI 0064H.b. + */ + isb(); + /* read the status of the single shot comparators */ for (i = 0; i < drvdata->nr_ss_cmp; i++) { config->ss_status[i] = -- 2.34.1

7 months, 1 week

Re: [PATCH v8 2/5] Coresight: Add trace_id function to retrieving the trace ID

by James Clark

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(a)quicinc.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 >>> >> >

7 months, 1 week

Re: [PATCH v10 04/10] arm64/sysreg: Add a comment that the sysreg file should be sorted

by James Clark

On 14/01/2025 6:16 pm, Rob Herring wrote: > On Mon, Jan 13, 2025 at 10:49 AM Marc Zyngier <maz(a)kernel.org> wrote: >> >> On Mon, 13 Jan 2025 15:43:39 +0000, >> James Clark <james.clark(a)linaro.org> wrote: >>> >>> >>> >>> On 12/01/2025 12:49 pm, Marc Zyngier wrote: >>>> On Tue, 07 Jan 2025 11:32:41 +0000, >>>> James Clark <james.clark(a)linaro.org> wrote: >>>>> >>>>> From: James Clark <james.clark(a)arm.com> >>>>> >>>>> There are a few entries particularly at the end of the file that aren't >>>>> in order. To avoid confusion, add a comment that might help new entries >>>>> to be added in the right place. >>>>> >>>>> Reviewed-by: Mark Brown <broonie(a)kernel.org> >>>>> Signed-off-by: James Clark <james.clark(a)arm.com> >>>>> Signed-off-by: James Clark <james.clark(a)linaro.org> >>>>> --- >>>>> arch/arm64/tools/sysreg | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg >>>>> index b081b54d6d22..4ba167089e2a 100644 >>>>> --- a/arch/arm64/tools/sysreg >>>>> +++ b/arch/arm64/tools/sysreg >>>>> @@ -48,6 +48,8 @@ >>>>> # feature that introduces them (eg, FEAT_LS64_ACCDATA introduces enumeration >>>>> # item ACCDATA) though it may be more taseful to do something else. >>>>> +# Please try to keep entries in this file sorted by sysreg >>>>> encoding. >>>>> + >>>>> Sysreg OSDTRRX_EL1 2 0 0 0 2 >>>>> Res0 63:32 >>>>> Field 31:0 DTRRX >>>> >>>> "Do as I say, don't do as I do". >>>> >>>> I don't think this makes any sense if we don't actually sort the file >>>> the first place. >>>> >>>> M. >>>> >>> >>> I think it's ok if it avoids review comments that new entries should >>> be sorted. Or maybe we do the opposite and the comment should say this >>> file is allowed to be unsorted... >> >> The better option would be to add the comment *and* sort the file. >> Leading by example has some value, it seems. > > IME, it's better if documentation just states what the tools enforce. > > Can't we add something like this to the header generation: > > $ grep '^Sysreg\s' arch/arm64/tools/sysreg | sort -n -k3 -k4 -k5 -k6 -k7 -c > sort: -:22: disorder: Sysreg ID_MMFR4_EL1 3 0 0 > 2 6 > > Rob Actually I updated gen-sysreg.awk to fail the build if it's not sorted, was just about to post it. I don't know if a build failure or a warning is preferred but I can do either. James

7 months, 1 week

Re: [PATCH FOR-NEXT] perf tools: Expose quiet/verbose variables in Makefile.perf

by Arnaldo Carvalho de Melo

On Tue, Jan 14, 2025 at 11:35:44AM -0800, Charlie Jenkins wrote: > The variables to make builds silent/verbose live inside > tools/build/Makefile.build. Move those variables to the top-level > Makefile.perf to be generally available. > > Committer testing: Re-tested, now the lines below continue to appear, and also the 'make -C tools/perf build-test' that was failiong on the first patch submission is now ok as well, so I re issue my: Tested-by: Arnaldo Carvalho de Melo <acme(a)redhat.com> At this point this will go thru Namhyung, that will process patches while I'm in vacation for perf-tools-next as well, thanks! - Arnaldo > See the SYSCALL lines, now they are consistent with the other > operations in other lines: > SYSTBL /tmp/build/perf-tools-next/arch/x86/include/generated/asm/syscalls_32.h > SYSTBL /tmp/build/perf-tools-next/arch/x86/include/generated/asm/syscalls_64.h > GEN /tmp/build/perf-tools-next/common-cmds.h > GEN /tmp/build/perf-tools-next/arch/arm64/include/generated/asm/sysreg-defs.h > PERF_VERSION = 6.13.rc2.g3d94bb6ed1d0 > GEN perf-archive > MKDIR /tmp/build/perf-tools-next/jvmti/ > MKDIR /tmp/build/perf-tools-next/jvmti/ > MKDIR /tmp/build/perf-tools-next/jvmti/ > MKDIR /tmp/build/perf-tools-next/jvmti/ > GEN perf-iostat > CC /tmp/build/perf-tools-next/jvmti/libjvmti.o > > Reported-by: Arnaldo Carvalho de Melo <acme(a)redhat.com> > Signed-off-by: Charlie Jenkins <charlie(a)rivosinc.com> > Tested-by: Arnaldo Carvalho de Melo <acme(a)redhat.com> > --- > tools/build/Makefile.build | 20 ----------------- > tools/perf/Makefile.perf | 37 ++++++++++++++++++++++++++++++- > tools/perf/tests/shell/coresight/Makefile | 2 +- > 3 files changed, 37 insertions(+), 22 deletions(-) > > diff --git a/tools/build/Makefile.build b/tools/build/Makefile.build > index 5fb3fb3d97e0fd114e245805809e4fc926b4343e..e710ed67a1b49d9fda11db02821bbd8d36066b44 100644 > --- a/tools/build/Makefile.build > +++ b/tools/build/Makefile.build > @@ -12,26 +12,6 @@ > PHONY := __build > __build: > > -ifeq ($(V),1) > - quiet = > - Q = > -else > - quiet=quiet_ > - Q=@ > -endif > - > -# If the user is running make -s (silent mode), suppress echoing of commands > -# make-4.0 (and later) keep single letter options in the 1st word of MAKEFLAGS. > -ifeq ($(filter 3.%,$(MAKE_VERSION)),) > -short-opts := $(firstword -$(MAKEFLAGS)) > -else > -short-opts := $(filter-out --%,$(MAKEFLAGS)) > -endif > - > -ifneq ($(findstring s,$(short-opts)),) > - quiet=silent_ > -endif > - > build-dir := $(srctree)/tools/build > > # Define $(fixdep) for dep-cmd function > diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf > index a449d0015536442273a9268b37be34e4757f577a..55d6ce9ea52fb2a57b8632cc6d0ddc501e29cbfc 100644 > --- a/tools/perf/Makefile.perf > +++ b/tools/perf/Makefile.perf > @@ -161,12 +161,47 @@ export VPATH > SOURCE := $(shell ln -sf $(srctree)/tools/perf $(OUTPUT)/source) > endif > > +# Beautify output > +# --------------------------------------------------------------------------- > +# > +# Most of build commands in Kbuild start with "cmd_". You can optionally define > +# "quiet_cmd_*". If defined, the short log is printed. Otherwise, no log from > +# that command is printed by default. > +# > +# e.g.) > +# quiet_cmd_depmod = DEPMOD $(MODLIB) > +# cmd_depmod = $(srctree)/scripts/depmod.sh $(DEPMOD) $(KERNELRELEASE) > +# > +# A simple variant is to prefix commands with $(Q) - that's useful > +# for commands that shall be hidden in non-verbose mode. > +# > +# $(Q)$(MAKE) $(build)=scripts/basic > +# > +# To put more focus on warnings, be less verbose as default > +# Use 'make V=1' to see the full commands > + > ifeq ($(V),1) > + quiet = > Q = > else > - Q = @ > + quiet=quiet_ > + Q=@ > endif > > +# If the user is running make -s (silent mode), suppress echoing of commands > +# make-4.0 (and later) keep single letter options in the 1st word of MAKEFLAGS. > +ifeq ($(filter 3.%,$(MAKE_VERSION)),) > +short-opts := $(firstword -$(MAKEFLAGS)) > +else > +short-opts := $(filter-out --%,$(MAKEFLAGS)) > +endif > + > +ifneq ($(findstring s,$(short-opts)),) > + quiet=silent_ > +endif > + > +export quiet Q > + > # Do not use make's built-in rules > # (this improves performance and avoids hard-to-debug behaviour); > MAKEFLAGS += -r > diff --git a/tools/perf/tests/shell/coresight/Makefile b/tools/perf/tests/shell/coresight/Makefile > index b070e779703e9fbd70f88c826172b2150ee3d302..fa08fd9a5991dd889583edc1afe8304e72278f64 100644 > --- a/tools/perf/tests/shell/coresight/Makefile > +++ b/tools/perf/tests/shell/coresight/Makefile > @@ -24,6 +24,6 @@ CLEANDIRS = $(SUBDIRS:%=clean-%) > > clean: $(CLEANDIRS) > $(CLEANDIRS): > - $(call QUIET_CLEAN, test-$(@:clean-%=%)) $(Q)$(MAKE) -C $(@:clean-%=%) clean >/dev/null > + $(call QUIET_CLEAN, test-$(@:clean-%=%)) $(MAKE) -C $(@:clean-%=%) clean >/dev/null > > .PHONY: all clean $(SUBDIRS) $(CLEANDIRS) $(INSTALLDIRS) > > --- > base-commit: e9cbc854d8b148e3491291fb615e94261970fb54 > change-id: 20250114-perf_make_test-1141d4ad8877 > -- > - Charlie

7 months, 1 week

Re: [PATCH v8 2/5] Coresight: Add trace_id function to retrieving the trace ID

by James Clark

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(a)quicinc.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? > >> 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 >

7 months, 1 week

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight