Re: [PATCH RFC 1/3] coresight: tmc: add the handle of the event to the path

[Suzuki K Poulose <suzuki.poulose@arm.com>]

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


> 
> 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