Message-ID: <635ba698-d7a9-40d0-9285-4ec108d4a536@linaro.org>
Date: Thu, 25 Sep 2025 11:10:51 +0100
From: James Clark <james.clark@...aro.org>
To: Suzuki K Poulose <suzuki.poulose@....com>,
 Mike Leach <mike.leach@...aro.org>, Jie Gan <jie.gan@....qualcomm.com>,
 Leo Yan <leo.yan@....com>
Cc: Carl Worth <carl@...amperecomputing.com>,
 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



On 24/09/2025 5:42 pm, Suzuki K Poulose wrote:
> On 24/09/2025 11:21, Mike Leach wrote:
>> 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.
> 
> 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
> 
> 

Yeah, and perf_sink_id_map only "needs" to be in the csdev because it 
controls sharing IDs between multiple paths which can't be accomplished 
by storing it in the path.

This one is just a pointer to the perf handle which really does belong 
to the session rather than the device. This makes it more of a path 
thing than a csdev thing. Maybe we can rename path to be more like 
"session", which also happens to contain a path. But I think path is 
fine for now.

However in this case handle is per-cpu data that is only accessed on the 
same cpu in tmc_etr_get_buffer(). Assigning it in etm_event_start() just 
copies the same per-cpu variable into a non per-cpu place that 
eventually gets accessed on the same cpu anyway.

If we exported it then it can be used directly without worrying where to 
store it:

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c 
b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 17afa0f4cdee..4c33f442c80b 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -42,12 +42,8 @@ static bool etm_perf_up;
   * the ETM. Thus the event_data for the session must be part of the 
ETM context
   * to make sure we can disable the trace path.
   */
-struct etm_ctxt {
-       struct perf_output_handle handle;
-       struct etm_event_data *event_data;
-};
-
-static DEFINE_PER_CPU(struct etm_ctxt, etm_ctxt);
+DEFINE_PER_CPU(struct etm_ctxt, etm_ctxt);
+EXPORT_SYMBOL_GPL(etm_ctxt);
  static DEFINE_PER_CPU(struct coresight_device *, csdev_src);

  /*
diff --git a/drivers/hwtracing/coresight/coresight-priv.h 
b/drivers/hwtracing/coresight/coresight-priv.h
index fd896ac07942..b834e8bef2a5 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -14,6 +14,7 @@

  extern struct mutex coresight_mutex;
  extern const struct device_type coresight_dev_type[];
+DECLARE_PER_CPU(struct etm_ctxt, etm_ctxt);

  /*
   * Coresight management registers (0xf00-0xfcc)
@@ -49,6 +50,11 @@ extern const struct device_type coresight_dev_type[];
  #define ETM_MODE_EXCL_HOST     BIT(32)
  #define ETM_MODE_EXCL_GUEST    BIT(33)

+struct etm_ctxt {
+       struct perf_output_handle handle;
+       struct etm_event_data *event_data;
+};
+
  struct cs_pair_attribute {
         struct device_attribute attr;
         u32 lo_off;
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c 
b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index bf08f6117a7f..7026994b02b3 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -1328,8 +1328,9 @@ struct etr_buf *tmc_etr_get_buffer(struct 
coresight_device *csdev,
                                    enum cs_mode mode,
                                    struct coresight_path *path)
  {
-       struct perf_output_handle *handle = path->handle;
         struct etr_perf_buffer *etr_perf;
+       struct etm_ctxt *ctxt = this_cpu_ptr(&etm_ctxt);
+       struct perf_output_handle *handle = &ctxt->handle;

         switch (mode) {
         case CS_MODE_SYSFS:


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

Powered by blists - more mailing lists