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

[Jie Gan <jie.gan@oss.qualcomm.com>]

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

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