On 24/09/2025 5:42 pm, Suzuki K Poulose wrote:
On 24/09/2025 11:21, Mike Leach wrote:
Hi,
On Tue, 23 Sept 2025 at 02:49, Jie Gan jie.gan@oss.qualcomm.com wrote:
On 9/23/2025 1:31 AM, Carl Worth wrote:
Jie Gan jie.gan@oss.qualcomm.com writes:
From: Carl Worth carl@os.amperecomputing.com
The handle is essential for retrieving the AUX_EVENT of each CPU and is required in perf mode. It has been added to the coresight_path so that dependent devices can access it from the path when needed.
I'd still like to have the original command I used to trigger the bug in the commit message. I really like having reproduction steps captured in commit messages when I look back at commits in the future. So, that was:
perf record -e cs_etm//k -C 0-9 dd if=/dev/zero of=/dev/null
Sure, I’ll include your commit message in the formal patch series, I think it's V3 since you have submitted two versions, if you're okay with me sending it out.
/** * struct coresight_path - data needed by enable/disable path
- @path_list: path from source to sink.
- @trace_id: trace_id of the whole path.
- @path_list: path from source to sink.
- @trace_id: trace_id of the whole path.
- struct perf_output_handle: handle of the aux_event.
*/
Fixing to "@handle" was mentioned in another comment already.
Something about the above still feels a little off to me. It feels like we're throwing new data into a structure just because it happens to be conveniently at hand for the code paths we're needing, and not because it really _belongs_ there.
This data is perf specific - not path generic; so I agree that this structure should go elsewhere.
I would suggest in the csdev (coresight_device) structure itself. We already have some sink specific data in here e.g. perf_sink_id_map.
This could then be set/clear in the functions coresight-etm-perf.c file, where there is a significant amount of code dealing with the perf handle and ensuring it is valid and in scope.
This can then be set only when appropriate - for source / sink devices and only when in perf mode, and avoid the need to pass the handle around as API call parameters.
I think this data is specific to the session we are enabling the device(s) in. e.g., we keep the trace-id in the path. So, I don't mind having this in the path structure. Instead of modifying csdev with additional locking from "etm-perf" it is always cleaner to handle this in the path.
Suzuki
Yeah, and perf_sink_id_map only "needs" to be in the csdev because it controls sharing IDs between multiple paths which can't be accomplished by storing it in the path.
This one is just a pointer to the perf handle which really does belong to the session rather than the device. This makes it more of a path thing than a csdev thing. Maybe we can rename path to be more like "session", which also happens to contain a path. But I think path is fine for now.
However in this case handle is per-cpu data that is only accessed on the same cpu in tmc_etr_get_buffer(). Assigning it in etm_event_start() just copies the same per-cpu variable into a non per-cpu place that eventually gets accessed on the same cpu anyway.
If we exported it then it can be used directly without worrying where to store it:
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 17afa0f4cdee..4c33f442c80b 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -42,12 +42,8 @@ static bool etm_perf_up; * the ETM. Thus the event_data for the session must be part of the ETM context * to make sure we can disable the trace path. */ -struct etm_ctxt { - struct perf_output_handle handle; - struct etm_event_data *event_data; -}; - -static DEFINE_PER_CPU(struct etm_ctxt, etm_ctxt); +DEFINE_PER_CPU(struct etm_ctxt, etm_ctxt); +EXPORT_SYMBOL_GPL(etm_ctxt); static DEFINE_PER_CPU(struct coresight_device *, csdev_src);
/* diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h index fd896ac07942..b834e8bef2a5 100644 --- a/drivers/hwtracing/coresight/coresight-priv.h +++ b/drivers/hwtracing/coresight/coresight-priv.h @@ -14,6 +14,7 @@
extern struct mutex coresight_mutex; extern const struct device_type coresight_dev_type[]; +DECLARE_PER_CPU(struct etm_ctxt, etm_ctxt);
/* * Coresight management registers (0xf00-0xfcc) @@ -49,6 +50,11 @@ extern const struct device_type coresight_dev_type[]; #define ETM_MODE_EXCL_HOST BIT(32) #define ETM_MODE_EXCL_GUEST BIT(33)
+struct etm_ctxt { + struct perf_output_handle handle; + struct etm_event_data *event_data; +}; + struct cs_pair_attribute { struct device_attribute attr; u32 lo_off; diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index bf08f6117a7f..7026994b02b3 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1328,8 +1328,9 @@ struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev, enum cs_mode mode, struct coresight_path *path) { - struct perf_output_handle *handle = path->handle; struct etr_perf_buffer *etr_perf; + struct etm_ctxt *ctxt = this_cpu_ptr(&etm_ctxt); + struct perf_output_handle *handle = &ctxt->handle;
switch (mode) { case CS_MODE_SYSFS:
Regards
Mike.
The core idea behind coresight_path is that it can hold all the data potentially needed by any device along the path.
For example with the path ETM->Link->ETR->CATU:
All the mentioned devices operate by forming a path, for which the system constructs a coresight_path. This 'path' is then passed to each device along the route, allowing any device to directly access the required data from coresight_path instead of receiving it as a separate argument.
Imagine a device that requires more than two or three arguments, and you want to pass them through coresight_enable_path or similar functions...
For certain coresight_path instances, we may not need the handle or other parameters. Since these values are initialized, it's acceptable to leave them as NULL or 0.
Or, maybe it's the right place for it, and the cause of my concern is that "path" is an overly-narrow name in struct coresight_path?
It defines the direction of data flow—serving as the path for trace data.
Thanks, Jie
But if a renaming of this structure would improve the code, I'd also be fine with that happening in a subsequent commit, so I won't try to hold up the current series based on that.
-Carl
-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK