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

[Jie Gan]

On 9/23/2025 1:31 AM, Carl Worth wrote:
> Jie Gan <jie.gan@xxxxxxxxxxxxxxxx> writes:
> > From: Carl Worth <carl@xxxxxxxxxxxxxxxxxxxxxx>
> >
> > 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