On Thu, Sep 25, 2025 at 11:10:51AM +0100, James Clark wrote:
[...]
In short, I prefer to store perf handle in coresight path, as Jie has done in this series.
I will give details below, sorry for a long replying.
[...]
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.
Yes, renaming 'coresight_path' (to like 'coresight_runtime_ctx') better reflects the structure’s purpose.
The point is we can take chance to separate runtime parameters from device and driver instances.
- 'coresight_path': runtime parameters for an active session - 'coresight_device': a device instance registered on the bus - driver data: after probe, the driver maintains driver-specific attributes (e.g., the ETMv4 driver keeps mode and filters in struct etmv4_drvdata)
These structures have different lifetimes. For example, coresight_path is valid only during a session; otherwise, it should be cleared.
From a lifecycle perspective, storing the perf handle in coresight_path makes sense, since both are valid only for the duration of a session (the perf handle isn't used in sysfs mode, in which case we can simply leave it unset).
Furthermore, the perf handle is not just a handle; it lets us easily retrieve private event data (see perf_get_aux()).
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:
We need to be careful for exporting data structures across modules, as it makes them harder to manage.
In fact, we already share 'etm_event_data' for ETR driver for the same purpose, and seems to me, it is redendant to export another structure 'etm_ctxt' to just make it convenient for obtaining a buffer config pointer.
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);
I'd suggest a different approach: drop the etm_ctxt structure and instead define a per-CPU pointer to etm_event_data:
static DEFINE_PER_CPU(struct etm_event_data *, etm_ev_ctx);
As mentioned above, if perf handle is maintained in coresight_path, we can easily retrieve the etm_event_data via the perf handle.
A more aggressive refactoring would remove etm_ctxt from coresight-etm-perf.c entirely, relying on the perf event to manage private context data. For now, we keep it only to validate whether ETM is enabled (see multiple place to validate ctxt->event_data).
Thanks, Leo