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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJ9a7VjaUE+iy=FFwPhCdfXgoGR3rP9WSx-ZkKYeCHhqJ2yzMg@mail.gmail.com>
Date: Wed, 24 Sep 2025 11:21:10 +0100
From: Mike Leach <mike.leach@...aro.org>
To: Jie Gan <jie.gan@....qualcomm.com>
Cc: Carl Worth <carl@...amperecomputing.com>, Suzuki K Poulose <suzuki.poulose@....com>, 
	James Clark <james.clark@...aro.org>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, 
	Tingwei Zhang <tingwei.zhang@....qualcomm.com>, coresight@...ts.linaro.org, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC 1/3] coresight: tmc: add the handle of the event to
 the path

Hi,

On Tue, 23 Sept 2025 at 02:49, Jie Gan <jie.gan@....qualcomm.com> wrote:
>
>
>
> On 9/23/2025 1:31 AM, Carl Worth wrote:
> > Jie Gan <jie.gan@....qualcomm.com> writes:
> >> From: Carl Worth <carl@...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.

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