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/nullSure, 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
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