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

[Suzuki K Poulose]

On 24/09/2025 11:21, Mike Leach wrote:
> Hi,
>
> On Tue, 23 Sept 2025 at 02:49, Jie Gan <jie.gan@xxxxxxxxxxxxxxxx> wrote:
> > 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.
> 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