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] [day] [month] [year] [list]

Message-ID: <f7be3833-9b33-4935-b821-a2e0000a2557@oss.qualcomm.com>
Date: Tue, 23 Sep 2025 09:49:21 +0800
From: Jie Gan <jie.gan@....qualcomm.com>
To: Carl Worth <carl@...amperecomputing.com>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        Mike Leach
 <mike.leach@...aro.org>,
        James Clark <james.clark@...aro.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Tingwei Zhang <tingwei.zhang@....qualcomm.com>
Cc: 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

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

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